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

Drop log unsupported "spdy", make "http2" a server level option. #346

Merged
merged 17 commits into from
Jul 5, 2023

Conversation

oxpa
Copy link
Contributor

@oxpa oxpa commented Jun 29, 2023

Proposed changes

Removes long abandoned SPDY listening socket option.
Also moves http2 to a server level

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING document
  • I have added Molecule tests that prove my fix is effective or that my feature works
  • I have checked that any relevant Molecule tests pass after adding my changes
  • I have updated any relevant documentation (defaults/main/*.yml, README.md and CHANGELOG.md)

@oxpa oxpa requested a review from alessfg as a code owner June 29, 2023 14:29
@oxpa
Copy link
Contributor Author

oxpa commented Jun 29, 2023

commit message is wrong >_<

@alessfg alessfg added the enhancement Enhance an existing feature label Jun 29, 2023
@alessfg alessfg added this to the 0.6.1 milestone Jun 29, 2023
Copy link
Collaborator

@alessfg alessfg 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 the PR! I missed spdy no longer being supported.

Re implementing the new http2 directives -- the way the templates are organized, each "module" has its own template. Your current placement is within the core template, whereas the http2 directive should live inside its own module template. This helps in maintaining and tracking down any potential issues with the code further down the line.

I think the best location for the module would be before this macro block https://github.com/nginxinc/ansible-role-nginx-config/blob/main/templates/http/modules.j2#L23. You will then need to add the template to https://github.com/nginxinc/ansible-role-nginx-config/blob/main/templates/http/default.conf.j2. (There is no need to implement all the http2 directives right now, but creating its own template section will help with implementing the remaining directives further down the line 😄)

@oxpa
Copy link
Contributor Author

oxpa commented Jun 29, 2023

OK, will do.
It's also better to postpone merging this PR till R30, when these changes are in N+

@oxpa oxpa requested a review from alessfg July 3, 2023 19:20
Copy link
Collaborator

@alessfg alessfg left a comment

Choose a reason for hiding this comment

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

Looks great! I added a few suggestions re the wording in comments (to keep all comments as standard as possible) and fix some Jinja2 quirks that might lead to unnecessary whitespaces.

One more thing, can you update the Changelog? Thanks!

defaults/main/template.yml Outdated Show resolved Hide resolved
defaults/main/template.yml Outdated Show resolved Hide resolved
defaults/main/template.yml Outdated Show resolved Hide resolved
molecule/default/converge.yml Outdated Show resolved Hide resolved
templates/http/modules.j2 Outdated Show resolved Hide resolved
templates/http/modules.j2 Outdated Show resolved Hide resolved
templates/http/modules.j2 Outdated Show resolved Hide resolved
templates/http/modules.j2 Outdated Show resolved Hide resolved
oxpa and others added 9 commits July 3, 2023 22:11
Co-authored-by: Alessandro Fael Garcia <alessfg@hotmail.com>
Co-authored-by: Alessandro Fael Garcia <alessfg@hotmail.com>
Co-authored-by: Alessandro Fael Garcia <alessfg@hotmail.com>
Co-authored-by: Alessandro Fael Garcia <alessfg@hotmail.com>
Co-authored-by: Alessandro Fael Garcia <alessfg@hotmail.com>
Co-authored-by: Alessandro Fael Garcia <alessfg@hotmail.com>
Co-authored-by: Alessandro Fael Garcia <alessfg@hotmail.com>
@alessfg alessfg self-requested a review July 5, 2023 21:07
@alessfg alessfg merged commit 7ee4830 into nginxinc:main Jul 5, 2023
@alessfg alessfg mentioned this pull request Feb 29, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhance an existing feature
Development

Successfully merging this pull request may close these issues.

2 participants