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

Upgrade packit.yaml config to have integration tests #804

Closed
wants to merge 15 commits into from

Conversation

r0x0d
Copy link
Member

@r0x0d r0x0d commented Nov 14, 2022

This pull request introduces the execution of leapp integration tests as a packit job instead of the current behavior of using a GitHub Actions to trigger the tests by a comment.

Signed-off-by: Rodolfo Olivieri [email protected]

@r0x0d r0x0d self-assigned this Nov 14, 2022
@github-actions
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 mergable.
If you want to request a review or rebuild a package in copr, you can use following commands as a comment:

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

To launch regression testing public members of oamg organization can leave the following comment:

  • /rerun to schedule basic regression tests using this pr build and leapp-repository*master* as artifacts
  • /rerun 42 to schedule basic regression tests using this pr build and leapp-repository*PR42* as artifacts
  • /rerun-all to schedule all tests (including sst) using this pr build and leapp-repository*master* as artifacts
  • /rerun-all 42 to schedule all tests (including sst) using this pr build and leapp-repository*PR42* as artifacts

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 consider rerunning the CI by commenting leapp-ci build (might require several comments). If the problem persists, contact leapp-infra.

@r0x0d r0x0d force-pushed the integration-tests-with-packit branch from a77b3d5 to f603155 Compare November 14, 2022 17:11
@oamg oamg deleted a comment from packit-as-a-service bot Nov 14, 2022
@oamg oamg deleted a comment from packit-as-a-service bot Nov 14, 2022
@r0x0d
Copy link
Member Author

r0x0d commented Nov 14, 2022

Maybe some targets will fail right now because of:

  • This is the first time I'm trying this without my demo repository
  • Packit will deploy the latest changes to production tomorrow, so some workflows will fail until then

@r0x0d
Copy link
Member Author

r0x0d commented Nov 16, 2022

/packit test

.packit.yaml Outdated
@@ -54,7 +54,7 @@ jobs:
distros: [RHEL-7.9-ZStream]
identifier: tests-79to84
tmt_plan: "^(?!.*upgrade_plugin)(?!.*c2r)(?!.*sap)(?!.*8to9)(?!.*morf)"
tf_post_install_script: "#!/bin/sh\nsudo sed -i s/.*ssh-rsa/ssh-rsa/ /root/.ssh/authorized_keys;curl https://copr.fedorainfracloud.org/coprs/g/oamg/leapp/repo/epel-7/group_oamg-leapp-epel-7.repo --output /etc/yum.repos.d/copr_build.repo; yum install -y leapp*master*"
tf_post_install_script: "#!/bin/sh\nsudo sed -i s/.*ssh-rsa/ssh-rsa/ /root/.ssh/authorized_keys; yum install -y leapp-repository*master*"
Copy link
Member

@fernflower fernflower Nov 18, 2022

Choose a reason for hiding this comment

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

I remember our agreement about enabling tests with leapp-repositorymaster only now, but to have a plan for enabling testing with custom build from leapp-repository PR later.
I am just curious, how will this be done? Are we waiting for some feature to land in packit that will enable this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I think they implemented that already: packit/packit-service#1544

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I am wondering if executing /packit test <namespace>/<repo>#<pr_id> as recommended by packit will work fine if we have leapp-repositorymaster installation hardcoded as a tf_post_install_script..

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, to be honest, that was a super recent change they made that may not be 100% working right now. In any way, I'm hoping to test that feature here instead of my demo repository, I figured that it would be nicer to have actual (real) results instead of the made up ones I have in my repo.

Since they included that command especially because we asked, I don't mind go to them and ask for an update on that if we see that the behavior is incorrect.

.packit.yaml Outdated Show resolved Hide resolved
@packit-as-a-service
Copy link

Failed to load packit config file:

