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

Jump to bottom

Two fixes #110

Merged
merged 3 commits into from Sep 4, 2018
Merged

Two fixes #110

merged 3 commits into from Sep 4, 2018

Conversation

bmwiedemann
Copy link
Contributor

  • Fix named_directory usage
  • Sort hashes

only tested on SUSE OSes

@bmwiedemann
Fix named_directory usage
993b7d3 
similar to e573baf
was broken by 019e1e4
@javierbertoli
Copy link
Member

@bmwiedemann can you add the same |sort parameter in the other distros' files, for consistency?

@bmwiedemann
Sort hashes
353a8df 
To process hash entries in deterministic order.

Without this patch, config entries were different for every run
and required a service restart when nothing actually changed.

Doing it similar to
saltstack-formulas/munin-formula@0fe2f7e
@bmwiedemann
Copy link
Contributor Author

@javierbertoli I had already done that.

Switched to |dictsort now as seen in saltstack-formulas/munin-formula@0fe2f7e ; also looks nicer.

There are some iteritems() remaining where I'm not sure, if they need treatment.

@bmwiedemann
Copy link
Contributor Author

ping. Anything left that prevents merging?

@javierbertoli
Copy link
Member

@bmwiedemann sorry I missed your previous comment. I think you can replace those iteritems() too. Would you change those, and then I'll merge, if the tests pass?

@bmwiedemann
Use dictsort instead of iteritems
7f50076 
to process entries in deterministic order
@bmwiedemann
Copy link
Contributor Author

Did the s/iteritems/dictsort/
However, I dont know much about iterables etc in jinja/python so doing guessing here.

@javierbertoli javierbertoli merged commit 703a5e3 into saltstack-formulas:master Sep 4, 2018
@javierbertoli
Copy link
Member

@bmwiedemann , thanks to the tests introduced by @ryanwalder in #105, we can be quite sure nothing broke, so merging 😄

Thanks for your work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@bmwiedemann @javierbertoli