Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Make configmapref check case insensitive #6804

Merged
merged 2 commits into from
Oct 26, 2023
Merged

Make configmapref check case insensitive #6804

merged 2 commits into from
Oct 26, 2023

Conversation

anshulahuja98
Copy link
Collaborator

Thank you for contributing to Velero!

Please add a summary of your change

For resource modifier and resource policies, make the check for configmap reference case insensitive.

Does your change fix a particular issue?

Fixes #(issue)

Please indicate you've done the following:

  • Accepted the DCO. Commits without the DCO will delay acceptance.
  • [ X] Created a changelog file or added /kind changelog-not-required as a comment on this pull request.
  • Updated the corresponding documentation in site/content/docs/main.

@anshulahuja98
Copy link
Collaborator Author

/kind changelog-not-required

@github-actions github-actions bot added the kind/changelog-not-required PR does not require a user changelog. Often for docs, website, or build changes label Sep 12, 2023
@anshulahuja98
Make configmapref check case insensitive
5e94d91 
Signed-off-by: Anshul Ahuja <anshul.ahu@gmail.com>
@anshulahuja98 anshulahuja98 marked this pull request as ready for review September 12, 2023 06:35
@codecov
Copy link

codecov bot commented Sep 12, 2023 •
edited
Loading

Codecov Report

Merging #6804 (c58f2eb) into main (246831d) will increase coverage by 0.00%.
Report is 6 commits behind head on main.
The diff coverage is 50.00%.

@@           Coverage Diff           @@
##             main    #6804   +/-   ##
=======================================
  Coverage   60.45%   60.46%           
=======================================
  Files         242      242           
  Lines       26031    26033    +2     
=======================================
+ Hits        15738    15740    +2     
  Misses       9187     9187           
  Partials     1106     1106
Files Changed Coverage Δ
pkg/controller/backup_controller.go 61.05% <0.00%> (ø)
pkg/controller/restore_controller.go 65.96% <100.00%> (ø)

... and 5 files with indirect coverage changes

mateusoliveira43
mateusoliveira43 reviewed Sep 12, 2023
View reviewed changes
pkg/controller/backup_controller.go
@@ -476,7 +477,7 @@ func (b *backupReconciler) prepareBackupRequest(backup *velerov1api.Backup, logg
request.Status.ValidationErrors = append(request.Status.ValidationErrors, "encountered labelSelector as well as orLabelSelectors in backup spec, only one can be specified")
}

if request.Spec.ResourcePolicy != nil && request.Spec.ResourcePolicy.Kind == resourcepolicies.ConfigmapRefType {
if request.Spec.ResourcePolicy != nil && strings.EqualFold(request.Spec.ResourcePolicy.Kind, resourcepolicies.ConfigmapRefType) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to add a simple test just to check this new behavior?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the test case for resourcemodifier, that should be enough coverage.
For resourcepolicies, it is not trivial to add for now, since there are no controller side tests from what I can see.

update resourcemodfier test case to validate case
c58f2eb 
Signed-off-by: Anshul Ahuja <anshulahuja@microsoft.com>
@reasonerjt
Copy link
Contributor

@anshulahuja98
What's the reason to make it case-insensitive?

@anshulahuja98
Copy link
Collaborator Author

@anshulahuja98 What's the reason to make it case-insensitive?

When adding the reference through CLI it doesn't matter, but if we try using backupcr / restore CR, it is easy to make a case choice such as "ConfigMap", and that will not log any error, it will lead to skipping of resource mod/policies silently.

It's a minor change just for ease and avoiding mistakes.

@anshulahuja98
Copy link
Collaborator Author

Do you see any concern with this @reasonerjt ?
it's a minor change...

@shubham-pampattiwar
Copy link
Collaborator

@reasonerjt Any concerns with this PR ? We encountered this issue in our testing as well.

@shubham-pampattiwar
Copy link
Collaborator

@anshulahuja98 It would be really great to get this in for velero 1.12.2

@anshulahuja98 anshulahuja98 merged commit 20a1118 into vmware-tanzu:main Oct 26, 2023
anshulahuja98 added a commit to anshulahuja98/velero that referenced this pull request Oct 26, 2023
@anshulahuja98
Make configmapref check case insensitive (vmware-tanzu#6804)
341a429 
* Make configmapref check case insensitive

Signed-off-by: Anshul Ahuja <anshul.ahu@gmail.com>

* update resourcemodfier test case to validate case

Signed-off-by: Anshul Ahuja <anshulahuja@microsoft.com>

---------

Signed-off-by: Anshul Ahuja <anshul.ahu@gmail.com>
Signed-off-by: Anshul Ahuja <anshulahuja@microsoft.com>
Co-authored-by: Anshul Ahuja <anshulahuja@microsoft.com>
sseago pushed a commit that referenced this pull request Oct 26, 2023
@anshulahuja98
Make configmapref check case insensitive (#6804) (#7023)
7b76047 
* Make configmapref check case insensitive



* update resourcemodfier test case to validate case



---------

Signed-off-by: Anshul Ahuja <anshul.ahu@gmail.com>
Signed-off-by: Anshul Ahuja <anshulahuja@microsoft.com>
Co-authored-by: Anshul Ahuja <anshulahuja@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mateusoliveira43 mateusoliveira43 mateusoliveira43 left review comments

@shubham-pampattiwar shubham-pampattiwar shubham-pampattiwar approved these changes

@kaovilai kaovilai kaovilai approved these changes

@sseago sseago sseago approved these changes

@reasonerjt reasonerjt Awaiting requested review from reasonerjt

@ywk253100 ywk253100 Awaiting requested review from ywk253100

Assignees

@anshulahuja98 anshulahuja98

Labels
has-unit-tests kind/changelog-not-required PR does not require a user changelog. Often for docs, website, or build changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@anshulahuja98 @reasonerjt @shubham-pampattiwar @sseago @kaovilai @mateusoliveira43