Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce starter module blueprint #262

Merged
merged 4 commits into from
Dec 20, 2023
Merged

Introduce starter module blueprint #262

merged 4 commits into from
Dec 20, 2023

Conversation

stefanprodan
Copy link
Owner

@stefanprodan stefanprodan commented Dec 2, 2023

This PR introduces a blueprint for a minimal Timoni module with the goal of helping people gets started writing modules. Compared to the current module generated by timoni mod init, the starter blueprint contains just a Kubernetes Deployment and a Service. The cue.mod contains only the corev1 and appsv1 Kubernetes schemas. The #Config definition documentation tries to explain the magic behind Timoni autogenerated fields.

Ref: #251


// The resources allows setting the container resource requirements.
// By default, the container requests 10m CPU and 32Mi memory.
resources: timoniv1.#ResourceRequirements & {
Copy link
Contributor

@jmgilman jmgilman Dec 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment applies to lines 45 and 65 as well.

I've been going back and forth on where these types of things should be configured. In a lot of cases, you can make a very good guess about things like security contexts and resource constraints. Defaulting these reduces the cognitive load on a consumer to figure these out, so I think it's a good practice to implement.

However, should these be defined in config.cue or values.cue? The values.cue technically contains the "defaults" according to the documentation. So, as an end-user, I'm more likely to look there than in config.cue. As it stands, we're sort of intermixing defaults. The config.cue holds both the schema and some default configuration which then has the defaults of values.cue applied over it. This can be a bit jarring to someone reading the code.

Hopefully, that makes sense. All that to say, I'm leaning towards moving these to values.cue and keeping config.cue purely schema driven.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose this would also apply to things like the default service port as well. I guess the question becomes where do you add a default value: config.cue or values.cue?. If they are literally interchangeable (and perhaps there's a distinction I'm not understanding here), then it seems like without standardizing on one you're going to end up looking in two places to understand a module's default values.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The values.cue serves as a default file when no values files are supplied by the user, and the fields there get overwritten not unified. The config.cue is the place where CUE defaults can be set. The reason I've put the image concrete values in values.cue is for Dependabot or other automation to bump them without having to parse CUE, just JSON. I get that this is very confusing and I haven't got to the Dependabot part yet. I will change the blueprint to set the image defaults in config.cue.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm leaning towards moving these to values.cue and keeping config.cue purely schema driven.

I suggest you use a dedicated file for this, like defaults.cue and leave values.cue empty. As documented in the header, values.cue can't used for anything but concrete fields.

// Note that this file must have no imports and all values must be concrete.

Signed-off-by: Stefan Prodan <[email protected]>
@stefanprodan stefanprodan force-pushed the blueprint-minimal branch 3 times, most recently from 9e1bbf4 to 0a11dff Compare December 18, 2023 22:00
@stefanprodan stefanprodan changed the title Introduce minimal module blueprint Introduce starter module blueprint Dec 18, 2023
@stefanprodan stefanprodan marked this pull request as ready for review December 18, 2023 22:12
@stefanprodan stefanprodan added area/api API related issues and pull requests area/blueprints Module blueprints related issues and pull requests labels Dec 18, 2023
Signed-off-by: Stefan Prodan <[email protected]>
Signed-off-by: Stefan Prodan <[email protected]>
@stefanprodan stefanprodan merged commit f7c37ef into main Dec 20, 2023
4 checks passed
@stefanprodan stefanprodan deleted the blueprint-minimal branch December 20, 2023 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api API related issues and pull requests area/blueprints Module blueprints related issues and pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants