-
Notifications
You must be signed in to change notification settings - Fork 981
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 #36547 - Fix parsing of Ubuntu version in fact parsers #10145
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'm wondering about this line here:
foreman/app/models/operatingsystem.rb
Line 25 in 43a7a4f
validates :major, numericality: true, presence: { message: N_("Operating System version is required") } |
Looking at https://guides.rubyonrails.org/active_record_validations.html#numericality it should recognize it as a float so I have a hard time explaining the test failures.
Thanks for your review. Ewoud. I also added some suggestions from Bernhard. |
Fixed it :) Could you please have a look again @sbernhard and @ekohl? |
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 a complete review, mostly making some notes for myself.
major = os_major_version | ||
minor = os_minor_version |
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.
On a related note (but not necessarily in this PR): I'm still inclined to make this the default, not just for Ubuntu but for all. I'd be happy to accept a follow up PR that implements that.
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.
Yes, but this should definitely be handled by an extra PR as mentioned here
test "should correctly identify Ubuntu 18.04" do | ||
assert_equal 'Ubuntu', subject.send(:os_name) | ||
assert_equal '18.04', subject.send(:os_major_version) | ||
assert_nil subject.send(:os_minor_version) |
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.
Note to self: this was surprising, but there is no minor
for Ubuntu releases. I checked Puppet 8's facter version on 20.04 and it wasn't there.
Thanks @nadjaheitmann for your review again. Although the possible minor versions are limited by the database model we decided to limit them in the migration to only affect valid Ubuntu minor versions. |
Tested the host registration part and the migration and both works fine. |
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.
Thanks @goarsna
Replacement PR for #9760 which has been closed by the github-actions bot and was unfortunately force pushed afterwards.