Cannot parse package config: ValidationError({'jobs': {0: {'trigger': ['Invalid enum member comment']}, 2: {'trigger': ['Invalid enum member comment']}, 3: {'trigger': ['Invalid enum member comment']}, 4: {'trigger': ['Invalid enum member comment']}, 5: {'trigger': ['Invalid enum member comment']}, 6: {'trigger': ['Invalid enum member comment']}, 7: {'trigger': ['Invalid enum member comment']}, 8: {'trigger': ['Invalid enum member comment']}, 9: {'trigger': ['Invalid enum member comment']}}}).

For more info, please check out the documentation or contact the Packit team.

@r0x0d r0x0d force-pushed the integration-tests-with-packit branch from 1410943 to 15c6b6f Compare November 18, 2022 14:15
@r0x0d
Copy link
Member Author

r0x0d commented Nov 18, 2022

/packit test oamg/leapp-repository#master

@r0x0d
Copy link
Member Author

r0x0d commented Nov 18, 2022

/packit test oamg/leapp-repository#989

@r0x0d
Copy link
Member Author

r0x0d commented Nov 18, 2022

/packit test oamg/leapp-repository#989

@r0x0d
Copy link
Member Author

r0x0d commented Nov 18, 2022

/packit test oamg/leapp-repository#988

@r0x0d
Copy link
Member Author

r0x0d commented Nov 25, 2022

/packit test oamg/leapp-repository#990

@r0x0d
Copy link
Member Author

r0x0d commented Dec 6, 2022

/packit test oamg/leapp-repository#990

@r0x0d
Copy link
Member Author

r0x0d commented Dec 8, 2022

/packit test

@r0x0d
Copy link
Member Author

r0x0d commented Dec 8, 2022

/packit test oamg/leapp-repository#990

1 similar comment
@r0x0d
Copy link
Member Author

r0x0d commented Dec 8, 2022

/packit test oamg/leapp-repository#990

@r0x0d
Copy link
Member Author

r0x0d commented Dec 8, 2022

@fernflower the discovery of the rhel-7.9-rhui tests are failing right now because of this comparison distro == rhel-7.9, I think.

packit is passing the distro name in a slightly differently way --context distro=RHEL-7.9-rhui. If I manually change to when: distro == RHEL-7.9-rhui it works just fine.

I'm not sure if packit offers a way to pass a different context/distro name in the request. @TomasTomecek could you help us here?

If that's not possible, should we change the condition in the test then, @fernflower?

@FrNecas
Copy link

FrNecas commented Dec 12, 2022

Hello @r0x0d , I first thought that this is a problem with the -rhui suffix, however it turns out that it is a problem with how fmf/tmt evaluate context. Currently, it is case-sensitive (this is undocumented), consider the following plan:

execute:
    how: tmt
    script: /bin/true
enabled: false
adjust:
    enabled: true
    when: distro == rhel-7.9

With tmt -c distro="rhel-7.9-rhui" run, the plan runs, whereas with tmt -c distro="RHEL-7.9-rhui" run it does not run.

I've filed an issue for fmf: teemtee/fmf#186 and we are going to dicuss this tomorrow at tmt hacking session with other tmt contributors. We may add some form of case-insensitivity to context matching. For packit, we also have packit/packit-service#1797 to provide custom context, however I do not think that passing a custom distro will/should be supported. The distro value depends on what you have in target in your config (and this is passed to Testing Farm as compose), it IMO does not make much sense to override this.

You could try using rhel-7.9-rhui as your target in the config but I am not sure if that will work, it depends on how Testing Farm matches composes (if it is case-sensitive or not). Alternatively, you could use a workaround such as when: distro == rhel-7.9 or distro == RHEL-7.9 in the meantime (before fmf is extended).

@r0x0d
Copy link
Member Author

r0x0d commented Dec 12, 2022

Hello @r0x0d , I first thought that this is a problem with the -rhui suffix, however it turns out that it is a problem with how fmf/tmt evaluate context. Currently, it is case-sensitive (this is undocumented), consider the following plan:

execute:
    how: tmt
    script: /bin/true
enabled: false
adjust:
    enabled: true
    when: distro == rhel-7.9

