-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Backup warning for inclusion of NS managed by ArgoCD #8257
Add Backup warning for inclusion of NS managed by ArgoCD #8257
Conversation
b9371c8
to
86d0f0d
Compare
86d0f0d
to
23c28af
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8257 +/- ##
==========================================
- Coverage 59.21% 58.96% -0.25%
==========================================
Files 367 368 +1
Lines 30841 39000 +8159
==========================================
+ Hits 18262 22996 +4734
- Misses 11119 14539 +3420
- Partials 1460 1465 +5 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
} | ||
|
||
nsLabels := ns.GetLabels() | ||
if len(nsLabels[ArgoCDManagedByNamespaceLabel]) > 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we check if label exist here instead of checking if its value is not an empty string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would probably be better.. as long as we know it is possible label only need key and empty value is ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rationale here is the having an empty value for the ArgoCD label key would not make much sense, so this acts as a additional check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While it may not make sense, we need to match whatever argocd logic is regarding namespace it manages. If it disallows empty value label then current code is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the value here should never be empty according to docs. Value would be namespace where argocd instance lives. An Argo CD instance always live in a namespace, and namespaces are never empty string.
We can just check if length is > 0 in this case.
af65ba2
to
272b77e
Compare
272b77e
to
8134c57
Compare
8134c57
to
67732a7
Compare
67732a7
to
dceaf7d
Compare
Signed-off-by: Shubham Pampattiwar <[email protected]> add changelog file Signed-off-by: Shubham Pampattiwar <[email protected]> run make update Signed-off-by: Shubham Pampattiwar <[email protected]> re-position import Signed-off-by: Shubham Pampattiwar <[email protected]> update argo cd label comment Signed-off-by: Shubham Pampattiwar <[email protected]> add nil check for backupRequest.Spec.IncludedNamespaces Signed-off-by: Shubham Pampattiwar <[email protected]> minor fix Signed-off-by: Shubham Pampattiwar <[email protected]> fix edge cases Signed-off-by: Shubham Pampattiwar <[email protected]> add gh issue link in code comments Signed-off-by: Shubham Pampattiwar <[email protected]>
dceaf7d
to
738bb79
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
aed944c
into
vmware-tanzu:main
Thank you for contributing to Velero!
Please add a summary of your change
This change adds a warning to velero backup operation if there are namespaces included in the backup which are managed by ArgoCD.
We’ve observed conflicts between Velero and ArgoCD in such environments and produce undesirable outcomes. This backup warning serves as a notification to ensure users are aware of the potential problems.
Does your change fix a particular issue?
Fix Related to #7905
Please indicate you've done the following:
make new-changelog
) or comment/kind changelog-not-required
on this PR.site/content/docs/main
.