Add one SetFromTypedObject method to KubeObjectExt that allows to set all/any fields at once

[kispaljr]

Inspired by this comment from @johnbelamaric : this proposal adds a setter for the whole KubeObjectExt that allows to set all/any fields at once

[gvbalaji]

I am thinking instead of doing the following logic in the method safeSetNestedFieldKeepFormatting

if len(fields) == 0 {
		obj2, err := fn.NewFromTypedObject(value)
		if err != nil {
			return err
		}
		*obj = *obj

Can we create the new object(obj2) in this method only, convert it to value equivalent and pass it to safeSetNestedFieldKeepFormatting. That will keep that method consistent without any changes needed.

Does it make sense?

[kispaljr]

pass [the new object] to safeSetNestedFieldKeepFormatting

what would you put into the fields parameter, when you passing the new object?
In its current form if you call safeSetNestedFieldKeepFormatting without any fields parameter it will do nothing (this behavior is inherited from obj.SetNestedField that it calls internally.

[gvbalaji]

Well in SetFromTypedObject we will create the value for the passes in go struct and simply call
setNestedFieldKeepFormatting(obj, value) No fields passed i n.
Let SetNestedField take care of it.

[kispaljr]

Unfortunately calling SetNestedField() with no fields at all is a noop: it doesn't return an error, but it doesn't do anything at all either.

[gvbalaji]

isnt that what we want in this case as we are not passing any fields from SetFromTypedObject?

[henderiw]

When testing this this does not work. When applying SetStatus it works fine.

func (r *fnCtx) updateIPAllocationResource(forObj *fn.KubeObject, objs fn.KubeObjects) (*fn.KubeObject, error) {
if forObj == nil {
return nil, fmt.Errorf("expected a for object but got nil")
}
fn.Logf("ipalloc: %v\n", forObj)
allocKOE, err := ko.NewFromKubeObject*ipamv1alpha1.IPAllocation
if err != nil {
return nil, err
}
alloc, err := allocKOE.GetGoStruct()
if err != nil {
return nil, err
}
resp, err := r.IpamClientProxy.Allocate(context.Background(), alloc, nil)
if err != nil {
return nil, err
}
alloc.Status = resp.Status

if alloc.Status.Prefix != nil {
	fn.Logf("ipalloc resp prefix: %v\n", *resp.Status.Prefix)
}
if alloc.Status.Gateway != nil {
	fn.Logf("ipalloc resp gateway: %v\n", *resp.Status.Gateway)
}
// set the status
//err = allocKOE.SetStatus(resp.Status)
err = allocKOE.SetFromTypedObject(resp)
return &allocKOE.KubeObject, err

}

testable using github.com/henderiw-nephio/pkg-examples -> ipam-fn

[henderiw]

When testing this this does not work. When applying SetStatus it works fine.

func (r *fnCtx) updateIPAllocationResource(forObj *fn.KubeObject, objs fn.KubeObjects) (*fn.KubeObject, error) {
if forObj == nil {
return nil, fmt.Errorf("expected a for object but got nil")
}
fn.Logf("ipalloc: %v\n", forObj)
allocKOE, err := ko.NewFromKubeObject*ipamv1alpha1.IPAllocation
if err != nil {
return nil, err
}
alloc, err := allocKOE.GetGoStruct()
if err != nil {
return nil, err
}
resp, err := r.IpamClientProxy.Allocate(context.Background(), alloc, nil)
if err != nil {
return nil, err
}
alloc.Status = resp.Status

if alloc.Status.Prefix != nil {
fn.Logf("ipalloc resp prefix: %v\n", *resp.Status.Prefix)
}
if alloc.Status.Gateway != nil {
fn.Logf("ipalloc resp gateway: %v\n", *resp.Status.Gateway)
}
// set the status
//err = allocKOE.SetStatus(resp.Status)
err = allocKOE.SetFromTypedObject(resp)
return &allocKOE.KubeObject, err
}

testable using github.com/henderiw-nephio/pkg-examples -> ipam-fn

[kispaljr]

What is the error message that you got?

For me the following testcase is green: (however see my next comment)

func TestIpalloc(t *testing.T) {
	var inputYaml string = `
apiVersion: ipam.alloc.nephio.org/v1alpha1
kind: IPAllocation
metadata:
  name: internetpool1
spec:
  kind: pool
  selector:
  matchLabels:
    nephio.org/site: edge1
  networkInstance: {}
  prefixLength: 8
`

	allocKOE, err := NewFromYaml[*ipamv1alpha1.IPAllocation]([]byte(inputYaml))
	assertNoError(t, err)
	alloc, err := allocKOE.GetGoStruct()
	assertNoError(t, err)

	alloc.Status.Gateway = "2.2.2.2"
	alloc.Status.AllocatedPrefix = "3.3.3.0"

	// set the status
	//err = allocKOE.SetStatus(resp.Status)
	err = allocKOE.SetFromTypedObject(alloc)
	assertNoError(t, err)

	var expectedYaml string = `apiVersion: ipam.alloc.nephio.org/v1alpha1
kind: IPAllocation
metadata:
  name: internetpool1
spec:
  kind: pool
  networkInstance: {}
  prefixLength: 8
status:
  prefix: 3.3.3.0
  gateway: 2.2.2.2`

	actualYaml := strings.TrimSpace(allocKOE.String())
	if actualYaml != expectedYaml {
		t.Errorf(actualYaml)
	}

}

[kispaljr]

Having said that you are using KubeObjectExt[*ipamv1alpha1.IPAllocation].
So far I thought that KubeObjectExt's type parameter should be a struct, as opposed to a pointer-to-struct. I came to this conclusion by looking into kubeobject_test.go (even the parts that wasn't written by me 🙂), and into the implementation of GetGoStruct():
GetGoStruct accidentally works with the pointer type parameter, but in that case we are passing a **ipamv1alpha1.IPAllocation to As(), while I think it was designed to receive a *ipamv1alpha1.IPAllocation.

This might also cause your different behavior between SetFromTypedObject and SetSpec. For SetSPec you pass resp.Status that is a struct, but to SetFromTypedObject() you pass resp that is a pointer-to-struct.

The important question here: Should we support pointer or struct type parameters?

[kispaljr]

@henderiw I've managed to run your github.com/henderiw-nephio/pkg-examples -> ipam-fn code,
and for me the SetStatus and SetFromTypedObject versions ouput the exact same result (character-by character).
What is your error?

[johnbelamaric]

This LGTM, but has @henderiw 's issue been figured out?

[kispaljr]

This LGTM, but has @henderiw 's issue been figured out?

Yes. @henderiw, you confirmed to me that your tests are green now. Is that correct?

[ganchandrasekaran]

/lgtm

[johnbelamaric]

/approve

[nephio-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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 [johnbelamaric]

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