Fixes #30647 - Parameterize redhat_register install parameters for kt_activation_keys. #7904
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Parameterize redhat_install_agent, redhat_install_host_tools, and redhat_install_host_tracer_tools. Change default for redhat_install_agent from false to true to align with deprecation and bootstrap.py behavior.
|
Can one of the admins verify this patch? |
2 similar comments
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
ok to test |
|
@jeffmcutter please update the commit message to also start with @jturel one more PR I'd like your consultation on. It seems reasonable to me, I wonder if katello seeds these as global params and if not, should it, so people know what they can override/fine-tune? |
@ares it sounds like a good thing to do but I'm unfamiliar with the idea. can you point me to where/how global params are seeded in Foreman as an example? |
jturel
reviewed
Aug 26, 2020
| redhat_install_host_tools = true | ||
| redhat_install_host_tracer_tools = false | ||
| redhat_install_host_tools = false if host_param_false?('redhat_install_host_tools') | ||
| redhat_install_host_tracer_tools = host_param_true?('redhat_install_host_tracer_tools') | ||
| else |
There was a problem hiding this comment.
I think this can be consolidated like so:
redhat_install_agent = host_param_true?('redhat_install_agent', true)
redhat_install_host_tools = host_param_true?('redhat_install_host_tools', true)
redhat_install_host_tracer_tools = host_param_true?('redhat_install_host_tracer_tools')
I would prefer to continue installing katello-agent by default even though it's deprecated because I think changing the default behavior at this time would cause a bit of a headache for some.
|
I think this change needs to be made in the community-templates repo since we sync this template from there periodically: https://github.com/theforeman/community-templates/blob/develop/provisioning_templates/snippet/redhat_register.erb |
we no longer sync from there, the repo was archived - https://community.theforeman.org/t/provisioning-templates-testing-separate-repository/13654/49 for more details |
Wow I missed that! Thanks |
we don't really do that right now in core, we should though. This is how foreman_chef does it https://github.com/theforeman/foreman_chef/blob/master/db/seeds.rb#L33-L48, similarly you can take a look at the Satellite branding plugin. |
|
Hello @jeffmcutter, would you mind sharing the status on this? There were several suugestion made as part of the code review. Could you please take a look or let us know, if/when you'll get back to it? |
|
This might be (partially) solved by #7994 |
|
I'm afraid it's a bit over my head. If I had a more clear understanding of which comments need to be addressed, I would love to be able to contribute. |
|
Thanks for the reply. I believe the agent installation has been addressed by the linked PR already. This PR only needs to be rebased and only ruby should be kept. Then I believe it's good to be merged. Please let me know if you need any further clarification. Thanks! |
theforeman-bot
added
Waiting on contributor
Templates
and removed
Waiting on contributor
labels
|
The PR is now ready, however the tests are now failing, since the template renders to a different output now. To fix that, please run the |
|
Any thoughts on the errors I'm encountering for that? Thanks, |
|
Try |
|
Thank you, that helped some. I have a new error now, not really sure... |
|
Hmm, it looks like it wants you to be in the 'test' rails environment which you can accomplish by adding RAILS_ENV=test to your command! Hopefully that's the last hurdle for you :) |
|
That does make sense to me, however the centos7-katello-devel forklift doesn't seem to have a test environment? |
|
It never ends! Try |
|
Thanks! |
|
We can do this. Try |
|
Hmm, the db::seed seemed to run OK, but no change for the snapshots:generate. |
|
I'm kinda stumped. Try hacking around it by commenting out the |
tbrisker
reviewed
Oct 28, 2020
| @@ -292,3 +292,4 @@ snippet: true | |||
| echo "No activation key found: Not registering" | |||
| <% end %> | |||
| <% end %> | |||
|
|
|||
There was a problem hiding this comment.
I've managed to generate the snapshots locally, looks like the only change is actually due to this extra newline. If you remove it from the commit snapshots should pass.
Suggested change
There was a problem hiding this comment.
Thank you @tbrisker. Maybe I did that wrong. I see now that I could have committed your suggestion vs. pushing my own attempt to make that change.
theforeman-bot
added
Needs re-review
Waiting on contributor
and removed
Waiting on contributor
labels
tbrisker
approved these changes
Oct 29, 2020
There was a problem hiding this comment.
Thanks @jeffmcutter and congrats on getting your first PR in Foreman! looking forward for more 😉
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For redhat_register provisioning template snippet when passing kt_activation_keys parameter as in the case for when an activation key is defined in the hosts host group:
Parameterize redhat_install_agent, redhat_install_host_tools, and redhat_install_host_tracer_tools.
Change default for redhat_install_agent from false to true to align with deprecation and bootstrap.py behavior.
Fixes https://projects.theforeman.org/issues/30647.