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

Fixes #36547 - Fix parsing of Ubuntu version in fact parsers #9760

Closed
wants to merge 10 commits into from
Closed

Fixes #36547 - Fix parsing of Ubuntu version in fact parsers #9760

wants to merge 10 commits into from

Conversation

goarsna
Copy link
Contributor

@goarsna goarsna commented Jun 29, 2023

No description provided.

@theforeman-bot
Copy link
Member

Issues: #36547

@theforeman-bot
Copy link
Member

Can one of the admins verify this patch?

2 similar comments
@theforeman-bot
Copy link
Member

Can one of the admins verify this patch?

@theforeman-bot
Copy link
Member

Can one of the admins verify this patch?

ekohl
ekohl reviewed Jun 29, 2023
View reviewed changes
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

While technically correct, I think this may cause problems. For example, in searching it splits on the dot here:

https://github.com/ekohl/foreman/blob/0eb81758b02d0a4a397e14e076fec6a70a3aa2ee/app/models/operatingsystem.rb#L169-L178

I also wonder about migrations. Should we introduce a DB migration where we change the existing OS versions in the DB?

app/services/puppet_fact_parser.rb Outdated Show resolved Hide resolved
@goarsna
Copy link
Contributor Author

goarsna commented Jul 24, 2023

Hey @ekohl
Thanks for your feedback. And I have to apologize that my response took so long...

I have incorporated your feedback. I also searched for all areas of code where the version is parsed / used and fixed them. Hopefully I got them all. :)

Regarding db migrations: Should we consider anything else despite changing the version of the operating system (for example migrating to existing OSes with the correct version)?

@goarsna
Copy link
Contributor Author

goarsna commented Aug 17, 2023

Hey @ekohl,
I have rebased my PR and added a db migration that migrates Ubuntu OSes with non-empty minor version if no Ubuntu OS with the target major version is present.

Please let me know if anything is missing.

ekohl
ekohl reviewed Sep 14, 2023
View reviewed changes
app/models/operatingsystem.rb Outdated Show resolved Hide resolved
app/models/operatingsystem.rb Outdated Show resolved Hide resolved
app/services/foreman_ansible/operating_system_parser.rb Outdated Show resolved Hide resolved
app/services/katello/rhsm_fact_parser.rb Outdated Show resolved Hide resolved
app/services/katello/rhsm_fact_parser.rb Outdated Show resolved Hide resolved
app/services/puppet_fact_parser.rb
Comment on lines +9 to +10
major = os_major_version
minor = os_minor_version
Copy link
Member

Choose a reason for hiding this comment

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

I've been debating making this the behavior everywhere and would be OK with trying that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So that we simply leave out the if / else part and use os_major_version and os_minor_version for all operating systems? I also noticed that the if orel.present? isn't necessary here anymore, as the whole part from line 8 to line 24 is no longer dependent on it.

Copy link
Member

Choose a reason for hiding this comment

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

indeed, get rid of orel altogether.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had a look at relying on the major and minor versions reported by facter and it doesn't fit for all operating systems. The os_release often uses the full release reported by facter and parses it as necessary. Relying on the reported major and minor versions breaks some of the OSes (like Solaris and CentOS 8).

IMO it would be best to rework detection of the OS version in the PuppetFactParser in a separate PR.

db/migrate/20230719080900_fix_ubuntu_versions.rb Outdated Show resolved Hide resolved
ekohl
ekohl reviewed Oct 4, 2023
View reviewed changes
db/migrate/20230719080900_fix_ubuntu_versions.rb Outdated Show resolved Hide resolved
app/models/operatingsystem.rb Outdated Show resolved Hide resolved
@goarsna goarsna mentioned this pull request Oct 26, 2023
Merged
@goarsna
Copy link
Contributor Author

goarsna commented Jan 3, 2024

Good morning and a happy new year :)
I finally made it to work on this PR again and fixed some things, added tests and fixed failing tests.
I'm looking forward to any feedback :)

@goarsna
Copy link
Contributor Author

goarsna commented Jan 11, 2024

Thanks @ekohl , I'll have a look at the usage of factories and facterdb and adjust my changes.

@github-actions github-actions bot added the Stale label Apr 11, 2024
@github-actions GitHub Actions
Copy link

Thank you for your contribution! This PR has been inactive for 3 months, closing for now. Feel free to reopen when you return to it. This is an automated process.

@github-actions github-actions bot closed this Apr 19, 2024
@goarsna goarsna mentioned this pull request Apr 29, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ekohl ekohl ekohl requested changes

Assignees

@ekohl ekohl

Labels
Inactive Stale Waiting on contributor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@goarsna @theforeman-bot @ekohl