Skip to content

OADP-4001 Self-Service #89043

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
Merged
michaelryanpeter merged 1 commit into from
May 28, 2025
Merged

OADP-4001 Self-Service #89043

michaelryanpeter merged 1 commit into from
May 28, 2025

Conversation

@shdeshpa07
Copy link
Contributor

@shdeshpa07 shdeshpa07 commented Feb 24, 2025 •
edited
Loading

Jira

OADP Self-Service feature

Version

  • OCP 4.19 only

Preview

QE Review

  • QE has approved this change.

@shdeshpa07
Copy link
Contributor Author

shdeshpa07 commented Feb 24, 2025

/label OADP

@openshift-ci openshift-ci bot added OADP Label for all OADP PRs size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 24, 2025
@ocpdocs-previewbot
Copy link

ocpdocs-previewbot commented Feb 24, 2025 •
edited
Loading

🤖 Mon May 26 06:43:43 - Prow CI generated the docs preview:

https://89043--ocpdocs-pr.netlify.app/

@openshift-ci openshift-ci bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 4, 2025
@openshift-ci openshift-ci bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 13, 2025
@shdeshpa07 shdeshpa07 force-pushed the OADP-4001-Self-Service branch from dc07fa0 to d87b95c Compare March 21, 2025 05:51
@shdeshpa07 shdeshpa07 closed this Mar 26, 2025
@shdeshpa07 shdeshpa07 reopened this Mar 26, 2025
@shdeshpa07 shdeshpa07 force-pushed the OADP-4001-Self-Service branch from e672e74 to 568298f Compare April 4, 2025 09:47
@shdeshpa07
Copy link
Contributor Author

shdeshpa07 commented Apr 4, 2025

@anarnold97 - Requesting your review please and thanks.

anarnold97
anarnold97 reviewed Apr 6, 2025
View reviewed changes
anarnold97
anarnold97 reviewed Apr 6, 2025
View reviewed changes
anarnold97
anarnold97 reviewed Apr 6, 2025
View reviewed changes
anarnold97
anarnold97 reviewed Apr 6, 2025
View reviewed changes
anarnold97
anarnold97 reviewed Apr 6, 2025
View reviewed changes
anarnold97
anarnold97 reviewed Apr 6, 2025
View reviewed changes
anarnold97
anarnold97 reviewed Apr 6, 2025
View reviewed changes
anarnold97
anarnold97 reviewed Apr 6, 2025
View reviewed changes
anarnold97
anarnold97 reviewed Apr 6, 2025
View reviewed changes
anarnold97
anarnold97 reviewed Apr 6, 2025
View reviewed changes
anarnold97
anarnold97 reviewed Apr 6, 2025
View reviewed changes
anarnold97
anarnold97 reviewed Apr 6, 2025
View reviewed changes
anarnold97
anarnold97 reviewed Apr 6, 2025
View reviewed changes
anarnold97
anarnold97 reviewed Apr 6, 2025
View reviewed changes
anarnold97
anarnold97 reviewed Apr 6, 2025
View reviewed changes
mail2nadeem92
mail2nadeem92 reviewed May 14, 2025
View reviewed changes
modules/oadp-self-service-enabling-nabsl-approval.adoc Outdated

The `NonAdminBackupStorageLocation` (NABSL) CR administrator approval workflow is an opt-in feature. As a cluster administrator, you must explicitly enable the feature in the `DataProtectionApplication` (DPA) CR by setting the `nonAdmin.requireApprovalForBSL` field to `true`.

You also need to set the `noDefaultBackupLocation` field in the DPA to `true`. This setting indicates that, there is no default backup storage location configured in the DPA and the namespace admin user can create a NABSL CR and send the CR request for approval.
Copy link

@mail2nadeem92 mail2nadeem92 May 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

In my opinion, setting the noDefaultBackupLocation field to true in the DPA is optional in this context. To make that clearer, consider rephrasing this section to reflect that. For example

The noDefaultBackupLocation field can be optionally set to true only if the cluster admin does not wish to configure a default backup storage location (BSL) in the DPA. When this is set, namespace admin must explicitly create a NABSL CR and submit it for approval.

If a default BSL is desired, the cluster admin may configure it and omit setting noDefaultBackupLocation to true. In this case, namespace admin can still request their own NABSLs with approval, or use the default BSL if permitted.
~~~~~~~~~~~~~~~~~~~~~~

@weshayutin  @shubham-pampattiwar  WDYT ^^ ?

@xenolinux xenolinux added peer-review-in-progress Signifies that the peer review team is reviewing this PR and removed peer-review-needed Signifies that the peer review team needs to review this PR labels May 15, 2025
@xenolinux
Copy link
Contributor

xenolinux commented May 15, 2025

Taking this XXL to review between today and tomorrow IST EOD.

xenolinux
xenolinux reviewed May 16, 2025
View reviewed changes
Copy link
Contributor

@xenolinux xenolinux left a comment

Choose a reason for hiding this comment

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

Provided the peer review comments/changes. Overall it looks really great! Very nice job putting this all together.

Some general comments about consistency:

  • The object names: Velero and velero are used inconsistent in the PR and in the repo too. So I am not sure what is the correct form. Should there be back ticks and the lowercase letter v -- velero?
  • I am wondering how is the resource name and its abbreviation used consistently; one example NonAdminController (NAC) CR. Is the full CR name specified for its first instance and then for the later instances is the abbreviation (NAC) used? OR both are used interchangeably?
  • Should admin from “namespace admin user.” be in back ticks? Consider keeping either of it consistently throughout the PR.

@xenolinux xenolinux added peer-review-done Signifies that the peer review team has reviewed this PR and removed peer-review-in-progress Signifies that the peer review team is reviewing this PR labels May 16, 2025
@shdeshpa07
Copy link
Contributor Author

shdeshpa07 commented May 20, 2025

Provided the peer review comments/changes. Overall it looks really great! Very nice job putting this all together.

Some general comments about consistency:

  • The object names: Velero and velero are used inconsistent in the PR and in the repo too. So I am not sure what is the correct form. Should there be back ticks and the lowercase letter v -- velero?
  • I am wondering how is the resource name and its abbreviation used consistently; one example NonAdminController (NAC) CR. Is the full CR name specified for its first instance and then for the later instances is the abbreviation (NAC) used? OR both are used interchangeably?
  • Should admin from “namespace admin user.” be in back ticks? Consider keeping either of it consistently throughout the PR.

@xenolinux - Thank you so much for taking this time to review an XXL sized PR. I know it must have taken up a lot of your time. I always like your reviews, Servesha. They are very thorough and with explanations. I really appreciate you patiently reviewing each and every line and providing suggestions.

My PRs always come out cleaner after your reviews. I have applied all your suggestions. Just a couple of places where need to verify with SMEs, and the places where I need to remove extra lines.

Thanks again Servesha for your time, patience, and effort!

cc: @kalexand-rh - I can't thank Servesha enough for her very meticulous and detailed review :)

@shdeshpa07
OADP-4001 Self-Service
7c5ea32 
Signed-off-by: Shruti Deshpande <shdeshpa@redhat.com>
@shdeshpa07 shdeshpa07 force-pushed the OADP-4001-Self-Service branch from bf1f18e to 7c5ea32 Compare May 26, 2025 06:34
@openshift-ci
Copy link

openshift-ci bot commented May 26, 2025

@shdeshpa07: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@shdeshpa07
Copy link
Contributor Author

shdeshpa07 commented May 26, 2025 •
edited
Loading

/label merge-review-needed

@ merge reviewer, As this is a huge PR, I would like to get the PR merge-reviewed and merged. I have commented-out the topic_map.yml file entry for this feature. After this PR gets merged, I will un-comment the topic_map.yml lines and create another PR and get that merged on the date of GA.

I discussed this approach with @kalexand-rh in a group chat with my content strategist and manager, and everyone agrees to the approach.

Thanks.

@shdeshpa07
Copy link
Contributor Author

shdeshpa07 commented May 26, 2025

/label merge-review-needed

@openshift-ci openshift-ci bot added the merge-review-needed Signifies that the merge review team needs to review this PR label May 26, 2025
@weshayutin
Copy link

weshayutin commented May 27, 2025

can someone repost a preview link

@michaelryanpeter
Copy link
Contributor

michaelryanpeter commented May 27, 2025

/label merge-review-in-progress

@openshift-ci openshift-ci bot added the merge-review-in-progress Signifies that the merge review team is reviewing this PR label May 27, 2025
@michaelryanpeter michaelryanpeter added this to the Planned for 4.19 GA milestone May 27, 2025
@michaelryanpeter
Copy link
Contributor

