Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

remove setting quiet requests inside puma rack handler #2075

Merged
merged 4 commits into from Feb 19, 2020
Merged

remove setting quiet requests inside puma rack handler #2075

merged 4 commits into from Feb 19, 2020

Conversation

jchristie55332
Copy link
Contributor

@jchristie55332 jchristie55332 commented Nov 13, 2019 •
edited

Description

Remove setting quiet in user settings by default

Closes #2074

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added an entry to History.md if this PR fixes a bug or adds a feature. If it doesn't need an entry to HISTORY.md, I have added [changelog skip] to all commit messages.
  • I have added appropriate tests if this PR fixes a bug or adds a feature.
  • My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.

composerinteralia
composerinteralia reviewed Nov 14, 2019
View reviewed changes
Copy link
Contributor

@composerinteralia composerinteralia left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this was added in 03ed48c to fix #852. Will we reintroduce that issue if we change the default here?

We might also need to update the cli text

puma/lib/puma/cli.rb

Line 168 in befe00a

o.on "-q", "--quiet", "Do not log requests internally (default true)" do
if we are to change this default.

My initial thought is that we probably don't want to change the default, and should perhaps explore other ways to fix the bug you described in #2074. Also, if there is a bug here it would be nice to get a failing test case that catches it.

Many thanks!

@jchristie55332 jchristie55332 changed the title remove setting quiet requests inside puma rack handler [ci skip] remove setting quiet requests inside puma rack handler Nov 19, 2019
@jchristie55332
Copy link
Contributor Author

It looks like this was added in 03ed48c to fix #852. Will we reintroduce that issue if we change the default here?

In 03ed48c] the default configuration was updated, which I agree with, however forcing to quiet requests in the rack handler seems unnecessary to me (as this has been defaulted already)

I have added a couple of test cases to ensure that both the user and file defaults are working as expected. Note that the file configuration does not currently work in master branch

@nateberkopec nateberkopec added this to the 5.0.0 milestone Nov 27, 2019
nateberkopec
nateberkopec reviewed Nov 27, 2019
View reviewed changes
test/test_rack_handler.rb Outdated
@@ -226,4 +226,39 @@ def test_user_port_wins_over_config
end
end
end

def test_file_log_requests_wins_over_deafult_config
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo default

nateberkopec
nateberkopec reviewed Nov 27, 2019
View reviewed changes
test/test_rack_handler.rb Outdated
file_log_requests_config = true

Dir.mktmpdir do |d|
Dir.chdir(d) do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather just have a new, pre-baked config file on the filesystem, rather than create it dynamically. That way we can run the test in parallel.

@nateberkopec
Copy link
Member

So just to confirm, this PR does not cause double logging with Sinatra?

@nateberkopec
Copy link
Member

I can confirm that this does not reintroduce #852. I still don't really understand what it fixes though because I haven't reproduced #2074 locally, still working on that.

@nateberkopec
Copy link
Member

Tested in Rails using provided repro and it LGTM

@nateberkopec nateberkopec merged commit 5b8f557 into puma:master Feb 19, 2020
@nateberkopec nateberkopec mentioned this pull request Feb 19, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nateberkopec nateberkopec nateberkopec left review comments

@composerinteralia composerinteralia composerinteralia left review comments

Assignees
No one assigned
Labels
feature
Projects
None yet
Milestone
5.0.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jchristie55332 @nateberkopec @composerinteralia