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

test: Adjust stratis pool stop to stratis-cli 3.6.0 #19525

Merged
merged 1 commit into from
Oct 25, 2023

Conversation

martinpitt
Copy link
Member

The CLI changed in an incompatible way: Earlier versions just expected the pool name/UUID as positional argument, 3.6.0 now expects it through an --uuid or --name option.


This fixes the current rawhide failure.

@martinpitt martinpitt added release-blocker Targetted for next release no-test For doc/workflow changes, or experiments which don't need a full CI run, labels Oct 25, 2023
The CLI changed in an incompatible way: Earlier versions just expected
the pool name/UUID as positional argument, 3.6.0 now expects it through
an `--uuid` or `--name` option.
@martinpitt martinpitt changed the title test: Adjust stratis pool stopping/destroying to stratis-cli 3.6.0 test: Adjust stratis pool stop to stratis-cli 3.6.0 Oct 25, 2023
@martinpitt
Copy link
Member Author

Hmmkay, so stratis pool destroy did not change. How inconsistent..

@martinpitt martinpitt removed the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Oct 25, 2023
Copy link
Member

@allisonkarlitskaya allisonkarlitskaya left a comment

Choose a reason for hiding this comment

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

I think this is fine to go as it is, but if you'd like to move around some deck chairs, please see below.

@@ -36,12 +36,18 @@ class TestStorageStratis(storagelib.StorageCase):

self.stratis_v2 = self.image.startswith("rhel-8") or self.image == "centos-8-stream"

# the CLI changed in an incompatible way in Fedora 40
if '--name' in exe("stratis pool stop --help"):
Copy link
Member

Choose a reason for hiding this comment

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

At first my eyes popped out of my head, but then I said "oh, it's only a test".

@@ -36,12 +36,18 @@ class TestStorageStratis(storagelib.StorageCase):

self.stratis_v2 = self.image.startswith("rhel-8") or self.image == "centos-8-stream"

# the CLI changed in an incompatible way in Fedora 40
if '--name' in exe("stratis pool stop --help"):
self.stop_type_opt = "--name"
Copy link
Member

Choose a reason for hiding this comment

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

Maybe you could turn this info pool_stop_cmd and have stratis pool stop or stratis pool stop --name in there, accordingly. Would be a nice factor-out?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, can do. I initially even had this, then changed it to this because I thought "oh, stratis pool destroy is going to need that, too", only to find out that it doesn't..

Copy link
Member Author

Choose a reason for hiding this comment

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

On second thought, it may happen to other commands, this makes all PRs red, I actually find it nicer to read most of the command to see what's going on, and there's still so much other stuff to pilot. So I'll be so cheeky and land it as-is.

@martinpitt martinpitt merged commit 118d89a into cockpit-project:main Oct 25, 2023
47 checks passed
@martinpitt martinpitt deleted the stratis-cli branch October 25, 2023 13:38
@allisonkarlitskaya
Copy link
Member

This landed with an incomplete test slate and is causing storage tests to fail in other PRs. See https://cockpit-logs.us-east-1.linodeobjects.com/pull-19527-20231025-171100-5b8a565c-arch-storage/log.html#65-2 for example.

@martinpitt
Copy link
Member Author

Argh, sorry! → PR #19531

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-blocker Targetted for next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants