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

feat: implement multi header support #443

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

theseion
Copy link
Collaborator

  • support multiple headers with the same name

  • support multiple headers with identical name and value

  • make list of headers ordered; in the future, tests should be able to enforce the order of headers in a request

  • improve API, tests, and documentation of ftwhttp.Header

  • disable logging in tests where possible

  • enable self-updater test (go-critic was complaining because the test file was touched)

Fixes #332

@theseion theseion requested review from fzipi and M4tteoP January 12, 2025 11:25
@theseion
Copy link
Collaborator Author

Note that our test format currently does not support multiple headers with the same name, because we use a map for headers. In the future we should use a list instead in order to allow for repeating headers and values.

Copy link
Member

@fzipi fzipi left a comment

Choose a reason for hiding this comment

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

Thanks for taking this. Big change.

I'll suggest you to split in 3 PRs, as we are mixing stuff here.

  1. All the zerolog changes (🙏 good catch). E.g. inclusion and initialization.
func (s *responseTestSuite) SetupSuite() {
	zerolog.SetGlobalLevel(zerolog.Disabled)
}
  1. The uncommented self_updater tests
  2. This PR, rebased after the above.

It will make things more clear if we need to revert something.

@theseion theseion force-pushed the multi-headers branch 4 times, most recently from 411541d to d98119c Compare January 12, 2025 19:14
@theseion theseion requested a review from fzipi January 25, 2025 16:22
fzipi
fzipi previously approved these changes Feb 3, 2025
Copy link
Member

@fzipi fzipi left a comment

Choose a reason for hiding this comment

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

Looks like tests are broken?

@theseion theseion requested a review from fzipi February 5, 2025 06:05
fzipi
fzipi previously approved these changes Feb 14, 2025
@theseion
Copy link
Collaborator Author

Extended PR with updated schema and support for new ordered_headers field.

- support multiple headers with the same name
- support multiple headers with identical name and value
- make list of headers ordered; in the future, tests should be able to
  enforce the order of headers in a request
- improve API, tests, and documentation of ftwhttp.Header

- disable logging in tests where possible
- enable self-updater test (go-critic was complaining because the test
  file was touched)

Fixes coreruleset#332
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement multi-value headers
2 participants