Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added new generic parser interface #121

Merged
merged 11 commits into from
Apr 20, 2023

Conversation

henderiw
Copy link
Contributor

this is the generic parser interface which offloads a set of method implementations from the libraries

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 19, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link

@gvbalaji gvbalaji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @henderiw . My initial thinking is the following. I have updated this after seeing how this is consumed in the interface lib. Here are my suggestions.

  1. I think it may be simpler to have only helper functions in this base library.

type Entity struct {
Obj *fn.KubeObject
}

  • NewFromKubeObject(*fn.KubeObject) (Entity, error)
  • NewFromYaml(b []byte) (Entity, error)
  • NewFromGoStruct(x any) (Entity, error)
  • GostructFromKubeObjectT any (T, err)

This should cover the creation of objects for all entities ( ip, interface etc) and base methods. Then we do not need to implement in the individual libraries.

  1. Let the callers directly call get/set/delete methods on the kubeobject. I am saying this because we are not providing any additional processing (for e.g like we did with GetInterface... methods to hide the YAML path from the user).
  2. Then in individual libraries only define getters/setters that will do further help like in the interface library GetAttachmentType(), GetNetworkInstanceName() etc. Implement these methods directly on the Entity structure defined above ( in this base library). This will avoid lot of code in each of the libraries.

in interface library

type InterfaceEntity interface {
GetAttachmentType()
GetNetworkInstanceName()
SetNetworkInstanceName()
.
.
.
}

func (r *Entity) GetAttachmentType() string
func (r *Entity) GetNetworkInstanceName() string

Users will first create the entity and then call the methods based on function they are implementing.

In fact we can directly implement methods on the structure without an interface.

This was referenced Apr 20, 2023
@kispaljr
Copy link
Contributor

I agree with @gvbalaji to have only helper functions in the base library.
To help the argument, this: nokia#19 alternative (but functionally equivalent) implementation of the same Parser interface shows, that most of the Parser methods are just aliases of KubeObject methods.

The same implementation also shows that the 4 functions @gvbalaji mentioned, are also mostly aliases to existing upstream SDK functions. So in his proposal we only need conversion functions between Entity and KubeObject. Or, we can just use type Entity fn.KubeObject instead of the type Entity struct {...} to make it trivial.

Copy link

@gvbalaji gvbalaji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @henderiw . In line with our discussion. Added a minor comment about naming.

krm-functions/lib/kubeobject/kubeobject.go Outdated Show resolved Hide resolved
Copy link

@gvbalaji gvbalaji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@nephio-prow
Copy link
Contributor

nephio-prow bot commented Apr 20, 2023

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

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

@nephio-prow nephio-prow bot removed the lgtm label Apr 20, 2023
@gvbalaji
Copy link

/lgtm

@nephio-prow nephio-prow bot added the lgtm label Apr 20, 2023
@nephio-prow nephio-prow bot merged commit 739ebfb into nephio-project:main Apr 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants