Fixes #38930 - Make fact parsers use proper os #10789

ShimShtein · 2025-12-03T14:50:10Z

Make sure the fact parser searches by all unique constraint combinations before creating a new object.

Steps to reproduce:
his is from memory, exact steps may be a bit different.

Create an OS with:

name: RedHat
major: 9 (or whatever matches your host OS)
minor: 7 (or whatever matches your host OS)
description: RHEL 9.7
Ensure no OS exists with name RHEL & major 9 & minor 7
Run the installer on a RHEL 9.7 Satellite server

ekohl

I didn't mean to do a review, but this caught my eye. So take my comments here as suggestions.

Looking at the failed tests I think we're using assert_equal in the wrong order, leading to confusing errors. Not a blocker.

But the failing tests do look related so please have a look.

ekohl · 2025-12-03T15:28:59Z

test/static_fixtures/facts/puppet_rhel_9_facter_4.1.json

We already have FacterDB as a dependency which contains these kinds of fact sets. Is it possible to reuse that? I see others use static files so don't consider this blocking.

I see there is a warning under the hood of FacterDB:

./home/vagrant/foreman/.vendor/ruby/3.0.0/gems/jgrep-1.5.4/lib/jgrep.rb:27: warning: detected duplicate key "osfamily" in JSON object. This will raise an error in json 3.0 unless enabled via `allow_duplicate_key: true` at line 86377 column 2 /home/vagrant/foreman/.vendor/ruby/3.0.0/gems/jgrep-1.5.4/lib/jgrep.rb:27: warning: detected duplicate key "fqdn" in JSON object. This will raise an error in json 3.0 unless enabled via `allow_duplicate_key: true` at line 197772 column 2 /home/vagrant/foreman/.vendor/ruby/3.0.0/gems/jgrep-1.5.4/lib/jgrep.rb:27: warning: detected duplicate key "hostname" in JSON object. This will raise an error in json 3.0 unless enabled via `allow_duplicate_key: true` at line 402388 column 2

It is not problematic yet, but I preferred not to mess with external dependency.

ShimShtein · 2025-12-03T20:17:39Z

Fixed the original errors that @ekohl was referring to. The new errors seem trickier, since they do not appear in my local environment.

ShimShtein · 2025-12-03T20:32:56Z

[test]

ShimShtein · 2025-12-07T15:02:02Z

Found out that the errors came from newer gem version in the CI. Updated the gem versions locally and the errors appeared for me too.
The errors were related to problems parsing fact files in the facterdb gem. Some of those files contain invalid json.

I have fixed the issue by forcing facterdb to load only the files that are relevant to the checks we perform. Luckily those files were OK, and the tests could run successfully.

ekohl · 2025-12-07T19:31:48Z

Did you file an issue for the broken files?

ShimShtein · 2025-12-08T09:00:28Z

Did you file an issue for the broken files?

We're using an antique version of facterdb gem. In the newer versions of the gem we don't have older facter versions of the data, so I didn't test our code against the newer facterdb. If we want to file a ticket, it would not make sense to file it against v1.27 when v4.0 is already out.

stejskalleos

Should the foreman_ansible fact parser be updated as well?
I've never had a problem with facts & duplicate OS; Could you please share the steps on how to reproduce the issue?

ShimShtein · 2025-12-10T16:08:38Z

Added ansible fact management and added the reproducer steps to the description.

Basically you want to create an operating system with the same description as the one that will be generated by the fact parser, and then run the parser.

For puppet parser it's crucial, since it is used during the upgrade procedure.

ekohl · 2025-12-11T12:06:27Z

We're using an antique version of facterdb gem. In the newer versions of the gem we don't have older facter versions of the data, so I didn't test our code against the newer facterdb. If we want to file a ticket, it would not make sense to file it against v1.27 when v4.0 is already out.

That's fair. We should look at updating, but that's for another PR.

ekohl · 2025-12-11T14:33:14Z

app/models/operatingsystem.rb

  default_scope -> { order(:title) }

+  # Find all operatingsystems that are duplicates of the given operating system according to all unique constraints
+  def self.duplicate_of(os)


I had a hard time interpreting what duplicate_of meant when reading it out of context. Isn't this effectively something like find_existing_matching_entry?

I also find the naming not as clear, when reading it in context and would appreciate a different naming for the method.

This reads a lot like Rails' find_or_(create|initialize)_by, especially as in most cases you pass in a newly created object just to verify it's a dupe or not.

So couldn't we re-use that in a slightly different way (pseudocode, not actually working)?

