-
Notifications
You must be signed in to change notification settings - Fork 284
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 #37416 - assign templates for new Debian/Suse OSs #10990
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.
I didn't test this in the scenario where the OS is automatically created during a puppet check-in (what the commit 5befbde references)
Instead I manually created OSes for Debian "bookworm" 12.5 and SLES 15-SP5, populating only the minimum required fields; after creation, I went to edit the newly created OS and observed that, e.g. "AutoYaST entire SCSI disk" and "AutoYaST default iPXE" etc. were already selected in the respective Partition Table and Templates tabs, which wasn't the case when I tested without this commit.
The code looks good to me and seems to work as intended.
The existing test at test/concerns/operatingsystem_extensions_test.rb already covers the core logic, and the new bits are relatively simple and just involve returning strings for various cases. It could still potentially be worth adding tests for the new OSes to protect against future bugs, but I could honestly go either way on it... what do you think @sbernhard ?
Thanks @wbclark for having a look at it already. Actually,I think it would be even better to use templates assignment from the past if a new OS is auto-generated. Like, if the OS "Ubuntu 24.4.1" is auto-generated, it should use the OS "Ubuntu 23.4" (without minor) and re-use the template associatons. Only, if no data from the past can be re-used, take the hard-coded templates - which is not perfect either but better than nothing. Therefore, I set this PR to draft again. What are your thoughts about this behavior @wbclark? Does it make sense? |
34d1df7
to
c38e1dd
Compare
Updated the PR @wbclark with the mentioned assign templates from related os. (Tests are missing right now) |
Hmmm, it's a neat idea. My initial questions and thoughts about it are
|
Aha, @sbernhard you were a bit faster than me. :-) Would you kindly take a look at my above questions about your new proposal whenever you have a few minutes to do so, and in the meantime, I'll familiarize myself with the new changes and solicit some feedback about the idea from my team at our standup tomorrow. Thanks! |
I had a look at the changes and on my first tests everything worked as expected. |
2cbec66
to
e49f4b5
Compare
Hey @sbernhard , thanks for your patience. Consensus from the colleagues was that as long as it only affects Debian and SUSE, this should be totally fine 👍 |
0f0fd47
to
fa470cf
Compare
d15d0c7
to
9bb95e4
Compare
[test foreman] |
The change is ready @wbclark |
Thanks @sbernhard -- I'm returning from vacation today and will take a fresh look after I get through some meetings. :) |
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.
@sbernhard I couldn't make this work at present.
My steps were
- Rebased the commit on master
- Created an SLES OS and changed the default partition table to AutoYaST LVM
- Created a new SLES OS and observed that the default partition table was still AutoYaST entire SCSI disk
I tried it a few different ways, and added some debugging, and found that the find_related_os
method was indeed finding the prior OSes to inherit from:
21:50:48 rails.1 | 2024-05-29T21:50:48 [W|app|6cec470c] !!!! ALL_RELATED_OS: [{"id"=>17, "major"=>"15", "name"=>"SLES", "minor"=>"5", "nameindicator"=>nil, "created_at"=>"2024-05-29T21:48:06.770-04:00", "updated_at"=>"2024-05-29T21:48:06.770-04:00", "release_name"=>"", "description"=>"", "password_hash"=>"SHA256", "title"=>"SLES 15.5"}, {"id"=>16, "major"=>"15", "name"=>"SLES", "minor"=>"4", "nameindicator"=>nil, "created_at"=>"2024-05-29T21:43:50.634-04:00", "updated_at"=>"2024-05-29T21:43:50.634-04:00", "release_name"=>"", "description"=>"", "password_hash"=>"SHA256", "title"=>"SLES 15.4"}, {"id"=>15, "major"=>"15", "name"=>"SLES", "minor"=>"3", "nameindicator"=>nil, "created_at"=>"2024-05-29T21:34:33.190-04:00", "updated_at"=>"2024-05-29T21:34:33.190-04:00", "release_name"=>"", "description"=>"", "password_hash"=>"SHA256", "title"=>"SLES 15.3"}, {"id"=>14, "major"=>"15", "name"=>"SLES", "minor"=>"2", "nameindicator"=>nil, "created_at"=>"2024-05-29T21:33:42.363-04:00", "updated_at"=>"2024-05-29T21:33:42.363-04:00", "release_name"=>"", "description"=>"", "password_hash"=>"SHA256", "title"=>"SLES 15.2"}, {"id"=>12, "major"=>"15", "name"=>"SLES", "minor"=>"", "nameindicator"=>nil, "created_at"=>"2024-05-29T21:31:02.558-04:00", "updated_at"=>"2024-05-29T21:31:02.558-04:00", "release_name"=>"", "description"=>"", "password_hash"=>"SHA256", "title"=>"SLES 15"}] !!!!
Could you confirm I understood the intent correctly -- that my overridden default partition table should then have been inherited by the newly created OS in the same family?
In that case, I think the issue should be somewhere in the assign_related_os_templates
method
@@ -1,4 +1,4 @@ | |||
# encoding: utf-8 | |||
#false encoding: utf-8 |
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.
#false encoding: utf-8 | |
# encoding: utf-8 |
A find-and-replace editor mishap? :D
What are the changes introduced in this pull request?
Considerations taken when implementing this change?
What are the testing steps for this pull request?