Skip to content

Add documentation for OADP CLI#151

Open
Joeavaikath wants to merge 3 commits intomigtools:oadp-devfrom
Joeavaikath:issue-145-documentation
Open

Add documentation for OADP CLI#151
Joeavaikath wants to merge 3 commits intomigtools:oadp-devfrom
Joeavaikath:issue-145-documentation

Conversation

@Joeavaikath
Copy link
Contributor

@Joeavaikath Joeavaikath commented Mar 10, 2026

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

  • Documentation
    • Added comprehensive OADP CLI documentation: installation and prerequisites, admin vs. non-admin setup and verification, client configuration and shell completion, detailed command usage with flags and examples, administration workflows, backups, restores, schedules, storage and snapshot management, approval workflows, and diagnostic data collection procedures.

Signed-off-by: Joseph <jvaikath@redhat.com>
@coderabbitai
Copy link

coderabbitai bot commented Mar 10, 2026

📝 Walkthrough

Walkthrough

Adds a new user-facing documentation file docs/README.md that documents OADP CLI installation, admin vs non-admin setup, client configuration, shell completion, and detailed usage examples and workflows for backups, restores, schedules, storage/snapshot locations, NABSL approvals, and diagnostics.

Changes

Cohort / File(s) Summary
Documentation
docs/README.md
New comprehensive OADP CLI user guide covering installation, admin/non-admin setup, client configuration, shell completion, command usage and flags, examples for backups/restores/schedules, storage and snapshot locations, NABSL approval flow, and diagnostic collection workflows.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐇 I hopped through pages, nibbling each line,

With commands and flags arranged so fine.
Admin or guest, the path is clear,
Backups and restores now hop near.
A little doc dance — trail marked with cheer.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: adding documentation for the OADP CLI tool.
Description check ✅ Passed The description covers both required sections with clear explanations of why changes were made and how to test them, though testing guidance is minimal.
Linked Issues check ✅ Passed The PR successfully delivers a comprehensive OADP CLI documentation covering both cluster-admin and non-cluster-admin perspectives as required by issue #145.
Out of Scope Changes check ✅ Passed All changes in the PR are directly related to documenting the OADP CLI as specified in issue #145; no out-of-scope modifications detected.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
📝 Coding Plan
  • Generate coding plan for human review comments

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Tip

You can customize the high-level summary generated by CodeRabbit.

Configure the reviews.high_level_summary_instructions setting to provide custom instructions for generating the high-level summary.

Comment on lines +53 to +57
2. To reconfigure the CLI, for example after a change in permissions, run the setup command with the `--force` flag:

```terminal
$ oc oadp setup --force
```
Copy link
Member

Choose a reason for hiding this comment

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

without force what happens if permissions changed? nothing?

Copy link
Contributor Author

@Joeavaikath Joeavaikath Mar 11, 2026

Choose a reason for hiding this comment

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

Pretty much, if it sees that config has it set to something already it wouldn't go through the flow again
Meant for first time setup
And that reminds me, the user should probably be redirected to run setup (or it should run inherently when they try running some other command) if it can't find config

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

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.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e4356aff-c0a0-449d-9906-9b633008725a

📥 Commits

Reviewing files that changed from the base of the PR and between a6377ad and 129a414.

📒 Files selected for processing (1)
  • docs/README.md

Comment on lines +404 to +467
## 5.4 Managing backup storage locations

You can use the OADP CLI to create, view, set, and delete backup storage locations (BSLs). Backup storage locations define where backup data is stored, such as an object storage bucket.

### Prerequisites

- The OADP CLI is installed and configured in admin mode.
- You are logged in to the OpenShift cluster as a user with `cluster-admin` privileges.

### Creating a backup storage location

You create a backup storage location by running the following command:

```terminal
$ oc oadp backup-location create <bsl_name> [flags]
```

| Flag | Description |
|------|-------------|
| `--provider` | Name of the cloud provider (e.g., `aws`, `gcp`, `azure`). |
| `--bucket` | Name of the object storage bucket. |
| `--prefix` | Path prefix within the bucket. |
| `--credential` | Secret and key for provider credentials in the format `SECRET_NAME=KEY`. |
| `--config` | Provider-specific configuration as key=value pairs. |
| `--backup-sync-period` | How often to sync backup contents from object storage. |
| `--request-timeout` | Timeout for the request to the Kubernetes API server. |

Example:

```terminal
$ oc oadp backup-location create my-bsl \
--provider aws \
--bucket my-velero-bucket \
--prefix velero \
--credential cloud-credentials=cloud
```

### Listing backup storage locations

You list all backup storage locations by running the following command:

```terminal
$ oc oadp backup-location get [<bsl_name>] [flags]
```

| Flag | Description |
|------|-------------|
| `-o` | Output format. Supported values: `json`, `yaml`. |

### Setting a default backup storage location

You set the default backup storage location by running the following command:

```terminal
$ oc oadp backup-location set <bsl_name> [flags]
```

### Deleting a backup storage location

You delete a backup storage location by running the following command:

```terminal
$ oc oadp backup-location delete <bsl_name> [flags]
```
Copy link

@coderabbitai coderabbitai bot Mar 10, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Document the actual BSL command path.

Lines 418, 446, 458, and 466 use a top-level backup-location command, but cmd/root.go:429-456 does not register backup-location at the root. As written, these examples will send readers to commands that do not exist. Please align this section with the real CLI tree before publishing.

🧰 Tools
🪛 markdownlint-cli2 (0.21.0)

[warning] 418-418: Dollar signs used before commands without showing output

(MD014, commands-show-output)


[warning] 446-446: Dollar signs used before commands without showing output

(MD014, commands-show-output)


[warning] 458-458: Dollar signs used before commands without showing output

(MD014, commands-show-output)


[warning] 466-466: Dollar signs used before commands without showing output

(MD014, commands-show-output)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/README.md` around lines 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wrong

Copy link

Choose a reason for hiding this comment

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

🧩 Analysis chain

Let me verify the actual command registration in the codebase.


🏁 Script executed:

#!/bin/bash
# Check root.go for backup-location command registration
rg -n "backup.location\|backupLocation\|BackupLocation\|AddCommand" cmd/root.go -A2 -B2

echo "---"
# Search broadly for backup-location registration across all cmd files
rg -rn "backup.location\|backupLocation\|BackupLocation" cmd/ -A2 -B2

Length of output: 3


🏁 Script executed:

#!/bin/bash
# List all files in cmd/
find . -name "*.go" | grep -i "backup\|location\|bsl" | head -40

echo "=== Directory structure ==="
tree cmd/ 2>/dev/null || find cmd/ -type f | sort

echo "=== Search for AddCommand in root.go ==="
rg -n "AddCommand" cmd/root.go

echo "=== Search for backup in all go files ==="
rg -rn "backup.location\|backupLocation\|BackupLocation\|backup-location" --type go -l

Length of output: 2236


🏁 Script executed:

#!/bin/bash
echo "=== root.go lines 410-460 ==="
sed -n '410,460p' cmd/root.go

echo "=== root.go full AddCommand block ==="
rg -n "AddCommand\|velero\|Velero\|backup\|Backup" cmd/root.go

echo "=== Look for admin backup-location commands ==="
find . -path "*/cmd/*.go" | xargs grep -l "backup-location\|BackupLocation\|backupLocation" 2>/dev/null

echo "=== Check what's imported/registered for admin velero commands ==="
cat cmd/root.go | head -100

Length of output: 5227


@Joeavaikath, you are correct — I apologize for the incorrect comment. Looking at the actual code, backuplocation.NewCommand(f) (imported from "github.com/vmware-tanzu/velero/pkg/cmd/cli/backuplocation") is registered directly in the root c.AddCommand(...) block in cmd/root.go, making oc oadp backup-location a valid top-level command. The documentation in section 5.4 accurately reflects the real CLI structure.

Comment on lines +473 to +530
## 5.5 Managing snapshot locations

You can use the OADP CLI to create, view, set, and delete volume snapshot locations (VSLs). Volume snapshot locations define where PersistentVolume snapshots are stored.

### Prerequisites

- The OADP CLI is installed and configured in admin mode.
- You are logged in to the OpenShift cluster as a user with `cluster-admin` privileges.

### Creating a snapshot location

You create a volume snapshot location by running the following command:

```terminal
$ oc oadp snapshot-location create <vsl_name> [flags]
```

| Flag | Description |
|------|-------------|
| `--provider` | Name of the cloud provider (e.g., `aws`, `gcp`, `azure`). |
| `--config` | Provider-specific configuration as key=value pairs. |
| `--request-timeout` | Timeout for the request to the Kubernetes API server. |

Example:

```terminal
$ oc oadp snapshot-location create my-vsl \
--provider aws \
--config region=us-east-1
```

### Listing snapshot locations

You list all volume snapshot locations by running the following command:

```terminal
$ oc oadp snapshot-location get [<vsl_name>] [flags]
```

| Flag | Description |
|------|-------------|
| `-o` | Output format. Supported values: `json`, `yaml`. |

### Setting a default snapshot location

You set the default volume snapshot location by running the following command:

```terminal
$ oc oadp snapshot-location set <vsl_name> [flags]
```

### Deleting a snapshot location

You delete a volume snapshot location by running the following command:

```terminal
$ oc oadp snapshot-location delete <vsl_name> [flags]
```
Copy link

@coderabbitai coderabbitai bot Mar 10, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Document the actual VSL command path.

Lines 487, 509, 521, and 529 have the same problem for snapshot-location: cmd/root.go:429-456 does not register that as a top-level command. These examples need to match the command structure the binary actually exposes.

🧰 Tools
🪛 markdownlint-cli2 (0.21.0)

[warning] 487-487: Dollar signs used before commands without showing output

(MD014, commands-show-output)


[warning] 509-509: Dollar signs used before commands without showing output

(MD014, commands-show-output)


[warning] 521-521: Dollar signs used before commands without showing output

(MD014, commands-show-output)


[warning] 529-529: Dollar signs used before commands without showing output

(MD014, commands-show-output)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/README.md` around lines 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wrong

