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.

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

commonlogger.rb - fix HIJACK time format, use constants, not strings #3074

Merged
merged 2 commits into from Mar 14, 2023

Conversation

MSP-Greg
Copy link
Member

@MSP-Greg MSP-Greg commented Feb 9, 2023

Description

Current log output from a request and a 'full-hijacked' request:

127.0.0.1 - - [08/Feb/2023:18:47:21 -0600] "GET /users/profiles HTTP/1.1" 200 - 0.1126
127.0.0.1 - - [08/Feb/2023 18:47:22] "GET /cable HTTP/1.1" HIJACKED -1 0.6079

Note the hijack log line doesn't have a time zone offset...

Fixed and used constants for the env keys instead of strings. Not sure if local constants are better than using one from Puma::Const?

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added (or updated) 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.

@MSP-Greg MSP-Greg added the bug label Feb 9, 2023
@nateberkopec
Copy link
Member

Log formatting is so sensitive sometimes, I feel like we have to consider this a breaking change.

@MSP-Greg MSP-Greg added breaking change v7 Breaking changes for v7 labels Feb 10, 2023
@dentarg
Copy link
Member

dentarg commented Mar 12, 2023

Log formatting is so sensitive sometimes, I feel like we have to consider this a breaking change.

Then we ought to have tests covering this? Only implementation changed here

Also, isn't this a bugfix? Bug fixes always have the potential to be breaking for someone

@nateberkopec
Copy link
Member

Alright, you convinced me 😆

@nateberkopec nateberkopec removed breaking change v7 Breaking changes for v7 labels Mar 14, 2023
@nateberkopec nateberkopec merged commit 113b2e5 into puma:master Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants