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

https://issues.redhat.com/browse/ACM-16769 cleanupall fix #7683

Open
wants to merge 2 commits into
base: 2.13_stage
Choose a base branch
from

Conversation

ragevou
Copy link
Collaborator

@ragevou ragevou commented Apr 2, 2025

only for 2.13

Copy link

openshift-ci bot commented Apr 2, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ragevou

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

Needs approval from an approver in each of these files:

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

@@ -99,7 +99,11 @@ The post restore cleanup uses the `cleanupBeforeRestore` property to identify th

- `None`: No clean up necessary, just begin Velero restore. Use `None` on a brand new hub cluster.
- `CleanupRestored`: Clean up all resources created by a previous {acm-short} restore that are not part of the currently restored backup.
- `CleanupAll`: Clean up all resources on the hub cluster that might be part of a {acm-short} backup, even if they were not created as a result of a restore operation. This is to be used when extra content is created on a hub cluster before the restore operation starts.
- `CleanupAll`: Clean up all resources on the hub cluster that might be part of a {acm-short} backup, even if they were not created as a result of a restore operation.
+
Copy link
Contributor

Choose a reason for hiding this comment

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

The tagging here we don't use. This is in our internal doc, @jc-berger can you find those directions and update here. (It has to do with all caps, etc...; I have raised this with RH teams for accessibility and will continue.)

+
CAUTION: The `CleanupAll` option removes all hub cluster data, including the content that the user creates.
+
IMPORTANT: Always use the `cleanupBeforeRestore = None` to avoid losing restored resources that you filter out with the restore options `excludedResource`, `excludedNamespaces`, `excludedResources`, or other options that filter out the restored resources.
+
Copy link
Contributor

Choose a reason for hiding this comment

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

other content looks great. I will let Jake handle merge/approval since it's his area. Thanks for this!!

@@ -24,7 +24,7 @@ To fully restore a backup, complete the following:
[#restoring-backup-restore-operation]
== Using the restore operation for backup types

Use the restore operation to restore all 3 backup types that are created by the backup operation. You can choose to install only a certain type of backup, such as only managed clusters, only user credentials, or only hub cluster resources.
Use the restore operation to restore all three backup types that the backup operation creates. You can choose to install only a certain type of backup, such as only managed clusters, only user credentials, or only hub cluster resources.
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes!!! (This is style standards, TY for changing)

@swopebe
Copy link
Contributor

swopebe commented Apr 2, 2025

Please look at linter and main file to resolve before merging. Reach out if it is not obvious:

- `CleanupAll`: Clean up all resources on the hub cluster that might be part of a {acm-short} backup, even if they were not created as a result of a restore operation. This is to be used when extra content is created on a hub cluster before the restore operation starts.
- `CleanupAll`: Clean up all resources on the hub cluster that might be part of a {acm-short} backup, even if they were not created as a result of a restore operation.
+
CAUTION: The `CleanupAll` option removes all hub cluster data, including the content that the user creates.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
CAUTION: The `CleanupAll` option removes all hub cluster data, including the content that the user creates.
*Important:* The `CleanupAll` option removes all hub cluster data, including the content that the user creates.

+
CAUTION: The `CleanupAll` option removes all hub cluster data, including the content that the user creates.
+
IMPORTANT: Always use the `cleanupBeforeRestore = None` to avoid losing restored resources that you filter out with the restore options `excludedResource`, `excludedNamespaces`, `excludedResources`, or other options that filter out the restored resources.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
IMPORTANT: Always use the `cleanupBeforeRestore = None` to avoid losing restored resources that you filter out with the restore options `excludedResource`, `excludedNamespaces`, `excludedResources`, or other options that filter out the restored resources.
*Important:* Always use the `cleanupBeforeRestore = None` to avoid losing restored resources that you filter out with the restore options `excludedResource`, `excludedNamespaces`, `excludedResources`, or other options that filter out the restored resources.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ragevou thanks for your awesome PR with you insightful changes! As Brandi recommended, I made changes to remove the all caps. We tend to simply say "Important" iinstead of "caution" and I think it reads just as well to use "important' here.

If you commit these changes, then I can go ahead and merge, thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

@jc-berger I think you can make your change and merge this into your book.

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