Fix map.jinja and add more OSes
#23
Conversation
javierbertoli
left a comment
javierbertoli
left a comment
There was a problem hiding this comment.
@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.
Is it possible that adding all these empty os_familys 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.
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. |
javierbertoli
left a comment
javierbertoli
left a comment
There was a problem hiding this comment.
Awesome! I think it's ready to merge. Thanks, @myii !
|
@aboe76 @javierbertoli Thanks to both of you. |
3 participants
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.yamlandosmap.yaml.