-
Notifications
You must be signed in to change notification settings - Fork 146
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
convert dot-less CentOS versions to X.999 #1139
base: main
Are you sure you want to change the base?
Conversation
Thank you for contributing to the Leapp project!Please note that every PR needs to comply with the Leapp Guidelines and must pass all tests in order to be mergeable.
Packit will automatically schedule regression tests for this PR's build and latest upstream leapp build. If you need a different version of leapp from PR#42, use It is possible to schedule specific on-demand tests as well. Currently 2 test sets are supported,
[Deprecated] To launch on-demand regression testing public members of oamg organization can leave the following comment:
Please open ticket in case you experience technical problem with the CI. (RH internal only) Note: In case there are problems with tests not being triggered automatically on new PR/commit or pending for a long time, please contact leapp-infra. |
release_id = data.get('ID', '').strip('"') | ||
version_id = data.get('VERSION_ID', '').strip('"') | ||
if release_id == 'centos' and '.' not in version_id: | ||
version_id = "{}.999".format(version_id) |
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.
I am not too happy about 999 magic constant tbh :(
Is there a universe where version_id doesn't have 999 and leapp is still happy?
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.
Honestly, I do not think this is the right solution for this. This should not be resolved by magical/crafted values that have nothing to do with the real system. This particular problem will need to be refink, together with the target os version - where this magic even cannot be used as it has effect on baseurls of target repositories if $releasever
is present.
Please note that we do not have a capacity right now to think about the right solution, so it's possible we will need to postpone this for a time. But when we have a time, we could invite you to our team tech. mtg to discuss the solution if you are interested. Once we have an agreement how the solution should look like, we can specify in github issues to set properly the expectation about the solution.
Yeah, I agree, this is not a nice solution (but it works 🙃) I'll happily join the mtg to discuss this! |
Just in case here are the commits we made in ELevate project to fix this CentOS version issue: But this PR looks cleaner. |
CentOS Stream doesn't do dot releases, so it always has the major version in the version_id field of os-release. Our code, in multiple places, expects versions in the form X.Y and doesn't cope well when given only X. The value "999" is taken arbitrarily, with the sole assumption that it will be always higher than any RHEL release, as CentOS Stream is ahead of RHEL. See https://issues.redhat.com/browse/CS-1757 for further discussion and alternative numbering options.
1682dd9
to
c272dc7
Compare
CentOS Stream doesn't do dot releases, so it always has the major version in the version_id field of os-release.
Our code, in multiple places, expects versions in the form X.Y and doesn't cope well when given only X.
The value "999" is taken arbitrarily, with the sole assumption that it will be always higher than any RHEL release, as CentOS Stream is ahead of RHEL.
See https://issues.redhat.com/browse/CS-1757 for further discussion and alternative numbering options.