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

vars/kola: drop --basic-qemu-scenarios #154

Merged
merged 1 commit into from
Mar 11, 2024

Conversation

prestist
Copy link
Contributor

@prestist prestist commented Nov 8, 2023

The argument for --basic-qemu-scenarios has been removed. The tests invoked by it are now formal kola tests.

See coreos/coreos-assembler#3652

Copy link
Member

@ravanelli ravanelli left a comment

Choose a reason for hiding this comment

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

Also remove in line 10:

//    skipBasicScenarios  boolean  -- skip basic qemu scenarios

@ravanelli
Copy link
Member

@prestist
Copy link
Contributor Author

prestist commented Nov 9, 2023

// skipBasicScenarios boolean -- skip basic qemu scenarios

Ahhhh thank you :)

vars/kola.groovy Outdated
Comment on lines 120 to 125
// Check if kola has old or new basic scenarios
def out = shwrapCapture("""
cd ${cosaDir}
cosa kola list
""")
def newBasic.nvmeExist = out.readLines().any { it =~ /basic\.nvme/ }
Copy link
Member

Choose a reason for hiding this comment

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

Maybe something like:

Suggested change
// Check if kola has old or new basic scenarios
def out = shwrapCapture("""
cd ${cosaDir}
cosa kola list
""")
def newBasic.nvmeExist = out.readLines().any { it =~ /basic\.nvme/ }
// Check if kola has old or new basic scenarios
def availableTests = shwrapCapture("kola list --json | jq -r '.[].Name'").split()
def basicScenariosBuiltIn = "basic.nvme" in availableTests

?

vars/kola.groovy Outdated Show resolved Hide resolved
@prestist prestist force-pushed the make-kola-changes branch 3 times, most recently from 7e7a251 to d645ea8 Compare March 1, 2024 15:45
vars/kola.groovy Outdated Show resolved Hide resolved
vars/kola.groovy Outdated Show resolved Hide resolved
vars/kola.groovy Outdated Show resolved Hide resolved
vars/kola.groovy Outdated Show resolved Hide resolved
vars/kola.groovy Outdated Show resolved Hide resolved
vars/kola.groovy Outdated Show resolved Hide resolved
@prestist prestist force-pushed the make-kola-changes branch from d645ea8 to 2c140ed Compare March 5, 2024 15:59
vars/kola.groovy Outdated
if (basicScenariosBuiltIn) {
if (params['skipSecureBoot']) {
// skip secureboot tests using kola's denylist
runKola(id, 'run', "--denylist-test *.uefi-secure")
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't run kola here, just e.g. modify args to add the --denylist-test flag.

vars/kola.groovy Show resolved Hide resolved
vars/kola.groovy Outdated
Comment on lines 123 to 124
id = marker == "" ? "kola-basic" : "kola-basic-${marker}"
ids += id
Copy link
Member

Choose a reason for hiding this comment

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

This should only be done if we actually do a separate basic scenarios kola run (so in the else block below).

@prestist prestist force-pushed the make-kola-changes branch 2 times, most recently from 589b0c9 to f3ab9b5 Compare March 8, 2024 16:28
@prestist
Copy link
Contributor Author

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.

One final comment, but LGTM otherwise.

Build with new cosa => jenkins-fedora-coreos-pipeline.apps.ocp.stg.fedoraproject.org/job/build/132

That build no-op'ed so we didn't actually get to see the new logic here kick in in the new cosa case. If you've verified that path works already in another build, WFM.

vars/kola.groovy Outdated
def skipSecureBootArg = ""
// Check if the basic tests are registered kola tests or not.
def availableTests = shwrapCapture("kola list --json | jq -r '.[].Name'").split()
// This essentually checks if the running has https://github.com/coreos/coreos-assembler/pull/3652
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
// This essentually checks if the running has https://github.com/coreos/coreos-assembler/pull/3652
// This essentially checks if the running cosa has https://github.com/coreos/coreos-assembler/pull/3652

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow, I guess my spell check is not working :? anyways thank you for seeing that, we are lucky I did not have more lol, I depend on my spellcheck a bit too much.

@prestist
Copy link
Contributor Author

prestist commented Mar 11, 2024

One final comment, but LGTM otherwise.

Build with new cosa => jenkins-fedora-coreos-pipeline.apps.ocp.stg.fedoraproject.org/job/build/132

That build no-op'ed so we didn't actually get to see the new logic here kick in in the new cosa case. If you've verified that path works already in another build, WFM.

The new logic was tested here there have been updates to the other branch of logic (old style) since this though. So I will replay with a force this time, to be sure;

The argument for --basic-qemu-scenarios has been deprecated.
Depending on the version of cosa, the tests that would be skipped by these args are
now formal kola tests. And to skip them you can use --denylist-test commands.

skipSecureBoot now translates to  --denylist-test *.uefi-secure
@prestist prestist force-pushed the make-kola-changes branch from f3ab9b5 to ed73e05 Compare March 11, 2024 20:04
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.

LGTM! Feel free to merge when ready.

@prestist prestist merged commit cf8e6a1 into coreos:main Mar 11, 2024
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.

3 participants