fix issue#2413: treat namespaces with exclude label as excludedNamespaces #5178
Conversation
This file contains 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
allenxu404
changed the title
…amespaces Signed-off-by: allenxu404 <[email protected]>
blackpiglet
previously approved these changes
Aug 5, 2022
reasonerjt
reviewed
Aug 5, 2022
pkg/controller/backup_controller.go
Outdated
| @@ -436,6 +436,15 @@ func (c *backupController) prepareBackupRequest(backup *velerov1api.Backup) *pkg | |||
| request.Annotations[velerov1api.SourceClusterK8sMajorVersionAnnotation] = c.discoveryHelper.ServerVersion().Major | |||
| request.Annotations[velerov1api.SourceClusterK8sMinorVersionAnnotation] = c.discoveryHelper.ServerVersion().Minor | |||
|
|
|||
| // Add namespaces with label velero.io/exclude-from-backup=true into request.Spec.ExcludedNamespaces | |||
| // Essentially, adding the label velero.io/exclude-from-backup=true to a namespace would be equivalent to setting spec.ExcludedNamespaces | |||
| namespaces, excludeLabel := corev1api.NamespaceList{}, "velero.io/exclude-from-backup" | |||
There was a problem hiding this comment.
This is trivial but it seems we don't need excludeLabel as a variable.
There was a problem hiding this comment.
Would it be better if I move excludeLabel to pkg/controller/constants.go as a const or simply define it as const on the top of the code?
pkg/controller/backup_controller.go
Outdated
| // Add namespaces with label velero.io/exclude-from-backup=true into request.Spec.ExcludedNamespaces | ||
| // Essentially, adding the label velero.io/exclude-from-backup=true to a namespace would be equivalent to setting spec.ExcludedNamespaces | ||
| namespaces, excludeLabel := corev1api.NamespaceList{}, "velero.io/exclude-from-backup" | ||
| if err := c.kbClient.List(context.Background(), &namespaces, kbclient.MatchingLabels{excludeLabel: "true"}); err == nil { |
There was a problem hiding this comment.
This is not a validation error strictly speaking but I don't think it's right to eat the error.
Would there be a problem if we set it into request.Status.ValidationErrors and make it fail early and explicitly?
There was a problem hiding this comment.
Seems it's better to append the potential err into request.Status.ValidationErrors with explicit err message.
Signed-off-by: allenxu404 <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #5178 +/- ##
==========================================
+ Coverage 41.20% 41.60% +0.40%
==========================================
Files 212 215 +3
Lines 18666 18628 -38
==========================================
+ Hits 7692 7751 +59
+ Misses 10399 10299 -100
- Partials 575 578 +3
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
blackpiglet
approved these changes
Aug 8, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Thank you for contributing to Velero!
Please add a summary of your change
Does your change fix a particular issue?
Fixes #2413
Please indicate you've done the following:
/kind changelog-not-requiredas a comment on this pull request.site/content/docs/main.