fix: krm exec function working dir #4654

aabouzaid · 2022-05-28T23:24:29Z

Based on @seh tip in #4347

This change makes a deep copy from the loader config instead of passing a reference to the config.

How to test: Follow the steps in #4347.

I've added 2 tests:

TestFnExecTransformer: This test will pass in all cases because before the fix the base path is used and there is only the base and no overlay.
TestFnExecTransformerWithOverlay:
- Before the fix: The test failed with exact error as in 4347 because the working dir is set to the base path.
- After the fix: The test passed where the working dir is updated with the overlay path.

linux-foundation-easycla · 2022-05-28T23:24:32Z

The committers listed above are authorized under a signed CLA.

✅ login: aabouzaid / name: Ahmed AbouZaid (15866d7)

k8s-ci-robot · 2022-05-28T23:24:37Z

Welcome @aabouzaid!

It looks like this is your first PR to kubernetes-sigs/kustomize 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kustomize has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2022-05-28T23:24:38Z

Hi @aabouzaid. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

aabouzaid · 2022-05-28T23:30:03Z

/check-cla

seh · 2022-06-14T23:18:21Z

@KnVerey, does the implementation match your expectations?

@aabouzaid, have you found an existing test that we can augment or duplicate to test this change?

natasha41575

Please feel free to add a unit test, this approach seems fine to me. Thanks!

api/internal/target/kusttarget.go

natasha41575 · 2022-06-14T23:52:30Z

/ok-to-test

natasha41575 · 2022-06-15T00:01:21Z

have you found an existing test that we can augment or duplicate to test this change?

@seh @aabouzaid, https://github.com/kubernetes-sigs/kustomize/blob/master/api/krusty/fnplugin_test.go has some tests for KRM exec functions that you may be able to model after.

k8s-ci-robot · 2022-07-13T01:42:04Z

@aabouzaid: This PR has multiple commits, and the default merge method is: merge.
You can request commits to be squashed using the label: tide/merge-method-squash

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

aabouzaid · 2022-07-13T03:20:17Z

@natasha41575 I've created the DeepCopy, and also added 2 tests one with just the base and another with overly (I've updated the PR body with the test results before and after the fix).

aabouzaid · 2022-08-26T23:39:33Z

@KnVerey @natasha41575 could you please take a look at this PR? It's ready.

aabouzaid · 2022-09-20T16:10:17Z

@KnVerey @natasha41575 could we at least run the CI pipeline to make sure everything as expected 😅

KnVerey

Can you please also rebase when you address the comments?
Re: the tests, Github isn't even offering me the button to run them for some reason. I've never seen that before, and I'll look into it if it doesn't start working after you push again.

api/internal/plugins/loader/loader.go

natasha41575 · 2022-10-06T16:30:50Z

@aabouzaid do you plan to continue this PR? It looks like we may need this functionality for #3980, which @annasong20 is working on. She'd like to pick this up unless you are still working on it.

aabouzaid · 2022-10-06T22:06:54Z

@natasha41575 Yes, I will try to finish it over the weekend.

annasong20 · 2022-10-19T14:51:33Z

/ok-to-test

annasong20 · 2022-10-19T14:53:50Z

/retest

api/internal/plugins/loader/loader.go

api/internal/plugins/loader/loader_test.go

annasong20 · 2022-10-19T15:14:29Z

api/krusty/fnplugin_test.go

@@ -126,7 +132,7 @@ spec:
 	assert.NoError(t, fSys.RemoveAll(tmpDir.String()))
 }

