-
Notifications
You must be signed in to change notification settings - Fork 419
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
Simplify map.jinja revert deep_merge solution from #193 #395
Conversation
@aboe76 Initial tests are getting consistent results so far. One section of the rendering is different, though: --- master
+++ aboe76-simplify-map.jinja
@@ -1,13 +1,9 @@
'gitfs': {
'pygit2': {
'git': {
- 'install_from_package': None,
+ 'install_from_package': 'None',
'require_state': False
},
- 'install_from_source': False,
- 'libgit2': {
- 'install_from_source': False
- },
- 'version': '0.22.1'
+ 'install_from_source': False
}
},
}
}, Update: Forgot to mention, these are various Ubuntu minions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensures 'install_from_package': None
rather than as a string: 'None'
.
Changed the title to prefix with |
@myii about the |
@aboe76 Got a new diff for you after 66c0699: --- master
+++ aboe76-simplify-map.jinja
@@ -1,5 +1,10 @@
- 'gitfs': {'pygit2': {'git': {'install_from_package': None,
+ 'gitfs': {'dulwich': {'install_from_source': True},
+ 'gitpython': {'install_from_source': False},
+ 'pygit2': {'git': {'install_from_package': None,
'require_state': False},
'install_from_source': False,
- 'libgit2': {'install_from_source': False},
- 'version': '0.22.1'}},
+ 'libgit2': {'build_parent_dir': '/usr/src/',
+ 'download_hash': '683d1164e361e2a0a8d52652840e2340',
+ 'install_from_source': False,
+ 'version': '0.23.0'},
+ 'version': '0.22.1'}}, As far as I can tell, this is an improvement, where the |
@myii nice, thanks for reviewing, Installing a centos 6 vm to test against. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@noelmcloughlin would like some more input before merging from: and maybe somebody from: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It LGTM too. Nice refactoring, @aboe76! As soon as you don't consider it more a WIP I think we're OK to merge?
@noelmcloughlin @aboe76 Can this be merged? I've got another PR to submit for |
+1 merge. @javierbertoli gave +1 while back. |
@noelmcloughlin Great, I've gone ahead and removed the references to |
No merge away!!! |
Confirmed all OK with @noelmcloughlin via. Slack. Great work, @aboe76. Thanks to @javierbertoli and @noelmcloughlin for the reviews. |
@myii, @noelmcloughlin and @javierbertoli thanks for reviewing.. |
Pull request saltstack-formulas#395 indicates that this is no longer needed.
Pull request saltstack-formulas#395 indicates that this is no longer needed.
In a discussion on #193 and a pr #248 there is a attempt to make map.jinja easier to maintain and functional.
The deep_merge solution was created based on the fact that centos 6 didn't have
mapping
support in jinja2, but with the saltstack repos and epel repos having a newer version of jinja2 >2.6
this is no longer necessary. Jinja2 from 2.6 and higher support mapping.I have tested this new setup, with defaults.yaml, osfamilymap.yaml and osmap.yaml on the following systems without issues:
!!!! Please be carefull with merging, could lead to unexpected behavior without testing first. !!!!