Default namespace to openshift-adp instead of velero#182
Default namespace to openshift-adp instead of velero#182Joeavaikath merged 1 commit intomigtools:oadp-devfrom
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughSets the default admin namespace to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
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)
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. Review rate limit: 0/1 reviews remaining, refill in 30 minutes and 58 seconds.Comment |
|
/cherry-pick oadp-1.6 |
|
@Joeavaikath: once the present PR merges, I will cherry-pick it on top of DetailsIn response to this:
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. |
|
/lgtm |
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/root.go`:
- Around line 391-392: The code assumes the VeleroConfig map returned by
clientcmd.LoadConfig() is non-nil and directly assigns
config[clientcmd.ConfigKeyNamespace] = "openshift-adp", which can panic if
LoadConfig() returned nil; update the branch that currently references config
(after calling clientcmd.LoadConfig()) to guard against a nil config by either
initializing config = make(clientcmd.VeleroConfig) before setting
config[clientcmd.ConfigKeyNamespace] or returning an error, and keep the check
around config.Namespace() and the else-if block consistent so you never index
into config when it may be nil (references: clientcmd.LoadConfig, VeleroConfig,
and the assignment to config[clientcmd.ConfigKeyNamespace]).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e794dda1-00dc-4956-846f-a717624106d7
📒 Files selected for processing (3)
cmd/root.gocmd/root_test.gocmd/shared/factories.go
💤 Files with no reviewable changes (1)
- cmd/shared/factories.go
When no VELERO_NAMESPACE env var or config file is present, the Velero library falls back to "velero" as the default namespace. Set it to "openshift-adp" in the root command when no namespace is configured. Guard against nil config map from LoadConfig errors. Remove unused CreateVeleroFactory and CreateNonAdminFactory functions from factories.go. Signed-off-by: Joseph <jvaikath@redhat.com>
36fa18b to
416d616
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Joeavaikath, kaovilai, weshayutin 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 |
|
@Joeavaikath: new pull request created: #183 DetailsIn response to this:
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. |
Why the changes were made
Fixes OADP-6555 — the CLI defaults to the
veleronamespace when noVELERO_NAMESPACEenv var or~/.config/velero/config.jsonexists. This is because the upstream Velero library hardcodes"velero"asDefaultNamespace. For OADP, the correct default isopenshift-adp.The fix sets
openshift-adpas the namespace in the root command config when no namespace is otherwise configured. Users who have runoc oadp setupor setVELERO_NAMESPACEare unaffected.Also removes dead code:
CreateVeleroFactoryandCreateNonAdminFactoryinfactories.gowere never called.How to test the changes made
oc oadp --helpwithout a config file orVELERO_NAMESPACEenv var(default "openshift-adp")instead of(default "velero")go test ./cmd/ -run TestDefaultNamespaceIsOpenShiftADP -vSummary by CodeRabbit
Bug Fixes
Tests
Refactor