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.

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

Merged
merged 5 commits into from
Jul 13, 2022

Conversation

Andrew-ang9
Copy link
Contributor

@Andrew-ang9 Andrew-ang9 commented Jan 26, 2022

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

Copy link
Member

@Venefilyn Venefilyn left a 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

convert2rhel/checks.py Outdated Show resolved Hide resolved
convert2rhel/checks.py Outdated Show resolved Hide resolved
convert2rhel/checks.py Outdated Show resolved Hide resolved
convert2rhel/checks.py Outdated Show resolved Hide resolved
r0x0d
r0x0d previously requested changes Jan 26, 2022
Copy link
Member

@r0x0d r0x0d left a 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.

convert2rhel/checks.py Outdated Show resolved Hide resolved
convert2rhel/checks.py Outdated Show resolved Hide resolved
convert2rhel/checks.py Outdated Show resolved Hide resolved
convert2rhel/checks.py Show resolved Hide resolved
convert2rhel/checks.py Outdated Show resolved Hide resolved
convert2rhel/checks.py Outdated Show resolved Hide resolved
convert2rhel/checks.py Outdated Show resolved Hide resolved
Copy link
Member

@r0x0d r0x0d left a 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.

convert2rhel/checks.py Show resolved Hide resolved
convert2rhel/checks.py Show resolved Hide resolved
convert2rhel/checks.py Outdated Show resolved Hide resolved
convert2rhel/unit_tests/checks_test.py Outdated Show resolved Hide resolved
convert2rhel/unit_tests/checks_test.py Outdated Show resolved Hide resolved
convert2rhel/checks.py Outdated Show resolved Hide resolved
convert2rhel/checks.py Outdated Show resolved Hide resolved
convert2rhel/checks.py Outdated Show resolved Hide resolved
convert2rhel/checks.py Outdated Show resolved Hide resolved
convert2rhel/checks.py Outdated Show resolved Hide resolved
convert2rhel/checks.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 8, 2022

Codecov Report

Merging #415 (a3b7318) into main (30dcc73) will increase coverage by 0.13%.
The diff coverage is 97.29%.

