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

Update porch-controllers use of the Kpt Fn SDK #3940

Merged
merged 5 commits into from
May 4, 2023

Conversation

johnbelamaric
Copy link
Contributor

This PR changes the porch-controllers binary to use its own go.mod, and updates to the latest Kpt Function SDK.

By using a separate go.mod, we don't have to update all the compiled in functions and other areas.

cc @yuwenma @mortent @natasha41575

Copy link
Contributor

@mortent mortent left a comment

Choose a reason for hiding this comment

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

I think running make tidy in the porch directory should update the porch go module deps and address the failing test.

I think separating this into its own go mod is fine for now. When/If cncf/sandbox#34 is approved and we can move it all into a separate github org, we should look into whether we can split things up into either multiple repos or have clearer structure between the different go modules within the repo. I find nested go modules somewhat hard to manage.

Separately, we should look into whether the replace directives are always appropriate.

@@ -722,27 +722,13 @@ func ensurePackageContext(pv *api.PackageVariant,
return nil
}

// find the kptfile.kpt.dev ConfigMap, it must be in package-context.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

Are the changes in this file related to adding the go mod?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some of them, because the fn lib was updated. I pulled this set of change from a different PR.

return nil, fmt.Errorf("%q not found in PackageRevisionResources '%s/%s'", file, prr.Namespace, prr.Name)
}

ko, err := fn.ParseKubeObject([]byte(prr.Spec.Resources[file]))
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the resources is a fileName-fileContent key-value pair, I'm just curious whether we have considered the content overflow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Overflow in what sense? There is a potential http request size limit for posts, I think there's an open issue on that. Is there a different overflow you are thinking of?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good. Yeah, HTTP request size is the one I'm thinking.

@johnbelamaric
Copy link
Contributor Author

So, go mod tidy removed the Azure mods, but now the build is failing. Nested go modules :(

@johnbelamaric johnbelamaric merged commit 14c7b35 into kptdev:main May 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants