-
Notifications
You must be signed in to change notification settings - Fork 43
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
Cleanup configuration file #340
base: master
Are you sure you want to change the base?
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.
Overall I think this makes sense.
@@ -135,7 +134,7 @@ def modules | |||
end | |||
|
|||
def module(name) | |||
modules.find { |m| m.name == name } | |||
modules.find { |m| m.identifier == name } |
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.
This looks like a bugfix. Could you provide a bit more info about why it's wrong?
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.
I tried to update the commit message with more information. The most telling is to look at the hook context documentation. Unfortunately, I think name
in some areas of code refers to the Puppet naming scheme with ::
and in other places it references this internal "name" construct that Kafo creates -- https://github.com/theforeman/kafo/blob/master/lib/kafo/puppet_module.rb#L175
The previous implementation relied on the name of the module to be specified which is a specially constructured replacement of colons with underscores. The hook context calls on this method and indicates using the identifier as a typicaly Puppet user would encounter it. For example, my_example::foo would be specified rather than a user thinking to convert this to my_example_foo. Thus this worked for single level classes, this would have failed for multilevel classes.
c85e949
to
6defecf
Compare
@ekohl Could you revisit a review on this? |
No description provided.