Fix and cleanup Kpt fn integration #5886

yhrn · 2021-05-21T11:02:32Z

Description

This PR attempts to fix and clean up Kpt hydration.

It removes a workaround for a Kpt issue that is no longer relevant. The Kpt version shipped with gcloud and several version before does not have this issue. So instead this PR bumps the required Kpt version. The workaround was problematic because:
1. In some cases it was completely broken. In my case I had no Kustomization and was using kpt.fn.fnPath and the input
  Skaffold gave to my function was just the first resource in kpt.dir and the function config.
2. It didn't match the behavior of using kpt fn run --fn-path which will cause function configs to be used only from the
  specified path whereas the workaround added those to whatever was in kpt.dir
The order in which kpt fn run and kustomize build are executed has been reversed. I have outlined my reasoning
in code comments.
Some general cleanup to get rid of duplicated logic. The only visible difference from these changes should be that dumping
to kpt.live.apply.dir is now using the same mechanism as when writing to kpt.fn.sinkDir and the kustomize temp dir
(kpt fn sink instead of manifest.Write) so this directory will contain one file per resource instead of a single file.

User facing changes (remove if N/A)

The order in which kpt fn run and kustomize build are executed has changed from Kustomize first to Kpt first. I don't believe there is any user facing documentation for this.

Follow-up Work (remove if N/A)

I have not updated any tests yet because I want to first see if these changes have a chance of being accepted, which is also why I'm publishing it as a draft.

Also after cleaning stuff up I wonder if there is any point to kpt.fn.sinkDir or if should be deprecated? If you do skaffold render you get the same stuff on on stdout and if you do skaffold deploy you get the same plus the inventory template in kpt.live.apply.dir

MarlonGamez · 2021-05-25T17:57:12Z

hi @yhrn , thanks for opening up this pr. Is it ready to be reviewed? Asking since you have it marked as draft :)

yhrn · 2021-05-25T18:24:29Z

Hi @MarlonGamez ,

The reason I marked it as draft is because I haven't looked at the tests yet because I first wanted to confirm that you would accept the user facing changes, mainly the change in order of execution for kpt fn run and kustomize build. I tried to describe this and my reasoning behind it in the PR description.

yuwenma

Hi Mattias, Thank you so much for improving kpt deployer! I didn't realize the workaround is no longer needed (and actually add bugs).

I have several questions want to confirm:

kpt and kustomize are decided to not be unified for now 🤷 . Thus the directories of kustomize and kpt are better not be overlapped when used together. Technically they can, this requires users to handle the potential issues themselves and better use kustomize as a kpt fn (and we will support that in the skaffold v2). So it seems the "removing comments" and "the kpt relative file breakage" are less of a concern.
kustomize build is considered as the generator, and kpt fn provides the functions to validate and transform these resources. So the order is 1) generate, 2) validate 3) mutate. From this perspective, ignoring whether doing kustomize first may break kpt features or vice versa, running kustomize build first seems to be the right approach. And if anything's broken during the process, we shall fix the breakage.
Does this PR fix the bug that "using --fn-path will cause function configs to be used only from the specified path whereas the workaround added those to whatever was in kpt.dir"? Maybe I miss something but it seems that this issue still exists. Do you mind updating the PR to filter out the kptFn from .kpt.dir if --fnPath is given? somewhere after the kpt source.
I feel sorry that the kpt deployer breaks. The kpt v0.X.X are not guaranteed to be backward compatible (but kpt v1.0 will do much better on this). Thus, instead of fixing kpt deployer in skaffold everytime it breaks (I've done similar fixes a couple of times), do you think it may actually make users life easier if we require a specific kpt version? e.g. here we ask users to install kpt 0.34.0 and do not upgrade to newer kpt 0.X.XX versions until the skaffold kpt v1.0 is ready (I'm currently implementing the new kpt hydration/deployment based on kpt v1.0).

pkg/skaffold/deploy/kpt/kpt.go

yuwenma · 2021-05-25T18:05:01Z

pkg/skaffold/deploy/kpt/kpt.go

+	flags, err := k.getKptFnRunArgs()
+	if err != nil {
+		return nil, err
+	}


I like this change. renderManifests is the right place for this code.

yuwenma · 2021-05-25T18:48:11Z

Thanks Mattias, open to further discussions about the kpt/kustomize behavior

…

On Tue, May 25, 2021 at 11:24 AM Mattias Öhrn ***@***.***> wrote: Hi @marlon-gamez <https://github.com/marlon-gamez>, The reason I marked it as draft is because I haven't looked at the tests yet because I first wanted to confirm that you would accept the user facing changes, mainly the change in order of execution for kpt fn run and kustomize build. I tried to describe this and my reasoning behind it in the PR description. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#5886 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABKYVUWJWOTMSTREGAIDIVTTPPTPDANCNFSM45I5XXRQ> .

-- Yuwen Ma | Software Engineer - Google Cloud | ***@***.*** ***@***.***>

yhrn · 2021-05-25T19:31:18Z

Hi Mattias, Thank you so much for improving kpt deployer! I didn't realize the workaround is no longer needed (and actually add bugs).

I have several questions want to confirm:

kpt and kustomize are decided to not be unified for now shrug . Thus the directories of kustomize and kpt are better not be overlapped when used together. Technically they can, this requires users to handle the potential issues themselves and better use kustomize as a kpt fn (and we will support that in the skaffold v2). So it seems the "removing comments" and "the kpt relative file breakage" are less of a concern.

I don't understand what you mean with "not unified" and that "directories should not be overlapped". Do you mean that one shouldn't really use both? That would contradict the next item a bit where you explain why you think the existing order is the right one. Could you also elaborate on why you think "removing comments" and "the kpt relative file breakage" are less of a concern? Especially the former is a blocker for us.

Running kustomize as a kpt fn sounds very interesting though. Can you provide any more information on this? I assume that is not going to be a Skaffold specific feature but rather something that Kustomize will support, or?

kustomize build is considered as the generator, and kpt fn provides the functions to validate and transform these resources. So the order is 1) generate, 2) validate 3) mutate. From this perspective, ignoring whether doing kustomize first may break kpt features or vice versa, running kustomize build first seems to be the right approach. And if anything's broken during the process, we shall fix the breakage.

Kpt functions that are validators and should run last is a fair point. For me the ability to use functions that require comments and the ability for the functions to modify Kustomizations is more important, but I realize that this will not apply to everybody. If you believe that changing the order is the wrong thing to do then I'll drop this PR.

Does this PR fix the bug that "using --fn-path will cause function configs to be used only from the specified path whereas the workaround added those to whatever was in kpt.dir"? Maybe I miss something but it seems that this issue still exists. Do you mind updating the PR to filter out the kptFn from .kpt.dir if --fnPath is given? somewhere after the kpt source.

No, but I don't think this is a bug, kpt fn run --help explicitly says this about --fn-path: read functions from these directories instead of the configuration directory. In my opinion it is unexpected that Skaffold tries to change intended behavior of Kpt. The way I read the existing code and the comments was that the function configs in kpt.fn.fnPath were added to kpt.dir as a workaround for --fn-path blowing up and I assumed that the additive behavior was an unintended side effect.

I feel sorry that the kpt deployer breaks. The kpt v0.X.X are not guaranteed to be backward compatible (but kpt v1.0 will do much better on this). Thus, instead of fixing kpt deployer in skaffold everytime it breaks (I've done similar fixes a couple of times), do you think it may actually make users life easier if we require a specific kpt version? e.g. here we ask users to install kpt 0.34.0 and do not upgrade to newer kpt 0.X.XX versions until the skaffold kpt v1.0 is ready (I'm currently implementing the new kpt hydration/deployment based on kpt v1.0).

I'm not sure if that would make things better, it's also pretty inconvenient to not be able to upgrade to the latest version for other use cases.

yuwenma · 2021-05-25T20:02:32Z

