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

BMO: Make CAPM3 e2e tests optional #707

Merged

Conversation

lentzi90
Copy link
Member

The BMO e2e tests have proven themselves over several months now. They are on par with the CAPM3 tests regarding the BMO features they test and they do it faster and more efficiently. By requiring both CAPM3 and BMO e2e tests to run on all BMO PRs we make the PR process unnessarily heavy, time consuming and resource intense.

This commit is for making the last remaining CAPM3 e2e test optional on BMO PRs. Note that CAPM3 tests can still be triggered if wanted, they will just not be required. They are also still running as daily jobs so we will quickly notice if something breaks.

I think metal3-ubuntu-e2e-integration-test-main was mistakenly set to required when switching to prow triggered jobs. This has not been required in BMO before.

@metal3-io-bot metal3-io-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Apr 22, 2024
@lentzi90
Copy link
Member Author

/hold
Let's check with the community that this is ok before we merge.

@metal3-io-bot metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 22, 2024
@mboukhalfa
Copy link
Member

ubuntu was set as required after some local discussion to run both os on all main PRs ! After this PR get merged we should remove these from gh branch protection as well

@mboukhalfa
Copy link
Member

/approve

@metal3-io-bot metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 22, 2024
@metal3-io-bot metal3-io-bot added the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Apr 22, 2024
@mboukhalfa
Copy link
Member

Does BMO tests test the old release of BMO (and later ironic) with the same main test or we need to create release tests like we do in e2e integration ?

@lentzi90 lentzi90 force-pushed the lentzi90/e2e-only-required-bmo branch from 79ccf75 to e78aee5 Compare April 24, 2024 05:45
@metal3-io-bot metal3-io-bot removed the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Apr 24, 2024
@lentzi90
Copy link
Member Author

Does BMO tests test the old release of BMO (and later ironic) with the same main test or we need to create release tests like we do in e2e integration ?

Yes, BMO e2e tests can be used to test PRs to any release branch where the tests exists. This means from 0.5.
I'm not sure what you mean with "and later ironic"? The tests will use the Ironic version that is defined in the repo. For example for release-0.6 that means ironic:release-24.1. If the tag is changed in a PR, that same PR would also then test the new version.

@mboukhalfa
Copy link
Member

Does BMO tests test the old release of BMO (and later ironic) with the same main test or we need to create release tests like we do in e2e integration ?

Yes, BMO e2e tests can be used to test PRs to any release branch where the tests exists. This means from 0.5. I'm not sure what you mean with "and later ironic"? The tests will use the Ironic version that is defined in the repo. For example for release-0.6 that means ironic:release-24.1. If the tag is changed in a PR, that same PR would also then test the new version.

I mean later if we want to run those tests on ironic would them be feasible to test ironic releases as well

@lentzi90
Copy link
Member Author

Test them from where and how? In BMO it is very easy to test multiple Ironic releases. We just add overlays and config for each that we want to test like here

@mboukhalfa
Copy link
Member

Test them from where and how? In BMO it is very easy to test multiple Ironic releases. We just add overlays and config for each that we want to test like here

yeah I understand from BMO side. I am thinking if we can run these test to test ironic-image PRs if so will these will also help test releases ! mostly thinking about a roadmap to drop integration e2e and replace them by bmo tests on ironic-image repo

@lentzi90
Copy link
Member Author

Ok I see. That will not be automatically possible and I strongly suggest that we do not include such functionality in BMO directly.
It can be done manually by building the image, pushing it to some registry and then run the BMO tests with that image.

If we need to automate more, I suggest we add some minimal e2e tests in the ironic-image repo directly or we put the ironic-image in the BMO repo directly.

Copy link
Member

@kashifest kashifest left a comment

Choose a reason for hiding this comment

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

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 25, 2024
The BMO e2e tests have proven themselves over several months now. They
are on par with the CAPM3 tests regarding the BMO features they test and
they do it faster and more efficiently. By requiring both CAPM3 and BMO
e2e tests to run on all BMO PRs we make the PR process unnessarily
heavy, time consuming and resource intense.

This commit is for making the last remaining CAPM3 e2e test optional on
BMO PRs. Note that CAPM3 tests can still be triggered if wanted, they
will just not be required. They are also still running as daily jobs so
we will quickly notice if something breaks.

Signed-off-by: Lennart Jern <lennart.jern@est.tech>
@lentzi90 lentzi90 force-pushed the lentzi90/e2e-only-required-bmo branch from e78aee5 to 07eab91 Compare April 25, 2024 12:03
@metal3-io-bot metal3-io-bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 25, 2024
@furkatgofurov7
Copy link
Member

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label May 2, 2024
Copy link
Member

@tuminoid tuminoid left a comment

Choose a reason for hiding this comment

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

/approve

Slowly moving to correct direction not centering everything in CAPM3 and dev-env. Execllent job with the BMO e2e @lentzi90 !

@metal3-io-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mboukhalfa, tuminoid

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [mboukhalfa,tuminoid]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@lentzi90
Copy link
Member Author

lentzi90 commented May 7, 2024

I have heard no objections, and there has been plenty of time to voice concerns, so I'm going to remove the hold and merge this. If we change our minds it is easy enough to revert later 🙂
/hold cancel

@metal3-io-bot metal3-io-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 7, 2024
@lentzi90
Copy link
Member Author

lentzi90 commented May 7, 2024

/override metal3-ubuntu-e2e-integration-test-main

@metal3-io-bot
Copy link
Collaborator

@lentzi90: Overrode contexts on behalf of lentzi90: metal3-ubuntu-e2e-integration-test-main

In response to this:

/override metal3-ubuntu-e2e-integration-test-main

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@metal3-io-bot metal3-io-bot merged commit 2a2095c into metal3-io:main May 7, 2024
5 checks passed
@metal3-io-bot
Copy link
Collaborator

@lentzi90: Updated the config configmap in namespace prow at cluster default using the following files:

  • key config.yaml using file prow/manifests/overlays/metal3/config.yaml

In response to this:

The BMO e2e tests have proven themselves over several months now. They are on par with the CAPM3 tests regarding the BMO features they test and they do it faster and more efficiently. By requiring both CAPM3 and BMO e2e tests to run on all BMO PRs we make the PR process unnessarily heavy, time consuming and resource intense.

This commit is for making the last remaining CAPM3 e2e test optional on BMO PRs. Note that CAPM3 tests can still be triggered if wanted, they will just not be required. They are also still running as daily jobs so we will quickly notice if something breaks.

I think metal3-ubuntu-e2e-integration-test-main was mistakenly set to required when switching to prow triggered jobs. This has not been required in BMO before.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@tuminoid tuminoid deleted the lentzi90/e2e-only-required-bmo branch May 7, 2024 06:35
@tuminoid
Copy link
Member

tuminoid commented May 7, 2024

Updated BMO branch protection to match.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants