Feat/image builder api #614

SalimKayal · 2025-01-21T16:24:17Z

This PR provides a draft of what could be the updated API to include ImageBuilders.

Panaetius · 2025-01-22T07:49:37Z

components/renku_data_services/session/api.spec.yaml

          $ref: "#/components/schemas/ContainerImage"
+        builder_id:
+          $ref: "#/components/schemas/ULID"


do we want a separate endpoint for creating builders and then have image and builder_id as properties here, or should they just be POST on this endpoint with a oneOf: body?

This was one of the main questions. I am open to both. Waiting for input.

The indirection adds complexity for clients. I do not think that we have an expectation of the relation going from {0,1}:1 to 1:N at any time. A "builder" belongs to a single session launcher.

Does it ? I'm currently unsure about all the implications. Let's discuss this in a technical meeting.

I removed these endpoints. Now, BuildParameter is part of session launcher API.

Panaetius · 2025-01-22T07:56:33Z

components/renku_data_services/session/api.spec.yaml

+        - CONDA
+        - PIP
+        - R
+        - DOCKERFILE
+    EditorKind:
+      description: User's Editor Choice.
+      type: string
+      enum:
+        - VSCODIUM
+        - JUPYTERLAB
+        - STREAMLIT


I know this is consistent with what was in this apispec before, but I noticed that a lot of the enums in our API use lower-case values, and this file is the only place that uses UPPER case ones. maybe this build would be a good opportunity to make this consistent?

Noted. I will make it consistent

Just one thing, the UPPER CASE might have been because of the global reserved keyword. How do you think this should be resolved ?

I resolved it the way the python operator library does, by adding an underscore after the reserved keyword. It's now global_. I'm not very happy with that but that would be a way of doing it.

oh I didn't think of that. I wonder if there's some customization that can be done in the datamodel codegen. Also note that this is a breaking change in the API and shoudl be discussed with the UI

components/renku_data_services/session/api.spec.yaml

Panaetius · 2025-01-22T07:59:31Z

components/renku_data_services/session/api.spec.yaml

@@ -531,11 +641,105 @@ components:
      description: A description for the resource
      type: string
      maxLength: 500
+    ImageBuilder:


I wonder if we should call this something else, as this is not really a builder. We have a BuildStrategy which references a Builder in k8s, having Builder here again could cause confusion. It's more like a build strategy reference with parameters?

instead of ImageBuilder would something like ImageBuildParametersdo the trick ?

Renamed to BuildParameter for now.

components/renku_data_services/session/api.spec.yaml

leafty

Some comments about the API spec:

components/renku_data_services/session/api.spec.yaml

leafty · 2025-01-22T12:46:25Z

components/renku_data_services/session/api.spec.yaml

          $ref: "#/components/schemas/ContainerImage"
+        builder_id:
+          $ref: "#/components/schemas/ULID"


The indirection adds complexity for clients. I do not think that we have an expectation of the relation going from {0,1}:1 to 1:N at any time. A "builder" belongs to a single session launcher.

components/renku_data_services/session/api.spec.yaml

leafty · 2025-01-22T12:50:19Z

components/renku_data_services/session/api.spec.yaml

+        - global
+        - custom
+        - builder


Changing the casing creates an API breaking change. Do we want this?

