🌱 fix tilt with envsubst #3361

alexeldeib · 2020-07-16T23:27:12Z

Signed-off-by: Alexander Eldeib [email protected]

What this PR does / why we need it:
Make tilt work with envsubst

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #3326

fabriziopandini

Thanks @alexeldeib for porting this fix in CAPI

fabriziopandini · 2020-07-17T09:09:09Z

Makefile

@@ -173,6 +176,11 @@ $(LINK_CHECKER): $(TOOLS_DIR)/go.mod
 $(GO_APIDIFF): $(TOOLS_DIR)/go.mod
 	cd $(TOOLS_DIR) && go build -tags=tools -o $(GO_APIDIFF_BIN) github.com/joelanford/go-apidiff

+$(ENVSUBST): $(TOOLS_DIR)/go.mod
+	cd $(TOOLS_DIR) && go build -tags=tools -o $(ENVSUBST_BIN) github.com/a8m/envsubst/cmd/envsubst


If I'm not wrong we should add a new import

cluster-api/hack/tools/tools.go

Line 23 in 5596ab2

_ "github.com/go-bindata/go-bindata"

Also, why are we using github.com/a8m/envsubst instead of github.com/drone/envsubst? my concern is that there will be subtle differences between how those library behaves...
cc @wfernandes

drone doesn't offer a binary, I think they are good enough in compatibility

@benmoss drone doesn't offer a binary as part of it's releases but there is a cmd/envsubst in drone that can be built. So I believe we can build the binary if we needed to.

There are some subtle differences between drone/envsubst and a8m/envsubst. The one difference I know of is that a8m handles the differences with operators such as :=, =, :- whereas drone treats them all the same. Also drone documents that it doesn't support :+ notation as a8m does.

Oversight on the package differences, I'll grab drone/envsubst and build the binary

docs/book/src/developer/tilt.md

benmoss · 2020-07-17T14:53:24Z

Tiltfile

@@ -231,6 +234,9 @@ def enable_providers():
    for name in union_enable_providers:
        enable_provider(name)

+def kustomize(path):
+    return str(local("kustomize build {} | {}".format(path, envsubst_cmd), quiet = True))


WDYT about running local("make envsubst") before this so we install envsusbt if it hasn't already been? Should be a no-op if it's already there from Make's logic.

Just out of curiosity, if this method is being added now what was the tilt script calling before? Or is this overriding some kustomize(path) function that's defined elsewhere?

I should probably rename the func, tilt provides a function with the same name which does its own handling (that's what was invoked before)

Yeah, Tilt's falls back to kubectl kustomize if kustomize isn't on the PATH, but that fails with our usage of kustomize (as I learned on my newly provisioned machine today)

https://docs.tilt.dev/api.html#api.kustomize

Well, TIL. Yeah, you don't have to change the name of the method if you don't want to. Perhaps a commenting explaining above would do just fine. Or both 🤷 🙂

benmoss · 2020-07-17T14:54:33Z

Great work, I was trying to do this yesterday too and got very close but you figured out the remaining bit of piping kustomize to envsubst 😄

wfernandes

Thanks for the PR @alexeldeib!

wfernandes · 2020-07-17T15:26:03Z

docs/book/src/developer/tilt.md

@@ -13,9 +13,16 @@ workflow that offers easy deployments and rapid iterative builds.
 1. [kustomize](https://github.com/kubernetes-sigs/kustomize/blob/master/docs/INSTALL.md) standalone
   (`kubectl kustomize` does not work because it is missing some features of kustomize v3)
 1. [Tilt](https://docs.tilt.dev/install.html) v0.12.0 or newer
+1. [envsubst](https://github.com/a8m/envsubst) v1.1.0 or similar to handle clusterctl var replacement.


If we really are unable to use drone/envsubst as a binary here, I suggest we document that drone/envsubst and a8m/envsubst have similar behavior because the docs in https://cluster-api.sigs.k8s.io/clusterctl/provider-contract.html#variables call out our usage of drone/envsubst.
This inconsistency may be confusing to users.

wfernandes · 2020-07-17T15:31:36Z

Tiltfile

@@ -231,6 +234,9 @@ def enable_providers():
    for name in union_enable_providers:
        enable_provider(name)

+def kustomize(path):
+    return str(local("kustomize build {} | {}".format(path, envsubst_cmd), quiet = True))


Just out of curiosity, if this method is being added now what was the tilt script calling before? Or is this overriding some kustomize(path) function that's defined elsewhere?

wfernandes

Minor thing but this LGTM. I haven't tested this out myself so I'll leave approval for @benmoss

wfernandes · 2020-07-17T17:55:17Z

docs/book/src/developer/tilt.md

 1. Clone the [Cluster API](https://github.com/kubernetes-sigs/cluster-api) repository locally
 1. Clone the provider(s) you want to deploy locally as well

+We provider a make target to generate the envsubst binary if desired.


nit:

Suggested change

We provider a make target to generate the envsubst binary if desired.

We provide a make target to generate the envsubst binary if desired.

CecileRobertMichon · 2020-07-17T18:04:34Z

Makefile

@@ -173,6 +176,11 @@ $(LINK_CHECKER): $(TOOLS_DIR)/go.mod
 $(GO_APIDIFF): $(TOOLS_DIR)/go.mod
 	cd $(TOOLS_DIR) && go build -tags=tools -o $(GO_APIDIFF_BIN) github.com/joelanford/go-apidiff

+$(ENVSUBST): $(TOOLS_DIR)/go.mod
+	cd $(TOOLS_DIR) && go build -tags=tools -o $(ENVSUBST_BIN) github.com/drone/envsubst/cmd/envsubst


we should do the same in capz, was wondering about the impact the subtle differences between the two could have as well.

Signed-off-by: Alexander Eldeib <[email protected]>

alexeldeib · 2020-07-17T18:42:13Z

added a quick note about drone/envsubst binary since it's only available on untagged commits

alexeldeib · 2020-07-17T18:57:36Z

/test pull-cluster-api-e2e

wfernandes · 2020-07-17T21:18:55Z

/lgtm

CecileRobertMichon

/lgtm
/approve

k8s-ci-robot · 2020-07-17T23:22:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

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

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

CecileRobertMichon · 2020-07-17T23:22:34Z

/milestone v0.3.x

k8s-ci-robot · 2020-07-17T23:22:35Z

@CecileRobertMichon: You must be a member of the kubernetes-sigs/cluster-api-maintainers GitHub team to set the milestone. If you believe you should be able to issue the /milestone command, please contact your Cluster API Maintainers and have them propose you as an additional delegate for this responsibility.

In response to this:

/milestone v0.3.x

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.

vincepri · 2020-07-18T14:41:27Z

Thanks for fixing this 😄 !

In the future, should we have a sub-command that exposes our implementation of envsubst within clusterctl and shell out to that instead? It'd be also great to have a --variables option that prints out which vars are required, which are optional and what their defaults are.

It'd make a better UX both for clusterctl users, and for developers when using Tilt. Today, we only return an error which isn't clear https://github.com/kubernetes-sigs/cluster-api/blob/master/Tiltfile#L203

ncdc · 2020-07-22T14:29:01Z

FYI, I think this breaks kustomize_substitutions. The line

yaml = str(kustomize_with_envsubst(context + "/config"))

replaces all ${...} placeholders, and if values are unset (not in the environment), they're replaced with "". The lines immediately following that attempt to process kustomize_substitutions now have nothing to replace. I noticed this when trying to test CAPA, where I have AWS_B64ENCODED_CREDENTIALS in my kustomize_substitutions and it's showing up as "".

ncdc · 2020-07-22T14:57:59Z

I wonder if we can copy all the kustomize_substitutions and put them into https://docs.tilt.dev/api.html#modules.os.environ before running envsubst?

alexeldeib · 2020-07-22T15:02:36Z

I was just noticing the same thing yesterday with machine pool feature flags, I think what you suggested is correct. I can make a PR a little later this morning if you don’t beat me to it.

alexeldeib · 2020-07-22T15:02:38Z

I was just noticing the same thing yesterday with machine pool feature flags, I think what you suggested is correct. I can make a PR a little later this morning if you don’t beat me to it.

ncdc · 2020-07-22T15:04:02Z

I'll let you know if I get started on it; otherwise, I'll wait for you. Thanks!

ncdc · 2020-07-22T15:06:43Z

I filed #3375 for this

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 16, 2020

k8s-ci-robot requested review from benmoss and CecileRobertMichon July 16, 2020 23:27

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jul 16, 2020

alexeldeib force-pushed the ace/tiltenv branch from 7e62dbc to e6de1b9 Compare July 17, 2020 01:12

fabriziopandini reviewed Jul 17, 2020

View reviewed changes

benmoss reviewed Jul 17, 2020

View reviewed changes

wfernandes reviewed Jul 17, 2020

View reviewed changes

alexeldeib force-pushed the ace/tiltenv branch from e6de1b9 to 0dd1668 Compare July 17, 2020 17:39

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 17, 2020

wfernandes reviewed Jul 17, 2020

View reviewed changes

CecileRobertMichon reviewed Jul 17, 2020

View reviewed changes

alexeldeib force-pushed the ace/tiltenv branch 3 times, most recently from 27dea82 to 428091c Compare July 17, 2020 18:20

🌱 fix tilt with envsubst

73b18d5

Signed-off-by: Alexander Eldeib <[email protected]>

alexeldeib force-pushed the ace/tiltenv branch from 428091c to 73b18d5 Compare July 17, 2020 18:41

k8s-ci-robot assigned wfernandes Jul 17, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 17, 2020

CecileRobertMichon approved these changes Jul 17, 2020

View reviewed changes

k8s-ci-robot assigned CecileRobertMichon Jul 17, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 17, 2020

k8s-ci-robot merged commit 9aaae9f into kubernetes-sigs:master Jul 17, 2020

vincepri added this to the v0.3.8 milestone Jul 18, 2020

ncdc mentioned this pull request Jul 22, 2020

kustomize_substitutions in tilt-settings.json is broken #3375

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 fix tilt with envsubst #3361

🌱 fix tilt with envsubst #3361

alexeldeib commented Jul 16, 2020

fabriziopandini left a comment

fabriziopandini Jul 17, 2020

benmoss Jul 17, 2020

wfernandes Jul 17, 2020

alexeldeib Jul 17, 2020

benmoss Jul 17, 2020 •

edited

Loading

wfernandes Jul 17, 2020

alexeldeib Jul 17, 2020

benmoss Jul 17, 2020

wfernandes Jul 17, 2020

benmoss commented Jul 17, 2020

wfernandes left a comment

wfernandes Jul 17, 2020

wfernandes Jul 17, 2020

wfernandes left a comment

wfernandes Jul 17, 2020

CecileRobertMichon Jul 17, 2020

alexeldeib commented Jul 17, 2020

alexeldeib commented Jul 17, 2020

wfernandes commented Jul 17, 2020

CecileRobertMichon left a comment

k8s-ci-robot commented Jul 17, 2020

CecileRobertMichon commented Jul 17, 2020

k8s-ci-robot commented Jul 17, 2020

vincepri commented Jul 18, 2020

ncdc commented Jul 22, 2020

ncdc commented Jul 22, 2020

alexeldeib commented Jul 22, 2020

alexeldeib commented Jul 22, 2020

ncdc commented Jul 22, 2020

ncdc commented Jul 22, 2020

	We provider a make target to generate the envsubst binary if desired.
	We provide a make target to generate the envsubst binary if desired.

🌱 fix tilt with envsubst #3361

🌱 fix tilt with envsubst #3361

Conversation

alexeldeib commented Jul 16, 2020

fabriziopandini 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

benmoss 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

benmoss commented Jul 17, 2020

wfernandes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wfernandes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexeldeib commented Jul 17, 2020

alexeldeib commented Jul 17, 2020

wfernandes commented Jul 17, 2020

CecileRobertMichon left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 17, 2020

CecileRobertMichon commented Jul 17, 2020

k8s-ci-robot commented Jul 17, 2020

vincepri commented Jul 18, 2020

ncdc commented Jul 22, 2020

ncdc commented Jul 22, 2020

alexeldeib commented Jul 22, 2020

alexeldeib commented Jul 22, 2020

ncdc commented Jul 22, 2020

ncdc commented Jul 22, 2020

benmoss Jul 17, 2020 •

edited

Loading