Add fallback to kubectl kustomize if kustomize binary isn't present #4484

MarlonGamez · 2020-07-16T19:15:23Z

Addresses #1781, specifically, @nkubala's comment here: #1781 (comment)

Description
Changes the kustomize deployer to prioritize using the user's kustomize binary, and if one doesn't exist in the user's PATH, then use kubectl kustomize.

…n user's PATH

codecov · 2020-07-16T19:24:52Z

Codecov Report

Merging #4484 into master will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4484      +/-   ##
==========================================
+ Coverage   71.70%   71.75%   +0.04%     
==========================================
  Files         348      348              
  Lines       12068    12082      +14     
==========================================
+ Hits         8653     8669      +16     
+ Misses       2773     2772       -1     
+ Partials      642      641       -1

Impacted Files	Coverage Δ
pkg/skaffold/deploy/kpt.go	`84.61% <100.00%> (ø)`
pkg/skaffold/deploy/kubectl/cli.go	`87.50% <100.00%> (+0.15%)`	⬆️
pkg/skaffold/deploy/kustomize.go	`78.12% <100.00%> (+1.93%)`	⬆️
pkg/skaffold/util/cmd.go	`51.42% <0.00%> (+5.71%)`	⬆️

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 9ba0e85...8706968. Read the comment docs.

pkg/skaffold/deploy/kustomize.go

briandealwis · 2020-07-17T14:20:38Z

pkg/skaffold/deploy/kustomize.go

+		var out []byte
+		var err error
+
+		if k.useKubectl {


Worth noting that kubectl kustomize only supports a subset of kustomize functionality?

yeah, this could manifest through build args being rejected by the binary depending on which version the user thinks they're using - but I think that's ok left up to them to handle? we can't really know which version of kustomize a user is targeting with their arguments without asking them, so I think it's ok to assume that whatever they pass will be compatible with whatever kustomize binary we end up using. this is of course assuming that they can easily know which one we'll select based on good documentation :)

briandealwis · 2020-07-17T14:24:24Z

pkg/skaffold/deploy/kustomize.go

 }

 func NewKustomizeDeployer(runCtx *runcontext.RunContext) *KustomizeDeployer {
+	// if user has kustomize binary, prioritize that over kubectl kustomize
+	useKubectl := !kustomizeBinaryCheck()


Also need to check that kubectl version ≥ 1.14 too

nkubala · 2020-07-22T22:08:41Z

pkg/skaffold/deploy/kustomize.go

+		var out []byte
+		var err error
+
+		if k.useKubectl {


yeah, this could manifest through build args being rejected by the binary depending on which version the user thinks they're using - but I think that's ok left up to them to handle? we can't really know which version of kustomize a user is targeting with their arguments without asking them, so I think it's ok to assume that whatever they pass will be compatible with whatever kustomize binary we end up using. this is of course assuming that they can easily know which one we'll select based on good documentation :)

nkubala · 2020-07-22T22:10:26Z

pkg/skaffold/deploy/kustomize.go

 }

 func NewKustomizeDeployer(runCtx *runcontext.RunContext) *KustomizeDeployer {
+	// if user has kustomize binary, prioritize that over kubectl kustomize
+	useKubectl := !kustomizeBinaryCheck()


nkubala · 2020-07-22T22:10:42Z

pkg/skaffold/deploy/kustomize.go

+		return false
+	}
+
+	return true


might also be worth logging the version of the binary to the user.

What if they have an old old version of kustomize?

i read this PR as if users have kustomize binary use it even if its older version.
Does not matter if they have higher version of kubectl 1.14.
it would be really hard for us to make sure the flags users have specified in the deploy.Kustomize.Flags config are for kustomize or kubectl kustomize

…rsion check

tejal29 · 2020-08-24T20:49:28Z

@MarlonGamez Can you please rebase and i can take look at this

MarlonGamez · 2020-08-24T21:01:03Z

@tejal29 I think I need to update my tests. When I last updated this they passed locally but for some reason didn't pass in Travis. I'll try to revisit some time today 👍🏽

briandealwis

A few nits

briandealwis · 2020-09-01T20:51:32Z

pkg/skaffold/deploy/kustomize.go

+		return false
+	}
+
+	return true


What if they have an old old version of kustomize?

briandealwis · 2020-09-01T20:52:40Z