def self.find_or_create_by(attributes) self.find_by(attributes.slice(:name, :major, :minor)) || self.find_by(attributes.slice(:description)) || self.find_by(title: generate_title) || self.create(attributes) end

Would find_by_attributes sound better? Or maybe something like find_by_unique?

As for find_or_create - I was thinking about it, but the code does not always creates the record immediately after the find, so I didn't want to change the behavior too much.

find_unique_operatingsystem? find_operatingsystem_by_attributes? that way the name implies this is a special thing for OSes

ehelms · 2025-12-11T15:38:23Z

app/models/operatingsystem.rb

+  def self.duplicate_of(os)
+    scope = where(name: os.name, major: os.major, minor: os.minor)
+    scope = scope.or(where(description: os.description)) if os.description.present?
+    scope.or(where(title: os.title || os.generate_title))


Looks like in all examples this returns an array. I would then expect the method name to reflect this returns a plural (e.g .duplicates_of).

ehelms · 2025-12-11T15:43:06Z

app/services/foreman_salt/fact_parser.rb


    def operatingsystem
-      os = Operatingsystem.where(os_hash).first_or_initialize
+      os = Operatingsystem.new(os_hash)


There appears to be a pattern of calling new and then checking if a duplicate exists. I can see this being overlooked as a step when re-factoring or writing new code. Would it then make sense to include the duplicate_of into the initialize method for Operatingsystem so that it's done by default?

I think we should go the other way around - instead of passing a new operatingsystem object to the method, just pass a set of attributes. It makes more sense in the usage: you gather a set of attributes, and then search for an existing object that meets the defined criteria.
Maybe find_existing with a concrete set of attributes would be a better signature for this method.

evgeni · 2025-12-12T10:57:23Z

test/unit/puppet_fact_parser_test.rb

+      expected = FactoryBot.create(:operatingsystem, name: 'RHEL', major: '9', minor: '6', description: 'RHEL 9.6')
+      # now we update the description in the DB to make sure we're testing the title comparison
+      Operatingsystem.where(id: expected.id).update_all(description: 'ZZZ 9.6')


When trying this out manually, I ended up using an OS without a description at all, should we excplicitly test that case too?

Since the description has a unique constraint in the DB, there would be at max one instance of such record. I think it's the same test as for any other value (that I already test).

evgeni · 2025-12-12T13:03:02Z

app/models/operatingsystem.rb

+  def self.duplicate_of(os)
+    scope = where(name: os.name, major: os.major, minor: os.minor)
+    scope = scope.or(where(description: os.description)) if os.description.present?
+    scope.or(where(title: os.title || os.generate_title))


you're passing os.title || os.generate_title because os might be unsaved yet and thus set_title didn't run yet?

Yes, that's why I extracted the generate_title out of the set_title hook.

ShimShtein · 2025-12-17T15:05:29Z

Updated the PR. Renamed the new search method and fixed all discrepancies associated with it.

ekohl · 2025-12-17T15:06:40Z

app/services/foreman_salt/fact_parser.rb

+      os = Operatingsystem.new(os_hash)
+      os = Operatingsystem.find_by_attributes(**os_hash).first || os


I'd like to avoid instantiating objects that may end up unused

Suggested change

os = Operatingsystem.new(os_hash)

os = Operatingsystem.find_by_attributes(**os_hash).first || os

os = Operatingsystem.find_by_attributes(**os_hash).first || Operatingsystem.new(os_hash)

ekohl · 2025-12-17T15:08:21Z

webpack/assets/javascripts/react_app/components/Editor/__tests__/Editor.test.js

@@ -1,5 +1,5 @@
 import React from 'react';
-import { act } from '@testing-library/react';
+import { act } from 'react-dom/test-utils';


Is it needed to rewrite the test to another library for this bugfix?

It was added by mistake. Removed from the PR.

ekohl · 2025-12-17T15:16:24Z

app/services/puppet_fact_parser.rb

    end

+    # Make sure the operating system object that we want to create is not a duplicate of an existing one.
+    os = Operatingsystem.find_by_attributes(name: os.name, major: os.major, minor: os.minor, description: os.description).first || os


This is duplicating logic we already have and firing off more database queries than needed. I'd prefer to replace the calls of find_or_initialize_by with find_by_attributes and dealing with the fact it can return no instances.

ekohl · 2025-12-17T15:17:05Z

app/services/foreman_ansible/operating_system_parser.rb

      args[:release_name] = os_release_name if os_name == 'Debian' || os_name == 'Ubuntu'
      # for Ansible, the CentOS Stream can be identified by missing minor version only
      args[:name] = "CentOS_Stream" if os_name == 'CentOS' && os_minor.blank?
