Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Refs #33470 - ensure OS.minor is not nil #9075

Closed
wants to merge 1 commit into from
Closed

Refs #33470 - ensure OS.minor is not nil #9075

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/models/operatingsystem.rb
View file
Open in desktop
Expand Up @@ -23,7 +23,7 @@ class Operatingsystem < ApplicationRecord
:reject_if => :reject_empty_provisioning_template

validates :major, numericality: true, presence: { message: N_("Operating System version is required") }
validates :minor, format: { with: /\A\d+(\.\d+)*\z/, message: "Operating System minor version must be in N or N.N format" }, allow_blank: true
validates :minor, format: { with: /\A\d+(\.\d+)*\z/, message: "Operating System minor version must be in N or N.N format" }, allow_blank: true, allow_nil: false
has_many :os_parameters, :dependent => :destroy, :foreign_key => :reference_id, :inverse_of => :operatingsystem
has_many :parameters, :dependent => :destroy, :foreign_key => :reference_id, :class_name => "OsParameter"
accepts_nested_attributes_for :os_parameters, :allow_destroy => true
Expand Down
4 changes: 2 additions & 2 deletions app/services/puppet_fact_parser.rb
View file
Open in desktop
Expand Up @@ -6,8 +6,8 @@ def operatingsystem

if orel.present?
major, minor = orel.split('.', 2)
major = major.to_s.gsub(/\D/, '')
minor = minor.to_s.gsub(/[^\d\.]/, '')
major = major.to_s.gsub(/\D/, '') || ''
minor = minor.to_s.gsub(/[^\d.]/, '') || ''
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think changing \. to . is correct. If it is, you change it to /[^.]/ or essentially . so it will always drop the character.

Funny thing: the git blame for this line is 1387528 and it speaks about minor being nil instead of "". I vaguely recall that nil.to_s returns an empty string instead of nil. We probably rely on this Ruby behavior to ensure non-null entries.

[1] pry(main)> nil.to_s
=> ""

That means this code is not the offender, right?

The problem does not show up with vanilla Foreman, only with Katello. Perhaps this is coming from the RHSM fact parser? However, that has this code:

foreman/app/services/katello/rhsm_fact_parser.rb

Line 60 in c693cd9

os_attributes = {:major => major, :minor => minor || '', :name => os_name}

I would think the || '' would take care of it.

So I'm not entirely sure where this comes from. I'll see if I can spin up a pipeline for a reproducer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I said in https://community.theforeman.org/t/katello-nightly-rpm-pipeline-1220-failed/27082/11: currently nightly is passed and I can no longer find a reproducer. That makes it very hard to review this. However, since nil.to_s is already "", the || '' part will never be triggered and I think it can be left out.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think changing . to . is correct

This was recommended by me by Rubocop: https://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Style/RedundantRegexpEscape

What is your concern? I don't understand.

currently nightly is passed and I can no longer find a reproducer

Well since we are at it and the code with to_s really smells, let's try to ensure there are no nil values via the migration and constraint rule? In the worst case, well, we break this. Not for the first time or last...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was recommended by me by Rubocop: https://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Style/RedundantRegexpEscape

Interesting. I didn't think it was redundant.

What is your concern? I don't understand.

I'm worried it'll be the catchall . instead of a literal .. However, testing suggests it is indeed the literal .:

[1] pry(main)> '2.1a'.to_s.gsub(/[^\d\.]/, '')
=> "2.1"
[2] pry(main)> '2.1a'.to_s.gsub(/[^\d.]/, '')
=> "2.1"

I must say I really dislike regexes and all these subtle nuances.

args = {:name => os_name, :major => major, :minor => minor}
os = Operatingsystem.find_or_initialize_by(args)
if os_name[/debian|ubuntu/i] || os.family == 'Debian'
Expand Down
12 changes: 12 additions & 0 deletions db/migrate/20220201065329_ensure_os_minor_is_not_nil.rb
View file
Open in desktop
@@ -0,0 +1,12 @@
class EnsureOsMinorIsNotNil < ActiveRecord::Migration[6.0]
def up
User.without_auditing do
Operatingsystem.unscoped.find_each do |os|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we filter with where(minor: nil) here and update at the DB level?

And I think the DB should be modified to have a NOT NULL, possibly also a DEFAULT empty string.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surprise! We already have the default and NOT NULL in the db, at least this is what I see in my dump:

    t.string "name", limit: 255, null: false
    t.string "major", limit: 5, default: "", null: false
    t.string "minor", limit: 16, default: "", null: false

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah this was implemented some time ago: https://projects.theforeman.org/issues/297

unless os.minor
os.minor = ''
os.save(validate: false)
end
end
end
end
end
6 changes: 3 additions & 3 deletions test/unit/puppet_fact_parser_test.rb
View file
Open in desktop
Expand Up @@ -57,9 +57,9 @@ def setup
end

test "should allow OS version minor component to be nil" do
@importer = PuppetFactParser.new({'operatingsystem' => 'AnyOS', 'operatingsystemrelease' => '6'})
assert_equal "AnyOS 6", os.to_s
assert_equal '6', os.major
@importer = PuppetFactParser.new({'operatingsystem' => 'CentOS_Stream', 'operatingsystemrelease' => '8'})
assert_equal "CentOS_Stream 8", os.to_s
assert_equal '8', os.major
assert_empty os.minor
assert_os_idempotent
end
Expand Down