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

Snapshot support #1100

Merged
merged 6 commits into from
Oct 4, 2024
Merged

Conversation

mulkieran
Copy link
Member

@mulkieran mulkieran commented Sep 25, 2024

Related #1085

@mulkieran mulkieran self-assigned this Sep 25, 2024
@mulkieran
Copy link
Member Author

Expect test failures

@mulkieran
Copy link
Member Author

Expect all tests to pass

@mulkieran
Copy link
Member Author

Really this time...

@mulkieran
Copy link
Member Author

/packit build

Copy link

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

  • sudo yum install -y dnf-plugins-core on RHEL 8
  • sudo dnf install -y dnf-plugins-core on Fedora
  • dnf copr enable packit/stratis-storage-stratis-cli-1100
  • And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

Copy link
Member

@jbaublitz jbaublitz left a comment

Choose a reason for hiding this comment

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

Overall this looks good. The one thing I noticed in the PR is that we seem to create a new error for every error from the CLI when the D-Bus API returns an Identity result. Is this to specifically tie a good error message to each of these cases? Is there an advantage to not using the same error with different error messages as input? This is unrelated to the PR, but more something I noticed while reviewing this.

Signed-off-by: mulhern <[email protected]>
Just return the engine error if origin not set.
When trying to cancel a revert, and the revert has not been scheduled
because the filesystem has no origin, better to reject the cancel
action because of the missing origin, rather than because the merge
status will not be changed.

Signed-off-by: mulhern <[email protected]>
Signed-off-by: mulhern <[email protected]>
(cherry picked from commit 39c87a8)
The revised text for the "Snapshot origin" field now looks like this:

   Snapshot origin
       If this filesystem is a snapshot, its origin. If the filesystem is
       a snapshot, whether or not it is scheduled to replace its origin
       next time the pool is started.

Signed-off-by: mulhern <[email protected]>
@mulkieran mulkieran force-pushed the snapshot-support branch 2 times, most recently from 3a9e88e to fae582b Compare October 4, 2024 00:55
@mulkieran mulkieran marked this pull request as ready for review October 4, 2024 00:56
@mulkieran
Copy link
Member Author

Overall this looks good. The one thing I noticed in the PR is that we seem to create a new error for every error from the CLI when the D-Bus API returns an Identity result. Is this to specifically tie a good error message to each of these cases? Is there an advantage to not using the same error with different error messages as input? This is unrelated to the PR, but more something I noticed while reviewing this.

I think you're correct, that class inheritance tree looks unnecessarily bushy. We can prune it this autumn.

@mulkieran
Copy link
Member Author

Expecting tests to pass now...

@mulkieran mulkieran merged commit 9370aff into stratis-storage:rebase-3.6.0 Oct 4, 2024
11 checks passed
@mulkieran mulkieran deleted the snapshot-support branch October 4, 2024 03:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done(1)
Development

Successfully merging this pull request may close these issues.

4 participants