See comments above, currently our API is inconsistent in terms of casing (most enums are lower case, this file is one of the few places where it's upper case). This isn't really related to this PR, my thinking was just, if we change the API here anyways, might be a good point to make it consistent

I undid this change since it requires a database migration as well. I'll re-introduce it later once working on the db.

components/renku_data_services/session/api.spec.yaml

leafty · 2025-01-30T08:29:03Z

@m-alisafaee Can you fix the commit list? This PR is against kpack-resources, so either rebase from there or update the kpack-resources branch first. The changeset is not readable at the moment.

leafty

The type union on EnvironmentPostInLauncher is nice 👍

I think the same should be done to Environment and EnvironmentPatchInLauncher because non-sensical values would be allowed.

components/renku_data_services/session/api.spec.yaml

mohammad-alisafaee · 2025-02-03T09:07:30Z

EnvironmentPatchInLauncher

I did the same for EnvironmentPatchInLauncher but not for Environment, since we always return global envs from that endpoint and they don't support build option yet. Instead, I've applied it to EnvironmentGetInLauncher.

leafty

There are two invalid schema errors (see below). There are more potential pitfalls (see below), but we can start from here.

components/renku_data_services/session/api.spec.yaml

leafty

👍 OK for api.spec.yaml 🙌

SalimKayal requested review from Panaetius, leafty and mohammad-alisafaee January 21, 2025 16:24

Panaetius reviewed Jan 22, 2025

View reviewed changes

leafty reviewed Jan 22, 2025

View reviewed changes

mohammad-alisafaee force-pushed the feat/image-builder-api branch from 35bcb23 to 36280a0 Compare January 30, 2025 01:54

mohammad-alisafaee changed the base branch from kpack-resources to main January 30, 2025 08:37

mohammad-alisafaee changed the base branch from main to kpack-resources January 30, 2025 08:55

Panaetius and others added 15 commits January 30, 2025 09:56

add k8s cache client for shipwright buildruns

e802870

fix(linting): import order

22749bc

feat(api): introduce endpoints for image builders

2ffa0ae

feat(models): add model for image builders

8d01b4c

fix(consistency): lowercase for StrEnums

8708ee5

fix(semantics): change EditorKind to FrontendKind

dada2a8

fix(apispec): assume container_image is deterministic for image builders

15d1041

fix(apispec): branch to revision in image builders

35705e4

fix(apispec): change required fields for image builders

651beed

Address (some) review comments

3e945bf

fix after rebase

9cd24a9

update

c96cfae

Change back EnvironmentKind enum to uppercase

739f3a9

Address comments

35e86de

Remove build_parameters endpoints

127e5f0

mohammad-alisafaee force-pushed the feat/image-builder-api branch from 36280a0 to 127e5f0 Compare January 30, 2025 08:57

Update API

ce620e0

leafty reviewed Jan 31, 2025

View reviewed changes

mohammad-alisafaee added 2 commits January 31, 2025 17:29

Remove Revision and SubDir for now

3f3adc9

Address review comments

c3f092e

Address comments

24e8145

mohammad-alisafaee force-pushed the feat/image-builder-api branch from e993bf5 to 24e8145 Compare February 3, 2025 09:10

leafty reviewed Feb 3, 2025

View reviewed changes

Address comments

05a69b1

mohammad-alisafaee force-pushed the feat/image-builder-api branch from b982af2 to 05a69b1 Compare February 3, 2025 10:09

leafty reviewed Feb 3, 2025

View reviewed changes

components/renku_data_services/session/api.spec.yaml Show resolved Hide resolved

Address comments

b5cbfc2

leafty reviewed Feb 3, 2025

View reviewed changes

Update API

622accd

mohammad-alisafaee force-pushed the feat/image-builder-api branch from bf24f13 to 622accd Compare February 7, 2025 13:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/image builder api #614

Feat/image builder api #614

SalimKayal commented Jan 21, 2025

Panaetius Jan 22, 2025

SalimKayal Jan 22, 2025

leafty Jan 22, 2025

SalimKayal Jan 23, 2025

mohammad-alisafaee Jan 30, 2025

Panaetius Jan 22, 2025

SalimKayal Jan 22, 2025

SalimKayal Jan 22, 2025

SalimKayal Jan 22, 2025

Panaetius Jan 22, 2025

Panaetius Jan 22, 2025

SalimKayal Jan 22, 2025

mohammad-alisafaee Jan 30, 2025

leafty left a comment

leafty Jan 22, 2025

leafty Jan 22, 2025

Panaetius Jan 22, 2025

mohammad-alisafaee Jan 30, 2025

leafty commented Jan 30, 2025

leafty left a comment

mohammad-alisafaee commented Feb 3, 2025

leafty left a comment

leafty left a comment

Feat/image builder api #614

Are you sure you want to change the base?

Feat/image builder api #614

Conversation

SalimKayal commented Jan 21, 2025

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

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

leafty 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

leafty commented Jan 30, 2025

leafty left a comment

Choose a reason for hiding this comment

mohammad-alisafaee commented Feb 3, 2025

leafty left a comment

Choose a reason for hiding this comment

leafty left a comment

Choose a reason for hiding this comment