Avoid writing environment variables in Tiltfile

[sbueringer]

Today we're writing to the os environment in our Tiltfile so that clusterctl can pick up the env vars when we use clusterctl to generate a cluster.

cluster-api/Tiltfile

Lines 429 to 432 in 835c589

	os.environ["NAMESPACE"] = substitutions.get("NAMESPACE", "default")
	os.environ["KUBERNETES_VERSION"] = substitutions.get("KUBERNETES_VERSION", "v1.24.0")
	os.environ["CONTROL_PLANE_MACHINE_COUNT"] = substitutions.get("CONTROL_PLANE_MACHINE_COUNT", "1")
	os.environ["WORKER_MACHINE_COUNT"] = substitutions.get("WORKER_MACHINE_COUNT", "3")

This was necessary because Tilt didn't support passing in env variables into cmd_button:

cluster-api/Tiltfile

Lines 484 to 490 in 835c589

	cmd_button(
	template_name + ":apply",
	argv = ["bash", "-c", apply_cluster_template_cmd],
	resource = template_name,
	icon_name = "add_box",
	text = "Create `" + template_name + "` cluster",
	)

This has been implemented now: tilt-dev/tilt#5910 (comment)

Let's try to set the env of the cmd_button (and local_resource above) and stop writing to os.environment

Note: This also requires bumping the minimum supported Tilt version (probably to v0.30.8)

/kind feature

[k8s-ci-robot]

@sbueringer: This issue is currently awaiting triage.

If CAPI contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

[sbueringer]

/triage accepted

[fabriziopandini]

/help

I think we should wait a little bit to give some room to folks to pick up with the latest tilt version, but +1 to get this cleanup implemented

[k8s-ci-robot]

@fabriziopandini:
This request has been marked as needing help from a contributor.

Guidelines

Please ensure that the issue body includes answers to the following questions:

• Why are we solving this issue?
• To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
• Does this issue have zero to low barrier of entry?
• How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

/help

I think we should wait a little bit to give some room to folks to pick up with the latest tilt version, but +1 to get this cleanup implemented

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.

[chiukapoor]

Hello Team, I have gone through the PR tilt-dev/tilt#5910 (comment). May I work on this issue?

[sbueringer]

Sure

[chiukapoor]

Along with the below cmd_button function's inputs parameter

cluster-api/Tiltfile

Lines 523 to 535 in dba1c70

	cmd_button(
	template_name + ":apply",
	argv = ["bash", "-c", apply_cluster_template_cmd],
	resource = template_name,
	icon_name = "add_box",
	text = "Create `" + template_name + "` cluster",
	inputs = [
	text_input("NAMESPACE", default = substitutions.get("NAMESPACE")),
	text_input("KUBERNETES_VERSION", default = substitutions.get("KUBERNETES_VERSION")),
	text_input("CONTROL_PLANE_MACHINE_COUNT", default = substitutions.get("CONTROL_PLANE_MACHINE_COUNT")),
	text_input("WORKER_MACHINE_COUNT", default = substitutions.get("WORKER_MACHINE_COUNT")),
	],
	)

Should I update the rest of the cmd_button function's inputs parameter with env = substitutions? @sbueringer

cluster-api/Tiltfile

Lines 537 to 546 in dba1c70

	cmd_button(
	template_name + ":delete",
	argv = ["bash", "-c", delete_clusters_cmd],
	resource = template_name,
	icon_name = "delete_forever",
	text = "Delete `" + template_name + "` clusters",
	inputs = [
	text_input("NAMESPACE", default = substitutions.get("NAMESPACE")),
	],
	)

cluster-api/Tiltfile

Lines 488 to 508 in dba1c70

	cmd_button(
	clusterclass_name + ":apply",
	argv = ["bash", "-c", apply_clusterclass_cmd],
	resource = clusterclass_name,
	icon_name = "note_add",
	text = "Apply `" + clusterclass_name + "` ClusterClass",
	inputs = [
	text_input("NAMESPACE", default = substitutions.get("NAMESPACE")),
	],
	)
	cmd_button(
	clusterclass_name + ":delete",
	argv = ["bash", "-c", delete_clusterclass_cmd],
	resource = clusterclass_name,
	icon_name = "delete_forever",
	text = "Delete `" + clusterclass_name + "` ClusterClass",
	inputs = [
	text_input("NAMESPACE", default = substitutions.get("NAMESPACE")),
	],
	)

/assign

[chiukapoor]

Along with the below cmd_button function's inputs parameter

cluster-api/Tiltfile

Lines 523 to 535 in dba1c70

	cmd_button(
	template_name + ":apply",
	argv = ["bash", "-c", apply_cluster_template_cmd],
	resource = template_name,
	icon_name = "add_box",
	text = "Create `" + template_name + "` cluster",
	inputs = [
	text_input("NAMESPACE", default = substitutions.get("NAMESPACE")),
	text_input("KUBERNETES_VERSION", default = substitutions.get("KUBERNETES_VERSION")),
	text_input("CONTROL_PLANE_MACHINE_COUNT", default = substitutions.get("CONTROL_PLANE_MACHINE_COUNT")),
	text_input("WORKER_MACHINE_COUNT", default = substitutions.get("WORKER_MACHINE_COUNT")),
	],
	)

Should I update the rest of the cmd_button function's inputs parameter with env = substitutions? @sbueringer

cluster-api/Tiltfile

Lines 537 to 546 in dba1c70

	cmd_button(
	template_name + ":delete",
	argv = ["bash", "-c", delete_clusters_cmd],
	resource = template_name,
	icon_name = "delete_forever",
	text = "Delete `" + template_name + "` clusters",
	inputs = [
	text_input("NAMESPACE", default = substitutions.get("NAMESPACE")),
	],
	)

cluster-api/Tiltfile

Lines 488 to 508 in dba1c70

	cmd_button(
	clusterclass_name + ":apply",
	argv = ["bash", "-c", apply_clusterclass_cmd],
	resource = clusterclass_name,
	icon_name = "note_add",
	text = "Apply `" + clusterclass_name + "` ClusterClass",
	inputs = [
	text_input("NAMESPACE", default = substitutions.get("NAMESPACE")),
	],
	)
	cmd_button(
	clusterclass_name + ":delete",
	argv = ["bash", "-c", delete_clusterclass_cmd],
	resource = clusterclass_name,
	icon_name = "delete_forever",
	text = "Delete `" + clusterclass_name + "` ClusterClass",
	inputs = [
	text_input("NAMESPACE", default = substitutions.get("NAMESPACE")),
	],
	)

/assign

@sbueringer @fabriziopandini May you please help me with this query

[sbueringer]

What do you mean with "update the rest of the cmd_button function's inputs parameter with env = substitutions?" ?

Adding env = substitutions to the cmd_button or something with the inputs parameter?

The idea would be to add env = substitutions to all buttons in deploy_cluster_template and deploy_clusterclass

[chiukapoor]

The idea would be to add env = substitutions to all buttons in deploy_cluster_template and deploy_clusterclass

Thanks for the clarification will update the above functions.

I meant to ask if the inputs parameter needs to be removed and the env parameter needs to be added, as once the env variable provides the content of list substitutions as an environment variable we won't need to pass it using the inputs parameter

cluster-api/Tiltfile

Lines 543 to 545 in dba1c70

	inputs = [
	text_input("NAMESPACE", default = substitutions.get("NAMESPACE")),
	],

Also, instead of using $NAMESPACE, an environment variable in below code snippets, we can directly use the substitutions list to get the NAMESPACE value

cluster-api/Tiltfile

Lines 511 to 512 in dba1c70

apply_cluster_template_cmd = "CLUSTER_NAME=" + template_name + "-$RANDOM;" + clusterctl_cmd + " generate cluster -n $NAMESPACE $CLUSTER_NAME --from " + filename + " | " + kubectl_cmd + " apply -f - && echo \"Cluster '$CLUSTER_NAME' created, don't forget to delete\n\""

delete_clusters_cmd = 'DELETED=$(echo "$(bash -c "' + kubectl_cmd + ' --namespace=$NAMESPACE get clusters -A --no-headers -o custom-columns=":metadata.name"")" | grep -E "^' + template_name + '-[[:digit:]]{1,5}$"); if [ -z "$DELETED" ]; then echo "Nothing to delete for cluster template ' + template_name + '"; else echo "Deleting clusters:\n$DELETED\n"; echo $DELETED | xargs -L1 ' + kubectl_cmd + ' delete cluster; fi; echo "\n"'

cluster-api/Tiltfile

Lines 476 to 477 in dba1c70

	apply_clusterclass_cmd = "cat " + filename + " | " + envsubst_cmd + " | " + kubectl_cmd + " apply --namespace=$NAMESPACE -f - && echo \"ClusterClass created from\'" + filename + "\', don't forget to delete\n\""
	delete_clusterclass_cmd = kubectl_cmd + " --namespace=$NAMESPACE delete clusterclass " + clusterclass_name + ' --ignore-not-found=true; echo "\n"'

Doing the above will help us finally remove the below workaround as well

cluster-api/Tiltfile

Lines 444 to 446 in dba1c70

	# Note: this is a workaround to pass env variables to cmd buttons while this is not supported natively like in local_resource
	for name, value in substitutions.items():
	os.environ[name] = value

I will like to know your views on it @sbueringer

[sbueringer]

Please keep inputs. With inputs users can set the fields in the Tilt UI. This is a feature we want to keep.

[chiukapoor]

@sbueringer, May you please help me understand how may I successfully test the changes?

While doing my research I came across https://cluster-api.sigs.k8s.io/developer/tilt.html to set up the development environment.

[sbueringer]

Follow the documentation to setup the tilt environment (with CAPD).

Then make your changes and verify that you can set the variables via kustomize_substitutions (tilt-settings.yaml). If that works, verify that you can also set them via the Tilt UI (the buttons will have an input form ~ like this: https://docs.tilt.dev/buttons.html). If the variables are set via the Tilt UI they should overwrite the ones set in tilt-settings.