deep copy slice object during append #242

kmala · 2023-07-07T00:15:49Z

Since p is slice, its value is changed during the tree traversal(iterate). Hence use deep copy instead.

fixes kubernetes/kubernetes#119141

k8s-ci-robot · 2023-07-07T00:15:57Z

Welcome @kmala!

It looks like this is your first PR to kubernetes-sigs/structured-merge-diff 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/structured-merge-diff has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2023-07-07T00:15:58Z

Hi @kmala. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

torredil · 2023-07-07T00:22:48Z

/ok-to-test

dims · 2023-07-07T00:40:45Z

/assign @liggitt

apelisse

Thanks, that looks good. A few comments for simplifying the test!

apelisse · 2023-07-07T15:49:47Z

merge/conflict_test.go

+	type args struct {
+		sets fieldpath.ManagedFields
+	}
+	tests := []struct {
+		name string
+		args args
+		want string
+	}{


Since you have only on test I think it's fine to remove the table-test. Also this repo generally doesn't use that format for table-tests.

apelisse · 2023-07-07T15:50:08Z

merge/conflict_test.go

+							_P("spec", "template", "spec", "containers", _KBF("name", "probe"), "livenessProbe", "exec", "command"),
+							_P("spec", "template", "spec", "containers", _KBF("name", "probe"), "livenessProbe", "periodSeconds"),
+							_P("spec", "template", "spec", "containers", _KBF("name", "probe"), "readinessProbe", "exec", "command"),
+							_P("spec", "template", "spec", "containers", _KBF("name", "probe"), "readinessProbe", "periodSeconds"),


Can we make these shorter? The simplest test to reproduce is usually best.

apelisse · 2023-07-07T15:50:47Z

merge/conflict_test.go

+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := merge.ConflictsFromManagers(tt.args.sets); !reflect.DeepEqual(got.Error(), tt.want) {


You don't need reflect.DeepEqual to compare strings.

kmala · 2023-07-07T17:10:47Z

A few comments for simplifying the test
@apelisse i had simplified the test, can you please review it again.

apelisse · 2023-07-07T17:28:03Z

merge/conflict_test.go

@@ -92,3 +92,23 @@ func TestToSet(t *testing.T) {
 		t.Fatalf("expected\n%v\n, but got\n%v\n", expected, actual)
 	}
 }
+
+func TestConflictsFromManagers(t *testing.T) {
+


Suggested change

apelisse · 2023-07-07T17:28:18Z

merge/conflict_test.go

+				_P("obj", "template", "obj", "list", _KBF("name", "a"), "id"),
+				_P("obj", "template", "obj", "list", _KBF("name", "a"), "key"),


Are these the simplest form that trigger the bug?!

yes, this is the simplest which triggers the bug.

@apelisse can you please review again when you get a chance

Tusenka · 2023-07-10T05:50:39Z

Could you explain why this PR will fix the issue. What is the cause of the issue?

kmala · 2023-07-10T16:49:53Z

Could you explain why this PR will fix the issue. What is the cause of the issue?

This will fix the issue because on apply this function gets called and it returns the wrong result. The cause as i mentioned in the description is because of doing shallow copy slice during iterate.

apelisse

/lgtm
/approve

k8s-ci-robot · 2023-07-10T17:13:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: apelisse, kmala

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [apelisse]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

deep copy slice object during append

24aa83d

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 7, 2023

k8s-ci-robot requested review from apelisse and jennybuckley July 7, 2023 00:15

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 7, 2023

kmala mentioned this pull request Jul 7, 2023

Kubernetes api returns incorrect conflicting fields during server-side apply in some cases kubernetes/kubernetes#119141

Closed

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 7, 2023

k8s-ci-robot assigned liggitt Jul 7, 2023

apelisse reviewed Jul 7, 2023

View reviewed changes

simplify test

6371a4d

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 7, 2023

apelisse reviewed Jul 7, 2023

View reviewed changes

address review comment

69bc7e7

apelisse reviewed Jul 10, 2023

View reviewed changes

k8s-ci-robot assigned apelisse Jul 10, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 10, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 10, 2023

k8s-ci-robot merged commit 7785c50 into kubernetes-sigs:master Jul 10, 2023

kmala mentioned this pull request Jul 14, 2023

update the dependency sigs.k8s.io/structured-merge-diff/v4 to latest tag kubernetes/kubernetes#119334

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deep copy slice object during append #242

deep copy slice object during append #242

kmala commented Jul 7, 2023

k8s-ci-robot commented Jul 7, 2023

k8s-ci-robot commented Jul 7, 2023

torredil commented Jul 7, 2023

dims commented Jul 7, 2023

apelisse left a comment

apelisse Jul 7, 2023

apelisse Jul 7, 2023

apelisse Jul 7, 2023

kmala commented Jul 7, 2023

apelisse Jul 7, 2023

kmala Jul 7, 2023

apelisse Jul 7, 2023

kmala Jul 7, 2023

kmala Jul 10, 2023

Tusenka commented Jul 10, 2023

kmala commented Jul 10, 2023

apelisse left a comment

k8s-ci-robot commented Jul 10, 2023

		_P("obj", "template", "obj", "list", _KBF("name", "a"), "id"),
		_P("obj", "template", "obj", "list", _KBF("name", "a"), "key"),

deep copy slice object during append #242

deep copy slice object during append #242

Conversation

kmala commented Jul 7, 2023

k8s-ci-robot commented Jul 7, 2023

k8s-ci-robot commented Jul 7, 2023

torredil commented Jul 7, 2023

dims commented Jul 7, 2023

apelisse left a 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

kmala commented Jul 7, 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

Tusenka commented Jul 10, 2023

kmala commented Jul 10, 2023

apelisse left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 10, 2023