Fix OADP-7562: handle wildcard in scoped excluded cluster resources#333
Fix OADP-7562: handle wildcard in scoped excluded cluster resources#333mpryc wants to merge 2 commits intomigtools:oadp-devfrom
Conversation
|
@mpryc: This pull request references OADP-7562 which is a valid jira issue. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@mpryc: This pull request references OADP-7562 which is a valid jira issue. 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 openshift-eng/jira-lifecycle-plugin repository. |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
/cherry-pick oadp-1.6 |
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughControllers now respect a wildcard ( ChangesWildcard-aware exclusion handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|
@mpryc: 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. |
|
/hold until it's confirmed to fix issue. |
|
@mpryc: This pull request references OADP-7562 which is a valid jira issue. 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 openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/controller/nonadminbackup_controller_test.go (1)
1549-1586: Good test coverage for the wildcard bug fix.The test correctly validates that:
ExcludedClusterScopedResources: ["*"]is preserved without appending defaultsExcludedNamespaceScopedResources(without wildcard) still gets the default NAC resources appendedFor completeness, consider adding additional test entries:
ExcludedNamespaceScopedResources: ["*"]scenario (to verify the symmetric case)- Both exclusion lists containing
"*"(to verify no defaults are appended to either)💡 Additional test entry suggestion
ginkgo.Entry("Should not append default excluded resources when wildcard is used in excludedNamespaceScopedResources", nonAdminBackupFullReconcileScenario{ spec: nacv1alpha1.NonAdminBackupSpec{ BackupSpec: &velerov1.BackupSpec{ ExcludedNamespaceScopedResources: []string{"*"}, IncludedClusterScopedResources: []string{"persistentvolumes"}, }, }, status: nacv1alpha1.NonAdminBackupStatus{ Phase: nacv1alpha1.NonAdminPhaseCreated, VeleroBackup: &nacv1alpha1.VeleroBackup{ Namespace: oadpNamespace, Status: nil, Spec: &velerov1.BackupSpec{ ExcludedNamespaceScopedResources: []string{"*"}, IncludedClusterScopedResources: []string{"persistentvolumes"}, ExcludedClusterScopedResources: []string{ "securitycontextconstraints", "clusterroles", "clusterrolebindings", "priorityclasses", "customresourcedefinitions", "virtualmachineclusterinstancetypes", "virtualmachineclusterpreferences", }, }, }, Conditions: []metav1.Condition{ { Type: "Accepted", Status: metav1.ConditionTrue, Reason: "BackupAccepted", Message: "backup accepted", }, { Type: "Queued", Status: metav1.ConditionTrue, Reason: "BackupScheduled", Message: "Created Velero Backup object", }, }, }, }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/nonadminbackup_controller_test.go` around lines 1549 - 1586, Add two additional ginkgo.Entry test cases to nonAdminbackup_controller_test.go using the same nonAdminBackupFullReconcileScenario pattern: one where BackupSpec.ExcludedNamespaceScopedResources == []string{"*"} to verify defaults are not appended to namespace-scoped exclusions, and another where both ExcludedNamespaceScopedResources and ExcludedClusterScopedResources are []string{"*"} to verify no defaults are appended to either; ensure each entry populates the top-level spec.BackupSpec and the nested status.VeleroBackup.Spec fields (referencing ExcludedNamespaceScopedResources and ExcludedClusterScopedResources) and includes the same Conditions and Phase as the existing entries so the test harness (nonAdminBackupFullReconcileScenario, VeleroBackup.Spec) validates the symmetric wildcard behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controller/nonadminbackup_controller.go`:
- Around line 709-719: The fallback path unconditionally appends to old-style
ExcludedResources which breaks validation when a user has ExcludedResources:
["*"]; update the logic in the non-admin backup and restore controllers to check
for the wildcard before appending: in the backup flow, guard the two append
calls with if !slices.Contains(backupSpec.ExcludedResources, "*") (targeting the
block that currently appends
alwaysExcludedNamespacedResources/alwaysExcludedClusterResources when scoped
fields are absent), and in the restore flow guard the append with if
!slices.Contains(restoreSpec.ExcludedResources, "*") so nothing is appended when
the wildcard already excludes everything.
---
Nitpick comments:
In `@internal/controller/nonadminbackup_controller_test.go`:
- Around line 1549-1586: Add two additional ginkgo.Entry test cases to
nonAdminbackup_controller_test.go using the same
nonAdminBackupFullReconcileScenario pattern: one where
BackupSpec.ExcludedNamespaceScopedResources == []string{"*"} to verify defaults
are not appended to namespace-scoped exclusions, and another where both
ExcludedNamespaceScopedResources and ExcludedClusterScopedResources are
[]string{"*"} to verify no defaults are appended to either; ensure each entry
populates the top-level spec.BackupSpec and the nested status.VeleroBackup.Spec
fields (referencing ExcludedNamespaceScopedResources and
ExcludedClusterScopedResources) and includes the same Conditions and Phase as
the existing entries so the test harness (nonAdminBackupFullReconcileScenario,
VeleroBackup.Spec) validates the symmetric wildcard behavior.
🪄 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: c38373e3-5363-4f87-921c-73088b1d6efd
📒 Files selected for processing (2)
internal/controller/nonadminbackup_controller.gointernal/controller/nonadminbackup_controller_test.go
|
@mpryc: This pull request references OADP-7562 which is a valid jira issue. 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 openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/controller/nonadminbackup_controller_test.go (1)
1549-1586: Consider adding a sibling case for namespace wildcard handling.A second table entry for
ExcludedNamespaceScopedResources: []string{"*"}would cover the parallel controller guard and close the remaining branch-level regression gap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/nonadminbackup_controller_test.go` around lines 1549 - 1586, Add a sibling ginkgo.Entry to the table-based tests (same nonAdminBackupFullReconcileScenario) that mirrors the existing case using ExcludedClusterScopedResources: []string{"*"} but instead sets the input spec's ExcludedNamespaceScopedResources: []string{"*"} and asserts the resulting VeleroBackup.Spec.ExcludedNamespaceScopedResources remains ["*"] (i.e., defaults are not appended); this will exercise the namespace-wildcard guard in the controller and close the branch-level regression gap.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/controller/nonadminbackup_controller_test.go`:
- Around line 1549-1586: Add a sibling ginkgo.Entry to the table-based tests
(same nonAdminBackupFullReconcileScenario) that mirrors the existing case using
ExcludedClusterScopedResources: []string{"*"} but instead sets the input spec's
ExcludedNamespaceScopedResources: []string{"*"} and asserts the resulting
VeleroBackup.Spec.ExcludedNamespaceScopedResources remains ["*"] (i.e., defaults
are not appended); this will exercise the namespace-wildcard guard in the
controller and close the branch-level regression gap.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 16f5d9fc-4ba2-4880-9887-9b41c8f6fb53
📒 Files selected for processing (3)
internal/controller/nonadminbackup_controller.gointernal/controller/nonadminbackup_controller_test.gointernal/controller/nonadminrestore_controller.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/controller/nonadminbackup_controller.go
|
@mpryc: This pull request references OADP-7562 which is a valid jira issue. 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 openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/controller/nonadminbackup_controller_test.go (1)
1549-1586: Add companion wildcard coverage for remaining exclusion paths.This entry covers the cluster-scoped wildcard well. Consider adding table entries for
ExcludedNamespaceScopedResources: []string{"*"}and old-styleExcludedResources: []string{"*"}to lock in all wildcard guards introduced in controller logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/nonadminbackup_controller_test.go` around lines 1549 - 1586, Add two new ginkgo.Entry test cases alongside the existing nonAdminBackupFullReconcileScenario entry: one where spec.BackupSpec.ExcludedNamespaceScopedResources = []string{"*"} and the expected status.VeleroBackup.Spec.ExcludedNamespaceScopedResources is also []string{"*"} (and include the same default ExcludedNamespaceScopedResources entries in the controller's created VeleroBackup when applicable), and another legacy-style case where spec.BackupSpec.ExcludedResources = []string{"*"} with the expected status.VeleroBackup.Spec.ExcludedResources = []string{"*"}; use the same nonAdminBackupFullReconcileScenario struct and mirror the Conditions/Phase fields from the existing entry so the test covers wildcard handling for both namespace-scoped and legacy exclusion fields.internal/controller/nonadminrestore_controller.go (1)
352-355: Wildcard guard is correct; add a restore wildcard test to prevent regressions.The guard is correct and aligns with the backup fix. Please add a
NonAdminRestoretest case forrestoreSpec.ExcludedResources = []string{"*"}to ensure"volumesnapshotclasses"is not appended in that path (current expectations aroundinternal/controller/nonadminrestore_controller_test.goLine 364 still only cover non-wildcard behavior).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/nonadminrestore_controller.go` around lines 352 - 355, Add a unit test that covers the wildcard exclusion path so we don't regress the behavior in nonadminrestore_controller.go: when restoreSpec.ExcludedResources is set to []string{"*"} the controller must NOT append "volumesnapshotclasses"; implement a new test case (or extend the existing NonAdminRestore test in nonadminrestore_controller_test) that sets restoreSpec.ExcludedResources = []string{"*"}, invokes the code path that runs the logic around slices.Contains and appending (the block that checks restoreSpec.ExcludedResources and may append "volumesnapshotclasses"), and assert that "volumesnapshotclasses" is not present in the final restoreSpec.ExcludedResources. Ensure the test fails if the append occurs to prevent regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/controller/nonadminbackup_controller_test.go`:
- Around line 1549-1586: Add two new ginkgo.Entry test cases alongside the
existing nonAdminBackupFullReconcileScenario entry: one where
spec.BackupSpec.ExcludedNamespaceScopedResources = []string{"*"} and the
expected status.VeleroBackup.Spec.ExcludedNamespaceScopedResources is also
[]string{"*"} (and include the same default ExcludedNamespaceScopedResources
entries in the controller's created VeleroBackup when applicable), and another
legacy-style case where spec.BackupSpec.ExcludedResources = []string{"*"} with
the expected status.VeleroBackup.Spec.ExcludedResources = []string{"*"}; use the
same nonAdminBackupFullReconcileScenario struct and mirror the Conditions/Phase
fields from the existing entry so the test covers wildcard handling for both
namespace-scoped and legacy exclusion fields.
In `@internal/controller/nonadminrestore_controller.go`:
- Around line 352-355: Add a unit test that covers the wildcard exclusion path
so we don't regress the behavior in nonadminrestore_controller.go: when
restoreSpec.ExcludedResources is set to []string{"*"} the controller must NOT
append "volumesnapshotclasses"; implement a new test case (or extend the
existing NonAdminRestore test in nonadminrestore_controller_test) that sets
restoreSpec.ExcludedResources = []string{"*"}, invokes the code path that runs
the logic around slices.Contains and appending (the block that checks
restoreSpec.ExcludedResources and may append "volumesnapshotclasses"), and
assert that "volumesnapshotclasses" is not present in the final
restoreSpec.ExcludedResources. Ensure the test fails if the append occurs to
prevent regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fb88cfda-ff88-4f9e-b763-10b6b6c8c842
📒 Files selected for processing (3)
internal/controller/nonadminbackup_controller.gointernal/controller/nonadminbackup_controller_test.gointernal/controller/nonadminrestore_controller.go
|
@mpryc: This pull request references OADP-7562 which is a valid jira issue. 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 openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/controller/nonadminbackupstoragelocation_controller.go (1)
649-653: Silent failure on type assertion could mask bugs.Returning
falsewhen the type assertion fails silently completes without making updates. While this prevents panics, it could hide programming errors where the wrong object type is passed. Consider adding a log statement at debug level when the assertion fails.🔧 Suggestion to add logging on assertion failure
if updateErr := r.updateStatusWithRetry(ctx, logger, nabslRequest, func(obj client.Object) bool { req, ok := obj.(*nacv1alpha1.NonAdminBackupStorageLocationRequest) if !ok { + // This should never happen - log for debugging if it does + logger.V(1).Info("Unexpected: failed to assert object as NonAdminBackupStorageLocationRequest") return false } return updatePhaseIfNeeded(&req.Status.Phase, req.Spec.ApprovalDecision) }); updateErr != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/nonadminbackupstoragelocation_controller.go` around lines 649 - 653, The type assertion for obj to *nacv1alpha1.NonAdminBackupStorageLocationRequest currently returns false silently on failure; add a debug log when !ok to record the unexpected type (e.g., log.Debugf or controllerLogger.V(1).Info) including the actual object type via fmt.Sprintf("%T", obj) and any identifying info, then return false as before. Update the block around the assertion (the req, ok := obj.(*nacv1alpha1.NonAdminBackupStorageLocationRequest) check that then calls updatePhaseIfNeeded) to emit this debug log before returning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/common/function/function_test.go`:
- Around line 381-385: The test includes a case named
"VolumeGroupSnapshotLabelKey" that references
BackupSpec.VolumeGroupSnapshotLabelKey which does not exist in the vendored
Velero v1.14.0; remove that test case (or replace it with a field present in
v1.14.0) so the completeness check in function_test.go (the reflection-based
validation around lines 460-475) no longer expects the nonexistent field;
alternatively, if you intend to keep the test, upgrade the Velero dependency to
v1.17.0+ so BackupSpec contains VolumeGroupSnapshotLabelKey and the reflection
check will pass.
In `@internal/controller/nonadminrestore_controller.go`:
- Around line 352-355: Add a unit test that exercises the code path which checks
slices.Contains(restoreSpec.ExcludedResources, constant.WildcardString) so we
verify the wildcard case is preserved: create a Restore (or restoreSpec) with
ExcludedResources set to []string{constant.WildcardString}, invoke the
controller logic that runs the exclusion-normalization (the code in
nonadminrestore_controller.go — the Reconcile / processing function that
contains the slices.Contains check), and assert that ExcludedResources still
equals []string{constant.WildcardString} (i.e., "volumesnapshotclasses" is not
appended). Ensure the test uses the same symbols restoreSpec.ExcludedResources
and constant.WildcardString for clarity.
---
Nitpick comments:
In `@internal/controller/nonadminbackupstoragelocation_controller.go`:
- Around line 649-653: The type assertion for obj to
*nacv1alpha1.NonAdminBackupStorageLocationRequest currently returns false
silently on failure; add a debug log when !ok to record the unexpected type
(e.g., log.Debugf or controllerLogger.V(1).Info) including the actual object
type via fmt.Sprintf("%T", obj) and any identifying info, then return false as
before. Update the block around the assertion (the req, ok :=
obj.(*nacv1alpha1.NonAdminBackupStorageLocationRequest) check that then calls
updatePhaseIfNeeded) to emit this debug log before returning.
🪄 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: 26d8ffa8-a72c-4d48-8e49-cecc5e777810
📒 Files selected for processing (6)
internal/common/constant/constant.gointernal/common/function/function_test.gointernal/controller/nonadminbackup_controller.gointernal/controller/nonadminbackup_controller_test.gointernal/controller/nonadminbackupstoragelocation_controller.gointernal/controller/nonadminrestore_controller.go
✅ Files skipped from review due to trivial changes (1)
- internal/common/constant/constant.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/controller/nonadminbackup_controller_test.go
| { | ||
| name: "VolumeGroupSnapshotLabelKey", | ||
| enforcedValue: "enforced-label-key", | ||
| overrideValue: "user-label-key", | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify VolumeGroupSnapshotLabelKey field exists in Velero BackupSpec
# Check go.mod for Velero version
echo "=== Velero version in go.mod ==="
rg -n 'github.com/vmware-tanzu/velero' go.mod
# Search for VolumeGroupSnapshotLabelKey usage in the codebase
echo -e "\n=== VolumeGroupSnapshotLabelKey references ==="
rg -n 'VolumeGroupSnapshotLabelKey'
# Verify the field exists in imported Velero types
echo -e "\n=== BackupSpec struct fields (if vendored) ==="
fd -t f 'backup_types.go' --exec grep -A 100 'type BackupSpec struct' {} \; 2>/dev/null | head -150Repository: migtools/oadp-non-admin
Length of output: 736
🏁 Script executed:
#!/bin/bash
# Check the BackupSpec struct definition in the project
echo "=== Looking for BackupSpec struct in project imports ==="
rg -n 'type BackupSpec struct' --type go
# Check the imports in function_test.go to see which Velero version is being used
echo -e "\n=== Imports in function_test.go ==="
head -50 internal/common/function/function_test.go | grep -A 20 'import'
# Check the completeness check logic at lines 460-475
echo -e "\n=== Completeness check logic ==="
sed -n '455,480p' internal/common/function/function_test.goRepository: migtools/oadp-non-admin
Length of output: 1531
🏁 Script executed:
#!/bin/bash
# Check when VolumeGroupSnapshotLabelKey was added to Velero
echo "=== Searching Velero upstream for VolumeGroupSnapshotLabelKey ==="
# Try to find any Velero types file that might be in vendor or cached
fd -t f -name '*velero*' -type f | grep -E '(types|backup)' | head -20
# Check if there's a vendor directory with Velero types
echo -e "\n=== Checking for vendored Velero types ==="
find . -path '*/velero/pkg/apis/velero/v1/*' -name '*.go' 2>/dev/null | head -10
# Look for any BackupSpec references that show actual field names
echo -e "\n=== Looking for BackupSpec field references in YAML/config ==="
rg -A 50 'type.*BackupSpec' --type go 2>/dev/null | head -100Repository: migtools/oadp-non-admin
Length of output: 4242
🌐 Web query:
Velero VolumeGroupSnapshotLabelKey release version when added
💡 Result:
The VolumeGroupSnapshotLabelKey feature, which allows customizing the label key used by Velero to group PVCs for VolumeGroupSnapshots (default: velero.io/volume-group), was added in Velero v1.17.0. It is configurable via the Backup spec field volumeGroupSnapshotLabelKey, the server flag --volume-group-snapshot-label-key, or defaults to velero.io/volume-group.
Citations:
- 1: https://velero.io/docs/v1.18/volume-group-snapshots
- 2: Implement VoulumeGroupSnapshot (VGS) support in Velero velero-io/velero#8865
- 3: https://github.com/vmware-tanzu/velero/releases
- 4: Add support for configuring VGS label key velero-io/velero#8938
CRITICAL: Test case references a non-existent BackupSpec field in current Velero version.
The VolumeGroupSnapshotLabelKey field was added in Velero v1.17.0, but this project uses v1.14.0. The test case at lines 381-385 will fail when the completeness check (lines 460-475) attempts to validate the field against the actual velerov1.BackupSpec struct via reflection. The field does not exist in v1.14.0's BackupSpec.
Either upgrade Velero to v1.17.0 or later, or remove this test case.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/common/function/function_test.go` around lines 381 - 385, The test
includes a case named "VolumeGroupSnapshotLabelKey" that references
BackupSpec.VolumeGroupSnapshotLabelKey which does not exist in the vendored
Velero v1.14.0; remove that test case (or replace it with a field present in
v1.14.0) so the completeness check in function_test.go (the reflection-based
validation around lines 460-475) no longer expects the nonexistent field;
alternatively, if you intend to keep the test, upgrade the Velero dependency to
v1.17.0+ so BackupSpec contains VolumeGroupSnapshotLabelKey and the reflection
check will pass.
|
/unhold |
|
/hold testing... |
|
/unhold Confirmed it's working |
|
/cherry-pick oadp-1.6 |
|
@mpryc: 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. |
|
@weshayutin should we also cherry pick to oadp-1.5 ? |
|
@weshayutin @shubham-pampattiwar @kaovilai @sseago @Joeavaikath May I ask for review & approval. Possibly we want to cherry pick that in the oadp 1.6 |
|
/cherry-pick oadp-1.6 |
|
@mpryc: 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. |
|
@mpryc before we merge a backport to 1.6 or 1.5 let's run through a real test and paste results from oadp-dev. |
| }, | ||
| addSyncLabel: true, | ||
| }), | ||
| ginkgo.Entry("Should not append default excluded resources when wildcard is used in excludedClusterScopedResources", nonAdminBackupFullReconcileScenario{ |
There was a problem hiding this comment.
For completeness, consider adding a test entry with ExcludedNamespaceScopedResources: ["*"]. Not a blocker.
There was a problem hiding this comment.
Added in second commit. Note I've also rebased this against latest oadp-dev, so had to force push.
| if updateErr := r.updateStatusWithRetry(ctx, logger, nabslRequest, func(obj client.Object) bool { | ||
| req := obj.(*nacv1alpha1.NonAdminBackupStorageLocationRequest) | ||
| req, ok := obj.(*nacv1alpha1.NonAdminBackupStorageLocationRequest) | ||
| if !ok { |
There was a problem hiding this comment.
Consider logging a warning when the assertion fails so it doesn't mask programming errors.
There was a problem hiding this comment.
Added in second commit. Note I've also rebased this against latest oadp-dev, so had to force push.
|
|
||
| restoreSpec.ExcludedResources = append(restoreSpec.ExcludedResources, | ||
| "volumesnapshotclasses") | ||
| if !slices.Contains(restoreSpec.ExcludedResources, constant.WildcardString) { |
There was a problem hiding this comment.
Consider adding a restore test case with ExcludedResources: ["*"]
There was a problem hiding this comment.
Added in second commit. Note I've also rebased this against latest oadp-dev, so had to force push.
When a user sets excludedClusterScopedResources or excludedNamespaceScopedResources to '*', skip appending default resources to avoid Velero FailedValidation. add VolumeGroupSnapshotLabelKey to enforced backup spec tests to allow tests passing. fix lint issues. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Michal Pryc <mpryc@redhat.com>
- Add backup test for ExcludedNamespaceScopedResources: ["*"] to verify default excluded namespace-scoped resources are not appended when wildcard is present - Add restore test for ExcludedResources: ["*"] to verify volumesnapshotclasses is not appended when wildcard is present - Log warning in BSL controller when type assertion to NonAdminBackupStorageLocationRequest fails Signed-off-by: Michal Pryc <mpryc@redhat.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mpryc 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 |
1 similar comment
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mpryc 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 |
6 participants
When a user sets excludedClusterScopedResources or excludedNamespaceScopedResources to '*', skip appending default resources to avoid Velero FailedValidation.
Why the changes were made
Fix: OADP-7562
How to test the changes made
Unit test was added to cover bug scenario.
Summary by CodeRabbit
Bug Fixes
Tests
Chores