Skip to content

feat: make http settings access_log_format and keepalive_timeout optional #100

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

Merged
merged 1 commit into from
Mar 24, 2021

Conversation

rolandjitsu
Copy link
Contributor

@rolandjitsu rolandjitsu commented Mar 24, 2021

Proposed changes

Describe the use case and detail of the change. If this PR addresses an issue on GitHub, make sure to include a link to that issue using one of the supported keywords here in this description (not in the title of the PR).

Make the following http_settings options:

  1. access_log_format
  2. keepalive_timeout

So that I can configure the main template like this (to enable the HTTP directive):

nginx_config_main_template_enable: true
nginx_config_main_template:
  http_enable: true
  http_settings:
    gzip:
      enable: true

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 all Molecule tests pass after adding my changes
  • I have updated any relevant documentation (defaults/main/*.yml, README.md and CHANGELOG.md)

NOTE: Let me know if I need to add some tests and where.

@rolandjitsu rolandjitsu changed the title WIP: feat: make http settings access_log_format and keepalive_timeout optional feat: make http settings access_log_format and keepalive_timeout optional Mar 24, 2021
@alessfg alessfg added the enhancement Enhance an existing feature label Mar 24, 2021
@alessfg alessfg added this to the 0.4.0 milestone Mar 24, 2021
@alessfg
Copy link
Member

alessfg commented Mar 24, 2021

I think it's safe to assume that is defined works as intended. I might circle back and add a proper check for each is defined in the role, but as of today I am more concerned with testing that any new template variables actually translate correctly to the templated config.

As an additional heads-up, as I continue refactoring individual NGINX modules to a more reusable and comprehensive macro pattern, both access_log_format and keepalive_timeout might undergo some changes before 0.4.0 proper is released.

@alessfg alessfg merged commit 8e072cd into nginx:main Mar 24, 2021
@rolandjitsu
Copy link
Contributor Author

I think it's safe to assume that is defined works as intended. I might circle back and add a proper check for each is defined in the role, but as of today I am more concerned with testing that any new template variables actually translate correctly to the templated config.

Thanks for merging! Sounds like a plan.

As an additional heads-up, as I continue refactoring individual NGINX modules to a more reusable and comprehensive macro pattern, both access_log_format and keepalive_timeout might undergo some changes before 0.4.0 proper is released.

Got it. I will keep an eye out for releases and breaking changes. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhance an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants