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.

Sign up for GitHub

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

Jump to bottom

update functionConfigs during render #3228

Merged
merged 6 commits into from
May 25, 2022
Merged

update functionConfigs during render #3228

merged 6 commits into from
May 25, 2022

Conversation

droot
Copy link
Contributor

@droot droot commented May 23, 2022 •
edited
Loading

kpt v1.0.0-beta15+ onwards functionConfigs are included in the function inputs by default during render. Before this change, functionConfigs were read from the filesystem once before rendering the pipeline and changes to functionConfigs during the render were ignored. This change ensures that functionConfigs are now updated for a function during rendering.

@droot
Copy link
Contributor Author

droot commented May 23, 2022

@mengqiy @natasha41575 Can you pl. review this PR. Thanks.

@droot droot mentioned this pull request May 24, 2022
Closed
@mengqiy
Copy link
Contributor

mengqiy commented May 24, 2022

kpt v1.0.0-beta15+ onwards functionConfigs are included in the function inputs by default during render.

I'm not sure if I understood this correctly. Before beta15, if the functionConfig is in the same directory alongside with other input resources, the functionConfig will be included in items.
Does it mean starting from beta15, if a functionConfig lives outside of current directory, it will still be included?

mengqiy
mengqiy reviewed May 24, 2022
View reviewed changes
e2e/testdata/fn-render/fnconfig-updated-in-render/.expected/config.yaml Outdated
# See the License for the specific language governing permissions and
# limitations under the License.

exitCode: 0
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the default and it can be removed.
So this file can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

e2e/testdata/fn-render/fnconfig-updated-in-render/README.md Outdated
@@ -0,0 +1,21 @@
# app-example
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems this file is just a template without much useful information. Should we remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed it.

internal/util/render/executor.go Show resolved Hide resolved
droot
droot commented May 25, 2022
View reviewed changes
Copy link
Contributor Author

@droot droot left a comment

Choose a reason for hiding this comment

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

Thanks for the review. Was able to root cause the issue and resolved it.

Pl. take a look.

e2e/testdata/fn-render/fnconfig-updated-in-render/.expected/config.yaml Outdated
# See the License for the specific language governing permissions and
# limitations under the License.

exitCode: 0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

e2e/testdata/fn-render/fnconfig-updated-in-render/README.md Outdated
@@ -0,0 +1,21 @@
# app-example
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed it.

internal/util/render/executor.go Show resolved Hide resolved
@droot
Copy link
Contributor Author

droot commented May 25, 2022

Does it mean starting from beta15, if a functionConfig lives outside of current directory, it will still be included?

No. If the functionConfig resides outside the package, it will not be included. User can still point to an outside functionConfig in kpt fn eval --fn-config syntax, but we will not include it in the resources.

@mengqiy
Copy link
Contributor

mengqiy commented May 25, 2022

Thanks for root causing the issue related to GlobalScope.
fixes: #3230

@droot droot merged commit 5187e2d into kptdev:main May 25, 2022
@mengqiy
Copy link
Contributor

mengqiy commented May 27, 2022 •
edited
Loading

This example (https://github.com/GoogleContainerTools/kpt-functions-catalog/tree/master/examples/upsert-resource-simple) is broken with the latest kpt built from HEAD. e.g. https://github.com/GoogleContainerTools/kpt-functions-catalog/runs/6630134377?check_suite_focus=true#step:7:841
@droot Can you please take a look?

@mengqiy mengqiy mentioned this pull request May 27, 2022
Merged
@mengqiy
Copy link
Contributor

mengqiy commented Jun 2, 2022 •
edited
Loading

upsert-resource example (https://github.com/GoogleContainerTools/kpt-functions-catalog/tree/master/examples/upsert-resource-simple) is broken due to this change. The CI in the catalog repo is failing because of that.
@droot Any update?

@droot
Copy link
Contributor Author

droot commented Jun 2, 2022

upsert-resource example (https://github.com/GoogleContainerTools/kpt-functions-catalog/tree/master/examples/upsert-resource-simple) is broken due to this change. The CI in the catalog repo is failing because of that. @droot Any update?

Sorry, couldn't get to it yet. Will take a look at it today.

This was referenced Jun 2, 2022
Merged
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@natasha41575 natasha41575 natasha41575 approved these changes

@mengqiy mengqiy mengqiy approved these changes

@justinsb justinsb Awaiting requested review from justinsb

@mortent mortent Awaiting requested review from mortent

@yuwenma yuwenma Awaiting requested review from yuwenma

Assignees

@mengqiy mengqiy

@natasha41575 natasha41575

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@droot @mengqiy @natasha41575