Validate namespace in velero backup create command #4057
Merged
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
reasonerjt
added
the
kind/release-blocker
zubron
suggested changes
Aug 31, 2021
There was a problem hiding this comment.
Thanks, @codegold79 - this is looking really good so far! I just have a couple of suggestions to expand the test coverage and also use the collections.ValidateNamespaceIncludesExcludes function in the CLI to avoid code duplication 👍
Signed-off-by: F. Gold <fgold@vmware.com>
Signed-off-by: F. Gold <fgold@vmware.com>
Signed-off-by: F. Gold <fgold@vmware.com>
Signed-off-by: F. Gold <fgold@vmware.com>
…to read Signed-off-by: F. Gold <fgold@vmware.com>
Signed-off-by: F. Gold <fgold@vmware.com>
- use one set of namespace validation logic instead of writing two - remove duplicate namespace validation functions and tests - add namespace validation tests in includes_excludes_test.go Signed-off-by: F. Gold <fgold@vmware.com>
codegold79
force-pushed
the
2609-validate-namespace-in-velero-backup-create-command
branch
from
e42249b
to
ec5f0a6
Compare
dsu-igeek
previously requested changes
Sep 1, 2021
Signed-off-by: F. Gold <fgold@vmware.com>
zubron
suggested changes
Sep 2, 2021
There was a problem hiding this comment.
Thanks for updating this again, @codegold79! I have one more request to help with the clarity of error messages in the output.
Signed-off-by: F. Gold <fgold@vmware.com>
zubron
approved these changes
Sep 2, 2021
reasonerjt
approved these changes
Sep 3, 2021
zubron
dismissed
dsu-igeek’s stale review
The requested changes were addressed in 694a64c.
This was referenced Oct 18, 2021
|
@codegold79 This change breaks pure cluster-scoped resource backups using incl_clusterresources=True with incl_namespaces=[""] which used to be working until Velero 1.6. Is there an alternative for this notation - ideally one that works for previous versions, too? Maybe excl_namespaces="*" ? |
Merged
3 tasks
danfengliu
pushed a commit
to danfengliu/velero
that referenced
this pull request
Jan 25, 2022
* Add namespace validation in the client Signed-off-by: F. Gold <fgold@vmware.com> * Add namespace validation in the backup controller Signed-off-by: F. Gold <fgold@vmware.com> * Add changelog for PR 4057 Signed-off-by: F. Gold <fgold@vmware.com> * Update Copyright notice Signed-off-by: F. Gold <fgold@vmware.com> * Update include_excludes_test.go to follow Go standards and be easier to read Signed-off-by: F. Gold <fgold@vmware.com> * Add unit tests for namespace validation functions Signed-off-by: F. Gold <fgold@vmware.com> * Make changes per review comments - use one set of namespace validation logic instead of writing two - remove duplicate namespace validation functions and tests - add namespace validation tests in includes_excludes_test.go Signed-off-by: F. Gold <fgold@vmware.com> * Return all ns validation err msgs as error list Signed-off-by: F. Gold <fgold@vmware.com> * Make error message more clear Signed-off-by: F. Gold <fgold@vmware.com>
gyaozhou
pushed a commit
to gyaozhou/velero-read
that referenced
this pull request
May 14, 2022
* Add namespace validation in the client Signed-off-by: F. Gold <fgold@vmware.com> * Add namespace validation in the backup controller Signed-off-by: F. Gold <fgold@vmware.com> * Add changelog for PR 4057 Signed-off-by: F. Gold <fgold@vmware.com> * Update Copyright notice Signed-off-by: F. Gold <fgold@vmware.com> * Update include_excludes_test.go to follow Go standards and be easier to read Signed-off-by: F. Gold <fgold@vmware.com> * Add unit tests for namespace validation functions Signed-off-by: F. Gold <fgold@vmware.com> * Make changes per review comments - use one set of namespace validation logic instead of writing two - remove duplicate namespace validation functions and tests - add namespace validation tests in includes_excludes_test.go Signed-off-by: F. Gold <fgold@vmware.com> * Return all ns validation err msgs as error list Signed-off-by: F. Gold <fgold@vmware.com> * Make error message more clear Signed-off-by: F. Gold <fgold@vmware.com>
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.
Please add a summary of your change
Added validation of namespaces included and excluded when creating Velero backups. The validatation was added both in the client and the server (the backup controller).
Does your change fix a particular issue?
Fixes #2690. In this issue, it was reported that when a forward slash was used in an included namespace for a backup, the backup proceeded instead of gracefully failing with useful information to the user.
Please indicate you've done the following:
/kind changelog-not-required.site/content/docs/main.