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

Move crypto-policies actors to common to execute it also in el9toel10 workflow #1337

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

The-Mule
Copy link

@The-Mule The-Mule commented Jan 31, 2025

It turns out that cryptopoliciescheck is not only useful for el8toel9 upgrade path but also for any upgrade path where crypto-policies is present.

RHEL-77169

Copy link

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.
If you want to request a review or rebuild a package in copr, you can use following commands as a comment:

  • review please @oamg/developers to notify leapp developers of the review request
  • /packit copr-build to submit a public copr build using packit

Packit will automatically schedule regression tests for this PR's build and latest upstream leapp build.
However, here are additional useful commands for packit:

  • /packit test to re-run manually the default tests
  • /packit retest-failed to re-run failed tests manually
  • /packit test oamg/leapp#42 to run tests with leapp builds for the leapp PR#42 (default is latest upstream - main - build)

Note that first time contributors cannot run tests automatically - they need to be started by a reviewer.

It is possible to schedule specific on-demand tests as well. Currently 2 test sets are supported, beaker-minimal and kernel-rt, both can be used to be run on all upgrade paths or just a couple of specific ones.
To launch on-demand tests with packit:

  • /packit test --labels kernel-rt to schedule kernel-rt tests set for all upgrade paths
  • /packit test --labels beaker-minimal-8.10to9.4,kernel-rt-8.10to9.4 to schedule kernel-rt and beaker-minimal test sets for 8.10->9.4 upgrade path

See other labels for particular jobs defined in the .packit.yaml file.

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.

@The-Mule
Copy link
Author

@Jakuje, you are the original author of this actor, can you take a quick look if moving it to common (from el8toel9) is done right? Compared to the original version I only made two changes:

  1. I made it 'no-op' for 7to8 upgrade
  2. Reference to Red Hat documentation is only used during 8to9 upgrade.

@Jakuje
Copy link
Contributor

Jakuje commented Jan 31, 2025

You need also the

https://github.com/oamg/leapp-repository/blob/main/repos/system_upgrade/el8toel9/actors/scancryptopolicies/actor.py

which produces the CryptoPolicyInfo message you need here.

@The-Mule
Copy link
Author

Thanks Jakub, there's also model that needs to be moved. However, neither cryptopoliciescheck nor scancryptopolicies are applicable to el7toel8 update path. Hence I would have to disable the tests somehow there and maybe the cleaner way would actually be moving both cryptopoliciescheck and scrancryptopolicies (and all other related stuff) to el8toel9. Once RHEL-7 will go out of support we can move it common, it would make more sense at that point.

@The-Mule The-Mule marked this pull request as draft January 31, 2025 13:06
@The-Mule The-Mule force-pushed the common-cryptopoliciescheck branch from 23055fe to 0e7b2ca Compare January 31, 2025 13:36
@The-Mule The-Mule changed the title [WIP] Move cryptopoliciescheck from el8toel9 to common [WIP] Keep crypto-policies actors from el8toel9 in el9toel10 Jan 31, 2025
@The-Mule The-Mule marked this pull request as ready for review January 31, 2025 13:37
@The-Mule
Copy link
Author

I just did that - instead of moving to common, I duplicated both cryptopoliciescheck and scancryptopolicies actors and their corresponding model to el9toel10, the only difference between their el8toel9 and el9toel10 version is the removal of documentation reference from el9toel10 version.

If there is some nice way how to have them in common while disabling them and their tests for el7toel8 upgrade path I'll be happy to move them there.

@The-Mule The-Mule force-pushed the common-cryptopoliciescheck branch from 0e7b2ca to 1a7977d Compare January 31, 2025 13:41
@Jakuje
Copy link
Contributor

Jakuje commented Feb 3, 2025

I think copying code is never way to go as it will make the maintainer to fix the bugs in two places, which is very error prone. I would rather have them in common + disable test with something like

import distro

@pytest.mark.skipif(int(distro.major_version()) < 8, reason="Requires EL8 or newer")

There are two actors related to crypto-policies in el8toel9 and their
corresponing models. They are still relevant for el9toel10 upgrade
so moving to common.

JIRA: RHEL-77169

Signed-off-by: Ondrej Moris <[email protected]>
@Jakuje Jakuje force-pushed the common-cryptopoliciescheck branch from 1a7977d to 6447709 Compare February 6, 2025 15:56
@Jakuje Jakuje changed the title [WIP] Keep crypto-policies actors from el8toel9 in el9toel10 Move crypto-policies actors to common to execute it also in el9toel10 workflow Feb 6, 2025
@Jakuje
Copy link
Contributor

Jakuje commented Feb 6, 2025

@The-Mule I took over the PR and went back to the using the "common" repo instead of having just another copy of the same actors and tests. We talked about the approach with @pirat89 today so this should be close enough to get merged soon (I hope :) )

@The-Mule
Copy link
Author

The-Mule commented Feb 6, 2025

Perfect, thanks @Jakuje, I greatly appreciate that!

@Jakuje Jakuje force-pushed the common-cryptopoliciescheck branch from 6447709 to 2bc3a5c Compare February 6, 2025 16:17
Even though it will never run on RHEL 7.

This can be safely reverted after we will drop support for Python2

Signed-off-by: Jakub Jelen <[email protected]>
@Jakuje Jakuje force-pushed the common-cryptopoliciescheck branch from 2bc3a5c to 3e2b2e9 Compare February 6, 2025 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants