support JSON Merge Patch and Strategic Merge Patch in Resource Modifiers #6917

27149chen · 2023-10-04T07:19:08Z

Please add a summary of your change

Support JSON Merge Patch and Strategic Merge Patch in Resource Modifiers, implement #6797

Does your change fix a particular issue?

Fixes #6728

Please indicate you've done the following:

Accepted the DCO. Commits without the DCO will delay acceptance.
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.

Signed-off-by: lou <[email protected]>

codecov · 2023-10-04T08:24:34Z

Codecov Report

Merging #6917 (e309375) into main (b316101) will increase coverage by 0.41%.
Report is 59 commits behind head on main.
The diff coverage is 63.03%.

@@            Coverage Diff             @@
##             main    #6917      +/-   ##
==========================================
+ Coverage   60.69%   61.11%   +0.41%     
==========================================
  Files         249      255       +6     
  Lines       26493    27072     +579     
==========================================
+ Hits        16081    16545     +464     
- Misses       9268     9348      +80     
- Partials     1144     1179      +35

Files	Coverage Δ
.../resourcemodifiers/resource_modifiers_validator.go	`100.00% <100.00%> (ø)`
pkg/restore/restore.go	`65.77% <0.00%> (+1.38%)`	⬆️
internal/resourcemodifiers/json_merge_patch.go	`55.00% <55.00%> (ø)`
internal/resourcemodifiers/json_patch.go	`81.48% <81.48%> (ø)`
internal/resourcemodifiers/resource_modifiers.go	`71.92% <75.47%> (-3.73%)`	⬇️
...nternal/resourcemodifiers/strategic_merge_patch.go	`36.61% <36.61%> (ø)`

... and 34 files with indirect coverage changes

Signed-off-by: lou <[email protected]>

go.mod

Signed-off-by: lou <[email protected]>

internal/resourcemodifiers/resource_modifiers.go

internal/resourcemodifiers/resource_modifiers_test.go

Signed-off-by: lou <[email protected]>

anshulahuja98 · 2023-10-10T09:26:29Z

@27149chen can you also please get patch code coverage to 60%

Signed-off-by: lou <[email protected]>

27149chen · 2023-10-10T14:53:57Z

@27149chen can you also please get patch code coverage to 60%

done

design/merge-patch-and-strategic-in-resource-modifier.md

anshulahuja98 · 2023-10-16T05:23:36Z

internal/resourcemodifiers/resource_modifiers.go

 	namespaceInclusion := collections.NewIncludesExcludes().Includes(r.Conditions.Namespaces...)
 	if !namespaceInclusion.ShouldInclude(obj.GetNamespace()) {
 		return nil
 	}

-	if r.Conditions.GroupResource != groupResource {
+	g, err := glob.Compile(r.Conditions.GroupResource)


one thing came in my mind
if we support globbing with namespace filter.

Scenario -> user provided NS filter, with path glob as blah*
Now blah* can contain both cluster scope and namespaced scope CRs.
We need to make it clear in docs that if user wants to do cluster scope they have to put NS filter as empty otherwise it won't work
and using it in conjunction with GroupResource glob path will have these further nuances.

we do support namespace glob

if user enters namespaces: ["blah*"], only namespaced resources in namespace started with blah will be matched

yeah
again the limitation is cluster scope won't be taken care of

sorry, not quite understand, can you give an example?

user provided NS filter (default), with GroupResource glob as blah*
Now blah* can contain both cluster scope and namespaced scope CRs.
We need to make it clear in docs that if user wants to do cluster scope they have to put NS filter as empty otherwise it won't work
and using it in conjunction with GroupResource glob path will have these further nuances.

Reworded my initial comment to make it more clear

" I think including the cluster resources by default is reasonable"
I feel this would lead to changes which are not intended.
I am worried about this.

In terms of REAL filters here - similar to what we have in backup /restore - includeClusterSCopeResources
We can add a bool for that.
that would make it consistent and give control to user.

This is only useful for globbed patterns
but would lead to confusion if explicit GroupResource is mentioned.

On further thought, I think it's fine.
But PLEASE just add this behaviour of cluster scope by default to docuemtnation.

Just to reiterate, I am fine with current changes you have. Just that I request you to document the behaviour of cluster scope by default in case of globbed GroupResourcce.

thanks, updated

Signed-off-by: lou <[email protected]>

internal/resourcemodifiers/resource_modifiers.go

internal/resourcemodifiers/resource_modifiers_test.go

anshulahuja98 · 2023-10-19T05:17:31Z

@ywk253100 I am done with my review
Some minor comments are pending fix - you can review from your end.

internal/resourcemodifiers/resource_modifiers_test.go

ywk253100 · 2023-10-24T03:23:59Z

I'm good for the changes.
@27149chen Could you also update the user guide about the update?

Signed-off-by: lou <[email protected]>

27149chen · 2023-10-25T00:57:38Z

I'm good for the changes. @27149chen Could you also update the user guide about the update?

done

site/content/docs/main/restore-resource-modifiers.md

anshulahuja98 · 2023-10-25T07:04:05Z

internal/resourcemodifiers/resource_modifiers.go

 	namespaceInclusion := collections.NewIncludesExcludes().Includes(r.Conditions.Namespaces...)
 	if !namespaceInclusion.ShouldInclude(obj.GetNamespace()) {
 		return nil
 	}

-	if r.Conditions.GroupResource != groupResource {
+	g, err := glob.Compile(r.Conditions.GroupResource)


Hey I am not sure if ignoring ns filter is the right way
I am wondering if we should add another flag which says includeClusterSCope similar to what we have in backup/restore APIs.

anshulahuja98 · 2023-10-25T07:05:41Z

site/content/docs/main/restore-resource-modifiers.md

+- The above configmap will apply the Merge Patch to all the pods in namespace ns1 and remove the annotation `foo` from the pods.
+- Both json and yaml format are supported for the patchData.
+
+#### Strategic Merge Patch


we should perhaps move these 2 sections above

this is currently below jsonpatch examples and adbacned scenarios.

We should probably refactor this whole doc to make things more clear.

we can refactor it later, I don't want to do it in this pr

anshulahuja98 · 2023-10-25T07:06:24Z

site/content/docs/main/restore-resource-modifiers.md

+        }
+      }
+```
+- The above configmap will apply the Merge Patch to all the pods in namespace ns1 and remove the annotation `foo` from the pods.


add links to kubectl merge patch docs / other docs for reference. YOU can even provide sample kubectl command for use to test out in local setup before creating CM.

added doc link

site/content/docs/main/restore-resource-modifiers.md

Signed-off-by: lou <[email protected]>

anshulahuja98 · 2023-10-26T04:24:11Z

#6917 (comment)

Hey I am not sure if ignoring ns filter is the right way
I am wondering if we should add another flag which says includeClusterSCope similar to what we have in backup/restore APIs.

@27149chen

Signed-off-by: lou <[email protected]>

support JSON Merge Patch and Strategic Merge Patch in Resource Modifiers

support JSON Merge Patch and Strategic Merge Patch in Resource Modifiers Signed-off-by: lou <[email protected]>

Cherry-pick #6917 to release 1.12

support JSON Merge Patch and Strategic Merge Patch in Resource Modifiers

d8b9328

Signed-off-by: lou <[email protected]>

github-actions bot assigned 27149chen Oct 4, 2023

github-actions bot requested review from qiuming-best and sseago October 4, 2023 07:19

add changelog

06ed9dc

Signed-off-by: lou <[email protected]>

github-actions bot added Dependencies Pull requests that update a dependency file has-changelog labels Oct 4, 2023

fix lint

58d8425

Signed-off-by: lou <[email protected]>

anshulahuja98 self-requested a review October 5, 2023 06:17

kaovilai reviewed Oct 5, 2023

View reviewed changes

go.mod Outdated Show resolved Hide resolved

update after review

e880c0d

Signed-off-by: lou <[email protected]>

anshulahuja98 requested changes Oct 9, 2023

View reviewed changes

danfengliu requested a review from ywk253100 October 10, 2023 07:25

update after review

5932e26

Signed-off-by: lou <[email protected]>

27149chen added 3 commits October 10, 2023 18:59

add deserialization tests

65082f3

Signed-off-by: lou <[email protected]>

Merge branch 'main' into rm-improvement

19d5bee

update design

a607810

Signed-off-by: lou <[email protected]>

github-actions bot added the Area/Design Design Documents label Oct 10, 2023

add more tests

6d89780

Signed-off-by: lou <[email protected]>

anshulahuja98 requested changes Oct 16, 2023

View reviewed changes

design/merge-patch-and-strategic-in-resource-modifier.md Outdated Show resolved Hide resolved

design/merge-patch-and-strategic-in-resource-modifier.md Show resolved Hide resolved

anshulahuja98 requested changes Oct 16, 2023

View reviewed changes

design/merge-patch-and-strategic-in-resource-modifier.md Outdated Show resolved Hide resolved

anshulahuja98 requested changes Oct 16, 2023

View reviewed changes

update after review

d1f5219

Signed-off-by: lou <[email protected]>

anshulahuja98 requested changes Oct 19, 2023

View reviewed changes

internal/resourcemodifiers/resource_modifiers_test.go Show resolved Hide resolved

update after review

4ead4d6

Signed-off-by: lou <[email protected]>

github-actions bot added the Documentation label Oct 24, 2023

anshulahuja98 requested changes Oct 25, 2023

View reviewed changes

update docs

f66016d

Signed-off-by: lou <[email protected]>

update after review

e309375

Signed-off-by: lou <[email protected]>

anshulahuja98 approved these changes Nov 1, 2023

View reviewed changes

anshulahuja98 requested a review from kaovilai November 1, 2023 16:13

ywk253100 approved these changes Nov 2, 2023

View reviewed changes

ywk253100 merged commit 73c948d into vmware-tanzu:main Nov 2, 2023
24 checks passed

27149chen pushed a commit to 27149chen/velero that referenced this pull request Nov 2, 2023

Merge pull request vmware-tanzu#6917 from 27149chen/rm-improvement

d439f39

support JSON Merge Patch and Strategic Merge Patch in Resource Modifiers

anshulahuja98 mentioned this pull request Nov 2, 2023

Resource Modifier Follow-on improvements #7050

Open

27149chen pushed a commit to 27149chen/velero that referenced this pull request Nov 15, 2023

Merge pull request vmware-tanzu#6917 from 27149chen/rm-improvement

d7b4b0a

support JSON Merge Patch and Strategic Merge Patch in Resource Modifiers Signed-off-by: lou <[email protected]>

Lyndon-Li added a commit that referenced this pull request Nov 17, 2023

Merge pull request #7049 from 27149chen/cherry-pick-6917

c042d47

Cherry-pick #6917 to release 1.12

27149chen mentioned this pull request Nov 20, 2023

[release-1.12 CP] Issue #6728: support JSON Merge Patch and Strategic Merge Patch in Resource Modifiers #7049

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support JSON Merge Patch and Strategic Merge Patch in Resource Modifiers #6917

support JSON Merge Patch and Strategic Merge Patch in Resource Modifiers #6917

27149chen commented Oct 4, 2023 •

edited

Loading

codecov bot commented Oct 4, 2023 •

edited

Loading

anshulahuja98 commented Oct 10, 2023

27149chen commented Oct 10, 2023

anshulahuja98 Oct 16, 2023

27149chen Oct 16, 2023 •

edited

Loading

anshulahuja98 Oct 17, 2023

27149chen Oct 17, 2023

anshulahuja98 Oct 19, 2023

anshulahuja98 Nov 1, 2023

anshulahuja98 Nov 1, 2023

anshulahuja98 Nov 1, 2023

anshulahuja98 Nov 1, 2023 •

edited

Loading

27149chen Nov 1, 2023

anshulahuja98 commented Oct 19, 2023

ywk253100 commented Oct 24, 2023

27149chen commented Oct 25, 2023

anshulahuja98 Oct 25, 2023

anshulahuja98 Oct 25, 2023

27149chen Oct 25, 2023

anshulahuja98 Oct 25, 2023

27149chen Oct 25, 2023

anshulahuja98 commented Oct 26, 2023

support JSON Merge Patch and Strategic Merge Patch in Resource Modifiers #6917

support JSON Merge Patch and Strategic Merge Patch in Resource Modifiers #6917

Conversation

27149chen commented Oct 4, 2023 • edited Loading

Please add a summary of your change

Does your change fix a particular issue?

Please indicate you've done the following:

codecov bot commented Oct 4, 2023 • edited Loading

Codecov Report

anshulahuja98 commented Oct 10, 2023

27149chen commented Oct 10, 2023

Choose a reason for hiding this comment

27149chen Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anshulahuja98 Nov 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anshulahuja98 commented Oct 19, 2023

ywk253100 commented Oct 24, 2023

27149chen commented Oct 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anshulahuja98 commented Oct 26, 2023

27149chen commented Oct 4, 2023 •

edited

Loading

codecov bot commented Oct 4, 2023 •

edited

Loading

27149chen Oct 16, 2023 •

edited

Loading

anshulahuja98 Nov 1, 2023 •

edited

Loading