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

Sort sub hash keys to have a stable ordering #532

Merged
merged 1 commit into from
Dec 30, 2014

Conversation

mbornoz
Copy link
Contributor

@mbornoz mbornoz commented Dec 17, 2014

No description provided.

@mbornoz
Copy link
Contributor Author

mbornoz commented Dec 17, 2014

@jfryman this is a very annoying bug because nginx service is refreshing on every puppet run :\

@3flex
Copy link
Contributor

3flex commented Dec 17, 2014

Which version of Ruby are you using? This is probably because on Ruby 1.8 Hashes aren't ordered.

The problem with sorting them all by key/value is that users might want to have directives set in a certain order, especially with custom added code added with these directives. Sorting them might break their configurations as some nginx directives are order dependent.

@jfryman might have to make a call whether Ruby 1.8 compatibility (which isn't guaranteed with this module) or sorting directives when a user might not expect that behavior is more important.

@mbornoz you can also use raw_append/raw_prepend. This is an array of lines to add to the config and will maintain sort order on Ruby 1.8.

@mbornoz
Copy link
Contributor Author

mbornoz commented Dec 17, 2014

The problem occurs on a Red Hat 7 with ruby 2.0.0p353 and something like this:

nginx::resource::location { 'XYZ':
  location             => '...',
  vhost                => '...',
  www_root             => '...',
  location_cfg_append  => {
    add_header => {
      'Access-Control-Allow-Origin'  => '*',
      'Access-Control-Allow-Methods' => 'GET',
    },
  },
}

To temporary avoid this problem I will use raw_append, thanks for the tips.

@xcompass
Copy link
Contributor

👍 We have the same issue. Was gonna do a pull request. Lucky I checked first.

@jfryman
Copy link
Contributor

jfryman commented Dec 30, 2014

Awesome. Yeah, this certainly is a bit of a pain... let's talk through it.

I don't really care about 1.8 support, to be frank. If it happens as a bi-product, then awesome! It is not something I think we should spend tons of effort on though. Puppet doesn't support it anymore, so I think it's unrealistic to try and support it as well. However, since this is affecting 2.x, something else is going on.

Let's dig....

  1. I think in principle this needs to be merged in. While Ruby 1.9 is supposed to keep some sort order, the fact is that there is some creative metaprogramming happening inside Puppet that may in fact be breaking that behavior.
  2. @3flex Your comments are spot on, but what do you think about item 1 in relation to hash ordering? I somewhat think that we should require raw_append to be relied on for absolute sorting.

Please drop dem comments!

@3flex
Copy link
Contributor

3flex commented Dec 30, 2014

Sounds good. I believe the latest from Puppet on this issue can be found at https://tickets.puppetlabs.com/browse/PUP-1755, TL;DR is that the module/template should handle hash sorting regardless of Ruby version, and we have raw_append if specific order is required. So let's merge.

As an aside I think we should move away from these methods to insert custom code into the manifests, and promote puppetlabs-concat as the best way to do this since it allows for total customization of any code that will be added to the vhost/location. But that's a discussion/PR for another day...

@jfryman
Copy link
Contributor

jfryman commented Dec 30, 2014

@3flex Most Excellent.

@mbornoz Thanks for the code! 🙇

jfryman added a commit that referenced this pull request Dec 30, 2014
Sort sub hash keys to have a stable ordering
@jfryman jfryman merged commit d42695c into voxpupuli:master Dec 30, 2014
mlafeldt added a commit to Jimdo/puppet-nginx that referenced this pull request Feb 5, 2015
The hash sorting was started in voxpupuli#532 and this fixes it for fastcgi as
well.
cegeka-jenkins pushed a commit to cegeka/puppet-nginx that referenced this pull request Oct 23, 2017
Sort sub hash keys to have a stable ordering
cegeka-jenkins pushed a commit to cegeka/puppet-nginx that referenced this pull request Oct 23, 2017
The hash sorting was started in voxpupuli#532 and this fixes it for fastcgi as
well.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants