-
Notifications
You must be signed in to change notification settings - Fork 991
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 #36525 - kickstart's RHSM line only works on RHEL hosts #9745
Conversation
Issues: #36525 |
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 think rhinstaller/anaconda#2546 was a mistake and should be reverted. That would remove the need for this.
Also linking back https://community.theforeman.org/t/el9-kickstart-using-rhsm/32666 here.
app/views/unattended/provisioning_templates/provision/kickstart_default.erb
Outdated
Show resolved
Hide resolved
Linking in https://bugzilla.redhat.com/show_bug.cgi?id=2211944 The initial driver for the anaconda patch is 99.9% of rebuild users don't use self managed katello so presenting them with the subscription-manager gui is a confusing experience. Rebuilds and CentOS Stream shouldn't have a UI box for setting up subscriptions to RHN. The problem being, for folks with katello having access to subscription manager is useful. |
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 tempted to suggest a top level rhsm_compatible = @host.operatingsystem.name == 'RedHat' && os_major >= 9
and use that in both cases, similar to how we have realm_compatible
on line 60, but I'm also fine with this version.
rebased off HEAD to try and get the tests to run better... |
Test failures look relevant. I've opened #9769 to show how I think we could also make the tests pass, rather than adjust the fixtures. Feel free to consider both options and see what works best for you. I think the advantage of mine is that the rendered template is easier to review. |
I should have noted that I didn't test my PR and I can now see it fails in various ways. Will you address the failures or should I? |
I'm juggleing a few things today. Odds are I wont get back to this for a week or two... If you've got the time, I think you've got write access to my branch. |
I finally found time to get back to this, |
It would be super helpful if the tests indicated which template from the list it doesn't like... I've never succeeded in generating the template snapshots locally... |
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.
With this PR rendering the kickstart_default
template on the CentOS host is failing on:
There was an error rendering the Kickstart default template: ERF42-7327 [Foreman::Exception]: The snippet 'redhat_register' threw an error: Safemode doesn't allow to access 'registration_type' on #<Safemode::ScopeObject>
Indeed it is. It seems an odd error since I didn't add the Since I don't think I broke it, I'm not sure how to fix it... |
@stejskalleos Any ideas how I can get this to work with |
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.
Hi, sorry for the late response.
I added my suggestions, plus it would be good to rebase the branch over the latest develop
, there were some changes, and task rails snapshots:generate
is failing.
app/views/unattended/provisioning_templates/provision/kickstart_default.erb
Outdated
Show resolved
Hide resolved
app/views/unattended/provisioning_templates/snippet/redhat_register.erb
Outdated
Show resolved
Hide resolved
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.
Changes look good to me but the snapshots must be updated. You can do this by running the command:
RAILS_ENV=test bundle exec rails snapshots:generate
hmmmmm
|
In theory I've sorted out the merge conflicts. |
I'd love to have this in place for foreman 3.10... |
@stejskalleos any luck with the templates? |
Generating snapshots is broken right now, reopening PR which fix the problem: https://github.com/theforeman/foreman/pull/9530/files |
@jcpunk with latest foreman develop branch generating snapshots works for me with the following command:
May I ask you to rebase and try again? |
Still no luck:
|
No clue why it still won't work for you. But, I think the remaining test-error is 'valid', because for the failing test-case (RHEL9) it should add the If |
I've just now added an I don't know why the factory uses I feel like somewhere in there should be a template that runs the subscription-manager snippet on CentOS Stream9 or the AltLinux distro from the tests, but I don't know how to add/generate that. |
Tests are green 👍 |
I am worried that there isn't an explicit test for non-RHEL systems >9.0 |
|
Changes LGTM, I'll ping our QAs for final verification |
app/views/unattended/provisioning_templates/provision/kickstart_default.erb
Outdated
Show resolved
Hide resolved
app/views/unattended/provisioning_templates/provision/kickstart_default.erb
Outdated
Show resolved
Hide resolved
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.
Not tested, but code-wise LGTM
Hi, #10121 has been merged, so if you rebase over latest develop and run the |
Rebased, I may have sorted the snippets out |
[test integration] |
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.
🍏
Everything is green, and the snapshots are OK. Let's get this in.
Thanks @jcpunk and everyone for the help.
This patch gets my self managed katello working again with foreman kickstarts.