pkg/skaffold/deploy/kustomize.go

 }

 func NewKustomizeDeployer(runCtx *runcontext.RunContext, labels map[string]string) *KustomizeDeployer {
+	// if user has kustomize binary, prioritize that over kubectl kustomize
+	useKubectl := !kustomizeBinaryCheck() && kubectlVersionCheck(runCtx)
+
 	return &KustomizeDeployer{
 		KustomizeDeploy: runCtx.Cfg.Deploy.KustomizeDeploy,
 		kubectl: deploy.CLI{


It seems odd to pass in a kubectl.CLI if we're not to use kubectl.

I think I should rename useKubectl to useKubectlKustomize. We still need the kubectl.CLI as the kustomize deployer still uses kubectl apply either way

briandealwis · 2020-09-01T20:54:58Z

pkg/skaffold/deploy/kustomize_test.go

+			},
+			commands: testutil.
+				CmdRunOut("kubectl version --client -ojson", kubectlVersion118).
+				AndRunOut("kubectl version --client -ojson", kubectlVersion118).


Do we really run kubectl version twice? That seems … suboptimal.

briandealwis · 2020-09-01T20:57:58Z

pkg/skaffold/deploy/kustomize_test.go

+		commands              util.Command
+		shouldErr             bool
+		forceDeploy           bool
+		useMockKustomizeCheck bool


From a reading perspective, it might be more intuitive for this to be kustomizeCmdAbsent? Or move the mockKustomizeCheck above with a comment as to its use.

tejal29 · 2020-09-10T22:40:30Z

pkg/skaffold/deploy/kustomize.go

+	if err != nil {
+		return false
+	}
+	if gt == 1 {


Suggested change

if gt == 1 {

return gt ==1

tejal29 · 2020-09-10T22:46:25Z

pkg/skaffold/deploy/kustomize.go

+		return false
+	}
+
+	return true


i read this PR as if users have kustomize binary use it even if its older version.
Does not matter if they have higher version of kubectl 1.14.
it would be really hard for us to make sure the flags users have specified in the deploy.Kustomize.Flags config are for kustomize or kubectl kustomize

tejal29 · 2020-09-10T22:49:57Z

pkg/skaffold/deploy/kustomize_test.go

+			if test.kustomizeCmdAbsent {
+				t.Override(&kustomizeBinaryCheck, kustomizeAbsent)
+			}


You can rename testKustomizeCmdAbsent to testKustomizeCmdPresent and instead override the function directly here.

Suggested change

if test.kustomizeCmdAbsent {

t.Override(&kustomizeBinaryCheck, kustomizeAbsent)

}

t.Override(&kustomizeBinaryCheck, func () bool { return test.kustomizeCmdPresent} )

add fallback to kubectl kustomize if kustomize binary isn't present i…

26ceb9e

…n user's PATH

pull-request-size bot added the size/L label Jul 16, 2020

googlebot added the cla: yes label Jul 16, 2020

MarlonGamez marked this pull request as ready for review July 16, 2020 20:28

MarlonGamez requested a review from a team as a code owner July 16, 2020 20:28

MarlonGamez requested a review from balopat July 16, 2020 20:28

briandealwis reviewed Jul 17, 2020

View reviewed changes

nkubala reviewed Jul 22, 2020

View reviewed changes

MarlonGamez added 2 commits July 23, 2020 13:09

use exec.LookPath("kustomize") over "which kustomize", add kubectl ve…

0c8e998

…rsion check

fix merge conflicts

aa3a086

briandealwis reviewed Sep 1, 2020

View reviewed changes

MarlonGamez added 5 commits September 9, 2020 11:12

fix kubectl version double call, nits

2fa2c99

trying to kickoff travis

c5a49d6

fix merge conflicts

1a00075

remove buildargs

61796f3

fix new test and kpt deployer kustomize call

9d6967d

MarlonGamez requested a review from yuwenma as a code owner September 9, 2020 19:39

add another mock kustomize check to fake true

1853c63

tejal29 requested changes Sep 10, 2020

View reviewed changes

nkubala assigned tejal29 Sep 21, 2020

cleanup some functions and tests

095909f

tejal29 approved these changes Sep 21, 2020

View reviewed changes

fix issues with rebase

8706968

MarlonGamez merged commit 35cee15 into GoogleContainerTools:master Sep 21, 2020

fmotrifork mentioned this pull request Oct 1, 2020

skaffold 1.15.0 fishworks/fish-food#989

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add fallback to kubectl kustomize if kustomize binary isn't present #4484

Add fallback to kubectl kustomize if kustomize binary isn't present #4484

MarlonGamez commented Jul 16, 2020

codecov bot commented Jul 16, 2020 •

edited

Loading

briandealwis Jul 17, 2020 •

edited

Loading

nkubala Jul 22, 2020

briandealwis Jul 17, 2020

nkubala Jul 22, 2020

nkubala Jul 22, 2020

nkubala Jul 22, 2020

nkubala Jul 22, 2020

briandealwis Sep 1, 2020

tejal29 Sep 10, 2020

tejal29 commented Aug 24, 2020

MarlonGamez commented Aug 24, 2020

briandealwis left a comment

briandealwis Sep 1, 2020

briandealwis Sep 1, 2020

MarlonGamez Sep 9, 2020

briandealwis Sep 1, 2020

briandealwis Sep 1, 2020

tejal29 Sep 10, 2020

tejal29 Sep 10, 2020

tejal29 Sep 10, 2020

Add fallback to kubectl kustomize if kustomize binary isn't present #4484

Add fallback to kubectl kustomize if kustomize binary isn't present #4484

Conversation

MarlonGamez commented Jul 16, 2020

codecov bot commented Jul 16, 2020 • edited Loading

Codecov Report

briandealwis Jul 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tejal29 commented Aug 24, 2020

MarlonGamez commented Aug 24, 2020

briandealwis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 16, 2020 •

edited

Loading

briandealwis Jul 17, 2020 •

edited

Loading