feat: Adds ExtractItems option to include key fields #275

yongruilin · 2024-12-07T07:13:09Z

This PR adds an option AppendKeyFields to the ExtractItems function.

When doing extracting with a fieldpath set which doesn't include the key fields, with this option, the result would include the key field values. So that the extraction result would follow the schema without hitting key field missing errors.

Ref: #273

andrii-korotkov-verkada · 2024-12-07T10:21:01Z

typed/typed.go

+					}
+					for _, keyField := range *pe.Key {
+						keyName := keyField.Name
+						keyFieldPath := append(path[:i+1:i+1], fieldpath.PathElement{FieldName: &keyName})


We may have to make a copy of path, at least I've run into some issues when not doing so in my workaround.

The copy doesn't make the difference here as the fields in the PathElement are pointers and they would still be copied to the new path, so that the new path still points to the same fields in the PathElements.

I think what matters is creating a new string keyName := keyField.Name in the for-loop, so that the appended PathElement.FieldName won't be overwritten on the same address.

Or, could you share the issues you encounter when not doing copy?

Ah, maybe you are right. I have argoproj/gitops-engine@e881697, but I indeed didn't create a new string, so that might have been an issue.

I'm fine with setting capacity here to prevent any overwriting of underlying slice values. I also wouldn't mind an extra comment for future readers stating what happens here since this is use of the language that not all readers may pick up on easily.

I've added the comment.

jpbetz

Two suggestions then LGTM

jpbetz · 2024-12-09T23:31:33Z

typed/typed.go

+// ExtractItemsOptions is the list of all the options available when extracting items.
+type ExtractItemsOptions int
+
+const (
+	// AppendKeyFields means that when extracting items, the key field would also be included.
+	AppendKeyFields ExtractItemsOptions = iota
+)
+


Are we confident all future options will be flags? Alternative form that would handle arbitrary option params: https://github.com/kubernetes/kube-openapi/pull/519/files#diff-72f8260914152fe9557b05d756ec94854184645e991ff755855628428d6d7a18R410-R421

SG, updated using this format.

jpbetz · 2024-12-09T23:32:36Z

typed/typed.go

+		case AppendKeyFields:
+			tvPathSet, err := tv.ToFieldSet()
+			if err != nil {
+				continue


Can we include a comment explaining why continue is safe and correct here?

Added the comment.

jpbetz · 2024-12-09T23:38:55Z

typed/typed.go

+					}
+					for _, keyField := range *pe.Key {
+						keyName := keyField.Name
+						keyFieldPath := append(path[:i+1:i+1], fieldpath.PathElement{FieldName: &keyName})


I'm fine with setting capacity here to prevent any overwriting of underlying slice values. I also wouldn't mind an extra comment for future readers stating what happens here since this is use of the language that not all readers may pick up on easily.

leoluz · 2024-12-10T15:12:43Z

typed/typed.go

+			// It is safe to continue and skip appending the key fields, because
+			// if there is error to convert to fieldPath.Set, there is
+			// no way to find the key fields values by following the schema.
+			goto startExtrcation


Suggested change

goto startExtrcation

goto startExtraction

IMO, it would be preferable to return the error and avoid the goto instead of failing silently. I understand that it would change the signature and break consumers. Maybe a new function?

It is a good practice to expose the error. But I'm not a fan to add a new interface here. We should address the non-error return interface(RemoveItems,ExtractItems) in another PR. wdyt?

LGTM. 👍🏻

typed/typed.go

typed/remove_test.go

leoluz · 2024-12-10T20:42:14Z

typed/typed.go

+	}
+	if options.appendKeyFields {
+		tvPathSet, err := tv.ToFieldSet()
+		if err != nil {


What if we do if err == nil { <Add the logic to set the keyname here> } ?
This way I think we could get rid of the goto.
Wdyt?

Sounds good. Updated.

jpbetz · 2024-12-10T23:06:58Z

typed/typed.go

@@ -187,7 +201,37 @@ func (tv TypedValue) RemoveItems(items *fieldpath.Set) *TypedValue {
 }

 // ExtractItems returns a value with only the provided list or map items extracted from the value.
-func (tv TypedValue) ExtractItems(items *fieldpath.Set) *TypedValue {
+func (tv TypedValue) ExtractItems(items *fieldpath.Set, opts ...extractItemsOption) *TypedValue {


Suggested change

func (tv TypedValue) ExtractItems(items *fieldpath.Set, opts ...extractItemsOption) *TypedValue {

func (tv TypedValue) ExtractItems(items *fieldpath.Set, opts ...ExtractItemsOption) *TypedValue {

Requiring a non-exported type as an argument of an exported function is surprising.

EDIT: I don't think this is strictly required to be usable from a separate package, but I can't think of any reason not to export the option function type (it's not possible to modify the option type since it is unexported). But let me know if there is something I'm not thinking of.

My original thought was that putting it non-exported can force user to only use the exported Withxxx() function to construct the options. But it indeed looks strange to put it in the exported function.

Updated.

Adds an option to `ExtractItems` to include key fields in the output.

jpbetz · 2024-12-11T18:28:48Z

/lgtm
/approve
Thanks for adding this!

k8s-ci-robot · 2024-12-11T18:28:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jpbetz, yongruilin

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~~ [jpbetz]

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

chore: Removes unused test schema

e96ad23

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

k8s-ci-robot requested review from cici37 and Jefftree December 7, 2024 07:13

yongruilin force-pushed the extend-extractitems branch from 0f81c16 to 8e5a6e6 Compare December 7, 2024 07:13

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 7, 2024

yongruilin mentioned this pull request Dec 7, 2024

Merge would fail if an rhs partially specified object omits key fields despite they can be derived from paths #273

Closed

andrii-korotkov-verkada reviewed Dec 7, 2024

View reviewed changes

jpbetz reviewed Dec 9, 2024

View reviewed changes

yongruilin force-pushed the extend-extractitems branch from 8e5a6e6 to d4fcd3b Compare December 10, 2024 00:19

leoluz reviewed Dec 10, 2024

View reviewed changes

typed/typed.go Outdated Show resolved Hide resolved

leoluz reviewed Dec 10, 2024

View reviewed changes

typed/typed.go Outdated Show resolved Hide resolved

yongruilin force-pushed the extend-extractitems branch 2 times, most recently from 0ee796b to 65746d2 Compare December 10, 2024 17:25

Jefftree reviewed Dec 10, 2024

View reviewed changes

typed/remove_test.go Outdated Show resolved Hide resolved

yongruilin force-pushed the extend-extractitems branch from 65746d2 to a6f15ab Compare December 10, 2024 18:34

leoluz reviewed Dec 10, 2024

View reviewed changes

yongruilin force-pushed the extend-extractitems branch from a6f15ab to 1d91b45 Compare December 10, 2024 21:10

jpbetz reviewed Dec 10, 2024

View reviewed changes

feat: Adds ExtractItems option to include key fields

a88b919

Adds an option to `ExtractItems` to include key fields in the output.

yongruilin force-pushed the extend-extractitems branch from 1d91b45 to a88b919 Compare December 11, 2024 17:42

k8s-ci-robot assigned jpbetz Dec 11, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 11, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 11, 2024

k8s-ci-robot merged commit 40c8ef9 into kubernetes-sigs:master Dec 11, 2024
4 of 5 checks passed

This was referenced Dec 11, 2024

fix: Server side diff now works correctly with some fields removal argoproj/gitops-engine#640

Merged

serverSideDiff error when removing block from spec.containers[] in Helm chart argoproj/argo-cd#20792

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Adds ExtractItems option to include key fields #275

feat: Adds ExtractItems option to include key fields #275

yongruilin commented Dec 7, 2024 •

edited

Loading

andrii-korotkov-verkada Dec 7, 2024

yongruilin Dec 7, 2024 •

edited

Loading

andrii-korotkov-verkada Dec 7, 2024

jpbetz Dec 9, 2024 •

edited

Loading

yongruilin Dec 10, 2024

jpbetz left a comment

jpbetz Dec 9, 2024

yongruilin Dec 10, 2024

jpbetz Dec 9, 2024

yongruilin Dec 10, 2024

jpbetz Dec 9, 2024 •

edited

Loading

leoluz Dec 10, 2024

leoluz Dec 10, 2024 •

edited

Loading

yongruilin Dec 10, 2024

leoluz Dec 10, 2024

leoluz Dec 10, 2024

yongruilin Dec 10, 2024

jpbetz Dec 10, 2024 •

edited

Loading

yongruilin Dec 11, 2024

jpbetz commented Dec 11, 2024

k8s-ci-robot commented Dec 11, 2024

	func (tv TypedValue) ExtractItems(items fieldpath.Set, opts ...extractItemsOption) TypedValue {
	func (tv TypedValue) ExtractItems(items fieldpath.Set, opts ...ExtractItemsOption) TypedValue {

feat: Adds ExtractItems option to include key fields #275

feat: Adds ExtractItems option to include key fields #275

Conversation

yongruilin commented Dec 7, 2024 • edited Loading

Choose a reason for hiding this comment

yongruilin Dec 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpbetz Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpbetz 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

Choose a reason for hiding this comment

jpbetz Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leoluz Dec 10, 2024 • 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

jpbetz Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpbetz commented Dec 11, 2024

k8s-ci-robot commented Dec 11, 2024

yongruilin commented Dec 7, 2024 •

edited

Loading

yongruilin Dec 7, 2024 •

edited

Loading

jpbetz Dec 9, 2024 •

edited

Loading

jpbetz Dec 9, 2024 •

edited

Loading

leoluz Dec 10, 2024 •

edited

Loading

jpbetz Dec 10, 2024 •

edited

Loading