-
Notifications
You must be signed in to change notification settings - Fork 48
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
Fixes #8585: Remove unused configuration and unneeded functions. #50
Conversation
22b6635
to
ce5dd15
Compare
49a936e
to
2304505
Compare
ACK |
Same as #64, I think you can drop |
|
||
candlepin: | ||
url: <%= scope.lookupvar("katello::params::candlepin_url") %> | ||
url: <%= @candlepin_url %> |
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'm slightly surprised this still works with scoping. I'd recommend adding a test to katello_config_spec.rb
that verifies one of the config vars (but possibly all).
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'd be ok with changing all of these to scope[]
syntax if that would alleviate the potential for strangeness in the future.
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.
https://docs.puppetlabs.com/guides/templating.html#referencing-variables mentions that they should be in scope and since the template is rendered from katello::config
, I'd expect everything from the katello
scope to be unavailable.
@ekohl Added tests, but didn't change the template. I wasn't sure if the tests would suffice or I needed to update to |
@ehelms since there's test coverage now, I assume it's ok. Once the aim is puppet 4 support, it can just be added to the test matrix and you'll find out easily enough. 👍 |
ACK |
Fixes #8585: Remove unused configuration and unneeded functions.
No description provided.