Fix api generation #35

efiacor · 2024-03-21T15:33:57Z

Upgrade code gen to latest and use the provided script to generate the api.
Bump versions to align with the code-gen version.
Fix cli e2e test suite.

The following files are auto generated and therefore do not require a "full" review:

api/generated/*
all files with a zz_* prefix
controllers/config/bases/crds
controller/**/config/*

kispaljr

As far as I can tell by looking through it:
/lgtm

Trigger CLI e2e

liamfallon · 2024-03-25T17:12:31Z

It also looks fine to me. Also I have built it locally and run the tests locally and they pass. I have also manually ran through the Porch tutorial locally, testing PVSs and VPs and it all works fine. However, let's leave the PR open for another day or so to get more reviews on it given that it is such a big PR.

liamfallon

It also looks fine to me. Also I have built it locally and run the tests locally and they pass. I have also manually ran through the Porch tutorial locally, testing PVSs and VPs and it all works fine. However, let's leave the PR open for another day or so to get more reviews on it given that it is such a big PR.

liamfallon · 2024-03-25T17:15:07Z

/assign @johnbelamaric @s3wong @henderiw

vjayaramrh · 2024-03-25T19:39:47Z

api/porch/register.go

@@ -38,8 +38,8 @@ var (

 func addKnownTypes(scheme *runtime.Scheme) error {
 	scheme.AddKnownTypes(SchemeGroupVersion,
-		&Package{},
-		&PackageList{},
+		&PorchPackage{},


Curious as to why the renaming is needed? Thanks

It uses kube-codegen to generate the api code.

With naming the CR Package, the code generator didn't like that apparently as package is a keyword in golang, so they forked it and customised it here. Didn't dig into what they did really.

As part of the migration/clean up plan, the Package and Function CRs will be removed, so the idea here is to provide a stop gap solution (hopefully without breaking anything), by renaming to "PorchPackage.."

Curious as to why the renaming is needed? Thanks

@vjayaramrh Are you OK with this?

@efiacor @liamfallon , Thanks for the response, I am good.

Trigger CI e2e

nagygergo · 2024-03-26T15:30:46Z

controllers/config/crd/bases/config.porch.kpt.dev_fleetmembershipbindings.yaml

@@ -1,24 +1,9 @@
-# Copyright 2023 The kpt and Nephio Authors


Why are these getting removed?

Good spot. Not sure how they were getting added before. I have update the controller-gen directives to add the header now.

nagygergo · 2024-03-26T16:59:19Z

controllers/packagevariants/pkg/controllers/packagevariant/packagevariant_controller.go

+func mapObjectsToRequests(mgrClient client.Reader) handler.MapFunc {
+	return func(ctx context.Context, obj client.Object) []reconcile.Request {
+		attachedPackageVariants := &api.PackageVariantList{}
+		err := mgrClient.List(context.TODO(), attachedPackageVariants, &client.ListOptions{


With the new function signature, you get the associated context as a param of the function, that should be used.

nagygergo · 2024-03-26T16:59:41Z

...rollers/packagevariantsets/pkg/controllers/packagevariantset/packagevariantset_controller.go

+func mapObjectsToRequests(mgrClient client.Reader) handler.MapFunc {
+	return func(ctx context.Context, obj client.Object) []reconcile.Request {
+		attachedPackageVariants := &api.PackageVariantSetList{}
+		err := mgrClient.List(context.TODO(), attachedPackageVariants, &client.ListOptions{


Same as above, use the provided context.

nagygergo · 2024-03-26T17:00:40Z

pkg/cmd/server/start.go

-		o.RecommendedOptions.Etcd.StorageConfig.Paging = utilfeature.DefaultFeatureGate.Enabled(features.APIListChunking)
-	}
+	// if o.RecommendedOptions.Etcd != nil {
+	// 	o.RecommendedOptions.Etcd.StorageConfig.Paging = utilfeature.DefaultFeatureGate.Enabled(features.APIListChunking)


Couldn't you just remove this?

Possibly but my knowledge of the Paging functionality in etcd is limited.
They removed it in the latest version. Plan was maybe to reintroduce it once I have the know how.
Can remove if it's cleaner. Don't think we are losing much.

nagygergo · 2024-03-26T17:06:04Z

pkg/oci/loader.go

 	if err != nil {
 		return nil, err
 	}
 	return resources, nil
 }

-func loadResourcesFromTar(ctx context.Context, tarReader *tar.Reader) (*repository.PackageResources, error) {
+func loadResourcesFromTar(tarReader *tar.Reader) (*repository.PackageResources, error) {


Why dropping context here? If anything, context cancelation should be part of the for loop at #195.

Slightly lazy one but it shows up as an unused param. There is a few of them in the code which prob should be cleaned up. Unless my understanding of how the context is passed/used in k8s is incorrect.

nagygergo · 2024-03-26T17:12:59Z

scripts/_trap

@@ -0,0 +1,39 @@
+


I'm not really a bash expert, I couldn't figure out what this is used for. Could you add 1-2 lines to explain it?

Nor am I :D
I borrowed this from another project. From my understanding, it catches the exit codes if any and gives some nice output to the user.

vjayaramrh · 2024-03-27T01:16:17Z

@efiacor any specific files that you would recommend a newbie porch reviewer to get familar with while reviewing this PR. Thanks

liamfallon

The updates all look fine @efiacor

efiacor · 2024-03-27T08:49:50Z

@efiacor any specific files that you would recommend a newbie porch reviewer to get familar with while reviewing this PR. Thanks

Hi @vjayaramrh .
To be honest I am still working my way around the code myself.
For this PR, the main objective was to fix the api code generation. Then I looked to upgrade as many dependencies as possible.
Most of the stuff under the api dir can be skimmed over really. All of the code-gen stuff is now handed off to the scripts generate-api and kube_code_gen. Also, we prob need to decide whether the auto gen code is checked in or not. For me it shouldn't be in the repo.
The relevant changes to the "controllers" are mostly related to upgrading in the dependencies. controller-runtime, etc. Some code gen stuff too but minimal.
The pkg dir contains most/all the business logic so should really be the main focus. Again, with the dependencies bump, there were some minor required changes.
Finally, I needed to fix an issue with the cli e2e test run where the ns delete was hanging due to Finalizers on the PackageRev resources in the NameSpace.

nagygergo · 2024-03-27T12:15:55Z

test/e2e/cli/cluster.go

@@ -174,6 +176,40 @@ func KubectlDeleteNamespace(t *testing.T, name string) {
 	t.Logf("output: %v", string(out))
 }

+func RemovePackagerevFinalizers(t *testing.T, namespace string) {


Was the test case not failing before because of finalizers?

Intermittently yes.
See the history here - https://github.com/nephio-project/porch/actions/workflows/porchctl-cli-e2e.yaml
I believe there is some timing/race condition on how the PackageRev api-resource gets updated. If the Finalizers remain, it blocks the NameSpace deletion.

Ok, that's fine, I'll open a separate issue on this one. Seems strange that porch-server is sometimes able to delete the packagerevs and sometimes not.

johnbelamaric · 2024-03-27T16:47:19Z

Nice work, @efiacor! I know how gnarly this problem was, I spent a fair bit of time on it myself and gave up in frustration :) - thanks for sticking through it.

/approve

liamfallon · 2024-03-28T09:07:49Z

/approve
/lgtm

nephio-prow · 2024-03-28T09:07:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: efiacor, johnbelamaric, liamfallon, nagygergo

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~~ [efiacor,johnbelamaric,liamfallon]

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

Fix api generation

897cb63

nephio-prow bot requested review from henderiw and s3wong March 21, 2024 15:34

nephio-prow bot added the approved label Mar 21, 2024

efiacor requested review from johnbelamaric and liamfallon and removed request for s3wong and henderiw March 21, 2024 15:34

efiacor mentioned this pull request Mar 21, 2024

Fix code generation in Porch #34

Closed

efiacor added 5 commits March 25, 2024 08:55

Add required SingularNameProvider interface

e61be6d

Fix rpkg pull unit test

8d161f6

Clean up code gen scripts

a9006e0

Fix script to run on Mac

5a0c514

Fix build errors

0e92b10

kispaljr reviewed Mar 25, 2024

View reviewed changes

Update Makefile

2285bfe

Trigger CLI e2e

liamfallon reviewed Mar 25, 2024

View reviewed changes

nephio-prow bot assigned henderiw, johnbelamaric and s3wong Mar 25, 2024

vjayaramrh reviewed Mar 25, 2024

View reviewed changes

efiacor added 2 commits March 25, 2024 22:19

Merge branch 'nephio-project:main' into upgrade_code_gen

6a12a62

Update .gitignore

faa66e6

Trigger CI e2e

nagygergo reviewed Mar 26, 2024

View reviewed changes

efiacor added 3 commits March 26, 2024 17:19

Fix missing headers from controller yaml

4d5355f

Fix cli e2e test run issue

e1b19ec

Address some review comments

67441f9

efiacor requested review from kispaljr, nagygergo, vjayaramrh, liamfallon and tliron and removed request for nagygergo March 26, 2024 20:41

liamfallon reviewed Mar 27, 2024

View reviewed changes

efiacor requested a review from nagygergo March 27, 2024 09:06

nagygergo reviewed Mar 27, 2024

View reviewed changes

efiacor requested a review from nagygergo March 27, 2024 14:52

nagygergo approved these changes Mar 27, 2024

View reviewed changes

nephio-prow bot assigned liamfallon Mar 28, 2024

nephio-prow bot added the lgtm label Mar 28, 2024

nephio-prow bot merged commit 8a84339 into nephio-project:main Mar 28, 2024
6 checks passed

efiacor deleted the upgrade_code_gen branch March 28, 2024 09:15

liamfallon mentioned this pull request Apr 12, 2024

Fix generate #25

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix api generation #35

Fix api generation #35

efiacor commented Mar 21, 2024 •

edited

Loading

kispaljr left a comment •

edited

Loading

liamfallon commented Mar 25, 2024

liamfallon left a comment

liamfallon commented Mar 25, 2024

vjayaramrh Mar 25, 2024

efiacor Mar 25, 2024

liamfallon Mar 26, 2024

vjayaramrh Mar 26, 2024 •

edited

Loading

nagygergo Mar 26, 2024

efiacor Mar 26, 2024

nagygergo Mar 26, 2024 •

edited

Loading

efiacor Mar 26, 2024

efiacor Mar 26, 2024

nagygergo Mar 26, 2024

efiacor Mar 26, 2024

efiacor Mar 26, 2024

nagygergo Mar 26, 2024

efiacor Mar 26, 2024

nagygergo Mar 26, 2024

efiacor Mar 26, 2024 •

edited

Loading

nagygergo Mar 26, 2024

efiacor Mar 26, 2024

vjayaramrh commented Mar 27, 2024

liamfallon left a comment

efiacor commented Mar 27, 2024

nagygergo Mar 27, 2024

efiacor Mar 27, 2024

nagygergo Mar 27, 2024 •

edited

Loading

johnbelamaric commented Mar 27, 2024

liamfallon commented Mar 28, 2024

nephio-prow bot commented Mar 28, 2024

Fix api generation #35

Fix api generation #35

Conversation

efiacor commented Mar 21, 2024 • edited Loading

kispaljr left a comment • edited Loading

Choose a reason for hiding this comment

liamfallon commented Mar 25, 2024

liamfallon left a comment

Choose a reason for hiding this comment

liamfallon commented Mar 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vjayaramrh Mar 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nagygergo Mar 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efiacor Mar 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vjayaramrh commented Mar 27, 2024

liamfallon left a comment

Choose a reason for hiding this comment

efiacor commented Mar 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nagygergo Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

johnbelamaric commented Mar 27, 2024

liamfallon commented Mar 28, 2024

nephio-prow bot commented Mar 28, 2024

efiacor commented Mar 21, 2024 •

edited

Loading

kispaljr left a comment •

edited

Loading

vjayaramrh Mar 26, 2024 •

edited

Loading

nagygergo Mar 26, 2024 •

edited

Loading

efiacor Mar 26, 2024 •

edited

Loading

nagygergo Mar 27, 2024 •

edited

Loading