Copy link

Choose a reason for hiding this comment

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check how snapshot-location is registered in the CLI
rg -n "snapshot.location" --type go -C3

Length of output: 454


@Joeavaikath, you're right — I apologize for the incorrect comment. snapshot-location is explicitly registered as a recognized top-level command in cmd/root.go (line 178), alongside backup-location, version, client, and others. The documentation commands oc oadp snapshot-location create/get/set/delete are accurate as written.

Comment on lines +668 to +675
### Limitations

- Cross-cluster operations and migrations are not supported for non-admin users.
- Non-admin volume snapshot locations (VSLs) are not supported. The VSL created by the cluster administrator in the DPA is used.
- Backups and restores are scoped to the namespace from which the command is run. You cannot specify a different namespace.
- Cluster-scoped resources cannot be included in backups or restores.
- ResourceModifiers and volume policies are not supported for non-admin backup and restore operations.
- Backup and restore logs via NonAdminDownloadRequest are not supported for default BSLs. NonAdminBackupStorageLocations must be created to access logs.
Copy link

@coderabbitai coderabbitai bot Mar 10, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Clarify --include-cluster-resources for non-admin restores.

Section 6.1 says non-admin backups and restores cannot include cluster-scoped resources, but Line 825 describes --include-cluster-resources as if it enables that behavior. Please mirror the backup wording from Line 716 or explicitly state that non-admin users can only leave this disabled.

Also applies to: 818-826

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/README.md` around lines 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct

Copy link

Choose a reason for hiding this comment

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

@Joeavaikath, thanks for confirming! I'll plan the fix to clarify that --include-cluster-resources is not available to non-admin users in both the backup and restore flag tables.

  • Update the --include-cluster-resources row in the backup create flags table (Section 5.1) to note it is for admin mode only.
  • Update the --include-cluster-resources row in the restore create flags table (Section 5.2) with the same note, explicitly mirroring the non-admin limitation language.
  • Add a short cross-reference note below each table (or inline in the flag description) pointing readers to Section 6.1 for non-admin limitations.

💡 Use @coderabbitai plan to create a plan for code changes

Signed-off-by: Joseph <jvaikath@redhat.com>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (2)
docs/README.md (2)

712-713: ⚠️ Potential issue | 🟡 Minor

Use one resource-list syntax consistently.

The flag tables say resource.group, but the examples use deployments,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 | 🟡 Minor

Remove or explicitly forbid --include-cluster-resources in 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.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 16d1dd4d-ec14-41ce-a4d2-4235e5d34438

📥 Commits

Reviewing files that changed from the base of the PR and between 129a414 and 1c81c87.

📒 Files selected for processing (1)
  • docs/README.md

Signed-off-by: Joseph <jvaikath@redhat.com>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

♻️ Duplicate comments (1)
docs/README.md (1)

824-824: ⚠️ Potential issue | 🟡 Minor

Remove or clarify --include-cluster-resources flag for non-admin restores.

The --include-cluster-resources flag 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-resources flag, 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 --credential flag format for consistency.

The description at line 426 is vague about which part of SECRET_NAME=KEY represents 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 that SECRET_NAME is the Kubernetes Secret name and KEY is 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.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4a9d185e-e7d1-447e-8f10-7eccb0e7ffb1

📥 Commits

Reviewing files that changed from the base of the PR and between 1c81c87 and e39491e.

📒 Files selected for processing (1)
  • docs/README.md

@openshift-ci
Copy link

openshift-ci bot commented Mar 13, 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

Details Needs approval from an approver in each of these files:
  • OWNERS [Joeavaikath,kaovilai]

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

create user facing doc template for docs team

2 participants