With tmt -c distro="rhel-7.9-rhui" run, the plan runs, whereas with tmt -c distro="RHEL-7.9-rhui" run it does not run.

I've filed an issue for fmf: teemtee/fmf#186 and we are going to dicuss this tomorrow at tmt hacking session with other tmt contributors. We may add some form of case-insensitivity to context matching. For packit, we also have packit/packit-service#1797 to provide custom context, however I do not think that passing a custom distro will/should be supported. The distro value depends on what you have in target in your config (and this is passed to Testing Farm as compose), it IMO does not make much sense to override this.

You could try using rhel-7.9-rhui as your target in the config but I am not sure if that will work, it depends on how Testing Farm matches composes (if it is case-sensitive or not). Alternatively, you could use a workaround such as when: distro == rhel-7.9 or distro == RHEL-7.9 in the meantime (before fmf is extended).

Hi, @FrNecas! Thank you for your reply and for all that explanation.

I will make sure that I follow both of those issues to keep updated with the current state of them. Also, for the workaround part, I will leave that decision to the @oamg/qe to see if it really fits in their use case or if they would like to go with some other route too. In any case, thanks again, I will wait a bit more for teemtee/fmf#186.

@fernflower
Copy link
Member

@fernflower the discovery of the rhel-7.9-rhui tests are failing right now because of this comparison distro == rhel-7.9, I think.

packit is passing the distro name in a slightly differently way --context distro=RHEL-7.9-rhui. If I manually change to when: distro == RHEL-7.9-rhui it works just fine.

I'm not sure if packit offers a way to pass a different context/distro name in the request. @TomasTomecek could you help us here?

If that's not possible, should we change the condition in the test then, @fernflower?

Hi @r0x0d , if we change the condition in the test to 'distro == RHEL-7.9-rhui' then this test won't be run in case of a typical rhel-7.9 upgrade and we will need to duplicate the test, which is not really a favorable outcome because it becomes harder to maintain. It would be great to solve that on packit side imho, but of course if it doesn't work we can go back to discussing changes in the test.

@r0x0d
Copy link
Member Author

r0x0d commented Dec 12, 2022

@fernflower the discovery of the rhel-7.9-rhui tests are failing right now because of this comparison distro == rhel-7.9, I think.
packit is passing the distro name in a slightly differently way --context distro=RHEL-7.9-rhui. If I manually change to when: distro == RHEL-7.9-rhui it works just fine.
I'm not sure if packit offers a way to pass a different context/distro name in the request. @TomasTomecek could you help us here?
If that's not possible, should we change the condition in the test then, @fernflower?

Hi @r0x0d , if we change the condition in the test to 'distro == RHEL-7.9-rhui' then this test won't be run in case of a typical rhel-7.9 upgrade and we will need to duplicate the test, which is not really a favorable outcome because it becomes harder to maintain. It would be great to solve that on packit side imho, but of course if it doesn't work we can go back to discussing changes in the test.

Seems reasonable to me. Let's wait a few more to see the outcome in the linked issues, maybe the solution on their side will be quick.

@r0x0d
Copy link
Member Author

r0x0d commented Dec 13, 2022

@fernflower the discovery of the rhel-7.9-rhui tests are failing right now because of this comparison distro == rhel-7.9, I think.
packit is passing the distro name in a slightly differently way --context distro=RHEL-7.9-rhui. If I manually change to when: distro == RHEL-7.9-rhui it works just fine.
I'm not sure if packit offers a way to pass a different context/distro name in the request. @TomasTomecek could you help us here?
If that's not possible, should we change the condition in the test then, @fernflower?

Hi @r0x0d , if we change the condition in the test to 'distro == RHEL-7.9-rhui' then this test won't be run in case of a typical rhel-7.9 upgrade and we will need to duplicate the test, which is not really a favorable outcome because it becomes harder to maintain. It would be great to solve that on packit side imho, but of course if it doesn't work we can go back to discussing changes in the test.

@fernflower just a follow-up on that, me and @FrNecas were talking about this issue today, and we thought that leapp would benefit more from packit/packit-service#1797 as it will let us define custom contexts are you guys have currently in your GitHub actions testing farm workflow.

