Add config support default non-admin storage location#144
Open
Joeavaikath wants to merge 5 commits intomigtools:oadp-devfrom
Open
Add config support default non-admin storage location#144Joeavaikath wants to merge 5 commits intomigtools:oadp-devfrom
Joeavaikath wants to merge 5 commits intomigtools:oadp-devfrom
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) sourced from the Velero client config: new config field/accessor, backup-create command reads and uses that default when no flag is provided, and user-facing messages updated to reflect/configure the default. 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)
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.
Comment on lines
+255
to
+264
| func getNABSLFromConfig() string { | ||
| clientConfig, err := shared.ReadVeleroClientConfig() | ||
| if err == nil && clientConfig != nil { | ||
| defaultNABSL := clientConfig.GetDefaultNABSL() | ||
| if defaultNABSL != "" { | ||
| return defaultNABSL | ||
| } | ||
| } | ||
| return "" | ||
| } |
There was a problem hiding this comment.
Don’t hide config read/parse errors.
Lines 255-264 return "" on error, which masks real failures and later produces a generic validation message instead of actionable feedback.
💡 Proposed fix
-func getNABSLFromConfig() string {
+func getNABSLFromConfig() (string, error) {
clientConfig, err := shared.ReadVeleroClientConfig()
- if err == nil && clientConfig != nil {
- defaultNABSL := clientConfig.GetDefaultNABSL()
- if defaultNABSL != "" {
- return defaultNABSL
- }
+ if err != nil {
+ return "", fmt.Errorf("failed to read Velero client config: %w", err)
}
- return ""
+ if clientConfig == nil {
+ return "", nil
+ }
+ return clientConfig.GetDefaultNABSL(), nil
}🤖 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 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.
Contributor
Author
There was a problem hiding this comment.
No config reading is kind of straightforward
There was a problem hiding this comment.
@Joeavaikath, understood. I'll leave the config reading as-is since it handles the straightforward case adequately.
✏️ Learnings added
Learnt from: Joeavaikath
Repo: migtools/oadp-cli PR: 144
File: cmd/non-admin/backup/create.go:260-269
Timestamp: 2026-02-27T19:32:59.144Z
Learning: In the oadp-cli project, for the getNABSLFromConfig() function in cmd/non-admin/backup/create.go, returning an empty string on config read errors is acceptable because config reading is straightforward and doesn't require explicit error propagation.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
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.
Comment on lines
+64
to
+66
| # Set default storage location for all backups. | ||
| oc oadp client config set default-nabsl=my-nabsl | ||
|
|
There was a problem hiding this comment.
Fix the example command scope/path for default-nabsl.
This example currently reads as global (“all backups”) and uses oc oadp client ..., while this feature is for non-admin backup flows. Updating this avoids copy/paste confusion.
💡 Proposed fix
- # 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📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # 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 | |
🤖 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, 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
| // Storage location validation | ||
| if o.StorageLocation == "" { | ||
| return fmt.Errorf("--storage-location is required") | ||
| return fmt.Errorf("--storage-location is required (can be set via flag or config 'default-nabsl')") |
Member
There was a problem hiding this comment.
I think it could be clarified a bit more how to set this config? or we wanna keep this short?
kaovilai
approved these changes
Feb 27, 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