feat: setup tilt for easier local controller development #220

Al-Pragliola · 2025-02-27T15:28:44Z

closes #41

This PR aims to add Tilt as a local development tool for the project, you can find an initial stub on how to try the PR locally here https://github.com/kubeflow/notebooks/blob/c38ac05c3dad7efd30a5935bbee8a6643baea817/devenv/README.md

ederign · 2025-02-27T18:13:15Z

/assign @thesuperzapper @ederign

thesuperzapper

@Al-Pragliola thanks so much for your work on this!

So we can merge it, can you make the following changes:

Lets scope all this within the workspaces/controller folder, rather than having things in the root of this repo.
Lets create a DEVELOPMENT.md in the root of workspaces/controller which explains the steps required to run a dev version of the controller with tilt (on any cluster the user might have in their kubecontext, or with kind if they want a clean setup)

In a future PR, we can then have a development guide in the workspaces/backend folder which just points to the controller one as a "first step" before explaining how to use make run in that case (which does not need tilt, at least at this stage).

thesuperzapper · 2025-03-06T23:12:57Z

devenv/configs/tiltfiles/cert-manager.tiltfile

+helm_repo(
+  name="jetstack",
+  url="https://charts.jetstack.io",
+  resource_name="jetstack-repo",
+  labels=["common"]
+)
+
+helm_resource(
+  name="cert-manager",
+  chart="jetstack/cert-manager",
+  release_name="cert-manager",
+  namespace="cert-manager",
+  flags=["--version=v1.17.0", "--set", "crds.enabled=true"],
+  deps=["namespaces", "cert-manager"],
+  labels=["common"]
+)


I think we should not automatically install cert-manager (or other deps), and instead tell the user to install them with specific commands (if they are not already installed on the cluster they are using).

That way we avoid people accidentally breaking prod clusters.

google-oss-prow · 2025-03-07T02:15:19Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from thesuperzapper. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

Al-Pragliola · 2025-03-11T17:53:51Z

@Al-Pragliola thanks so much for your work on this!

So we can merge it, can you make the following changes:
1. Lets scope all this within the `workspaces/controller` folder, rather than having things in the root of this repo.

2. Lets create a `DEVELOPMENT.md` in the root of `workspaces/controller` which explains the steps required to run a dev version of the controller with tilt (on any cluster the user might have in their kubecontext, or with kind if they want a clean setup)
In a future PR, we can then have a development guide in the workspaces/backend folder which just points to the controller one as a "first step" before explaining how to use make run in that case (which does not need tilt, at least at this stage).

Thanks for the feedback @thesuperzapper , in the latest commits:

Moved everything under the workspaces/controller folder
Removed the installation of cert-manager, added a check in the make tilt-up target to see if cert-manager is installed in the current cluster
Added a DEVELOPMENT.md file with instructions on how to spin up the development environment in the workspaces/controller folder

let me know wdyt 🙏🏼

workspaces/controller/DEVELOPMENT.md

+
+- Go >= 1.22
+- Kubectl >= 1.22
+- A Kubernetes cluster (e.g. [kind](https://kind.sigs.k8s.io/))


workspaces/controller/DEVELOPMENT.md

+
+## Getting Started
+
+This project uses [Tilt](https://tilt.dev/) to manage the development environment. Tilt will watch for changes in the project and automatically rebuild the Docker image and redeploy the application in the **current Kubernetes context**.


Al-Pragliola · 2025-03-21T21:38:50Z

workspaces/controller/DEVELOPMENT.md

+2. Run the following command to start Tilt:
+
+```bash
+make -C devenv tilt-up


should be fixed following the comment #220 (comment)

workspaces/controller/DEVELOPMENT.md

+2. Run the following command to start Tilt:
+
+```bash
+make -C devenv tilt-up
+```
+
+3. Hit `space` to open the Tilt dashboard in your browser and here you can see the logs and status of every resource deployed.


andyatmiami · 2025-03-21T17:50:33Z

workspaces/controller/devenv/Makefile

+
+## Requirements
+
+CERT_MANAGER_VERSION := 1.17.1


do we really wanna hard-code the version of CM here? If so, we probably need additional guidance on the CM install above... as directing users to the general Cert Manager install doc is going to end up with them installing whatever the latest version is...

You are right, let's tune down tech debt I have updated the message to

Verifying cert-manager installation... cert-manager is either not installed or not ready. Please install cert-manager or wait until it becomes ready. Installation instructions can be found here: https://cert-manager.io/docs/installation/

wdyt?

Al-Pragliola · 2025-03-21T21:38:08Z

workspaces/controller/devenv/Makefile

+$(TILT): $(LOCALBIN)
+	test -s $(LOCALBIN)/tilt || curl -fsSL https://github.com/tilt-dev/tilt/releases/download/v$(TILT_VERSION)/tilt.$(TILT_VERSION).$(detected_OS).$(arch).tar.gz | tar -xz -C $(LOCALBIN) tilt
+
+tilt-up: tilt check-cert-manager


thanks for checking this PR, yes the Makefile had a bug in line 25, cmctl was meant to be in line 26, I fixed it in the latest commit eda5804

Al-Pragliola · 2025-03-24T20:49:44Z

workspaces/controller/devenv/Makefile

+.PHONY: $(CMCTL)
+cmctl: $(CMCTL)
+$(CMCTL): $(LOCALBIN)
+	test -s $(LOCALBIN)/cmctl || curl -fsSL https://github.com/cert-manager/cmctl/releases/download/v$(CMCTL_VERSION)/cmctl_$(detected_OS)_$(goarch).tar.gz | tar -xz -C $(LOCALBIN) cmctl


good catch, fixed thanks

andyatmiami

Good news is I was finally able to get tilt-up to work - thanks for those changes...

I am now facing this error displayed in tilt dashboard:

The controller error seems to be the real error - so we should investigate that... possibly due to me being on arm64 machine?

I am also confused why the cert-manager-req check has a red X as its status 🤔

Please advise on both points - thanks!

andyatmiami · 2025-04-01T18:47:55Z

workspaces/controller/devenv/Tiltfile

+objects = decode_yaml_stream(manifests)
+
+for o in objects:
+    if o["kind"] == "Deployment" and o.get("metadata").get("name") in ["workspace-controller-controller-manager"]:
+        o["spec"]["template"]["spec"]["securityContext"] = {"runAsNonRoot": False, "readOnlyRootFilesystem": False}
+        o["spec"]["template"]["spec"]["containers"][0]["imagePullPolicy"] = "Always"
+
+overridden_manifests = encode_yaml_stream(objects)


Curious why we are programmatically modifying manifests here... instead of using kustomize overlays?

I didn't want to create an overlay for local development, but I am open to do it if the community agrees

Signed-off-by: Alessio Pragliola <seth.pro@gmail.com>

thesuperzapper · 2025-04-17T16:14:46Z

workspaces/controller/DEVELOPMENT.md

@@ -0,0 +1,71 @@
+# Development Guide


@Al-Pragliola can we please add something about running the the tests locally before raising a PR:

make lint

make test (for unit tests without a local kube/kind)

make test-e2e (run intergration tests on a local kube)

As people wont know they need to do this otherwise

github-project-automation bot added this to Kubeflow Notebooks Feb 27, 2025

google-oss-prow bot added the do-not-merge/work-in-progress label Feb 27, 2025

github-project-automation bot moved this to Needs Triage in Kubeflow Notebooks Feb 27, 2025

google-oss-prow bot requested review from kimwnasptd and thesuperzapper February 27, 2025 15:28

google-oss-prow bot added the size/L label Feb 27, 2025

Al-Pragliola changed the title ~~feat(devend): Notebooks 2.0 // Controller // Update local development guide~~ feat: Notebooks 2.0 // Controller // Update local development guide Feb 27, 2025

Al-Pragliola marked this pull request as ready for review February 27, 2025 16:17

google-oss-prow bot removed the do-not-merge/work-in-progress label Feb 27, 2025

google-oss-prow bot assigned ederign and thesuperzapper Feb 27, 2025

thesuperzapper changed the title ~~feat: Notebooks 2.0 // Controller // Update local development guide~~ feat: setup tilt for easier local controller development Mar 6, 2025

thesuperzapper requested changes Mar 7, 2025

View reviewed changes

Al-Pragliola requested a review from thesuperzapper March 11, 2025 17:50

ederign mentioned this pull request Mar 19, 2025

feat(ws): stop using mocks for frontend get workspaces #188

Open

andyatmiami reviewed Mar 21, 2025

View reviewed changes

workspaces/controller/DEVELOPMENT.md Outdated

- Go >= 1.22

- Kubectl >= 1.22

- A Kubernetes cluster (e.g. [kind](https://kind.sigs.k8s.io/))

This comment was marked as resolved.

Sign in to view

andyatmiami reviewed Mar 21, 2025

View reviewed changes

andyatmiami reviewed Mar 24, 2025

View reviewed changes

andyatmiami reviewed Apr 1, 2025

View reviewed changes

Al-Pragliola force-pushed the al-pragliola-tilt-integration branch from 8e6fc0b to 32585fc Compare April 2, 2025 20:32

Al-Pragliola added 2 commits April 2, 2025 22:35

feat(dev): initial tilt config

5b74df0

Signed-off-by: Alessio Pragliola <seth.pro@gmail.com>

fix(devenv): CRDs got deleted when forcing a reload

443f5a9

Signed-off-by: Alessio Pragliola <seth.pro@gmail.com>

Al-Pragliola and others added 8 commits April 2, 2025 22:35

chore(devenv): remove commented code

f237715

Signed-off-by: Alessio Pragliola <seth.pro@gmail.com>

feat(devenv): move to controller folder and remove reqs

3bd03c8

Signed-off-by: Alessio Pragliola <seth.pro@gmail.com>

feat(devenv): added DEVELOPMENT.md file

bc76a4e

Signed-off-by: Alessio Pragliola <seth.pro@gmail.com>

fix(devenv): missing cmctl in make target

01e2995

Signed-off-by: Alessio Pragliola <seth.pro@gmail.com>

chore(devenv): change cert-manager missing text in Makefile

ce0479f

Signed-off-by: Alessio Pragliola <seth.pro@gmail.com>

chore(devenv): change kind installation link

3063ce3

Signed-off-by: Alessio Pragliola <seth.pro@gmail.com>

fix(devenv): OS in cmctl make target

d582a12

Signed-off-by: Alessio Pragliola <seth.pro@gmail.com>

wording changes

efb3377

Signed-off-by: Alessio Pragliola <seth.pro@gmail.com>

Al-Pragliola force-pushed the al-pragliola-tilt-integration branch from 32585fc to efb3377 Compare April 2, 2025 20:35

Al-Pragliola requested a review from andyatmiami April 2, 2025 20:38

thesuperzapper requested changes Apr 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: setup tilt for easier local controller development #220

feat: setup tilt for easier local controller development #220

Al-Pragliola commented Feb 27, 2025 •

edited

Loading

ederign commented Feb 27, 2025

thesuperzapper left a comment

thesuperzapper Mar 6, 2025

google-oss-prow bot commented Mar 7, 2025

Al-Pragliola commented Mar 11, 2025 •

edited

Loading

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Al-Pragliola Mar 21, 2025

This comment was marked as resolved.

andyatmiami Mar 21, 2025

Al-Pragliola Mar 21, 2025

This comment was marked as resolved.

Al-Pragliola Mar 21, 2025

This comment was marked as resolved.

Al-Pragliola Mar 24, 2025

andyatmiami left a comment

andyatmiami Apr 1, 2025

Al-Pragliola Apr 2, 2025

thesuperzapper Apr 17, 2025


		## Getting Started

		This project uses [Tilt](https://tilt.dev/) to manage the development environment. Tilt will watch for changes in the project and automatically rebuild the Docker image and redeploy the application in the current Kubernetes context.

feat: setup tilt for easier local controller development #220

Are you sure you want to change the base?

feat: setup tilt for easier local controller development #220

Conversation

Al-Pragliola commented Feb 27, 2025 • edited Loading

ederign commented Feb 27, 2025

thesuperzapper left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

google-oss-prow bot commented Mar 7, 2025

Al-Pragliola commented Mar 11, 2025 • edited Loading

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

andyatmiami 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

Al-Pragliola commented Feb 27, 2025 •

edited

Loading

Al-Pragliola commented Mar 11, 2025 •

edited

Loading