Go kpt fn SDK should allow keeping the original YAML comments wherever it's possible

[kispaljr]

Problem description

I want to able to write a kpt function using the Go SDK that does the following:

• reads the contents of a KubeObject into a go struct (with the As() method),
• manipulates the go struct
• writes back e.g. the "spec" part of the modified go struct into the same KubeObject (e.g. with the SetNestedField() method)

After this whole cycle I want the original comments inside the "spec" part of the KubeObject to remain intact wherever the value of a field hasn't changed.
Keeping the comments is a requirement coming form the project I am working on (Nephio)

Implementation ideas I've tried

The main problem with implementing this on top of the current SDK is that if SetNestedField() is called with a big go struct with nested sub-structs inside, then it will only keep the head/line/foot comments of the YAML node of the top-level struct, but not of its child nodes. (The same is true for SetNestedSlice() btw.)

One workaround would be reading the head/line/foot comments from the original KubeObject and writing them back for the matching parts after calling SetNestedField(). However only head- and line-comments are exposed in the KubeObject interface, but not foot-comments (why?). Also I couldn't find a way to walk the subtree of a KubeObject/SubObject, because I couldn't found a way to list the names of the fields of a SubObject (at least not via the officially exposed API).

I have some suboptimal workarounds for this problem heavily relying on reflection, and walking the fields of the go struct instead of the SubObject, however I feel that this problem should be addressed in the SDK layer, not above it.

[kispaljr]

I feel that this might be a good point to implement support for keeping the comments:
https://github.com/GoogleContainerTools/kpt-functions-sdk/blob/e8e9cb3c3ae2a19c22f52701d1542cf24e541df0/go/fn/internal/map.go#L138

here the comments of the top-level node are transferred to the new node, but not recursively. It would be nice to (maybe optionally) transfer the comments to the subtree of YAML nodes under node where it matches with the subtree under oldNode .

(Maybe the TODO comment in the code tries to suggest something similar)

[yuwenma]

I don't see any blockers to add FootComment and SetFootComment methods to fn.

I feel that this might be a good point to implement support for keeping the comments: GoogleContainerTools/kpt-functions-sdk@e8e9cb3/go/fn/internal/map.go#L138

Yes! This is the right place to make the change.

@kispaljr do you want to help us making this change? I can help reviewing the code.

[kispaljr]

Yes, I want to help, and I already have a first implementation (without tests so far). But... my employer won't allow me to sign your CLA, so I have to figure out what can I do in this case.
So far it seems that I will be able to push my code into our own fork, and give you a pointer to it here. After that you can decide if you want to use/merge it.

[yuwenma]

@kispaljr Sorry for the CLA issue. The good news is that we are in the progress of donating kpt and KRM funcitons to CNCF. cncf/sandbox#34 and it is in good progress.
Once we are done, it should be much easier for you to contribute changes wherever you found useful. So I'd suggest we wait until the CNCF transferring is done. Unless this is urgent to you. In that case, I can cherry-pick your code.

[johnbelamaric]

@yuwenma if we can cherry-pick this, that would be best, although I do believe we working around it now.

[yuwenma]

@yuwenma if we can cherry-pick this, that would be best, although I do believe we working around it now.

Sounds good.

@kispaljr just to double check: In your PR only the first commit is relevant to the go/fn SDK, right? In that commit, do you expect all methods to go into go/fn SDK or only the SetNestedFieldKeepFormatting one? One potential confusion I'd like to raise is that a KRM resource (in go/fn) does not promise to have status and spec field. (but I agree that's a good practice in general)