Add config support default non-admin storage location#144
Merged
Joeavaikath merged 6 commits intoMar 2, 2026
Conversation
for non-admin backups Adds a config value to the Velero client config file to set the default storage location for non-admin backups. Adds a command output on NABSL creation to inform the user about the default storage location config. Signed-off-by: Joseph <jvaikath@redhat.com>
If default storage location is set in config, display a message that it is being used. Signed-off-by: Joseph <jvaikath@redhat.com>
Add line breaks in create output for non-admin backup and NABSL creation. Signed-off-by: Joseph <jvaikath@redhat.com>
Signed-off-by: Joseph <jvaikath@redhat.com>
📝 WalkthroughWalkthroughAdds support for a default non-admin backup storage location (NABSL) loaded from the Velero client config: new ClientConfig field and accessor, CreateOptions reads and uses the config default when Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cmd/non-admin/bsl/create.go (1)
204-204: Clarify scope as “default non-admin” storage location.Line 204 says “default” broadly; making it explicit helps avoid confusion with admin backup flows.
✏️ Suggested wording
-fmt.Printf("Use `oc oadp client config set default-nabsl=%s` to set this BSL as the default to avoid specifying the BSL name each time.\n", nabsl.Name) +fmt.Printf("Use `oc oadp client config set default-nabsl=%s` to set this as the default non-admin BSL and avoid specifying --storage-location each time.\n", nabsl.Name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/non-admin/bsl/create.go` at line 204, The user-facing message currently prints a broad "default" which is ambiguous; update the fmt.Printf call that references nabsl.Name (the line using fmt.Printf("Use `oc oadp client config set default-nabsl=%s`...")) to explicitly say "default non-admin storage location" or "default non-admin BSL" so it clarifies scope and avoids confusion with admin backup flows—replace the wording in that fmt.Printf invocation to include "non-admin" while preserving the command and nabsl.Name placeholder.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/non-admin/backup/create.go`:
- Around line 147-150: The code currently unconditionally sets o.StorageLocation
from getNABSLFromConfig() via defaultNABSL which overwrites an explicit
--storage-location flag; change the logic so you only assign o.StorageLocation =
defaultNABSL when the user did NOT provide a storage location flag (i.e., detect
whether the flag was set and only use getNABSLFromConfig() as a fallback).
Update the related reporting logic that mentions config usage (the message
around where o.StorageLocation is later reported) to only state "using
default-nabsl from config" when the config value was actually used; reference
the identifiers defaultNABSL, getNABSLFromConfig(), and o.StorageLocation to
locate and adjust the conditional assignment and the corresponding log/message.
- Around line 255-264: getNABSLFromConfig currently swallows errors from
shared.ReadVeleroClientConfig and returns "" which hides parse/read failures;
change getNABSLFromConfig to return (string, error), propagate and return any
error from shared.ReadVeleroClientConfig instead of returning an empty string,
and only return the default NABSL when clientConfig is non-nil and
GetDefaultNABSL() is non-empty; update callers of getNABSLFromConfig to handle
the error (e.g., validate and surface the error to the user) rather than relying
on an empty string.
---
Nitpick comments:
In `@cmd/non-admin/bsl/create.go`:
- Line 204: The user-facing message currently prints a broad "default" which is
ambiguous; update the fmt.Printf call that references nabsl.Name (the line using
fmt.Printf("Use `oc oadp client config set default-nabsl=%s`...")) to explicitly
say "default non-admin storage location" or "default non-admin BSL" so it
clarifies scope and avoids confusion with admin backup flows—replace the wording
in that fmt.Printf invocation to include "non-admin" while preserving the
command and nabsl.Name placeholder.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/non-admin/backup/create.go`:
- Around line 64-66: Update the example to use the non-admin command path and
remove the misleading “all backups” scope: replace the comment line "# Set
default storage location for all backups." with wording that indicates this is
for non-admin backup flows, and change the example command string "oc oadp
client config set default-nabsl=my-nabsl" to the non-admin variant (e.g. "oc
oadp non-admin client config set default-nabsl=my-nabsl") so the example
reflects the non-admin scope used by this feature.
kaovilai
reviewed
Feb 27, 2026
kaovilai
previously approved these changes
Feb 27, 2026
Signed-off-by: Joseph <jvaikath@redhat.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cmd/non-admin/backup/create.go (1)
64-66:⚠️ Potential issue | 🟡 MinorUse non-admin command path/scope in guidance text.
Line 64 and Line 141 still present this as global/all-backup guidance, but this flow is non-admin backup specific. Update both strings to avoid copy/paste confusion.
💡 Suggested text update
- # Set default storage location for all backups. - oc oadp client config set default-nabsl=my-nabsl + # Set default storage location for non-admin backups. + oc oadp nonadmin client config set default-nabsl=my-nabsl- return fmt.Errorf("--storage-location is required\n" + - "To avoid specifying the storage location each time:\n" + - "run `oc oadp client config set default-nabsl=<NABSL_NAME>` to set the default storage location") + return fmt.Errorf("--storage-location is required\n" + + "To avoid specifying the storage location each time for non-admin backups:\n" + + "run `oc oadp nonadmin client config set default-nabsl=<NABSL_NAME>` to set the default storage location")Also applies to: 139-142
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/non-admin/backup/create.go` around lines 64 - 66, The user-facing guidance strings in create.go that currently read "Set default storage location for all backups." and show the example "oc oadp client config set default-nabsl=my-nabsl" are incorrect for this non-admin flow; update both occurrences of those strings to reflect the non-admin scope (e.g., "Set default storage location for non-admin backups." and adjust the example command text to the non-admin command path) so the help text and example match the non-admin backup context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmd/non-admin/backup/create.go`:
- Around line 64-66: The user-facing guidance strings in create.go that
currently read "Set default storage location for all backups." and show the
example "oc oadp client config set default-nabsl=my-nabsl" are incorrect for
this non-admin flow; update both occurrences of those strings to reflect the
non-admin scope (e.g., "Set default storage location for non-admin backups." and
adjust the example command text to the non-admin command path) so the help text
and example match the non-admin backup context.
kaovilai
approved these changes
Mar 2, 2026
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
2 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
How to test the changes made
oc oadp nonadmin client config set default-nabsl=test-nabsloc oadp nonadmin backup create test-backupThe backup should be created successfully with a message stating that the config value was used for nonadmin backup storage location
Summary by CodeRabbit
New Features
Bug Fixes / UX
Documentation