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

Refs #37696 - Don't run sub facet fact parser on non registered hosts #11109

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Refs #37696 - Don't run sub facet fact parser on non registered hosts #11109

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion app/models/katello/host/subscription_facet.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,11 +298,12 @@ def backend_update_needed?
end

def self.populate_fields_from_facts(host, parser, _type, _source_proxy)
return unless host.subscription_facet # skip method if the host is not a registered host
has_convert2rhel = parser.facts.key?('conversions.env.CONVERT2RHEL_THROUGH_FOREMAN')
Comment on lines +301 to 302
Copy link
Member

@evgeni evgeni Aug 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Would the following also work?

Suggested change
return unless host.subscription_facet # skip method if the host is not a registered host
has_convert2rhel = parser.facts.key?('conversions.env.CONVERT2RHEL_THROUGH_FOREMAN')
has_convert2rhel = parser.facts.key?('conversions.env.CONVERT2RHEL_THROUGH_FOREMAN')
return unless (host.subscription_facet || has_convert2rhel) # skip method if the host is not a registered host

That'd create the facet if the host has the special fact, and skip otherwise? (If we keep the build_sub_facet call below)

Copy link
Member

Choose a reason for hiding this comment

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

I tried that in #11110 and ran a pipeline based on the packit build -- it passed.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's an interesting thing to think about. AFAIK populate_fields_from_facts is called for every fact source, so also Puppet. That makes me think the current code will result in unstable values if both RHSM and Puppet are used. Your code to skip if no fact exists is probably more reliable, but also means no mechanism exists to clear the value.

# Add in custom convert2rhel fact if system was converted using convert2rhel through Katello
# We want the value nil unless the custom fact is present otherwise we get a 0 in the database which if debugging
# might make you think it was converted2rhel but not with satellite, that is why I have the tenary below.
facet = host.subscription_facet || host.build_subscription_facet
Copy link
Member

Choose a reason for hiding this comment

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

You use facet.save unless facet.new_record? below -- now new_record? can never be true, right?

Copy link
Member

Choose a reason for hiding this comment

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

But I also do not exactly understand the reasoning for the unless new_record? -- why didn't we want to store it in the past?

facet = host.subscription_facet
facet.attributes = {
convert2rhel_through_foreman: has_convert2rhel ? ::Foreman::Cast.to_bool(parser.facts['conversions.env.CONVERT2RHEL_THROUGH_FOREMAN']) : nil
}.compact
Expand Down
9 changes: 9 additions & 0 deletions db/migrate/20240815215102_change_convert2_rhel_to_boolean.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
class ChangeConvert2RhelToBoolean < ActiveRecord::Migration[6.1]
def up
change_column :katello_subscription_facets, :convert2rhel_through_foreman, :boolean, using: 'convert2rhel_through_foreman::boolean'
Copy link
Member

Choose a reason for hiding this comment

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

convert2rhel_through_foreman is a tenary (according to the comment above). It can be nil if the fact is not present, and 0/1 if the fact is.
Wouldn't converting it to bool loose that detail? Or can the column still be NULL?

Copy link
Member

Choose a reason for hiding this comment

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

https://api.rubyonrails.org/v7.2.0/classes/ActiveRecord/ConnectionAdapters/SchemaStatements.html#method-i-add_column implies it's not specified in the resulting SQL so it's up to the DB implementation. Reading https://www.postgresql.org/docs/current/sql-altertable.html I get the impression the default is to allow NULL, which also makes sense: adding a column means you either need to provide a default value or allow NULL values.

end

def down
change_column :katello_subscription_facets, :convert2rhel_through_foreman, :integer, using: 'convert2rhel_through_foreman::integer'
end
end
2 changes: 1 addition & 1 deletion test/models/host/subscription_facet_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ def test_search_role

def test_convert2rhel_through_foreman_on_host
subscription_facet.update(convert2rhel_through_foreman: 1)
assert_equal 1, host.subscription_facet.convert2rhel_through_foreman
assert true, host.subscription_facet.convert2rhel_through_foreman
assert_includes ::Host.search_for("convert2rhel_through_foreman = 1"), host
end

Expand Down
Loading