michaelryanpeter commented May 27, 2025 •
edited
Loading

Still reviewing. Due to the scope and since I started late in the day, the review will carry over into tomorrow.

@shdeshpa07
Copy link
Contributor Author

shdeshpa07 commented May 28, 2025 •
edited
Loading

can someone repost a preview link

@weshayutin - Sorry, I should have kept you in loop. This PR is now on merge review, therefore I had to comment out the entries in the _topic_map.yaml file for the feature. This caused the preview links to no longer work.
Once the merge review is done and the files gets merged, I will create a separate PR to un-comment the _topic_map.yaml entries and get that PR merged on the day of GA. This approach is being used because of the size of the PR and to keep day of GA as smooth as possible.

But I understand that you might need to preview the Self-Service docs, so I created a separate temp branch (from this one) and a separate temp PR to generate a preview link.

TL;DR -> Here is the temporary preview link. OADP Self-Service preview

After this current PR gets merged, I will loop back and provide you with a latest preview link again.

Thanks!

michaelryanpeter
michaelryanpeter reviewed May 28, 2025
View reviewed changes
Copy link
Contributor

@michaelryanpeter michaelryanpeter left a comment

Choose a reason for hiding this comment

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

I found a few nits. Nothing major and nothing worth blocking a merge. If you get the chance, please try to address the nits before publication.

Great work!

modules/oadp-self-service-namespace-permissions.adoc
.Example RBAC
[source,yaml]
----
...
Copy link
Contributor

@michaelryanpeter michaelryanpeter May 28, 2025

Choose a reason for hiding this comment

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

Reminder to include the apiVersion and kind in YAML files. https://github.com/openshift/openshift-docs/blob/main/contributing_to_docs/doc_guidelines.adoc#yaml-formatting-for-kubernetes-and-openshift-api-objects

Not a blocker.

@michaelryanpeter michaelryanpeter merged commit ba661f9 into openshift:main May 28, 2025
2 checks passed
@michaelryanpeter
Copy link
Contributor

michaelryanpeter commented May 28, 2025

/cherrypick enterprise-4.19

@openshift-cherrypick-robot
Copy link

openshift-cherrypick-robot commented May 28, 2025

@michaelryanpeter: new pull request created: #93885

Details

In response to this:

/cherrypick enterprise-4.19

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request May 28, 2025
Merged
@shdeshpa07
Copy link
Contributor Author

shdeshpa07 commented May 29, 2025

I found a few nits. Nothing major and nothing worth blocking a merge. If you get the chance, please try to address the nits before publication.

Great work!

Thank you @michaelryanpeter . I'll address all of these merge review comments in a separate PR (I am opening one for uncommenting the _topic_map.yaml entries).

Thank you for your review!

@shdeshpa07 shdeshpa07 mentioned this pull request May 30, 2025
Merged
1 task
@shdeshpa07
Copy link
Contributor Author

shdeshpa07 commented May 30, 2025

I found a few nits. Nothing major and nothing worth blocking a merge. If you get the chance, please try to address the nits before publication.

Great work!

Thank you @michaelryanpeter. I have addressed most of your comments in this new PR.

@shdeshpa07 shdeshpa07 deleted the OADP-4001-Self-Service branch July 25, 2025 05:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@xenolinux xenolinux xenolinux left review comments

@michaelryanpeter michaelryanpeter michaelryanpeter left review comments

+7 more reviewers

@weshayutin weshayutin weshayutin left review comments

@kaovilai kaovilai kaovilai left review comments

@mail2nadeem92 mail2nadeem92 mail2nadeem92 left review comments

@mpryc mpryc mpryc left review comments

@shubham-pampattiwar shubham-pampattiwar shubham-pampattiwar left review comments

@PrasadJoshi12 PrasadJoshi12 PrasadJoshi12 left review comments

@anarnold97 anarnold97 anarnold97 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

branch/enterprise-4.19 merge-review-in-progress Signifies that the merge review team is reviewing this PR merge-review-needed Signifies that the merge review team needs to review this PR OADP Label for all OADP PRs peer-review-done Signifies that the peer review team has reviewed this PR size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Milestone

Planned for 4.19 GA

Development

Successfully merging this pull request may close these issues.