Hi Mattias, Thank you so much for improving kpt deployer! I didn't realize the workaround is no longer needed (and actually add bugs).
I have several questions want to confirm:

kpt and kustomize are decided to not be unified for now shrug . Thus the directories of kustomize and kpt are better not be overlapped when used together. Technically they can, this requires users to handle the potential issues themselves and better use kustomize as a kpt fn (and we will support that in the skaffold v2). So it seems the "removing comments" and "the kpt relative file breakage" are less of a concern.

I don't understand what you mean with "not unified" and that "directories should not be overlapped". Do you mean that one shouldn't really use both? That would contradict the next item a bit where you explain why you think the existing order is the right one. Could you also elaborate on why you think "removing comments" and "the kpt relative file breakage" are less of a concern? Especially the former is a blocker for us.

Unified --> See comment basically once they are unified, kpt can understand and work with kustomize config by default. I just want to make sure the previous comment is not misleading.

The new kpt v1.0 is going to update the hydrated config in-place (the config is updated in its original file) while kustomize outputs the config to stdout. From that perspective, the kustomize build --DIR and kpt fn DIR may mess up if the DIR are the same. So a clean approach would be make sure kpt dir and kustomize dir do not overlap. A useful but may not clean approach would be re-creating and applying the setters after kustomize build, which allows using kustomize config and kpt-setters in the same dir.

Running kustomize as a kpt fn sounds very interesting though. Can you provide any more information on this? I assume that is not going to be a Skaffold specific feature but rather something that Kustomize will support, or?

kustomize build is considered as the generator, and kpt fn provides the functions to validate and transform these resources. So the order is 1) generate, 2) validate 3) mutate. From this perspective, ignoring whether doing kustomize first may break kpt features or vice versa, running kustomize build first seems to be the right approach. And if anything's broken during the process, we shall fix the breakage.

Here's the issue. Eventually, we agree to have a skaffold specific kustomize kpt fn rather than a generic one (which needs to handle all possible use cases and we don't have many user requests yet).

Kpt functions that are validators and should run last is a fair point. For me the ability to use functions that require comments and the ability for the functions to modify Kustomizations is more important, but I realize that this will not apply to everybody. If you believe that changing the order is the wrong thing to do then I'll drop this PR.

Does this PR fix the bug that "using --fn-path will cause function configs to be used only from the specified path whereas the workaround added those to whatever was in kpt.dir"? Maybe I miss something but it seems that this issue still exists. Do you mind updating the PR to filter out the kptFn from .kpt.dir if --fnPath is given? somewhere after the kpt source.

No, but I don't think this is a bug, kpt fn run --help explicitly says this about --fn-path: read functions from these directories instead of the configuration directory. In my opinion it is unexpected that Skaffold tries to change intended behavior of Kpt. The way I read the existing code and the comments was that the function configs in kpt.fn.fnPath were added to kpt.dir as a workaround for --fn-path blowing up and I assumed that the additive behavior was an unintended side effect.

The expected behavior of skaffold kpt deployer should be: If .kpt.fn.fnPath is not given, skaffold searches and applies the kpt functions from .kpt.dir (declarative kpt functions); If .kpt.fn.fnPath is given, skaffold should only apply these fnPath functions and intentionally filter out the kpt functions defined in .kpt.dir. The existing code does not filter out the fn from .kpt.dir

I feel sorry that the kpt deployer breaks. The kpt v0.X.X are not guaranteed to be backward compatible (but kpt v1.0 will do much better on this). Thus, instead of fixing kpt deployer in skaffold everytime it breaks (I've done similar fixes a couple of times), do you think it may actually make users life easier if we require a specific kpt version? e.g. here we ask users to install kpt 0.34.0 and do not upgrade to newer kpt 0.X.XX versions until the skaffold kpt v1.0 is ready (I'm currently implementing the new kpt hydration/deployment based on kpt v1.0).

I'm not sure if that would make things better, it's also pretty inconvenient to not be able to upgrade to the latest version for other use cases.

yeah, and this is the common problem for most non-GA (v1.0) CLIs.

yhrn · 2021-05-26T08:36:52Z

kpt and kustomize are decided to not be unified for now shrug . Thus the directories of kustomize and kpt are better not be overlapped when used together. Technically they can, this requires users to handle the potential issues themselves and better use kustomize as a kpt fn (and we will support that in the skaffold v2). So it seems the "removing comments" and "the kpt relative file breakage" are less of a concern.

I don't understand what you mean with "not unified" and that "directories should not be overlapped". Do you mean that one shouldn't really use both? That would contradict the next item a bit where you explain why you think the existing order is the right one. Could you also elaborate on why you think "removing comments" and "the kpt relative file breakage" are less of a concern? Especially the former is a blocker for us.

Unified --> See comment basically once they are unified, kpt can understand and work with kustomize config by default. I just want to make sure the previous comment is not misleading.

Ok, the comment you are referencing wasn't added by me, it was just moved around as part of the refactoring I did. But if I understand you correctly there has been some decision somewhere that the "unification" mentioned there is not going to happen so we should probably just remove the comment?

As a side note, I don't fully understand why the Kustomize temp dir workaround was there in the first place? It's needed in this PR because of doing kustomize build after kpt fn run but when doing it in the current order you could just have run kustomize build on kpt.dir instead of first reading that dir with kpt fn source and then writing the resources to a temp dir with kpt fn sink and finally running kustomize build there.

The new kpt v1.0 is going to update the hydrated config in-place (the config is updated in its original file) while kustomize outputs the config to stdout. From that perspective, the kustomize build --DIR and kpt fn DIR may mess up if the DIR are the same. So a clean approach would be make sure kpt dir and kustomize dir do not overlap. A useful but may not clean approach would be re-creating and applying the setters after kustomize build, which allows using kustomize config and kpt-setters in the same dir.

Why is it relevant how kpt v1 (kpt fn render) will behave here? When implementing support for kpt v1 I suppose this is something to take into account but it seems to me like that will be a completely different beast. For kpt v0 kpt fn run without DIR will always output to stdout.
How could one automatically recreate setters after running kustomize build?

Running kustomize as a kpt fn sounds very interesting though. Can you provide any more information on this? I assume that is not going to be a Skaffold specific feature but rather something that Kustomize will support, or?

kustomize build is considered as the generator, and kpt fn provides the functions to validate and transform these resources. So the order is 1) generate, 2) validate 3) mutate. From this perspective, ignoring whether doing kustomize first may break kpt features or vice versa, running kustomize build first seems to be the right approach. And if anything's broken during the process, we shall fix the breakage.

Here's the issue. Eventually, we agree to have a skaffold specific kustomize kpt fn rather than a generic one (which needs to handle all possible use cases and we don't have many user requests yet).

I'm not sure how a Skaffold specific kpt function would look. In my mind it would mainly be an improvement if it would work as a transformer fn rather than a generator so that I could include it in my kpt v1 fn pipeline definition and thereby control the order, e.g. run generator fn A, then Kustomize fn (retaining comments), then generator fn B and finally some validator fns. But this sounds like it might require significant changes to Kustomize but it would also generally useful, not just for Skaffold.

Also reading through the linked issue and the Kpt as well as the Kpt v1 alpha docs it seems to me like Kpt v1 (at least the fn pipeline functionality) will be harder to compose with Kustomize, especially using Kustomize as a generator.

Kpt functions that are validators and should run last is a fair point. For me the ability to use functions that require comments and the ability for the functions to modify Kustomizations is more important, but I realize that this will not apply to everybody. If you believe that changing the order is the wrong thing to do then I'll drop this PR.

Does this PR fix the bug that "using --fn-path will cause function configs to be used only from the specified path whereas the workaround added those to whatever was in kpt.dir"? Maybe I miss something but it seems that this issue still exists. Do you mind updating the PR to filter out the kptFn from .kpt.dir if --fnPath is given? somewhere after the kpt source.

