-
Notifications
You must be signed in to change notification settings - Fork 130
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
rm unnecessary use of the foreman_proxy::plugin::module::features param #773
Conversation
Looks correct given the default in foreman_proxy::plugin::module for the
Further down puppet-foreman_proxy/manifests/module.pp Line 26 in 41bbfba
|
Excellent question. I think it was mostly because in core we have modules:
So it's really about 4 You could also argue for "explicit is better than implicit". Hopefully that explains the backstory. |
I don't have strong feelings about magic vs explicit but I do think that which ever style is used, it should be consistent. |
This I agree with. However, changing the default would technically be a major version bump and we just did a major version a few days ago. Right now I'm not sure what the best way to proceed is. Perhaps we should explicitly codify the expected feature. AFAIK |
It looks like with #776 we'll do a major bump anyway. Should we use this to fix the inconsistency? |
Sounds reasonable to me. |
foreman_proxy::plugin::module uses $title.capitalize() and now foreman::module does too. This makes it more consistent. It also explicitly sets the feature where needed. This was explained in an issue[1]. [1]: theforeman#773 (comment)
I came up with #788. Should we close this PR or would you still prefer to rely on defaults where possible? |
foreman_proxy::plugin::module uses $title.capitalize() and now foreman_proxy::module does too. This makes it more consistent. It also explicitly sets the feature where needed. This was explained in an issue[1]. [1]: theforeman#773 (comment)
foreman_proxy::plugin::module uses $title.capitalize() and now foreman_proxy::module does too. This makes it more consistent. It also explicitly sets the feature where needed. This was explained in an issue[1]. [1]: #773 (comment)
I guess we can close this now. |
No description provided.