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

kpt fn render includes local and meta resources by default #2781

Merged
merged 2 commits into from
Feb 23, 2022

Conversation

droot
Copy link
Contributor

@droot droot commented Feb 12, 2022

This PR adds support for including local and meta resources by default in fn render.

To exclude meta resource (just the Kptfile), users can run kpt fn render --exclude-meta-resources .

@@ -92,12 +92,18 @@ func (e *Executor) Execute(ctx context.Context) error {
at := attribution.Attributor{Resources: hctx.root.resources, CmdGroup: "fn"}
at.Process()

includeMetaResources := true
Copy link
Contributor

Choose a reason for hiding this comment

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

will this eventually be changed as a configurable flag? Or kpt always include meta resources in fn render

Copy link
Contributor Author

Choose a reason for hiding this comment

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

at the moment no plans to make it configurable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, thought some more and realized exposing this as configurable flag will make it consistent with other fn commands and also provide a way to address the breaking change. Users can run fn render --include-meta-resources false to get the previous behavior. I will update the PR.

@droot droot force-pushed the fn-enable-meta-resources branch 3 times, most recently from b627961 to 8a45dd9 Compare February 16, 2022 02:25
@droot
Copy link
Contributor Author

droot commented Feb 16, 2022

ref: #2590

@droot droot changed the title WIP: fn render includes local and meta resources by default kpt fn render includes local and meta resources by default Feb 18, 2022
@droot droot marked this pull request as ready for review February 18, 2022 17:18
@droot droot changed the base branch from main to variant-constructor February 22, 2022 18:27
@droot
Copy link
Contributor Author

droot commented Feb 22, 2022

Quick update: Now this PR is against the target feature branch variant-constructor where we plan to accumulate all changes related to variant constructor pattern /cc @yuwenma @bgrant0607

@yuwenma
Copy link
Contributor

yuwenma commented Feb 22, 2022

/lgtm

fnResults: fnresult.NewResultList(),
imagePullPolicy: e.ImagePullPolicy,
allowExec: e.AllowExec,
encludeMetaResources: e.ExcludeMetaResources,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: a typo "exclude"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching it.

@@ -168,6 +176,10 @@ type hydrationContext struct {
// imagePullPolicy controls the image pulling behavior.
imagePullPolicy fnruntime.ImagePullPolicy

// indicate if package meta resources such as Kptfile
// to be excluded from the function in put during render.
encludeMetaResources bool
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

@droot droot merged commit b84e666 into kptdev:variant-constructor Feb 23, 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.

2 participants