The main breakdown on all of this is that tmt will stick with being case-sensitive and will document that for future uses. So, looking for alternatives, we realized that waiting and going for packit/packit-service#1797 would be more clean and suitable for leapp right now.

@r0x0d
Copy link
Member Author

r0x0d commented Jan 13, 2023

Waiting for packit/packit-service#1797

This pull request introduces the execution of leapp integration tests as
a packit job instead of the current behavior of using a GitHub Actions
to trigger the tests by a comment.

Signed-off-by: Rodolfo Olivieri <[email protected]>
@fernflower
Copy link
Member

/packit test

@fernflower
Copy link
Member

Okay, looks like installation of leapp-repository via postinstall script doesn't work - the package gets removed during guest setup artifacts' installation :(
So we are back to figuring out how to build and install specific version (master by default) of leapp-repository.
I am gonna try the /packit test oamg/leapp-repository#master and then maybe reach out to packit team and reopen this discussion.

@fernflower
Copy link
Member

/packit test oamg/leapp-repository#master

@fernflower
Copy link
Member

/packit build oamg/leapp-repository#master

@fernflower
Copy link
Member

/packit test oamg/leapp-repository#1052

.packit.yaml Outdated Show resolved Hide resolved
@fernflower
Copy link
Member

/packit test oamg/leapp-repository#1052

.packit.yaml Outdated Show resolved Hide resolved
@fernflower
Copy link
Member

fernflower commented Mar 22, 2023

Short summary:
Bad news - leapp packit test runs will be complicated until packit/packit-service#1968 lands. As there is no leapp -> leapp-repository dependency, and /packit test is only capable of installing the-leapp-build-from-pr in the test environment, which is not enough for our tests. It is possible to use /packit test oamg/leapp-repository#SOME_LEAPP_REPOSITORY_PR_THAT_STILL_HAS_BUILD_ALIVE to install both leapp and leapp-repository in the test env, but it has the drawback that you have to make sure yourself that the selected leapp-repository build is indeed available as packit won't rebuild anything in case it's not.
When the issue #1968 is resolved, it will be possible to run the tests properly with /packit test oamg/leapp-repository#master.
Good news - the approach in this PR can be used for testing leapp-repository already, as there is a leapp-repository -> leapp dependency.

@r0x0d r0x0d force-pushed the integration-tests-with-packit branch 2 times, most recently from 630ac68 to 6ff55fe Compare March 22, 2023 12:42
Signed-off-by: Rodolfo Olivieri <[email protected]>
@r0x0d r0x0d force-pushed the integration-tests-with-packit branch from 6ff55fe to e1ef75b Compare March 22, 2023 12:43
@r0x0d r0x0d requested a review from fernflower March 22, 2023 12:45
@fernflower
Copy link
Member

/packit test oamg/leapp-repository#1052

@fernflower
Copy link
Member

/packit test oamg/leapp-repository#1052

@fernflower
Copy link
Member

/packit test oamg/leapp-repository#1052

@fernflower
Copy link
Member

/packit test oamg/leapp-repository#1052

@fernflower
Copy link
Member

/packit test oamg/leapp-repository#1052

@fernflower
Copy link
Member

Moving this to draft not to be accidentally merged until packit/packit-service#1972 is delivered.

@fernflower fernflower marked this pull request as draft April 3, 2023 08:13
@fernflower fernflower marked this pull request as ready for review January 18, 2024 10:36
@fernflower fernflower marked this pull request as draft January 18, 2024 10:37
@fernflower
Copy link
Member

We have finally got all the necessary features from tft to be able to install leapp in test env for packit-triggered tests! So #827 has this PR's commits with some updates on top and should be merged soon.
Thanks a lot for doing the most important 80% of packit adoption!

@fernflower fernflower closed this Jan 18, 2024
@r0x0d r0x0d deleted the integration-tests-with-packit branch January 19, 2024 01:35
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.

4 participants