refactor: #14 - improve image property#18
Conversation
jeanbaptisterevel
changed the title
jeanbaptisterevel
force-pushed
the
feat/improve-image-property
branch
from
a4e909f to
499d44a
Compare
| Builds a full OCI image reference from the given image object. | ||
|
|
||
| Usage: | ||
| {{ include "papermc-server.image" (dict "image" .Values.container.image) }} |
There was a problem hiding this comment.
Too complicated usage.
The include must be as simple as {{ include "papermc-server.image" . }}.
There was a problem hiding this comment.
- 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.
There was a problem hiding this comment.
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...
| {{ include "papermc-server.image" (dict "image" .Values.container.image) }} | ||
|
|
||
| If digest is "none", returns: <registry>/<name>:<tag> | ||
| Otherwise returns: <registry>/<name>@<digest> |
There was a problem hiding this comment.
Tag is required even when specifying digest.
At least, it works when having both the tag and digest defined.
| Otherwise returns: <registry>/<name>@<digest> | ||
| */}} | ||
| {{ define "papermc-server.image" -}} | ||
| {{ $image := .Values.container.image -}} |
There was a problem hiding this comment.
Improve readability by leveraging indentations.
| */}} | ||
| {{ define "papermc-server.image" -}} | ||
| {{ $image := .Values.container.image -}} | ||
| {{ $registry := $image.registry | default "docker.io" -}} |
There was a problem hiding this comment.
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.
| {{ $registry := $image.registry | default "docker.io" -}} | ||
| {{ $name := $image.name | default "djaytan/papermc-server" -}} | ||
| {{ $tag := $image.tag | default "latest" -}} | ||
| {{ $digest := $image.digest | default "none" -}} |
There was a problem hiding this comment.
That's weird to have as default none.
Having an undefined variable is fine and a good way to state a variable as "undefined".
| "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." |
There was a problem hiding this comment.
Not "Custom name", "Name" is enough even the property can be customized.
| "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." |
There was a problem hiding this comment.
Favor the default field of JSON Schema when possible.
2 participants
Improves the image property in the values.yml file
Closes #14