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

Add KptFileGVK to expose typed GVK information for Kptfile #3584

Merged
merged 1 commit into from
Oct 14, 2022

Conversation

justinsb
Copy link
Contributor

More ergonomic than a set of strings.

@justinsb justinsb force-pushed the add_kptfile_gvk branch 2 times, most recently from 87b6cae to fd3435c Compare September 27, 2022 14:57
@@ -20,17 +20,35 @@ package v1
import (
Copy link
Contributor

@yuwenma yuwenma Sep 27, 2022

Choose a reason for hiding this comment

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

What's our long term plan about the Kptfile schema @mortent @droot? I think kpt should import the Kptfile schema from kpt-functions-sdk, so as users and other tools which use the Kptfile schema do not need to import it from Kpt, and this can avoid expected Kpt exposure and dependent loop problems. For example, the go SDK also needs to use the Kptfile schema and it imports from the kpt-functions-sdk/go/api. This change could potentially make the go sdk incompatible with the kpt fn.

This is not a blocker for this PR. I can pull the change to the kpt-functions-sdk this time. But I'm a little bit worried that the go sdk and kpt could be out of sync if we only make the change on the kpt side and not converge the Kptfile schema.

Copy link
Contributor

@mortent mortent Oct 10, 2022

Choose a reason for hiding this comment

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

@yuwenma Why does the kpt-functions-sdk have a copy of the Kptfile type? It seems like the kpt repo would be the natural place have it and the sdk would import it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know the exact legacy reasons, but I think it makes sense for 2 reasons

  1. Users who need to use the Kptfile type (i.e. to write their own kptfile relevant functions), they can import from go-sdk and do not need to import the entire kpt pkg.
  2. kpt, as a CLI, is not designed for importing purpose so it should not worry too much about supporting its dependants.

It seems like the kpt repo would be the natural place have it and the sdk would import it.

I see kpt/porch has imported go/fn in several places. e.g. porch function wrapper main.go, porch builtin function runtime, which is aligned with the library dependencies I described above. And because of that, I don't think sdk can then import kpt/porch.

@justinsb justinsb requested a review from a team as a code owner October 14, 2022 13:58
@justinsb justinsb force-pushed the add_kptfile_gvk branch 4 times, most recently from 15cc8cf to 70237dd Compare October 14, 2022 17:44
More ergonomic than a set of strings.

Also do the same for ResultList and FunctionResultList
@justinsb justinsb merged commit b7174fd into kptdev:main Oct 14, 2022
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.

4 participants