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

interface-fn #69

Merged
merged 7 commits into from
May 8, 2023
Merged

interface-fn #69

merged 7 commits into from
May 8, 2023

Conversation

henderiw
Copy link
Contributor

interface-fn is a function in Nephio R1 that operates on the interface requirement CR. It performs CRUD operation in the package on the NAD, VLANALlocation and IP Allocation resources.

@gvbalaji
Copy link

gvbalaji commented Apr 7, 2023

@henderiw thanks. I understand this will change once the function runtime library and it's client intarface are finalized and merged. If my understanding is correct, can you please convert this into a draft PR?

@gvbalaji gvbalaji modified the milestones: sprint1, sprint2 Apr 11, 2023
@gvbalaji gvbalaji modified the milestones: sprint2, sprint3 Apr 25, 2023
@kispaljr
Copy link
Contributor

kispaljr commented May 2, 2023

@henderiw I believe you failing gosec test can be solved by go workspaces (as You suggested on Slack :) )
This is how I did it with dnn-fn: #70, notice the go.work file in root of the repo.

Copy link
Contributor

@kispaljr kispaljr left a comment

Choose a reason for hiding this comment

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

As I only have cosmetic comments, This looks good to me even in it current state

// ClusterContextCallbackFn provides a callback for the cluster context
// resources in the resourceList
func (r *itfceFn) ClusterContextCallbackFn(o *fn.KubeObject) error {
clusterKOE, err := kubeobject.NewFromKubeObject[*infrav1alpha1.ClusterContext](o)
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is just a reminder for a potential need for change in the future:
As I wrote on Slack we should decide whether to use pointer or struct type parameters of KubeObjectExt.
If the decision will be pointers, than no comment here, otherwise the code here and other places should be aligned to the decision.

if err != nil {
return nil, err
}
//alloc, err := ko.NewFromKubeObject[*vlanv1alpha1.VLANAllocation](vlanalloc)
Copy link
Contributor

Choose a reason for hiding this comment

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

this comment is not needed any more

@kispaljr
Copy link
Contributor

kispaljr commented May 2, 2023

/lgtm

@nephio-prow nephio-prow bot added lgtm and removed lgtm labels May 2, 2023
@kispaljr
Copy link
Contributor

kispaljr commented May 4, 2023

/test presubmit-nephio-go-test

@nephio-prow
Copy link
Contributor

nephio-prow bot commented May 4, 2023

@kispaljr: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

  • /test presubmit-nephio-go-test
  • /test presubmit-nephio-golangci-lint
  • /test presubmit-nephio-license-header-check
  • /test presubmit-test-nephio-inrepoconfig-validation

Use /test all to run the following jobs that were automatically triggered:

  • presubmit-nephio-go-test
  • presubmit-nephio-golangci-lint
  • presubmit-nephio-license-header-check

In response to this:

/retest presubmit-nephio-go-test

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.

@kispaljr
Copy link
Contributor

kispaljr commented May 4, 2023

/test presubmit-nephio-go-test

Copy link
Contributor

@kispaljr kispaljr left a comment

Choose a reason for hiding this comment

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

Nested go.mod files bites us again:
I've just realized that presubmit-nephio-go-test doesn't run any tests inside krm-functions/interface-fn at all.
Actually since the new (not backward compatible) KubeObjectExt API was merged into this branch, this code doesn't compile. This should have been detected by our presubmit tests.
Wim, I've left comments for the quick changes required for the code to compile, otherwise this LGTM

// ClusterContextCallbackFn provides a callback for the cluster context
// resources in the resourceList
func (r *itfceFn) ClusterContextCallbackFn(o *fn.KubeObject) error {
clusterKOE, err := kubeobject.NewFromKubeObject[*infrav1alpha1.ClusterContext](o)
Copy link
Contributor

Choose a reason for hiding this comment

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

simply change this to NewFromKubeObject[infrav1alpha1.ClusterContext]
(in theory nothing else have to be changed)

// belonging to the parent object
resources := fn.KubeObjects{}

itfceKOE, err := kubeobject.NewFromKubeObject[*nephioreqv1alpha1.Interface](o)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please loose the pointer from the type parameter: NewFromKubeObject[nephioreqv1alpha1.Interface]

if forObj == nil {
return nil, fmt.Errorf("expected a for object but got nil")
}
itfceKOE, err := kubeobject.NewFromKubeObject[*nephioreqv1alpha1.Interface](forObj)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please loose the pointer from the type parameter: NewFromKubeObject[nephioreqv1alpha1.Interface]

ipallocs := objs.Where(fn.IsGroupVersionKind(ipamv1alpha1.IPAllocationGroupVersionKind))
for _, ipalloc := range ipallocs {
if ipalloc.GetName() == forObj.GetName() {
alloc, err := kubeobject.NewFromKubeObject[*ipamv1alpha1.IPAllocation](ipalloc)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please loose the pointer from the type parameter: NewFromKubeObject[ipamv1alpha1.IPAllocation]

}
}
// set the status
err = itfceKOE.SetStatus(itfce.Status)
Copy link
Contributor

Choose a reason for hiding this comment

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

With the new API this should be:
itfceKOE.SetStatus(itfce)

@gvbalaji
Copy link

gvbalaji commented May 5, 2023

Thanks @henderiw . A rebase is needed for this PR.

@henderiw henderiw reopened this May 6, 2023
@nephio-prow
Copy link
Contributor

nephio-prow bot commented May 6, 2023

@henderiw: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
presubmit-nephio-gosec 218250a link true /test presubmit-nephio-gosec

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@kispaljr
Copy link
Contributor

kispaljr commented May 8, 2023

/lgtm

@nephio-prow nephio-prow bot added the lgtm label May 8, 2023
@henderiw
Copy link
Contributor Author

henderiw commented May 8, 2023

/approve

@nephio-prow
Copy link
Contributor

nephio-prow bot commented May 8, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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 added the approved label May 8, 2023
@nephio-prow nephio-prow bot merged commit 78e4b50 into nephio-project:main May 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved area/package-specialization SIG Automation Package Specialization Subproject lgtm
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Implement the interface function for package specialization
6 participants