Add documentation for OADP CLI#151
Open
Joeavaikath wants to merge 6 commits intomigtools:oadp-devfrom
Open
Conversation
Signed-off-by: Joseph <jvaikath@redhat.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new user-facing documentation file Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can customize the high-level summary generated by CodeRabbit.Configure the |
kaovilai
reviewed
Mar 10, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/README.md`:
- Around line 668-675: Update the README text to clarify that the
--include-cluster-resources flag cannot be used by non-admin users to include
cluster-scoped resources: in Section 6.1 and the paragraph describing
--include-cluster-resources, mirror the backup wording (around the non-admin
backup statement) to explicitly state that non-admin restores must leave
--include-cluster-resources disabled (or that the flag has no effect / is
disallowed for non-admins), and add a short note referencing NonAdmin
limitations so readers understand the behavior for both backups and restores.
- Around line 712-715: The docs are inconsistent about the expected format for
resource flags; pick one canonical syntax (either API-style group.resource like
`deployments.apps,services.v1` or simple kind names like `deployments,services`)
and make the README consistent by updating the flag table entries for
`--include-resources` and `--exclude-resources` and all example usages to use
that chosen format; search for occurrences of `--include-resources`,
`--exclude-resources`, and the examples that show `deployments,services` (and
related lines mentioned) and replace them so the table description and every
example use the same syntax and an explanatory note matches that syntax.
- Around line 473-530: Update the docs so the example commands match the actual
Cobra command path registered in the code: replace the four occurrences of "oc
oadp snapshot-location ..." with the real command invocation exposed by the
binary (the registered Cobra command for snapshot-location, e.g., the
snapshotLocationCmd/subcommand path). Locate the snapshot-location command
registration in the CLI code (the Cobra command variable that registers
"snapshot-location") and mirror that exact command hierarchy in the README
examples for create/get/set/delete so the docs reflect the actual binary usage.
- Around line 404-467: The docs reference a non-registered top-level command
"backup-location"; inspect the CLI registration in cmd/root.go to find the
actual command variable that was added to root (look for variables like
backupLocationCmd, backupLocationRootCmd, or the AddCommand(...) call that wires
the backup-location subcommands) and update the four examples
(create/get/set/delete) to use the exact, registered CLI path (e.g., the real
subcommand name or nested form as registered) so the examples match the
implemented command tree.
Signed-off-by: Joseph <jvaikath@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
docs/README.md (2)
712-713:⚠️ Potential issue | 🟡 MinorUse one resource-list syntax consistently.
The flag tables say
resource.group, but the examples usedeployments,services. Please align the descriptions and examples to the same syntax so readers know what the CLI actually accepts.Also applies to: 726-732, 820-821, 830-833
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/README.md` around lines 712 - 713, Update the README so the resource-list syntax is consistent across flag descriptions and examples: choose one canonical format (either fully-qualified "resource.group" or short names like "deployments,services") and apply it to the `--include-resources` and `--exclude-resources` flag descriptions and all examples referenced (including the blocks around the other occurrences noted). Specifically, pick the accepted CLI syntax, update the text for `--include-resources` and `--exclude-resources` to explain the chosen format, and adjust example CLI invocations to use that same format so readers see the exact input the CLI accepts.
817-824:⚠️ Potential issue | 🟡 MinorRemove or explicitly forbid
--include-cluster-resourcesin the non-admin restore table.This row contradicts Section 6.1, which says non-admin restores cannot include cluster-scoped resources. As written, it advertises a path that non-admin users should not be able to use.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/README.md` around lines 817 - 824, The docs table incorrectly lists the `--include-cluster-resources` flag for non-admin restores; update the README so that the `--include-cluster-resources` row is either removed from the non-admin restore flags table or explicitly marked forbidden/unsupported for non-admin restores to match Section 6.1; ensure the table entry for `--include-cluster-resources` in the non-admin restore section (and any nearby flag descriptions) clearly indicates that cluster-scoped resources cannot be included by non-admin users and reference Section 6.1 for details.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/README.md`:
- Around line 919-921: The README entry for the --credential flag currently
reverses the meaning of the parts in SECRET_NAME=KEY; update the description for
the `--credential` flag so it states that the secret name is the part before the
`=` and the data key is the part after the `=` (i.e., SECRET_NAME is the
Kubernetes Secret name and KEY is the data key within that Secret), ensuring the
`--credential` line and the `SECRET_NAME=KEY` example reflect this corrected
ordering.
---
Duplicate comments:
In `@docs/README.md`:
- Around line 712-713: Update the README so the resource-list syntax is
consistent across flag descriptions and examples: choose one canonical format
(either fully-qualified "resource.group" or short names like
"deployments,services") and apply it to the `--include-resources` and
`--exclude-resources` flag descriptions and all examples referenced (including
the blocks around the other occurrences noted). Specifically, pick the accepted
CLI syntax, update the text for `--include-resources` and `--exclude-resources`
to explain the chosen format, and adjust example CLI invocations to use that
same format so readers see the exact input the CLI accepts.
- Around line 817-824: The docs table incorrectly lists the
`--include-cluster-resources` flag for non-admin restores; update the README so
that the `--include-cluster-resources` row is either removed from the non-admin
restore flags table or explicitly marked forbidden/unsupported for non-admin
restores to match Section 6.1; ensure the table entry for
`--include-cluster-resources` in the non-admin restore section (and any nearby
flag descriptions) clearly indicates that cluster-scoped resources cannot be
included by non-admin users and reference Section 6.1 for details.
Signed-off-by: Joseph <jvaikath@redhat.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
docs/README.md (1)
824-824:⚠️ Potential issue | 🟡 MinorRemove or clarify
--include-cluster-resourcesflag for non-admin restores.The
--include-cluster-resourcesflag is listed in the non-admin restore flags table, but line 673 explicitly states that "Cluster-scoped resources cannot be included in backups or restores" for non-admin users. This is inconsistent. Note that the non-admin backup flags table (line 716 area) correctly omits this flag.According to the past review comment (lines 668-675), this was confirmed as needing correction.
🔧 Recommended fix
Remove the flag from the non-admin restore table since it's not supported:
| `--or-selector` | Restore resources matching at least one of the label selectors, separated by ` or `. | -| `--include-cluster-resources` | Include cluster-scoped resources in the restore. | | `--item-operation-timeout` | Timeout for asynchronous plugin operations. |Alternatively, if the flag exists but is ignored/disabled for non-admin users, add a clear note:
-| `--include-cluster-resources` | Include cluster-scoped resources in the restore. | +| `--include-cluster-resources` | Include cluster-scoped resources. **Not available for non-admin users.** See [Section 6.1 Limitations](`#61-about-oadp-self-service`). |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/README.md` at line 824, The non-admin restore flags table erroneously includes the `--include-cluster-resources` flag despite the README text stating cluster-scoped resources cannot be backed up/restored for non-admins; update the non-admin restore flags table by removing the `--include-cluster-resources` entry (or alternatively add a clear note next to that flag explaining it is not supported/ignored for non-admin users) so the table and the paragraph about non-admin restore limitations are consistent.
🧹 Nitpick comments (3)
docs/README.md (3)
673-673: Optional: Cross-reference the flag in the limitations statement.Consider enhancing the limitations statement to explicitly reference the
--include-cluster-resourcesflag, making it easier for users to understand which command-line options are affected by this restriction.📝 Suggested enhancement
-- Cluster-scoped resources cannot be included in backups or restores. +- Cluster-scoped resources cannot be included in backups or restores. The `--include-cluster-resources` flag is not available for non-admin operations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/README.md` at line 673, Update the limitations sentence to explicitly mention the affected CLI flag so users know which option is impacted: change the line "Cluster-scoped resources cannot be included in backups or restores." to reference the flag name (e.g., mention "--include-cluster-resources") and clarify that this flag cannot be used to include cluster-scoped resources during backup/restore; ensure the updated text includes the exact flag string --include-cluster-resources so it’s discoverable in searches and docs.
905-907: Optional: Vary sentence structure to improve readability.Three consecutive sentences begin with "You," which can feel repetitive. Consider varying the sentence structure for better flow.
✍️ Suggested rewording
- The OADP CLI is installed and configured in non-admin mode. -- You are logged in to the OpenShift cluster and your current namespace context is set to the target namespace. -- You have editor roles for `nonadminbackupstoragelocations.oadp.openshift.io` in your namespace. -- You have a Kubernetes Secret in your namespace containing the credentials for your object storage provider. +- Your current namespace context is set to the target namespace where you will create the BSL. +- Editor roles for `nonadminbackupstoragelocations.oadp.openshift.io` are granted in your namespace. +- A Kubernetes Secret containing credentials for your object storage provider exists in your namespace.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/README.md` around lines 905 - 907, The three consecutive bullet lines ("You are logged in to the OpenShift cluster...", "You have editor roles for `nonadminbackupstoragelocations.oadp.openshift.io`...", "You have a Kubernetes Secret...") are repetitive; rephrase them to vary sentence openings and improve flow—for example, make the first passive or descriptive ("Ensure the current namespace context is set to the target namespace."), turn the second into an imperative or conditional ("Confirm editor roles for `nonadminbackupstoragelocations.oadp.openshift.io` in the namespace."), and the third into a noun-starting phrase or imperative ("A Kubernetes Secret in the namespace must contain credentials for your object storage provider."). Apply these changes in the README.md bullet list where those exact sentences appear.
426-426: Clarify the--credentialflag format for consistency.The description at line 426 is vague about which part of
SECRET_NAME=KEYrepresents the Secret name versus the data key. For consistency with the clearer explanation at line 921 (non-admin BSL section), consider updating this description to explicitly state thatSECRET_NAMEis the Kubernetes Secret name andKEYis the data key within that Secret.📝 Suggested improvement
-| `--credential` | Secret and key for provider credentials in the format `SECRET_NAME=KEY`. | +| `--credential` | Credential for this location as `SECRET_NAME=KEY`, where `SECRET_NAME` is the Kubernetes Secret name and `KEY` is the data key within the Secret. |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/README.md` at line 426, Update the `--credential` flag description to explicitly state that in the format `SECRET_NAME=KEY`, `SECRET_NAME` refers to the Kubernetes Secret name and `KEY` refers to the data key within that Secret (i.e., the specific field inside the Secret), so replace the vague text for the `--credential` flag with this clarified wording to match the non-admin BSL explanation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@docs/README.md`:
- Line 824: The non-admin restore flags table erroneously includes the
`--include-cluster-resources` flag despite the README text stating
cluster-scoped resources cannot be backed up/restored for non-admins; update the
non-admin restore flags table by removing the `--include-cluster-resources`
entry (or alternatively add a clear note next to that flag explaining it is not
supported/ignored for non-admin users) so the table and the paragraph about
non-admin restore limitations are consistent.
---
Nitpick comments:
In `@docs/README.md`:
- Line 673: Update the limitations sentence to explicitly mention the affected
CLI flag so users know which option is impacted: change the line "Cluster-scoped
resources cannot be included in backups or restores." to reference the flag name
(e.g., mention "--include-cluster-resources") and clarify that this flag cannot
be used to include cluster-scoped resources during backup/restore; ensure the
updated text includes the exact flag string --include-cluster-resources so it’s
discoverable in searches and docs.
- Around line 905-907: The three consecutive bullet lines ("You are logged in to
the OpenShift cluster...", "You have editor roles for
`nonadminbackupstoragelocations.oadp.openshift.io`...", "You have a Kubernetes
Secret...") are repetitive; rephrase them to vary sentence openings and improve
flow—for example, make the first passive or descriptive ("Ensure the current
namespace context is set to the target namespace."), turn the second into an
imperative or conditional ("Confirm editor roles for
`nonadminbackupstoragelocations.oadp.openshift.io` in the namespace."), and the
third into a noun-starting phrase or imperative ("A Kubernetes Secret in the
namespace must contain credentials for your object storage provider."). Apply
these changes in the README.md bullet list where those exact sentences appear.
- Line 426: Update the `--credential` flag description to explicitly state that
in the format `SECRET_NAME=KEY`, `SECRET_NAME` refers to the Kubernetes Secret
name and `KEY` refers to the data key within that Secret (i.e., the specific
field inside the Secret), so replace the vague text for the `--credential` flag
with this clarified wording to match the non-admin BSL explanation.
kaovilai
previously approved these changes
Mar 13, 2026
Signed-off-by: Joseph <jvaikath@redhat.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
docs/README.md (1)
824-824:⚠️ Potential issue | 🟡 MinorFix contradictory non-admin guidance for
--include-cluster-resources.Line 824 says non-admin restore can include cluster-scoped resources, but Lines 673-674 explicitly say non-admin backups/restores cannot include them. Please align the restore flag description with the limitations section to avoid user confusion.
Suggested doc fix
-| `--include-cluster-resources` | Include cluster-scoped resources in the restore. | +| `--include-cluster-resources` | Include cluster-scoped resources in the restore. **Admin mode only.** Non-admin restores cannot include cluster-scoped resources (see Section 6.1 Limitations). |Also applies to: 673-674
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/README.md` at line 824, Update the README so the `--include-cluster-resources` flag description and the limitations section are consistent: change the flag text to clarify that including cluster-scoped resources requires admin privileges (or conversely, update the limitations text if flag truly supports non-admins). Locate occurrences of `--include-cluster-resources` and the sentence "non-admin backups/restores cannot include cluster-scoped resources" and make them state the same behavior (e.g., "Include cluster-scoped resources in the restore; requires cluster-admin privileges" or remove the contradictory claim), and ensure both the flag description and the limitations section use the exact same phrasing about admin requirement to avoid confusion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@docs/README.md`:
- Line 824: Update the README so the `--include-cluster-resources` flag
description and the limitations section are consistent: change the flag text to
clarify that including cluster-scoped resources requires admin privileges (or
conversely, update the limitations text if flag truly supports non-admins).
Locate occurrences of `--include-cluster-resources` and the sentence "non-admin
backups/restores cannot include cluster-scoped resources" and make them state
the same behavior (e.g., "Include cluster-scoped resources in the restore;
requires cluster-admin privileges" or remove the contradictory claim), and
ensure both the flag description and the limitations section use the exact same
phrasing about admin requirement to avoid confusion.
Signed-off-by: Joseph <jvaikath@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
docs/README.md (2)
271-271:⚠️ Potential issue | 🟡 MinorClarify that
--include-cluster-resourcesis admin-only in restores.Consistent with the backup flag, this restore flag description should explicitly state it's only available in admin mode.
📝 Suggested clarification
-| `--include-cluster-resources` | Include cluster-scoped resources. | +| `--include-cluster-resources` | Include cluster-scoped resources. **Admin mode only.** Non-admin users cannot include cluster-scoped resources in restores; see Section 6.1 limitations. |This addresses a confirmed issue from past reviews and maintains consistency with the backup flag documentation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/README.md` at line 271, Update the table row documenting the `--include-cluster-resources` flag so it clarifies that this restore flag is admin-only (mirroring the backup flag wording); modify the entry for `--include-cluster-resources` to explicitly state "Include cluster-scoped resources (admin-only for restores)" or similar, ensuring the phrasing is consistent with the backup flag description and appears in the restore flag section where `--include-cluster-resources` is listed.
176-176:⚠️ Potential issue | 🟡 MinorClarify that
--include-cluster-resourcesis admin-only.The flag description should explicitly state that this flag is only available in admin mode. Non-admin users cannot include cluster-scoped resources in backups, as documented in Section 6.1.
📝 Suggested clarification
-| `--include-cluster-resources` | Include cluster-scoped resources. | +| `--include-cluster-resources` | Include cluster-scoped resources. **Admin mode only.** Non-admin users cannot include cluster-scoped resources; see Section 6.1 limitations. |This addresses a confirmed issue from past reviews.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/README.md` at line 176, Update the flag description for `--include-cluster-resources` to explicitly state it is only available in admin mode and that non-admin users cannot include cluster-scoped resources in backups; amend the text to something like “Include cluster-scoped resources (admin only—non-admin users cannot include cluster-scoped resources; see Section 6.1)” so readers are directed to Section 6.1 for details about permissions and restrictions.
🧹 Nitpick comments (1)
docs/README.md (1)
673-673: Reference the flag name for clarity.For better cross-referencing with the admin sections, explicitly mention the
--include-cluster-resourcesflag name in this limitation.💡 Suggested enhancement
-- Cluster-scoped resources cannot be included in backups or restores. +- Cluster-scoped resources cannot be included in backups or restores. The `--include-cluster-resources` flag is not available to non-admin users.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/README.md` at line 673, Update the limitation sentence to explicitly reference the flag name by changing "Cluster-scoped resources cannot be included in backups or restores." to something that mentions the flag `--include-cluster-resources` (e.g., "Cluster-scoped resources cannot be included in backups or restores; the `--include-cluster-resources` flag does not include cluster-scoped resources."). Ensure the exact flag `--include-cluster-resources` appears in the sentence for clear cross-referencing with admin docs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/README.md`:
- Line 426: Update the `--credential` flag description so it explicitly states
that the part before `=` (SECRET_NAME) is the Kubernetes Secret name and the
part after `=` (KEY) is the data key inside that Secret; replace the current
sentence for `--credential` that reads "`Secret and key for provider credentials
in the format `SECRET_NAME=KEY`.`" with a clearer phrase like "`Kubernetes
Secret name and data key for provider credentials in the format SECRET_NAME=KEY
(SECRET_NAME is the Kubernetes Secret name; KEY is the data key within that
Secret).`" Ensure the example format `SECRET_NAME=KEY` remains unchanged.
---
Duplicate comments:
In `@docs/README.md`:
- Line 271: Update the table row documenting the `--include-cluster-resources`
flag so it clarifies that this restore flag is admin-only (mirroring the backup
flag wording); modify the entry for `--include-cluster-resources` to explicitly
state "Include cluster-scoped resources (admin-only for restores)" or similar,
ensuring the phrasing is consistent with the backup flag description and appears
in the restore flag section where `--include-cluster-resources` is listed.
- Line 176: Update the flag description for `--include-cluster-resources` to
explicitly state it is only available in admin mode and that non-admin users
cannot include cluster-scoped resources in backups; amend the text to something
like “Include cluster-scoped resources (admin only—non-admin users cannot
include cluster-scoped resources; see Section 6.1)” so readers are directed to
Section 6.1 for details about permissions and restrictions.
---
Nitpick comments:
In `@docs/README.md`:
- Line 673: Update the limitation sentence to explicitly reference the flag name
by changing "Cluster-scoped resources cannot be included in backups or
restores." to something that mentions the flag `--include-cluster-resources`
(e.g., "Cluster-scoped resources cannot be included in backups or restores; the
`--include-cluster-resources` flag does not include cluster-scoped resources.").
Ensure the exact flag `--include-cluster-resources` appears in the sentence for
clear cross-referencing with admin docs.
kaovilai
previously approved these changes
Mar 16, 2026
shubham-pampattiwar
previously approved these changes
Mar 16, 2026
Signed-off-by: Joseph <jvaikath@redhat.com>
Joeavaikath
dismissed stale reviews from shubham-pampattiwar and kaovilai
via
74702c6
Contributor
Author
|
/approve |
kaovilai
approved these changes
Mar 16, 2026
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Joeavaikath, kaovilai The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why the changes were made
Add comprehensive documentation for OADP CLI, going over installation, the commands in both admin and non-admin perspectives
Closes #145
How to test the changes made
Read the README.md
Summary by CodeRabbit