Add KubeObject setters that keep formatting to "kubeobject" lib

[kispaljr]

Add KubeObject setters that try to keep YAML comments and the order of fields while updating big chunks of a KubeObject.
Most notably: SetNestedFieldKeepFormatting(), SetSpec() and SetStatus()

I believe that the optimal place to solve this problem is in the upstream kpt-fn SDK (https://github.com/GoogleContainerTools/kpt-functions-sdk), and I already asked the maintainers in this issue: kptdev/kpt#3923
Naturally, if this functionality is implemented in the upstream SDK, then this code should be deleted from the Nephio repo.

[johnbelamaric]

Nice!!

/approve

[kispaljr]

The code uses the "unsafe" module to get access to the internals of KubeObject. This is a temporary solution, until kptdev/kpt#3923 is solved.
Due to using "unsafe", most probably something similar to /override presubmit-nephio-gosec will be necessary to merge this.

[gvbalaji]

Thanks @kispaljr . Overall looks good. I have a question:

1. I know we can use these as helper methods, but would it make more sense to implement some of this methods as part of the structure KubeObjectExt?

[kispaljr]

Thanks @kispaljr . Overall looks good. I have a question:

1. I know we can use these as helper methods, but would it make more sense to implement some of this methods as part of the structure KubeObjectExt?

yes, it would and that was my first instinct, as well.
I decided to add them as "external methods" of KubeObject itself, because the concept works without specifying the go type of the Kube object (that KubeObjectExt requires). This way you can use the functions on both KubeObject and KubeObjectExt (that contains a KubeObject inside). Basically I wanted to avoid casting to KubeObjectExt when it is not necessary.

[henderiw]

See my view is different here. Why did we create the KubeObjExt -> it is an extension of the KubeObject methods. If the SDK takes it in we can obsolete this. With your proposal the KubeObjExt is irrelevant. So we have a chicken and egg situation

[henderiw]

Moreover we need to add nil check everywhere, which we can avoid with what I propose.

[kispaljr]

I am all right with adding the setters as methods to KubeObjectExt. Actually we can keep both sets of functions, by adding methods like this:

func (o *KubeObjectExt[T1]) SetSpec(newSpec interface{}) error {
	return SetSpec(&o.KubeObject, newSpec)
}

We cannot add the getters as methods, since generic methods are not supported in go, but this is not a great loss: GetSpec and GetStatus are not really useful, to be honest :)

[kispaljr]

Moreover we need to add nil check everywhere, which we can avoid with what I propose.

I see the KubeObject pointers in SetXXX functions as "~receivers", so if we think nill-checking is needed there, then I think we should also check the receivers in KubeObjectExt, like this:

func (o *KubeObjectExt[T1]) SetSpec(newSpec interface{}) error {
	if o == nil {
		return fmt.Errorf("calling setter on a nil pointer")
	}
	...
}

All in all, I cannot see a big difference from that perspective.

[kispaljr]

I went ahead and added the setters as methods to KubeObjectExt (with tests)

[henderiw]

ok but we don't need both. Let's just extend KubeObjectExt and be done. Eventually this moves to the fn sdk. We should steer people into 1 thing.

[kispaljr]

I will try to make my case to keep both possibilities: setters defined on both KubeObject and KubeObjectExt.
If it is not convincing, just say so, and I promise I will keep only the KubeObjectExt versions of setters.

• having both options is the most convenient for developers, and since both relies on the same implementation I don't see any problems if both are used in parallel in KRM FN implementations
• it's hard to put KubeObjectExts of different go types into the same list, so in the majority of cases we will work with pure KubeObjects, but :
• if we define the methods only on KubeObjectExt, then it's very cumbersome to call them on a pure KubeObject (it is trivial in the other direction btw.)
• after the code merged to the upstream SDK it is easier to refactor our code:
  • with KubeObject methods we simply have to convert SetXX(obj, val) to obj.SetXXX(val),
  • in many of the the KubeObjectExt cases we should also remove the conversion code to KubeObjectExt
• specifically SetNestedFieldKeepFormatting is defined on SubObjects (neither on KubeObject, nor on KubeObjectExt), in order so that it can be called on a nested subtree of a KubeObject. (This is how all SetNestedXXX methods work in the upstream SDK btw. ) This means that, if we define this method only on KubeObjectExt we loose valuable functionality

[johnbelamaric]

If Yuwen pulls your fixes into the fn SDK, what changes here?

[johnbelamaric]

If Yuwen pulls your fixes into the fn SDK, what changes here?

I guess you mostly answered that already. What I was trying to say is: I have asked Yuwen to do that, so you should take that into account. Hopefully she can do it quickly.

[gvbalaji]

I will try to make my case to keep both possibilities: setters defined on both KubeObject and KubeObjectExt. If it is not convincing, just say so, and I promise I will keep only the KubeObjectExt versions of setters.

• having both options is the most convenient for developers, and since both relies on the same implementation I don't see any problems if both are used in parallel in KRM FN implementations

• it's hard to put KubeObjectExts of different go types into the same list, so in the majority of cases we will work with pure KubeObjects, but :

• if we define the methods only on KubeObjectExt, then it's very cumbersome to call them on a pure KubeObject (it is trivial in the other direction btw.)

• after the code merged to the upstream SDK it is easier to refactor our code:

  • with KubeObject methods we simply have to convert SetXX(obj, val) to obj.SetXXX(val),
  • in many of the the KubeObjectExt cases we should also remove the conversion code to KubeObjectExt

• specifically SetNestedFieldKeepFormatting is defined on SubObjects (neither on KubeObject, nor on KubeObjectExt), in order so that it can be called on a nested subtree of a KubeObject. (This is how all SetNestedXXX methods work in the upstream SDK btw. ) This means that, if we define this method only on KubeObjectExt we loose valuable functionality

thanks @kispaljr for the detailed notes. We created the KubeObjextExt with the intention that functions will create that and use it as that will have extended functionalities such as the ones you have implemented here. If it is indeed the case we need these on the extended objects only and as @johnbelamaric suggested when they move to the base SDK we will simply remove those methods from our library and rest of the implementation will continue to work ( as we are composing the base structure and not instantiating in the parser). However if you think we have an immediate need for straight methods I am fine with keeping both as well. Either way it's easy to refactor when the base library implements this. Thanks a lot.

[henderiw]

I will put I more argument in which is in go people have tried to steer to 1 option only and this is helping things a lot. So I would vote for this. Since it is clear what people want and how to use it.

[henderiw]

and to complete we don't need advanced functionality on subjects for now since getspec/setspec does this for us. We just convert to go types and back, so this makes it also clear how we want people to use it.

So my point is once the sdk merges this we can leverage all the other parts and goodies.

[kispaljr]

I wasn't convincing enough :), so, as promised, I removed SetSpec and SetStatus working on pure KubeObjects.
I made setNestedFieldKeppFormatting private, since, it seems, we don't want to update other subtrees (SubObjects), only spec and status, and those two are already exposed, as @henderiw mentioned.

I cannot move the getters into methods, because the go language doesn't allow generic methods. If anybody has a workaround, I am open to suggestions.

[henderiw]

Given we have GetGoStruct we don't need the additional getSpec and getStatus as the goStruct gives all of them.

Also I don't see the value of ToStruct as the .AS method has this already. So better to remove

[gvbalaji]

thanks @kispaljr and @henderiw . Looks like a rebase needed as well.

[kispaljr]

All comments fixed (rebase, removed superfluous functions).
We still have the issue that I mentioned in a previous comment:

The code uses the "unsafe" module to get access to the internals of KubeObject. This is a temporary solution, until kptdev/kpt#3923 is solved.
Due to using "unsafe", something similar to /override presubmit-nephio-gosec will be necessary to merge this.

somebody with the right permissions can take care of this?

[henderiw]

/override presubmit-nephio-gosec
/approve

to overcome the issue which will finally be fixed in fn sdk

[nephio-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: henderiw, johnbelamaric

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 [henderiw,johnbelamaric]

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

[kispaljr]

/retest

[ganchandrasekaran]

/lgtm

[kispaljr]

/lgtm

[nephio-prow]

@kispaljr: you cannot LGTM your own PR.

In response to this:

/lgtm

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.

[henderiw]

/override presubmit-nephio-gosec

[nephio-prow]

@henderiw: Overrode contexts on behalf of henderiw: presubmit-nephio-gosec

In response to this:

/override presubmit-nephio-gosec

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.

[aakashchan]

/lgtm