Skip to content

refactor: #14 - improve image property #18

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.

Sign up for GitHub

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

Open
jeanbaptisterevel wants to merge 2 commits into
base: main
Choose a base branch
from
Open

refactor: #14 - improve image property #18

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1e575f7
docs(schema): improve json property descriptions
jeanbaptisterevel Jul 13, 2025
499d44a
refactor: improve image property
jeanbaptisterevel Jul 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions src/templates/_image.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
{{/*
Builds a full OCI image reference from the given image object.

Usage:
{{ include "papermc-server.image" (dict "image" .Values.container.image) }}

@Djaytan Djaytan Jul 15, 2025

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Too complicated usage.

The include must be as simple as {{ include "papermc-server.image" . }}.

@Djaytan Djaytan Jul 15, 2025

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

  • I realize that your implementation rely on the root context instead of a sub one.

{{ $image := .Values.container.image -}}

Definitively, you have something to do on this front for the reconciliation.

@jeanbaptisterevel jeanbaptisterevel Jul 16, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh yes you're right, my bad.
I was having fun using the tool to see what was possible before simplifying the template usage, and I forgot to update the documentation.
The correct usage is {{ include "papermc-server.image" . }}, this is what I used in the deployment manifest...


If digest is "none", returns: <registry>/<name>:<tag>
Otherwise returns: <registry>/<name>@<digest>

@Djaytan Djaytan Jul 15, 2025

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Tag is required even when specifying digest.
At least, it works when having both the tag and digest defined.

*/}}
{{ define "papermc-server.image" -}}
{{ $image := .Values.container.image -}}

@Djaytan Djaytan Jul 15, 2025

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Improve readability by leveraging indentations.

{{ $registry := $image.registry | default "docker.io" -}}

@Djaytan Djaytan Jul 15, 2025

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I feel like it would be a better to deal to directly specify the default values at values.yaml level.

This way, we separate the implementation from the default values.

{{ $name := $image.name | default "djaytan/papermc-server" -}}
{{ $tag := $image.tag | default "latest" -}}
{{ $digest := $image.digest | default "none" -}}

@Djaytan Djaytan Jul 15, 2025

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

That's weird to have as default none.

Having an undefined variable is fine and a good way to state a variable as "undefined".


