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.

Sign up for GitHub

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

Jump to bottom

Refactor testiso by breaking tests and adding denylist #3298

Merged
merged 2 commits into from Feb 28, 2023
Merged

Refactor testiso by breaking tests and adding denylist #3298

merged 2 commits into from Feb 28, 2023

Conversation

ravanelli
Copy link
Member

@ravanelli ravanelli commented Dec 23, 2022 •
edited

Fix #3177

Refactor testiso tests

  • This is an initial step in order to try to merge kola tests and testiso,
    much more work will be needed to create a final solution. Nonetheless,
    for now let's start by breaking the tests and adding the same denylist
    for both.

  • kola testiso now allows:

    • multipath, 4k, iso, pxe and firmware explicit tests;
    • Denylist for the pattern above and by test.
      Example:
      Add the following in kola-denylist.yaml to deny it:
    • pattern: iso-as-disk.*
    • pattern: iso-as-iso-live-login.*.4k
    • pattern: iso-offline-install.ppcfw.*

jlebon
jlebon reviewed Jan 3, 2023
View reviewed changes
mantle/kola/harness.go Outdated Show resolved Hide resolved
mantle/cmd/kola/testiso.go Outdated Show resolved Hide resolved
mantle/cmd/kola/testiso.go Outdated Show resolved Hide resolved
mantle/cmd/kola/testiso.go Outdated Show resolved Hide resolved
mantle/cmd/kola/testiso.go Outdated Show resolved Hide resolved
mantle/cmd/kola/testiso.go Outdated Show resolved Hide resolved
mantle/cmd/kola/testiso.go Outdated Show resolved Hide resolved
mantle/cmd/kola/testiso.go Outdated Show resolved Hide resolved
mantle/cmd/kola/testiso.go Outdated Show resolved Hide resolved
@ravanelli ravanelli force-pushed the kola_testiso branch 3 times, most recently from f8e26f3 to 5f1f7a8 Compare January 6, 2023 17:39
@ravanelli
Copy link
Member Author

@jlebon The only missing pattern is when we have "skip all" at the first field *.bios.4k, seems thekola-denylist.yaml schema doesn't allow it. I will need to dig more into it

dustymabe
dustymabe reviewed Jan 9, 2023
View reviewed changes
Copy link
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

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

A lot going on here. Sorry for so many comments.

mantle/cmd/kola/testiso.go Show resolved Hide resolved
mantle/cmd/kola/testiso.go Outdated Show resolved Hide resolved
mantle/cmd/kola/testiso.go Outdated Show resolved Hide resolved
mantle/cmd/kola/testiso.go Outdated Show resolved Hide resolved
mantle/cmd/kola/testiso.go Outdated Show resolved Hide resolved
mantle/cmd/kola/testiso.go Outdated Show resolved Hide resolved
mantle/cmd/kola/testiso.go Outdated Show resolved Hide resolved
mantle/cmd/kola/testiso.go Outdated Show resolved Hide resolved
mantle/cmd/kola/testiso.go Outdated Show resolved Hide resolved
mantle/cmd/kola/testiso.go Outdated Show resolved Hide resolved
jlebon
jlebon reviewed Jan 10, 2023
View reviewed changes
Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

I'll let Dusty's comments get addressed before doing another full review.

mantle/cmd/kola/testiso.go Outdated Show resolved Hide resolved
mantle/cmd/kola/testiso.go Outdated Show resolved Hide resolved
jlebon
jlebon reviewed Jan 10, 2023
View reviewed changes
mantle/cmd/kola/testiso.go Outdated Show resolved Hide resolved
@ravanelli ravanelli force-pushed the kola_testiso branch 3 times, most recently from 6b4dace to cd7ac6b Compare January 11, 2023 14:46
@dustymabe
Copy link
Member

2 global comments:

  • Add a blank line in your commits so there is separate from the title (first line) and the body (after the blank line)
  • Run gofmt -w file1 file2 on the changed files in the PR.

jlebon
jlebon reviewed Jan 11, 2023
View reviewed changes
mantle/cmd/kola/testiso.go Outdated Show resolved Hide resolved
@ravanelli ravanelli force-pushed the kola_testiso branch 2 times, most recently from 6ae6d9f to 9c0e1bb Compare January 12, 2023 14:41
@ravanelli
Copy link
Member Author

Now, should we go ahead and change the kolaTestIso in coreos-ci-lib? Or try to keep old/new style?

dustymabe
dustymabe previously approved these changes Jan 12, 2023
View reviewed changes
Copy link
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

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

LGTM - a few last suggestions

mantle/cmd/kola/testiso.go Outdated Show resolved Hide resolved
mantle/cmd/kola/testiso.go Outdated Show resolved Hide resolved
mantle/cmd/kola/testiso.go Outdated Show resolved Hide resolved
@dustymabe
Copy link
Member

Now, should we go ahead and change the kolaTestIso in coreos-ci-lib? Or try to keep old/new style?

Definitely something we're going to have to figure out before we merge this. Maybe we should get together to discuss the implications and best path forward.

cgwalters
cgwalters reviewed Jan 12, 2023
View reviewed changes
Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

I only gave this a superficial skim, but I like the direction!

@ravanelli @jlebon
Make functions public accessible to all other packages
1139dff 
 - Make hasString and parseDenyListYaml public accessible, in order
to use it as part of testiso too.

Signed-off-by: Renata Ravanelli <rravanel@redhat.com>
@ravanelli @jlebon
kola: Refactor testiso tests
36171de 
 - This is an initial step in order to try to merge kola tests and testiso,
much more work will be needed to create a final solution. Nonetheless,
for now let's start by breaking the tests and adding the same denylist
for both.

 - kola testiso now allows:
   - multipath, 4k, iso, pxe and firmware explicit tests;
   - Denylist for the pattern above and by test.
   Example:
    Add the following patterns in `kola-denylist.yaml` to deny it:
    - pattern: iso-as-disk.*
    - pattern: iso-as-iso-live-login.*.4k
    - pattern: iso-offline-install.ppcfw.*

Signed-off-by: Renata Andrade Matos Ravanelli <rravanel@redhat.com>
@jlebon jlebon dismissed stale reviews from dustymabe and themself via 36171de February 16, 2023 17:13
jlebon
jlebon approved these changes Feb 16, 2023
View reviewed changes
Copy link
Member

@jlebon jlebon left a comment •
edited

Choose a reason for hiding this comment

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

Rebased to fix trivial conflict! There's one last piece missing and otherwise I think we're ready to go! To summarize, all the following PRs will be merged at the same time:

Discussed this with @ravanelli. To lower the risk, we're going to do this early next week, after the FCOS releases are built.

@jlebon
Copy link
Member

jlebon commented Feb 21, 2023

Restarted CoreOS CI, which failed on a flake. Prow failure is unrelated, but actually we will also need to update https://github.com/openshift/os/blob/b7f60fc65851cdb01e21edfbcf0213d3aac4eebf/ci/prow-entrypoint.sh#L136-L149 as part of this.

/test rhcos

jlebon added a commit to jlebon/os that referenced this pull request Feb 21, 2023
@jlebon
ci/prow-entrypoint: simplify kola testiso invocation
b67ab07 
We've reworked `kola testiso` so that things like BIOS, UEFI, Secure
Boot, multipath, metal4k variants are tested by default when you just
call `kola testiso`:

coreos/coreos-assembler#3298

This will increase the number of tests this code currently runs. We can
always trim it back down with `--denylist-test` in the future if wanted.
@jlebon jlebon mentioned this pull request Feb 21, 2023
Merged
@jlebon
Copy link
Member

jlebon commented Feb 21, 2023

Opened openshift/os#1172 and added it to #3298 (review).

@jlebon
Copy link
Member

jlebon commented Feb 27, 2023

/retest

@jlebon
Copy link
Member

jlebon commented Feb 28, 2023

✔️ continuous-integration/jenkins/pr-merge — This commit looks good

🎉

Let's merge this so that we can sanity-check it in the pipelines and then merge the backports.

Nice work @ravanelli!

@jlebon jlebon merged commit f98481f into coreos:main Feb 28, 2023
ravanelli added a commit that referenced this pull request Feb 28, 2023
@ravanelli
kola: Refactor testiso tests
febe4a9 
 - This is an initial step in order to try to merge kola tests and testiso,
much more work will be needed to create a final solution. Nonetheless,
for now let's start by breaking the tests and adding the same denylist
for both.

 - kola testiso now allows:
   - multipath, 4k, iso, pxe and firmware explicit tests;
   - Denylist for the pattern above and by test.
   Example:
    Add the following patterns in `kola-denylist.yaml` to deny it:
    - pattern: iso-as-disk.*
    - pattern: iso-as-iso-live-login.*.4k
    - pattern: iso-offline-install.ppcfw.*

This patch is a backport of: #3298

Backport:
  * Patch refresh - no functional change.

Signed-off-by: Renata Ravanelli <rravanel@redhat.com>
ravanelli added a commit that referenced this pull request Feb 28, 2023
@ravanelli
kola: Refactor testiso tests
e73c569 
 - This is an initial step in order to try to merge kola tests and testiso,
much more work will be needed to create a final solution. Nonetheless,
for now let's start by breaking the tests and adding the same denylist
for both.

 - kola testiso now allows:
   - multipath, 4k, iso, pxe and firmware explicit tests;
   - Denylist for the pattern above and by test.
   Example:
    Add the following patterns in `kola-denylist.yaml` to deny it:
    - pattern: iso-as-disk.*
    - pattern: iso-as-iso-live-login.*.4k
    - pattern: iso-offline-install.ppcfw.*

This patch is a backport of: #3298

Backport:
  * Patch refresh - no functional change.

Signed-off-by: Renata Ravanelli <rravanel@redhat.com>
@bgilbert
Copy link
Contributor

bgilbert commented Mar 1, 2023

It seems this broke coreos-installer upstream CI by removing --add-nm-keyfile. What functionality should we be using instead of that option?

ravanelli added a commit that referenced this pull request Mar 1, 2023
@ravanelli
Make functions public accessible to all other packages
2eab8ab 
This patch is a backport of #3298

Backport:
* Patch refresh - no functional change.

Signed-off-by: Renata Ravanelli <rravanel@redhat.com>
ravanelli added a commit that referenced this pull request Mar 1, 2023
@ravanelli
kola: Refactor testiso tests
a522c22 
 - This is an initial step in order to try to merge kola tests and testiso,
much more work will be needed to create a final solution. Nonetheless,
for now let's start by breaking the tests and adding the same denylist
for both.

 - kola testiso now allows:
   - multipath, 4k, iso, pxe and firmware explicit tests;
   - Denylist for the pattern above and by test.
   Example:
    Add the following patterns in `kola-denylist.yaml` to deny it:
    - pattern: iso-as-disk.*
    - pattern: iso-as-iso-live-login.*.4k
    - pattern: iso-offline-install.ppcfw.*

This patch is a backport of: #3298

Backport:
  * Patch refresh - no functional change.

Signed-off-by: Renata Ravanelli <rravanel@redhat.com>
c4rt0
c4rt0 reviewed Mar 1, 2023
View reviewed changes
Copy link
Member

@c4rt0 c4rt0 left a comment •
edited

Choose a reason for hiding this comment

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

edited... just looking at code.

jlebon pushed a commit that referenced this pull request Mar 1, 2023
@ravanelli @jlebon
Make functions public accessible to all other packages
067be27 
This patch is a backport of #3298

Backport:
* Patch refresh - no functional change.

Signed-off-by: Renata Ravanelli <rravanel@redhat.com>
jlebon pushed a commit that referenced this pull request Mar 1, 2023
@ravanelli @jlebon
kola: Refactor testiso tests
6a7a6f5 
 - This is an initial step in order to try to merge kola tests and testiso,
much more work will be needed to create a final solution. Nonetheless,
for now let's start by breaking the tests and adding the same denylist
for both.

 - kola testiso now allows:
   - multipath, 4k, iso, pxe and firmware explicit tests;
   - Denylist for the pattern above and by test.
   Example:
    Add the following patterns in `kola-denylist.yaml` to deny it:
    - pattern: iso-as-disk.*
    - pattern: iso-as-iso-live-login.*.4k
    - pattern: iso-offline-install.ppcfw.*

This patch is a backport of: #3298

Backport:
  * Patch refresh - no functional change.

Signed-off-by: Renata Ravanelli <rravanel@redhat.com>
@jlebon
Copy link
Member

jlebon commented Mar 1, 2023

It seems this broke coreos-installer upstream CI by removing --add-nm-keyfile. What functionality should we be using instead of that option?

Submitted PRs to upstream CIs that used non-default options in:

@jlebon jlebon mentioned this pull request Mar 3, 2023
Closed
@jlebon jlebon mentioned this pull request Mar 31, 2023
Closed
@travier travier mentioned this pull request Apr 4, 2023
Closed
openshift-cherrypick-robot pushed a commit to openshift-cherrypick-robot/os that referenced this pull request Apr 4, 2023
@jlebon
ci/prow-entrypoint: simplify kola testiso invocation
d653202 
We've reworked `kola testiso` so that things like BIOS, UEFI, Secure
Boot, multipath, metal4k variants are tested by default when you just
call `kola testiso`:

coreos/coreos-assembler#3298

This will increase the number of tests this code currently runs. We can
always trim it back down with `--denylist-test` in the future if wanted.
openshift-cherrypick-robot pushed a commit to openshift-cherrypick-robot/os that referenced this pull request Apr 4, 2023
@jlebon
ci/prow-entrypoint: simplify kola testiso invocation
1ccea91 
We've reworked `kola testiso` so that things like BIOS, UEFI, Secure
Boot, multipath, metal4k variants are tested by default when you just
call `kola testiso`:

coreos/coreos-assembler#3298

This will increase the number of tests this code currently runs. We can
always trim it back down with `--denylist-test` in the future if wanted.
c4rt0 pushed a commit to c4rt0/os that referenced this pull request May 11, 2023
@jlebon @c4rt0
ci/prow-entrypoint: simplify kola testiso invocation
e0ea55d 
We've reworked `kola testiso` so that things like BIOS, UEFI, Secure
Boot, multipath, metal4k variants are tested by default when you just
call `kola testiso`:

coreos/coreos-assembler#3298

This will increase the number of tests this code currently runs. We can
always trim it back down with `--denylist-test` in the future if wanted.

(cherry picked from commit b67ab07)
@ravanelli ravanelli deleted the kola_testiso branch June 1, 2023 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cgwalters cgwalters cgwalters left review comments

@c4rt0 c4rt0 c4rt0 left review comments

@jlebon jlebon jlebon approved these changes

@dustymabe dustymabe dustymabe left review comments

@travier travier Awaiting requested review from travier

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support denylist for kola testiso
7 participants
@ravanelli @dustymabe @jlebon @bgilbert @cgwalters @c4rt0 @openshift-merge-robot