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

Add option http_cfg_prepend #870

Merged
merged 3 commits into from
Sep 26, 2016
Merged

Add option http_cfg_prepend #870

merged 3 commits into from
Sep 26, 2016

Conversation

abraham1901
Copy link
Contributor

Add option http_cfg_prepend

@abraham1901 abraham1901 changed the title Dev Add option http_cfg_prepend Sep 2, 2016
@ffrank
Copy link

ffrank commented Sep 19, 2016

Hi, thanks for the patch.

Are our tests generally broken?

What's the use case here? Is the append option insufficient?

In any case, the commits need squashing.

@abraham1901
Copy link
Contributor Author

abraham1901 commented Sep 26, 2016

Hi

Are our tests generally broken?

Yes

What's the use case here? Is the append option insufficient?

I'm have a trouble sort option in http_cfg_append.
For example if i'm use include custom directory with config and need added limit_conn_zone used in included config - it's not posible.
In config now:

include /etc/nginx/customdir/*.conf;
limit_conn_zone $server_name zone=perserver:100m;

And receive syntax nginx error. But need another sequence:

limit_conn_zone $server_name zone=perserver:100m;
include /etc/nginx/customdir/*.conf;

In any case, the commits need squashing.

Done.

@@ -245,6 +246,7 @@
fastcgi_cache_use_stale => $fastcgi_cache_use_stale,
gzip => $gzip,
http_access_log => $http_access_log,
http_cfg_prepend => $http_cfg_prepend,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is causing the Travis tests to fail, because the => is not aligned with the rest. Puppet Lint is running in Travis and checks for this.

@abraham1901
Copy link
Contributor Author

Sorry, fixed

@@ -36,6 +36,14 @@ events {
}

http {
<% if @http_cfg_append -%>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be checking for http_cfg_prepend

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ffrank
Copy link

ffrank commented Sep 26, 2016

Thanks @3flex. Now that this is fix, would you give this a thumbs up? Because if so, I'm inclined to squash-merge this as is now.

@3flex
Copy link
Contributor

3flex commented Sep 26, 2016

If there's a valid use case, I say go ahead and merge.

I have a personal issue with all the append/prepend parameters throughout the module, but until something is done about #538 there's not really a better alternative. That's a big job though.

One other possibility is to stop sorting the directives in the templates like we have been, but apparently even on Ruby 1.9.3 there were issues in the past with non-stable ordering, I think Puppet itself might reorder sometimes and makes no guarantees about that unfortunately.

That said, 👍 from me!

@ffrank
Copy link

ffrank commented Sep 26, 2016

Great, thanks for the summary.

Thanks to @abraham1901 for the contribution!

@ffrank ffrank merged commit 24c4354 into voxpupuli:master Sep 26, 2016
darken99 pushed a commit to darken99/puppet-nginx that referenced this pull request Oct 13, 2016
Add http_cfg_prepend option
cegeka-jenkins pushed a commit to cegeka/puppet-nginx that referenced this pull request Sep 13, 2019
Rubueno pushed a commit to Rubueno/puppet-nginx that referenced this pull request Oct 19, 2020
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.

3 participants