No, but I don't think this is a bug, kpt fn run --help explicitly says this about --fn-path: read functions from these directories instead of the configuration directory. In my opinion it is unexpected that Skaffold tries to change intended behavior of Kpt. The way I read the existing code and the comments was that the function configs in kpt.fn.fnPath were added to kpt.dir as a workaround for --fn-path blowing up and I assumed that the additive behavior was an unintended side effect.

The expected behavior of skaffold kpt deployer should be: If .kpt.fn.fnPath is not given, skaffold searches and applies the kpt functions from .kpt.dir (declarative kpt functions); If .kpt.fn.fnPath is given, skaffold should only apply these fnPath functions and intentionally filter out the kpt functions defined in .kpt.dir. The existing code does not filter out the fn from .kpt.dir

Ok, then yes this PR fixes the bug. There is no need to filter out the functions in kpt.dir, kpt fn run will ignore them when using --fn-path.

To summarize a bit, I don't see much that should change in this PR as a result of the discussion so far - we either agree to change the order and then I'll fix comments and tests or you decide you want to keep the current order, in which case I'll close this PR.

yuwenma · 2021-05-27T07:24:38Z

kpt and kustomize are decided to not be unified for now shrug . Thus the directories of kustomize and kpt are better not be overlapped when used together. Technically they can, this requires users to handle the potential issues themselves and better use kustomize as a kpt fn (and we will support that in the skaffold v2). So it seems the "removing comments" and "the kpt relative file breakage" are less of a concern.

I don't understand what you mean with "not unified" and that "directories should not be overlapped". Do you mean that one shouldn't really use both? That would contradict the next item a bit where you explain why you think the existing order is the right one. Could you also elaborate on why you think "removing comments" and "the kpt relative file breakage" are less of a concern? Especially the former is a blocker for us.

Unified --> See comment basically once they are unified, kpt can understand and work with kustomize config by default. I just want to make sure the previous comment is not misleading.

Ok, the comment you are referencing wasn't added by me, it was just moved around as part of the refactoring I did. But if I understand you correctly there has been some decision somewhere that the "unification" mentioned there is not going to happen so we should probably just remove the comment?

+1

As a side note, I don't fully understand why the Kustomize temp dir workaround was there in the first place? It's needed in this PR because of doing kustomize build after kpt fn run but when doing it in the current order you could just have run kustomize build on kpt.dir instead of first reading that dir with kpt fn source and then writing the resources to a temp dir with kpt fn sink and finally running kustomize build there.

ah, because we want to use kustomize as a kpt fn at the very beginning, and this kustomize build is just a placeholder. The whole process is supposed to be a pure kpt fn pipeline as kpt fn source --> kpt fn run --image=gcr.io/kpt-functions/kustomize --> kpt fn run --fnPath=<some validation functions> --> kpt fn sink. But the kustomize fn is still not ready yet. ¯_(ツ)_/¯

The new kpt v1.0 is going to update the hydrated config in-place (the config is updated in its original file) while kustomize outputs the config to stdout. From that perspective, the kustomize build --DIR and kpt fn DIR may mess up if the DIR are the same. So a clean approach would be make sure kpt dir and kustomize dir do not overlap. A useful but may not clean approach would be re-creating and applying the setters after kustomize build, which allows using kustomize config and kpt-setters in the same dir.

Why is it relevant how kpt v1 (kpt fn render) will behave here? When implementing support for kpt v1 I suppose this is something to take into account but it seems to me like that will be a completely different beast. For kpt v0 kpt fn run without DIR will always output to stdout.

Well, actually v1 is very different. For instance, the WET config will change in place (the original file where the kpt fn works on) rather than to stdout. FYI, the cmd kpt fn run will be deprecated.

How could one automatically recreate setters after running kustomize build?

Have you tried kpt cfg create-setter and kpt cfg create-subst? Imagine there would be some kpt-fns for these commands, which could search and recreate setters and add back the comment. I don't mean we'll add these kpt-fns in v1. But technically it is doable.

Running kustomize as a kpt fn sounds very interesting though. Can you provide any more information on this? I assume that is not going to be a Skaffold specific feature but rather something that Kustomize will support, or?

kustomize build is considered as the generator, and kpt fn provides the functions to validate and transform these resources. So the order is 1) generate, 2) validate 3) mutate. From this perspective, ignoring whether doing kustomize first may break kpt features or vice versa, running kustomize build first seems to be the right approach. And if anything's broken during the process, we shall fix the breakage.

Here's the issue. Eventually, we agree to have a skaffold specific kustomize kpt fn rather than a generic one (which needs to handle all possible use cases and we don't have many user requests yet).

I'm not sure how a Skaffold specific kpt function would look. In my mind it would mainly be an improvement if it would work as a transformer fn rather than a generator so that I could include it in my kpt v1 fn pipeline definition and thereby control the order, e.g. run generator fn A, then Kustomize fn (retaining comments), then generator fn B and finally some validator fns. But this sounds like it might require significant changes to Kustomize but it would also generally useful, not just for Skaffold.

I recalled kustomize fn was used to be placed in the kpt generator catalog, so as helm-inflator.

Also reading through the linked issue and the Kpt as well as the Kpt v1 alpha docs it seems to me like Kpt v1 (at least the fn pipeline functionality) will be harder to compose with Kustomize, especially using Kustomize as a generator.

You are right. We will try the best we can in skaffold v2. here's a trade-off doc, if you are interested.

Kpt functions that are validators and should run last is a fair point. For me the ability to use functions that require comments and the ability for the functions to modify Kustomizations is more important, but I realize that this will not apply to everybody. If you believe that changing the order is the wrong thing to do then I'll drop this PR.

Does this PR fix the bug that "using --fn-path will cause function configs to be used only from the specified path whereas the workaround added those to whatever was in kpt.dir"? Maybe I miss something but it seems that this issue still exists. Do you mind updating the PR to filter out the kptFn from .kpt.dir if --fnPath is given? somewhere after the kpt source.

No, but I don't think this is a bug, kpt fn run --help explicitly says this about --fn-path: read functions from these directories instead of the configuration directory. In my opinion it is unexpected that Skaffold tries to change intended behavior of Kpt. The way I read the existing code and the comments was that the function configs in kpt.fn.fnPath were added to kpt.dir as a workaround for --fn-path blowing up and I assumed that the additive behavior was an unintended side effect.

The expected behavior of skaffold kpt deployer should be: If .kpt.fn.fnPath is not given, skaffold searches and applies the kpt functions from .kpt.dir (declarative kpt functions); If .kpt.fn.fnPath is given, skaffold should only apply these fnPath functions and intentionally filter out the kpt functions defined in .kpt.dir. The existing code does not filter out the fn from .kpt.dir

Ok, then yes this PR fixes the bug. There is no need to filter out the functions in kpt.dir, kpt fn run will ignore them when using --fn-path.

To summarize a bit, I don't see much that should change in this PR as a result of the discussion so far - we either agree to change the order and then I'll fix comments and tests or you decide you want to keep the current order, in which case I'll close this PR.

Totally understand the questions you asked. I think most of them are due to the changes and decisions we made when designing kpt 1.0 from 0.X, and the changes in kpt/kustomzie unification. In short, go ahead and change the order if that can unblock you from using kpt deployer. Just a reminder that we are working on skaffold v2, which:

uses kpt v1. And Kpt v1 has some very different behaviors than kpt v0.X (e.g. in-place output, deprecated kpt cfg as you've been aware of)
make each hydration step declarative. kustomize would be a kpt-fn, so as helm and setters.

yhrn · 2021-05-28T07:28:43Z

@yuwenma Thank you! And I'm totally fine with having to rework our setup once Kpt v1/Skaffold v2 lands.

I'll go ahead and fix tests, update comments and rebase the PR on the latest release as soon as I get the chance.

yhrn · 2021-06-03T14:54:16Z

Hi again,

I finally found time to make the comment changes and fix the tests so I've marked the PR as ready for review.

yhrn · 2021-06-03T14:59:44Z

Makefile

@@ -73,7 +74,7 @@ $(BUILD_DIR)/$(PROJECT): $(STATIK_FILES) $(GO_FILES) $(BUILD_DIR)
 .PHONY: install
 install: $(BUILD_DIR)/$(PROJECT)
 	mkdir -p $(GOPATH)/bin
-	cp $(BUILD_DIR)/$(PROJECT) $(GOPATH)/bin/$(PROJECT)
+	cp $(BUILD_DIR)/$(PROJECT) $(GOBIN)/$(PROJECT)


I'm sneaking this in because not being able to control where make install puts the binary was annoying. Changing GOPATH isn't really feasible as that means go build will start pulling down dependencies in that location so honoring GOBIN seemed like the natural way to go.

Now one can do GOBIN=mypath make install similar to how one can do GOBIN=mypath go install and the default behavior remains unchanged. Let me know if this is problematic somehow and I'll remove it.

@tejal29 @nkubala any insights?

codecov · 2021-06-03T15:46:20Z

Codecov Report

Merging #5886 (2858bb1) into master (aa9b115) will decrease coverage by 0.02%.
The diff coverage is 68.08%.

@@            Coverage Diff             @@
##           master    #5886      +/-   ##
==========================================
- Coverage   70.66%   70.64%   -0.03%     
==========================================
  Files         454      457       +3     
  Lines       17399    17475      +76     
==========================================
+ Hits        12295    12345      +50     
- Misses       4196     4220      +24     
- Partials      908      910       +2

Impacted Files	Coverage Δ
pkg/skaffold/deploy/kpt/kpt.go	`82.04% <68.08%> (+4.78%)`	⬆️
pkg/skaffold/render/renderer/renderer.go	`41.17% <0.00%> (-8.83%)`	⬇️
...ffold/kubernetes/portforward/resource_forwarder.go	`78.87% <0.00%> (-1.73%)`	⬇️
pkg/skaffold/build/build_problems.go	`93.44% <0.00%> (-1.21%)`	⬇️
pkg/skaffold/server/v2/endpoints.go	`41.02% <0.00%> (-1.08%)`	⬇️
pkg/skaffold/runner/v1/deploy.go	`53.92% <0.00%> (-0.80%)`	⬇️
pkg/skaffold/event/v2/event.go	`73.33% <0.00%> (-0.71%)`	⬇️
cmd/skaffold/app/cmd/inspect_build_env.go	`55.83% <0.00%> (-0.22%)`	⬇️
pkg/skaffold/inspect/types.go	`100.00% <0.00%> (ø)`
pkg/skaffold/build/docker/types.go	`100.00% <0.00%> (ø)`
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa9b115...2858bb1. Read the comment docs.

yuwenma · 2021-06-07T03:54:08Z

pkg/skaffold/deploy/kpt/kpt.go

@@ -53,7 +52,7 @@ const (
 	kptFnLocalConfig  = "config.kubernetes.io/local-config"

 	kptDownloadLink = "https://googlecontainertools.github.io/kpt/installation/"
-	kptMinVersion   = "0.34.0"
+	kptMinVersion   = "0.38.1"


Do you mind also adding a check to make sure the kptMaxVersion is less than 1.0.0 which does not have kpt fn run?

yuwenma

LGTM. Defer to @tejal29 @nkubala for the Makefile change.

pull-request-size bot added the size/L label May 21, 2021

google-cla bot added the cla: yes label May 21, 2021

tejal29 requested a review from yuwenma May 24, 2021 20:46

yuwenma reviewed May 25, 2021

View reviewed changes

yhrn force-pushed the kpt-fn-fix branch from e4f1310 to 0c1ba92 Compare June 3, 2021 14:51

yhrn marked this pull request as ready for review June 3, 2021 14:53

yhrn requested a review from a team as a code owner June 3, 2021 14:53

yhrn requested a review from aaron-prindle June 3, 2021 14:53

yhrn commented Jun 3, 2021

View reviewed changes

Mattias Öhrn added 3 commits June 3, 2021 17:18

Fix and cleanup Kpt fn integration

3468009

Makfile to respect GOBIN

5eeae9d

Fixing tests and comments

7b25d00

yhrn force-pushed the kpt-fn-fix branch from 0c1ba92 to 7b25d00 Compare June 3, 2021 15:30

tejal29 enabled auto-merge (squash) June 4, 2021 23:13

tejal29 added the kokoro:run runs the kokoro jobs on a PR label Jun 4, 2021

kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Jun 4, 2021

yuwenma reviewed Jun 7, 2021

View reviewed changes

yuwenma approved these changes Jun 7, 2021

View reviewed changes

Checking kpt version < 1.0.0

2858bb1

auto-merge was automatically disabled June 7, 2021 07:48
Head branch was pushed to by a user without write access

tejal29 approved these changes Jun 7, 2021

View reviewed changes

tejal29 added the kokoro:run runs the kokoro jobs on a PR label Jun 7, 2021

tejal29 enabled auto-merge (squash) June 7, 2021 20:20

kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Jun 7, 2021

tejal29 merged commit 8c0e186 into GoogleContainerTools:master Jun 7, 2021

yhrn deleted the kpt-fn-fix branch June 8, 2021 07:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix and cleanup Kpt fn integration #5886

Fix and cleanup Kpt fn integration #5886

yhrn commented May 21, 2021 •

edited

Loading

MarlonGamez commented May 25, 2021

yhrn commented May 25, 2021 •

edited

Loading

yuwenma left a comment •

edited

Loading

yuwenma May 25, 2021 •

edited

Loading

yuwenma commented May 25, 2021 via email •

edited

Loading

yhrn commented May 25, 2021 •

edited

Loading

yuwenma commented May 25, 2021

yhrn commented May 26, 2021 •

edited

Loading

yuwenma commented May 27, 2021

yhrn commented May 28, 2021

yhrn commented Jun 3, 2021

yhrn Jun 3, 2021 •

edited

Loading

yuwenma Jun 7, 2021

codecov bot commented Jun 3, 2021 •

edited

Loading

yuwenma Jun 7, 2021

yuwenma left a comment

Fix and cleanup Kpt fn integration #5886

Fix and cleanup Kpt fn integration #5886

Conversation

yhrn commented May 21, 2021 • edited Loading

MarlonGamez commented May 25, 2021

yhrn commented May 25, 2021 • edited Loading

yuwenma left a comment • edited Loading

Choose a reason for hiding this comment

yuwenma May 25, 2021 • edited Loading

Choose a reason for hiding this comment

yuwenma commented May 25, 2021 via email • edited Loading

yhrn commented May 25, 2021 • edited Loading

yuwenma commented May 25, 2021

yhrn commented May 26, 2021 • edited Loading

yuwenma commented May 27, 2021

yhrn commented May 28, 2021

yhrn commented Jun 3, 2021

yhrn Jun 3, 2021 • edited Loading

Choose a reason for hiding this comment

yuwenma Jun 7, 2021

Choose a reason for hiding this comment

codecov bot commented Jun 3, 2021 • edited Loading

Codecov Report

yuwenma Jun 7, 2021

Choose a reason for hiding this comment

yuwenma left a comment

Choose a reason for hiding this comment

yhrn commented May 21, 2021 •

edited

Loading

yhrn commented May 25, 2021 •

edited

Loading

yuwenma left a comment •

edited

Loading

yuwenma May 25, 2021 •

edited

Loading

yuwenma commented May 25, 2021 via email •

edited

Loading

yhrn commented May 25, 2021 •

edited

Loading

yhrn commented May 26, 2021 •

edited

Loading

yhrn Jun 3, 2021 •

edited

Loading

codecov bot commented Jun 3, 2021 •

edited

Loading