-
Notifications
You must be signed in to change notification settings - Fork 899
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
Normalize ol_x64 as oracle OS #23290
Conversation
app/models/operating_system.rb
Outdated
@@ -28,7 +28,7 @@ class OperatingSystem < ApplicationRecord | |||
["linux_debian", %w[debian]], | |||
["linux_esx", %w[vmnixx86 vmwareesxserver esxserver vmwareesxi]], | |||
["linux_solaris", %w[solaris]], | |||
["linux_oracle", %w[oracle]], | |||
["linux_oracle", %w[oracle ol]], |
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.
Just to be clear, is this a substring match or a "starts_with" match? I'm asking because ol
seems very generic and could accidetally trip on certain strings.
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.
Additionally can you add some test cases for this?
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.
That's true, its just a .include?
https://github.com/ManageIQ/manageiq/blob/master/app/models/operating_system.rb#L82
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.
Yeah so a future "FooLinux" will match because it's just ol
in the middle. Might be nice to allow regexes in that list, and then we can use .match? if it's a regex. Then you can do something like /^ol_/
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.
We could also have a pre-parse step in the provider for weird cases like this
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.
Added oracle cases to the specs and regex matching
e60519e
to
488a238
Compare
Backported to
|
Normalize ol_x64 as oracle OS (cherry picked from commit a2a7b2d)
oVirt is returning
ol_8x64
andol_9x64
which is not being recognized as OracleFor this issue: ManageIQ/manageiq-providers-ovirt#683
@miq-bot add_label bug
@miq-bot assign @Fryguy
@miq-bot add_reviewer @agrare, @Fryguy