-func TestFnExecGeneratorWithOverlay(t *testing.T) {
+func TestFnExecGeneratorInOverlay(t *testing.T) {


Can we change this back to TestFnExecGeneratorWithOverlay? In my last comment, I was only referring to the new test you added, TestFnExecTransformerInOverlay. In this generator test, the exec generator is actually in the base, not the overlay, so it should be "with".

Sorry, my bad for not clarifying!

Actually, I've renamed the functions to make it clear and avoid all of that confusion.

For example, for the transformer, we now have:

TestFnExecTransformerInBase: The transformer is in the base, and there is no overlay.

TestFnExecTransformerInBaseWithOverlay: The transformer is in the base, and there is an overlay, but it has no transformer.

TestFnExecTransformerInOverlay: The transformer is in the overlay and not in the base.

And the generator follows the same pattern.

api/krusty/fnplugin_test.go

KnVerey

/approve

@annasong20 go ahead and lgtm once you're satisfied your comments are addressed

Thank you to both of you for your work on this fix and its review!

api/internal/plugins/loader/loader_test.go

api/internal/plugins/loader/loader.go

k8s-ci-robot · 2022-10-21T14:58:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aabouzaid, KnVerey

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~~ [KnVerey]

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

aabouzaid · 2022-10-22T01:39:13Z

I've addressed all comments. So we are ready to go. ✔️
I will rebase and update the commit messages once no more changes are required.

I see that there is alredy an error in the CI pipeline but it has nothing to do with this PR.
It's in TestRemoteLoad_LocalProtocol (I see the same error in other commits pipeline runs)

annasong20 · 2022-10-25T23:57:21Z

api/krusty/fnplugin_test.go

+`)
+
+	m := th.Run(prod, o)
+	assert.NoError(t, err)


This assert statement and the one here: https://github.com/kubernetes-sigs/kustomize/pull/4654/files#diff-34d93fef5d3973a037f7871d2010ebaa89547617081d5f672bac16be3bf88160R192

are still redundant, but not a big deal.

annasong20 · 2022-10-26T18:28:15Z

api/krusty/fnplugin_test.go

@@ -41,7 +41,19 @@ spec:
 EOF
 `

-func TestFnExecGenerator(t *testing.T) {
+const krmTransformerDotSh = `#!/bin/bash


Thank you for making this change!

annasong20 · 2022-10-26T18:29:32Z

/lgtm

aabouzaid · 2022-10-27T08:22:54Z

@annasong20 thanks for reviewing this PR 👌
The last step was to rebase and squash the last 2 commits to have a meaningful message 😅

annasong20 · 2022-10-27T16:13:48Z

@annasong20 thanks for reviewing this PR 👌 The last step was to rebase and squash the last 2 commits to have a meaningful message 😅

Totally on me. I didn't realize it would auto-merge... Thanks for bringing this up!

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label May 28, 2022

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 28, 2022

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label May 28, 2022

k8s-ci-robot requested review from KnVerey and natasha41575 May 28, 2022 23:24

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels May 28, 2022

aabouzaid mentioned this pull request May 28, 2022

KRM Exec Function can't locate executable when referencing a base #4347

Closed

natasha41575 reviewed Jun 14, 2022

View reviewed changes

api/internal/target/kusttarget.go Outdated Show resolved Hide resolved

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 14, 2022

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 13, 2022

aabouzaid force-pushed the fix-krm-exec-function branch from 5885360 to d1a4bcf Compare July 13, 2022 02:46

aabouzaid requested a review from natasha41575 July 13, 2022 03:17

KnVerey reviewed Sep 23, 2022

View reviewed changes

api/internal/plugins/loader/loader.go Outdated Show resolved Hide resolved

api/internal/plugins/loader/loader.go Outdated Show resolved Hide resolved

aabouzaid force-pushed the fix-krm-exec-function branch from 7b8484d to 8566b9b Compare October 9, 2022 00:05

aabouzaid requested review from annasong20 and KnVerey and removed request for KnVerey and annasong20 October 15, 2022 15:47

annasong20 reviewed Oct 19, 2022

View reviewed changes

KnVerey reviewed Oct 21, 2022

View reviewed changes

api/internal/plugins/loader/loader_test.go Outdated Show resolved Hide resolved

api/internal/plugins/loader/loader.go Show resolved Hide resolved

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 21, 2022

aabouzaid force-pushed the fix-krm-exec-function branch from e7c2348 to 73224d2 Compare October 22, 2022 01:08

aabouzaid added 4 commits October 22, 2022 03:16

Test transformers krm exec function

1b1e6cc

Fix krm exec function working dir

d29febe

proposal v2

ea21b37

proposal v2.1

3e447da

aabouzaid force-pushed the fix-krm-exec-function branch from 73224d2 to 3e447da Compare October 22, 2022 01:16

aabouzaid requested review from KnVerey and annasong20 and removed request for KnVerey October 24, 2022 09:41

annasong20 reviewed Oct 26, 2022

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 26, 2022

k8s-ci-robot merged commit daf81df into kubernetes-sigs:master Oct 26, 2022

KnVerey mentioned this pull request Dec 7, 2022

krm container mounts relative to base instead of overlay #4800

Closed

KnVerey mentioned this pull request Jan 5, 2023

kustomize build fails with remote transformer function with kustomize version 4.5.5+ #4958

Closed

mbigras mentioned this pull request Jan 26, 2023

Guess: Regression for kustomize build with git SSH URLs: no such file or directory #4998

Closed

starcraft66 mentioned this pull request Feb 7, 2023

Upgrade to Kustomize v5.0.0 viaduct-ai/kustomize-sops#170

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: krm exec function working dir #4654

fix: krm exec function working dir #4654

aabouzaid commented May 28, 2022 •

edited

Loading

linux-foundation-easycla bot commented May 28, 2022 •

edited

Loading

k8s-ci-robot commented May 28, 2022

k8s-ci-robot commented May 28, 2022

aabouzaid commented May 28, 2022

seh commented Jun 14, 2022

natasha41575 left a comment

natasha41575 commented Jun 14, 2022

natasha41575 commented Jun 15, 2022 •

edited

Loading

k8s-ci-robot commented Jul 13, 2022

aabouzaid commented Jul 13, 2022

aabouzaid commented Aug 26, 2022

aabouzaid commented Sep 20, 2022

KnVerey left a comment

natasha41575 commented Oct 6, 2022 •

edited

Loading

aabouzaid commented Oct 6, 2022

annasong20 commented Oct 19, 2022

annasong20 commented Oct 19, 2022

annasong20 Oct 19, 2022

aabouzaid Oct 22, 2022

annasong20 Oct 26, 2022

KnVerey left a comment

k8s-ci-robot commented Oct 21, 2022

aabouzaid commented Oct 22, 2022 •

edited

Loading

annasong20 Oct 25, 2022

annasong20 Oct 26, 2022

annasong20 commented Oct 26, 2022

aabouzaid commented Oct 27, 2022

annasong20 commented Oct 27, 2022

fix: krm exec function working dir #4654

fix: krm exec function working dir #4654

Conversation

aabouzaid commented May 28, 2022 • edited Loading

linux-foundation-easycla bot commented May 28, 2022 • edited Loading

k8s-ci-robot commented May 28, 2022

k8s-ci-robot commented May 28, 2022

aabouzaid commented May 28, 2022

seh commented Jun 14, 2022

natasha41575 left a comment

Choose a reason for hiding this comment

natasha41575 commented Jun 14, 2022

natasha41575 commented Jun 15, 2022 • edited Loading

k8s-ci-robot commented Jul 13, 2022

aabouzaid commented Jul 13, 2022

aabouzaid commented Aug 26, 2022

aabouzaid commented Sep 20, 2022

KnVerey left a comment

Choose a reason for hiding this comment

natasha41575 commented Oct 6, 2022 • edited Loading

aabouzaid commented Oct 6, 2022

annasong20 commented Oct 19, 2022

annasong20 commented Oct 19, 2022

annasong20 Oct 19, 2022

Choose a reason for hiding this comment

aabouzaid Oct 22, 2022

Choose a reason for hiding this comment

annasong20 Oct 26, 2022

Choose a reason for hiding this comment

KnVerey left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 21, 2022

aabouzaid commented Oct 22, 2022 • edited Loading

annasong20 Oct 25, 2022

Choose a reason for hiding this comment

annasong20 Oct 26, 2022

Choose a reason for hiding this comment

annasong20 commented Oct 26, 2022

aabouzaid commented Oct 27, 2022

annasong20 commented Oct 27, 2022

aabouzaid commented May 28, 2022 •

edited

Loading

linux-foundation-easycla bot commented May 28, 2022 •

edited

Loading

natasha41575 commented Jun 15, 2022 •

edited

Loading

natasha41575 commented Oct 6, 2022 •

edited

Loading

aabouzaid commented Oct 22, 2022 •

edited

Loading