-
Notifications
You must be signed in to change notification settings - Fork 682
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
feat:Add support for wildcards for include/exclude backups #4904
base: main
Are you sure you want to change the base?
feat:Add support for wildcards for include/exclude backups #4904
Conversation
Please review @byronvoorbach |
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.
Hey @Bhavyajain21 thanks for your submission.
Can you please include some tests as well?
You can find the unit test file here: entities/backup/descriptor_test.go
and the acceptance test files here: test/modules/backup-*
Sure @parkerduckworth, I'll do that. Can you please assign this issue under my name? |
Added the tests, please review @parkerduckworth |
Updated the code. Please check @parkerduckworth |
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.
I've thought about it a bit more, and I don't think the current implementation is going to work. It's misleading -- it's not really a wildcard operator, it's just a prefix matcher.
This could cause some potential issues. If there are 100k class, all prefixed with the token Product
, as well as a class just called Product
, and the user simply wants to backup the class called Product
, the current implementation will back up all 100,001 classes, even if *
is not appended to the end of the class name.
Or for example, what happens when a user wants to specify the wildcard first, to match a suffix (*Products
)? Or a token which exists in the middle of the class name (Some*Class
)?
I think we will need to rework this a bit to be more comprehensive. There should definitely not be a prefix match if *
is not even supplied
Hey @parkerduckworth I've updated the code based on the review comments |
Quality Gate passedIssues Measures |
/claim #2481