{{ if ne $digest "none" -}}
{{ printf "%s/%s@%s" $registry $name $digest }}
{{ else -}}
{{ printf "%s/%s:%s" $registry $name $tag }}
{{ end -}}
{{ end }}
2 changes: 1 addition & 1 deletion src/templates/deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ spec:
runAsUser: 2
containers:
- name: {{ include "papermc-server.name" . }}
image: {{ .Values.container.image }}
image: {{ include "papermc-server.image" . }}
stdin: true
tty: true
securityContext:
Expand Down
78 changes: 60 additions & 18 deletions src/values.schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,44 +6,86 @@
"properties": {
"name": {
"type": "string",
"description": "Optional name override for the server. Defaults to the Helm release name if not set."
"description": "Custom name for the server instance. If not set, the Helm release name will be used by default."

@Djaytan Djaytan Jul 15, 2025

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Not "Custom name", "Name" is enough even the property can be customized.

@Djaytan Djaytan Jul 15, 2025

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Favor the default field of JSON Schema when possible.

},
"namespace": {
"type": "string",
"description": "Kubernetes namespace for the deployment."
"description": "The Kubernetes namespace where the server will be deployed."
},
"eula": {
"type": "boolean",
"description": "Must be set to true to accept Minecraft's EULA."
"description": "You must set this to true to indicate acceptance of Minecraft's End User License Agreement (EULA). The server won't start unless this is accepted."
},
"container": {
"type": "object",
"description": "Container configuration.",
"description": "Configuration related to the server's container, like image and resource settings.",
"properties": {
"image": {
"type": "string",
"description": "Container image to use for the server."
"type": "object",
"description": "Detailed definition of the OCI image to use for running the Minecraft server.",
"properties": {
"registry": {
"type": "string",
"description": "Hostname of the registry from which to pull the OCI image.",
"default": "docker.io"
},
"name": {
"type": "string",
"description": "Name of the image repository (e.g., 'djaytan/papermc-server').",
"default": "djaytan/papermc-server"
},
"tag": {
"type": "string",
"description": "Tag of the image to pull.",
"default": "latest"
},
"digest": {
"type": "string",
"description": "Digest (checksum) to ensure image integrity; use 'none' to disable.",
"default": "none"
}
}
},
"resources": {
"type": "object",
"description": "Resource requests and limits.",
"description": "Kubernetes resource constraints for the container, used to control CPU, memory, and ephemeral storage usage. See: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/#resource-units-in-kubernetes",
"properties": {
"requests": {
"type": "object",
"properties": {
"ephemeralStorage": { "type": "string" },
"cpu": { "type": "integer", "minimum": 1 },
"memory": { "type": "string" }
"ephemeralStorage": {
"type": "string",
"description": "Amount of ephemeral (temporary) storage requested (e.g., '10Gi'). See: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/#resource-units-in-kubernetes"
},
"cpu": {
"type": "integer",
"minimum": 1,
"description": "Minimum number of CPU units requested for the container."
},
"memory": {
"type": "string",
"description": "Minimum amount of memory requested (e.g., '8Gi'). See: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/#resource-units-in-kubernetes"
}
},
"required": ["ephemeralStorage", "cpu", "memory"]
},
"limits": {
"type": "object",
"required": ["ephemeralStorage", "cpu", "memory"],
"properties": {
"ephemeralStorage": { "type": "string" },
"cpu": { "type": "integer", "minimum": 1 },
"memory": { "type": "string" }
"ephemeralStorage": {
"type": "string",
"description": "Maximum amount of ephemeral (temporary) storage requested (e.g., '10Gi').See: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/#resource-units-in-kubernetes"
},
"cpu": {
"type": "integer",
"minimum": 1,
"description": "Maximum number of CPU units the container can use."
},
"memory": {
"type": "string",
"description": "Maximum amount of memory requested (e.g., '8Gi').See: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/#resource-units-in-kubernetes"
}
}
}
},
Expand All @@ -62,7 +104,7 @@
"type": "integer",
"minimum": 30000,
"maximum": 32767,
"description": "NodePort to expose the Minecraft server on."
"description": "NodePort used to expose the Minecraft server to external clients. Must be within the Kubernetes NodePort range (30000–32767)."
}
},
"required": ["nodePort"]
Expand All @@ -72,23 +114,23 @@
},
"healthcheck": {
"type": "object",
"description": "Health check configuration to restart the server if it becomes unhealthy.",
"description": "Settings for monitoring server health and restarting it if it becomes unresponsive.",
"properties": {
"checkInterval": {
"type": "integer",
"minimum": 1,
"description": "Time interval in seconds between consecutive health checks."
"description": "Interval in seconds between health checks. Shorter intervals detect problems faster but increase resource use."
},
"failureThreshold": {
"type": "object",
"properties": {
"startup": {
"type": "integer",
"description": "Threshold in seconds to declare the server unhealthy during startup."
"description": "Maximum number of seconds the server can remain unresponsive during startup before being considered unhealthy."
},
"liveness": {
"type": "integer",
"description": "Threshold in seconds to declare the server unhealthy during normal operation."
"description": "Maximum number of seconds the server can be unresponsive during normal operation before it is restarted."
}
},
"required": ["startup", "liveness"]
Expand Down
6 changes: 5 additions & 1 deletion src/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,11 @@ namespace: default
eula: false

container:
image: djaytan/papermc-server:dev
image:
registry: docker.io
name: djaytan/papermc-server
tag: latest
digest: none
resources:
requests:
ephemeralStorage: 10Gi
Expand Down