fix Delta.DifferentAt behavior via changes to Path.Contains #20

echen-98 · 2021-06-04T19:00:13Z

Fixes issue aws-controllers-k8s/community#808

ack-bot · 2021-06-04T19:00:22Z

Hi @echen-98. Thanks for your PR.

I'm waiting for a aws-controllers-k8s 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.

echen-98 · 2021-06-04T19:08:10Z

pkg/compare/path.go

@@ -51,14 +51,30 @@ func (p Path) Pop() {
 	}
 }

-// Contains returns true if the supplied string appears within the Path
+// TODO: case sensitivity?


should we keep case-sensitive comparison?

I think so, yes. We expect case-sensitivity in all Go fields and I'd expect that these strings are reflections of Go fields.

Ok, removed TODO comment

RedbackThomson · 2021-06-04T19:17:03Z

/ok-to-test

RedbackThomson · 2021-06-04T21:20:31Z

pkg/compare/delta_test.go

+	require.True(d.DifferentAt("Baz.Y"))
+	require.False(d.DifferentAt("Y")) // there is no diff for top-level field "Y"
+	require.False(d.DifferentAt("Bar")) // diff exists but it was not added to Delta
+	require.False(d.DifferentAt("Baz.Y.Z")) // subject length exceeds length of diff Path
+	require.False(d.DifferentAt("Baz.Z"))  // matches Path top-level field but not sub-field


++ Super clear test cases! Love it!

agreed, excellent clarity @echen-98

RedbackThomson · 2021-06-04T21:21:00Z

pkg/compare/path.go

@@ -51,14 +51,30 @@ func (p Path) Pop() {
 	}
 }

-// Contains returns true if the supplied string appears within the Path
+// TODO: case sensitivity?


I think so, yes. We expect case-sensitivity in all Go fields and I'd expect that these strings are reflections of Go fields.

vijtrip2 · 2021-06-04T21:32:15Z

/approve

a-hilaly

Very nice, thanks Eric!
I left one comment below

a-hilaly · 2021-06-05T12:54:09Z

pkg/compare/path.go

+/* Contains returns true if the supplied string, delimited on ".", matches
+	p.parts up to the length of the supplied string.
+	e.g. if the Path p represents "A.B":
+		subject "A" -> true
+		subject "A.B" -> true
+		subject "A.B.C" -> false
+		subject "B" -> false
+		subject "A.C" -> false
+ */


Please use // not /** .. */ for muiltiline comments :)

You can also instruct godoc to generate testable examples in the package documentation: https://blog.golang.org/examples / https://blog.golang.org/godoc

jaypipes

Great stuff @echen-98 :) Thank you!

jaypipes · 2021-06-08T00:06:26Z

pkg/compare/delta_test.go

+	require.True(d.DifferentAt("Baz.Y"))
+	require.False(d.DifferentAt("Y")) // there is no diff for top-level field "Y"
+	require.False(d.DifferentAt("Bar")) // diff exists but it was not added to Delta
+	require.False(d.DifferentAt("Baz.Y.Z")) // subject length exceeds length of diff Path
+	require.False(d.DifferentAt("Baz.Z"))  // matches Path top-level field but not sub-field


agreed, excellent clarity @echen-98

jaypipes · 2021-06-08T00:08:40Z

pkg/compare/path.go

+//	Contains returns true if the supplied string, delimited on ".", matches
+//	p.parts up to the length of the supplied string.


femto-nit: not sure if it's your IDE or editor changing the above, but comments should have a single space (not \t) after the // and before the first letter of the comment...

Got it, will keep in mind for next time

jaypipes · 2021-06-08T00:09:41Z

/lgtm

ack-bot · 2021-06-08T00:09:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: echen-98, jaypipes, RedbackThomson, vijtrip2

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~~ [RedbackThomson,jaypipes,vijtrip2]

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

…rollers-k8s#20) Fixes issue aws-controllers-k8s/community#808

ack-bot requested review from RedbackThomson and vijtrip2 June 4, 2021 19:00

ack-bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 4, 2021

echen-98 commented Jun 4, 2021

View reviewed changes

ack-bot 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 Jun 4, 2021

RedbackThomson approved these changes Jun 4, 2021

View reviewed changes

ack-bot added the approved label Jun 4, 2021

echen-98 force-pushed the enhancements branch from 27cc085 to ebf31be Compare June 4, 2021 21:54

a-hilaly reviewed Jun 5, 2021

View reviewed changes

fix Delta.DifferentAt behavior via changes to Path.Contains

562010a

echen-98 force-pushed the enhancements branch from ebf31be to 562010a Compare June 7, 2021 22:53

jaypipes approved these changes Jun 8, 2021

View reviewed changes

ack-bot assigned jaypipes Jun 8, 2021

ack-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 8, 2021

ack-bot merged commit 974d6fb into aws-controllers-k8s:main Jun 8, 2021

jaypipes mentioned this pull request Jun 8, 2021

UserGroup support in ElastiCache aws-controllers-k8s/elasticache-controller#26

Merged

echen-98 mentioned this pull request Jun 16, 2021

address regression with User code, fix RG deletion test aws-controllers-k8s/elasticache-controller#35

Merged

RedbackThomson pushed a commit to RedbackThomson/ack-runtime that referenced this pull request Jun 21, 2021

fix Delta.DifferentAt behavior via changes to Path.Contains (aws-cont…

5fa1f0e

…rollers-k8s#20) Fixes issue aws-controllers-k8s/community#808

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix Delta.DifferentAt behavior via changes to Path.Contains #20

fix Delta.DifferentAt behavior via changes to Path.Contains #20

echen-98 commented Jun 4, 2021

ack-bot commented Jun 4, 2021

echen-98 Jun 4, 2021

RedbackThomson Jun 4, 2021

echen-98 Jun 4, 2021

RedbackThomson commented Jun 4, 2021

RedbackThomson Jun 4, 2021

jaypipes Jun 8, 2021

RedbackThomson Jun 4, 2021

vijtrip2 commented Jun 4, 2021

a-hilaly left a comment

a-hilaly Jun 5, 2021

a-hilaly Jun 5, 2021

jaypipes left a comment

jaypipes Jun 8, 2021

jaypipes Jun 8, 2021

echen-98 Jun 8, 2021

jaypipes commented Jun 8, 2021

ack-bot commented Jun 8, 2021

		// Contains returns true if the supplied string, delimited on ".", matches
		// p.parts up to the length of the supplied string.

fix Delta.DifferentAt behavior via changes to Path.Contains #20

fix Delta.DifferentAt behavior via changes to Path.Contains #20

Conversation

echen-98 commented Jun 4, 2021

ack-bot commented Jun 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RedbackThomson commented Jun 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vijtrip2 commented Jun 4, 2021

a-hilaly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaypipes 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

jaypipes commented Jun 8, 2021

ack-bot commented Jun 8, 2021