-
Notifications
You must be signed in to change notification settings - Fork 87
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
[RHELC-332] checks to see if C2R is the latest version #415
Conversation
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.
Also needs unit tests
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.
The implementation looks good, just a few things I've spotted on the code.
Feel free to accept or deny any of the requested changes I proposed.
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.
Looking good. Only a few comments I spotted reviewing this one more time.
Also, if I'm not mistaken, we could reorder the version testing, to make it a bit easier to read and not confusing.
Codecov Report
@@ Coverage Diff @@
## main #415 +/- ##
==========================================
+ Coverage 87.51% 87.65% +0.13%
==========================================
Files 17 17
Lines 2419 2454 +35
Branches 418 424 +6
==========================================
+ Hits 2117 2151 +34
Misses 241 241
- Partials 61 62 +1
Continue to review full report at Codecov.
|
This pull request introduces 3 alerts when merging a016089 into f02cc9f - view on LGTM.com new alerts:
|
This pull request introduces 3 alerts when merging 2120314 into 5fbe026 - view on LGTM.com new alerts:
|
This pull request introduces 3 alerts when merging e091971 into be0e55a - view on LGTM.com new alerts:
|
This pull request introduces 3 alerts when merging b073af9 into be0e55a - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging b4387a0 into 3e80024 - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging 290b339 into 154ac63 - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging 0eb2026 into b5c7b80 - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging 83be7ab into 23fe45a - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging 5deceb9 into 37b3a00 - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging 0a9c6c5 into 37b3a00 - view on LGTM.com new alerts:
|
This pull request introduces 3 alerts when merging 6115090 into a921086 - view on LGTM.com new alerts:
|
This pull request introduces 2 alerts when merging 3af8d90 into a921086 - view on LGTM.com new alerts:
|
This pull request introduces 2 alerts when merging b83814b into a921086 - view on LGTM.com new alerts:
|
This pull request introduces 2 alerts when merging 9ea4a59 into a921086 - view on LGTM.com new alerts:
|
This pull request introduces 3 alerts when merging bc93c2b into 8d72fb0 - view on LGTM.com new alerts:
|
This pull request introduces 2 alerts when merging b64d8ff into 8d72fb0 - view on LGTM.com new alerts:
|
This pull request introduces 2 alerts when merging a432b90 into 8d72fb0 - view on LGTM.com new alerts:
|
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.
Looks good. Assuming this passes tests, you can go ahead and merge!
Good work!
I've looked and all of these review comments have been addressed.
This pull request introduces 1 alert when merging 51932e5 into 30dcc73 - view on LGTM.com new alerts:
|
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.
The test looks great @danmyway, I like the approach.
The only way to improve this would be to add some clean-up after the last test case (restore the original c2r version and delete the env variable), but it's up to you if you want to add this.
You're right, let's do that! |
This pull request introduces 1 alert when merging 6fbbdbf into 30dcc73 - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging 120453b into 30dcc73 - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging ca7620d into 30dcc73 - view on LGTM.com new alerts:
|
* the new function finds the latest version of C2R then checks to see if the user is running the latest version
Integration tests to check if inhibition of continuation of Convert2RHEL check for latest version works properly. Version in the check itself is called from __init__.py file, in which is this value changed for the purpose of the integration test.
Rebased to upstream. |
This pull request introduces 1 alert when merging a3b7318 into 30dcc73 - view on LGTM.com new alerts:
|
/packit retest-failed |
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 know I'm late to the party. I don't want to block merging of the PR. My comments can serve as a basis for a code cleanup in a subsequent PR.
logger.critical( | ||
"You are currently running %s and the latest version of convert2rhel is %s.\n" | ||
"Only the latest version is supported for conversion. If you want to ignore" | ||
" this you can set 'CONVERT2RHEL_UNSUPPORTED_VERSION' to continue" |
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.
" this you can set 'CONVERT2RHEL_UNSUPPORTED_VERSION' to continue" | |
" this check, set the environment variable 'CONVERT2RHEL_UNSUPPORTED_VERSION=1' to continue." |
What is CONVERT2RHEL_UNSUPPORTED_VERSION? The user does not know where (in some config file?) or to what value to set it (export CONVERT2RHEL_UNSUPPORTED_VERSION
won't work).
# the release for latest_verion will be "1.el7" and the relase for convert2rhel_version will be hard coded as "0" below, | ||
# therefore when the versions are the same the latest_version's release field will cause it to evaluate as later | ||
ver_compare = rpm.labelCompare(("0", convert2rhel_version, "0"), ("0", latest_version[1], "0")) | ||
if ver_compare < 0: |
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.
What does this magic number/condition mean? It's unnecessarily difficult to understand.
This should be either wrapped in a function with a descriptive name (like is_latest_version_installed(latest_available_version)
) or at the very least have a comment under it describing the meaning of what does the ver_compare < 0
and else:
mean. That would be handy anyways because one can get easily lost in the nested elses below.
"""Make sure that we are running the latest version of convert2rhel""" | ||
gpg_path = os.path.join(utils.DATA_DIR, "gpg-keys", "RPM-GPG-KEY-redhat-release") | ||
ssl_cert_path = os.path.join(utils.DATA_DIR, "redhat-uep.pem") | ||
repo_content = ( |
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.
Move this constant to the module level.
repo_content = ( | ||
"[convert2rhel]\n" | ||
"name=Convert2RHEL Repository\n" | ||
"baseurl=https://cdn.redhat.com/content/public/convert2rhel/$releasever/x86_64/os/\n" |
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.
"baseurl=https://cdn.redhat.com/content/public/convert2rhel/$releasever/x86_64/os/\n" | |
"baseurl=https://cdn.redhat.com/content/public/convert2rhel/$releasever/$basearch/os/\n" |
Let's not hardcode this one architecture. We might be adding more in the future.
if int(system_info.version.major) <= 6: | ||
logger.warning( | ||
"You are currently running %s and the latest version of convert2rhel is %s.\n" | ||
"Only the latest version is supported for conversion." % (convert2rhel_version, latest_version[1]) |
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.
"Only the latest version is supported for conversion." % (convert2rhel_version, latest_version[1]) | |
"We encourage you to update to the latest version." % (convert2rhel_version, latest_version[1]) |
I'd not use the word supported here as Red Hat does not support convert2rhel on RHEL 6-like systems.
if "CONVERT2RHEL_UNSUPPORTED_VERSION" in os.environ: | ||
logger.warning( | ||
"You are currently running %s and the latest version of convert2rhel is %s.\n" | ||
"'CONVERT2RHEL_UNSUPPORTED_VERSION' environment detected, continuing conversion" |
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.
"'CONVERT2RHEL_UNSUPPORTED_VERSION' environment detected, continuing conversion" | |
"'CONVERT2RHEL_UNSUPPORTED_VERSION' environment variable detected, continuing conversion." |
@@ -67,6 +73,84 @@ def perform_pre_ponr_checks(): | |||
ensure_compatibility_of_kmods() | |||
|
|||
|
|||
def check_convert2rhel_latest(): | |||
"""Make sure that we are running the latest version of convert2rhel""" |
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.
"""Make sure that we are running the latest version of convert2rhel""" | |
"""Make sure that we are running the latest downstream version of convert2rhel.""" |
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.
Also, add this log message here:
logger.task("Prepare: Checking if this is the latest version of convert2rhel")
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.
And, skip the check if the internet connection is not available to prevent bloated log messages like:
19:19:29 out: [07/20/2022 19:19:29] DEBUG - Calling command 'repoquery --releasever=7 --setopt=reposdir=/var/lib/convert2rhel/convert2rhel_repo.DiUuQs convert2rhel'
19:19:30 out: WARNING - Couldn't check if this is the latest version of convert2rhel
19:19:30 out: repoquery failed 1, failure: repodata/repomd.xml from convert2rhel: [Errno 256] No more mirrors to try.
19:19:30 out: https://cdn.redhat.com/content/public/convert2rhel/7/x86_64/os/repodata/repomd.xml: [Errno 14] curl#7 - "Failed connect to cdn.redhat.com:443; Connection refused"
19:19:30 out: https://cdn.redhat.com/content/public/convert2rhel/7/x86_64/os/repodata/repomd.xml: [Errno 14] curl#7 - "Failed connect to cdn.redhat.com:443; Connection refused"
19:19:30 out: https://cdn.redhat.com/content/public/convert2rhel/7/x86_64/os/repodata/repomd.xml: [Errno 14] curl#7 - "Failed connect to cdn.redhat.com:443; Connection refused"
19:19:30 out: https://cdn.redhat.com/content/public/convert2rhel/7/x86_64/os/repodata/repomd.xml: [Errno 14] curl#7 - "Failed connect to cdn.redhat.com:443; Connection refused"
19:19:30 out: https://cdn.redhat.com/content/public/convert2rhel/7/x86_64/os/repodata/repomd.xml: [Errno 14] curl#7 - "Failed connect to cdn.redhat.com:443; Connection refused"
19:19:30 out: https://cdn.redhat.com/content/public/convert2rhel/7/x86_64/os/repodata/repomd.xml: [Errno 14] curl#7 - "Failed connect to cdn.redhat.com:443; Connection refused"
19:19:30 out: https://cdn.redhat.com/content/public/convert2rhel/7/x86_64/os/repodata/repomd.xml: [Errno 14] curl#7 - "Failed connect to cdn.redhat.com:443; Connection refused"
19:19:30 out: https://cdn.redhat.com/content/public/convert2rhel/7/x86_64/os/repodata/repomd.xml: [Errno 14] curl#7 - "Failed connect to cdn.redhat.com:443; Connection refused"
19:19:30 out: https://cdn.redhat.com/content/public/convert2rhel/7/x86_64/os/repodata/repomd.xml: [Errno 14] curl#7 - "Failed connect to cdn.redhat.com:443; Connection refused"
19:19:30 out: https://cdn.redhat.com/content/public/convert2rhel/7/x86_64/os/repodata/repomd.xml: [Errno 14] curl#7 - "Failed connect to cdn.redhat.com:443; Connection refused"
Thanks, @r0x0d, for spotting that.
def check_convert2rhel_latest(): | ||
"""Make sure that we are running the latest version of convert2rhel""" | ||
gpg_path = os.path.join(utils.DATA_DIR, "gpg-keys", "RPM-GPG-KEY-redhat-release") | ||
ssl_cert_path = os.path.join(utils.DATA_DIR, "redhat-uep.pem") |
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.
ssl_cert_path = os.path.join(utils.DATA_DIR, "redhat-uep.pem") | |
# The SSL certificate of the https://cdn.redhat.com/ server | |
ssl_cert_path = os.path.join(utils.DATA_DIR, "redhat-uep.pem") |
"Couldn't check if this is the latest version of convert2rhel\n" | ||
"repoquery failed %s, %s" % (return_code, raw_output_convert2rhel_versions) |
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.
"Couldn't check if this is the latest version of convert2rhel\n" | |
"repoquery failed %s, %s" % (return_code, raw_output_convert2rhel_versions) | |
"Couldn't check if this is the latest version of convert2rhel.\n" | |
"Repoquery failed with the following output:\n%s" % raw_output_convert2rhel_versions |
I don't think a return code is important for the user. Return codes are not even mentioned in the repoquery man page. The output should be enough.
# After the for loop latest_version will have the epoch ,version, and release ex:("0" "0.26" "1.el7") information from convert2rhel yum repo. | ||
# the release for latest_verion will be "1.el7" and the relase for convert2rhel_version will be hard coded as "0" below, | ||
# therefore when the versions are the same the latest_version's release field will cause it to evaluate as later | ||
ver_compare = rpm.labelCompare(("0", convert2rhel_version, "0"), ("0", latest_version[1], "0")) |
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.
ver_compare = rpm.labelCompare(("0", convert2rhel_version, "0"), ("0", latest_version[1], "0")) | |
ver_compare = rpm.labelCompare(("0", installed_convert2rhel_version, "0"), ("0", latest_available_version[1], "0")) |
Naming of variables could be better. It's difficult for the code reader to tell the difference between convert2rhel_version
, latest_version
, convert2rhel_versions
without looking at how are they being set.
this adds to the code a case for if the users version of convert 2 RHEL isn't the most recent version, then it will let the user know to either go and update to the current version or to set an envar to bypass it.
Jira ticket RHELC-332