+      args[:description] = os_description


This is subtly introducing a new feature. Is that intended?

In the original code, it was added during the generation of the new operatingsystem object. Now I have extracted it earlier, since we want to make sure no OS exists with the same description.

Make sure the fact parser searches by all unique constraint combinations before creating a new object.

ShimShtein · 2025-12-18T11:23:21Z

@ekohl The requirement to have only a single request in puppet fact parser caused a bit of refactoring. Hopefully I didn't change the logic too much.
Ready for another round though!

pr-processor bot added the Not yet reviewed label Dec 3, 2025

ekohl reviewed Dec 3, 2025

View reviewed changes

pr-processor bot removed the Not yet reviewed label Dec 3, 2025

ShimShtein force-pushed the fix_operatingsystem_deduplication branch from 5a2c434 to 65dacac Compare December 3, 2025 16:52

github-actions bot added the UI label Dec 3, 2025

ShimShtein force-pushed the fix_operatingsystem_deduplication branch from 65dacac to 8fa0cfa Compare December 3, 2025 17:14

ShimShtein force-pushed the fix_operatingsystem_deduplication branch 3 times, most recently from bf74f09 to 1f1517b Compare December 7, 2025 14:45

ShimShtein force-pushed the fix_operatingsystem_deduplication branch from 1f1517b to 99ebc5c Compare December 7, 2025 15:05

stejskalleos self-requested a review December 9, 2025 12:43

stejskalleos self-assigned this Dec 9, 2025

ShimShtein force-pushed the fix_operatingsystem_deduplication branch from 99ebc5c to c4e65cb Compare December 9, 2025 13:54

stejskalleos requested changes Dec 10, 2025

View reviewed changes

pr-processor bot added the Waiting on contributor label Dec 10, 2025

ShimShtein force-pushed the fix_operatingsystem_deduplication branch from c4e65cb to 8d671e0 Compare December 10, 2025 16:05

pr-processor bot added Needs re-review and removed Waiting on contributor labels Dec 10, 2025

ekohl reviewed Dec 11, 2025

View reviewed changes

pr-processor bot removed the Needs re-review label Dec 11, 2025

ehelms reviewed Dec 11, 2025

View reviewed changes

evgeni reviewed Dec 12, 2025

View reviewed changes

ShimShtein force-pushed the fix_operatingsystem_deduplication branch from 8d671e0 to 1336914 Compare December 17, 2025 15:00

pr-processor bot added the Waiting on contributor label Dec 17, 2025

evgeni marked this pull request as draft December 17, 2025 15:12

evgeni marked this pull request as ready for review December 17, 2025 15:12

ekohl reviewed Dec 17, 2025

View reviewed changes

evgeni marked this pull request as draft December 17, 2025 15:19

evgeni marked this pull request as ready for review December 17, 2025 15:19

ShimShtein force-pushed the fix_operatingsystem_deduplication branch from 1336914 to 93d71a5 Compare December 17, 2025 15:45

pr-processor bot added Needs re-review and removed Waiting on contributor labels Dec 17, 2025

ShimShtein force-pushed the fix_operatingsystem_deduplication branch 2 times, most recently from de7c5e1 to 58579ac Compare December 18, 2025 10:47

pr-processor bot removed the Needs re-review label Dec 18, 2025

Fixes #38930 - Make fact parsers use proper os

50e0ef0

Make sure the fact parser searches by all unique constraint combinations before creating a new object.

ShimShtein force-pushed the fix_operatingsystem_deduplication branch from 58579ac to 50e0ef0 Compare December 18, 2025 11:08

		os = Operatingsystem.new(os_hash)
		os = Operatingsystem.find_by_attributes(**os_hash).first \|\| os

Fixes #38930 - Make fact parsers use proper os #10789

Are you sure you want to change the base?

Fixes #38930 - Make fact parsers use proper os #10789

Uh oh!

Conversation

ShimShtein commented Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ekohl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShimShtein commented Dec 3, 2025

Uh oh!

ShimShtein commented Dec 3, 2025

Uh oh!

ShimShtein commented Dec 7, 2025

Uh oh!

ekohl commented Dec 7, 2025

Uh oh!

ShimShtein commented Dec 8, 2025

Uh oh!

stejskalleos left a comment

Choose a reason for hiding this comment

Uh oh!

ShimShtein commented Dec 10, 2025

Uh oh!

ekohl commented Dec 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShimShtein commented Dec 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShimShtein commented Dec 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ShimShtein commented Dec 3, 2025 •

edited

Loading