-
Notifications
You must be signed in to change notification settings - Fork 85
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
Fix map.jinja
and add more OSes
#23
Conversation
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.
@myii, a couple of suggestions? wdyt?
template/osfamilymap.yaml
Outdated
# vim: ft=yaml | ||
# | ||
# Setup variables using grains['os_family'] based logic. | ||
# Only add key:values that differ from `defaults.yaml`. |
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.
Is it possible that adding all these empty os_family
s hashes might confuse a newcomer and let her assume that they have to be listed even if not covered by the formula?
In that case, perhaps the line above can be rephrased, like
# You just need to add the key:values for an `os_family` that differ from `defaults.yaml`.
# Only add an `os_family` which is/will be supported by the formula (empty `os_family`s
# do not need to be listed, just added here as an example)
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.
Absolutely, just wanted to revive the useful comment that was in the map.jinja
previously and show a list of OSes that formulas support. I do feel like I've come across the use of {}
before somewhere... perhaps a situation where the OS family (e.g. Debian
) makes no changes to the defaults yet the OS derivative (e.g. Ubuntu
) does? Maybe just used to explicitly define the OSes?
In any case, your suggestion is excellent and I'll incorporate that.
@javierbertoli Thanks for the excellent review. I'll get those done soon and pushed into this PR. |
@javierbertoli I've applied those changes in b5c4987. Thanks again for your feedback. |
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.
Awesome! I think it's ready to merge. Thanks, @myii !
@aboe76 @javierbertoli Thanks to both of you. |
Fixed issues with
map.jinja
. Since this formula is for reference purposes, I've added the other OS values found in other formulas, in bothosfamilymap.yaml
andosmap.yaml
.