@@            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     
Impacted Files Coverage Δ
convert2rhel/checks.py 95.63% <97.29%> (+0.24%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 30dcc73...a3b7318. Read the comment docs.

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Mar 8, 2022

This pull request introduces 3 alerts when merging a016089 into f02cc9f - view on LGTM.com

new alerts:

  • 1 for Unnecessary pass
  • 1 for Unused import
  • 1 for Variable defined multiple times

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Mar 9, 2022

This pull request introduces 3 alerts when merging 2120314 into 5fbe026 - view on LGTM.com

new alerts:

  • 1 for Unnecessary pass
  • 1 for Unused import
  • 1 for Variable defined multiple times

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Mar 31, 2022

This pull request introduces 3 alerts when merging e091971 into be0e55a - view on LGTM.com

new alerts:

  • 1 for Unnecessary pass
  • 1 for Unused import
  • 1 for Variable defined multiple times

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Mar 31, 2022

This pull request introduces 3 alerts when merging b073af9 into be0e55a - view on LGTM.com

new alerts:

  • 1 for Unnecessary pass
  • 1 for Unused import
  • 1 for Variable defined multiple times

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Apr 7, 2022

This pull request introduces 1 alert when merging b4387a0 into 3e80024 - view on LGTM.com

new alerts:

  • 1 for Unused import

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Apr 14, 2022

This pull request introduces 1 alert when merging 290b339 into 154ac63 - view on LGTM.com

new alerts:

  • 1 for Unused import

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Apr 20, 2022

This pull request introduces 1 alert when merging 0eb2026 into b5c7b80 - view on LGTM.com

new alerts:

  • 1 for Unused import

@lgtm-com
Copy link
Contributor

lgtm-com bot commented May 10, 2022

This pull request introduces 1 alert when merging 83be7ab into 23fe45a - view on LGTM.com

new alerts:

  • 1 for Unused import

@lgtm-com
Copy link
Contributor

lgtm-com bot commented May 11, 2022

This pull request introduces 1 alert when merging 5deceb9 into 37b3a00 - view on LGTM.com

new alerts:

  • 1 for Unused import

@lgtm-com
Copy link
Contributor

lgtm-com bot commented May 12, 2022

This pull request introduces 1 alert when merging 0a9c6c5 into 37b3a00 - view on LGTM.com

new alerts:

  • 1 for Unused import

@lgtm-com
Copy link
Contributor

lgtm-com bot commented May 23, 2022

This pull request introduces 3 alerts when merging 6115090 into a921086 - view on LGTM.com

new alerts:

  • 2 for Unused import
  • 1 for Module is imported with 'import' and 'import from'

@lgtm-com
Copy link
Contributor

lgtm-com bot commented May 23, 2022

This pull request introduces 2 alerts when merging 3af8d90 into a921086 - view on LGTM.com

new alerts:

  • 1 for Unused import
  • 1 for Module is imported with 'import' and 'import from'

@lgtm-com
Copy link
Contributor

lgtm-com bot commented May 23, 2022

This pull request introduces 2 alerts when merging b83814b into a921086 - view on LGTM.com

new alerts:

  • 1 for Unused import
  • 1 for Module is imported with 'import' and 'import from'

@lgtm-com
Copy link
Contributor

lgtm-com bot commented May 24, 2022

This pull request introduces 2 alerts when merging 9ea4a59 into a921086 - view on LGTM.com

new alerts:

  • 1 for Unused import
  • 1 for Module is imported with 'import' and 'import from'

@lgtm-com
Copy link
Contributor

lgtm-com bot commented May 27, 2022

This pull request introduces 3 alerts when merging bc93c2b into 8d72fb0 - view on LGTM.com

new alerts:

  • 1 for Unused import
  • 1 for Unreachable code
  • 1 for Module is imported with 'import' and 'import from'

@lgtm-com
Copy link
Contributor

lgtm-com bot commented May 28, 2022

This pull request introduces 2 alerts when merging b64d8ff into 8d72fb0 - view on LGTM.com

new alerts:

  • 1 for Unused import
  • 1 for Module is imported with 'import' and 'import from'

@lgtm-com
Copy link
Contributor

lgtm-com bot commented May 28, 2022

This pull request introduces 2 alerts when merging a432b90 into 8d72fb0 - view on LGTM.com

new alerts:

  • 1 for Unused import
  • 1 for Module is imported with 'import' and 'import from'

convert2rhel/checks.py Outdated Show resolved Hide resolved
abadger
abadger previously approved these changes Jun 14, 2022
Copy link
Member

@abadger abadger left a 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!

@abadger abadger dismissed stale reviews from Venefilyn and r0x0d June 14, 2022 21:07

I've looked and all of these review comments have been addressed.

@danmyway danmyway requested a review from kokesak July 11, 2022 15:33
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Jul 11, 2022

This pull request introduces 1 alert when merging 51932e5 into 30dcc73 - view on LGTM.com

new alerts:

  • 1 for Clear-text storage of sensitive information

@danmyway
Copy link
Member

All tests are passing except for cve check on centos-8.4 which is stuck in execute phase for nearly 20 hours now.
@kokesak, @abadger, @r0x0d, @SpyTec whenever you have time for a final review, please do so.

kokesak
kokesak previously approved these changes Jul 12, 2022
Copy link
Member

@kokesak kokesak left a 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.

@danmyway
Copy link
Member

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!
As it's part of a set of sanity tests, we want to keep it clean.
Thank you for that.

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Jul 12, 2022

This pull request introduces 1 alert when merging 6fbbdbf into 30dcc73 - view on LGTM.com

new alerts:

  • 1 for Clear-text storage of sensitive information

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Jul 12, 2022

This pull request introduces 1 alert when merging 120453b into 30dcc73 - view on LGTM.com

new alerts:

  • 1 for Clear-text storage of sensitive information

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Jul 12, 2022

This pull request introduces 1 alert when merging ca7620d into 30dcc73 - view on LGTM.com

new alerts:

  • 1 for Clear-text storage of sensitive information

Andrew-ang9 and others added 5 commits July 12, 2022 17:48
* 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.
@danmyway
Copy link
Member

Rebased to upstream.

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Jul 12, 2022

This pull request introduces 1 alert when merging a3b7318 into 30dcc73 - view on LGTM.com

new alerts:

  • 1 for Clear-text storage of sensitive information

@danmyway
Copy link
Member

/packit retest-failed

Copy link
Member

@bocekm bocekm left a 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"
Copy link
Member

@bocekm bocekm Jul 12, 2022

Choose a reason for hiding this comment

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

Suggested change
" 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:
Copy link
Member

@bocekm bocekm Jul 12, 2022

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 = (
Copy link
Member

@bocekm bocekm Jul 12, 2022

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"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"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])
Copy link
Member

@bocekm bocekm Jul 12, 2022

Choose a reason for hiding this comment

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

Suggested change
"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"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"'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"""
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"""Make sure that we are running the latest version of convert2rhel"""
"""Make sure that we are running the latest downstream version of convert2rhel."""

Copy link
Member

@bocekm bocekm Jul 21, 2022

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")

Copy link
Member

@bocekm bocekm Jul 21, 2022

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")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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")

Comment on lines +107 to +108
"Couldn't check if this is the latest version of convert2rhel\n"
"repoquery failed %s, %s" % (return_code, raw_output_convert2rhel_versions)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"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"))
Copy link
Member

@bocekm bocekm Jul 12, 2022

Choose a reason for hiding this comment

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

Suggested change
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.

@bocekm bocekm merged commit fa3154e into oamg:main Jul 13, 2022
@Venefilyn Venefilyn changed the title checks to see if C2R is the latest version [RHELC-332] checks to see if C2R is the latest version Jul 22, 2022
@bocekm bocekm mentioned this pull request Aug 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants