values schema #811
values schema #811
Conversation
|
@d7oc could you skim those changes to check if you agree with the principle of this change? Most interesting from my side:
If we can agree on the schema beeing a good thing, I'll gonna do some polishing on this PR, especially on the configuration documentation that received a small fallout. |
wkloucek
force-pushed
the
values-schema
branch
from
34c49fa to
06f3d2b
Compare
|
@d7oc I'd like to get this into the chart soonish or agree on a timeframe when this change does fits our risk analysis / schedule / ... This week I had another occurence where this schema feature would have saved a college and my like 2 hours each because we had fine looking configuration on a wrong indention level. |
| sha: | ||
| # @schema | ||
| # type: [string, null] | ||
| # required: true | ||
| # @schema |
There was a problem hiding this comment.
We should somehow limit the allowed strings here, as per Kubernetes API this can only be Always, Never or IfNotPresent.
| sha: | |
| # @schema | |
| # type: [string, null] | |
| # required: true | |
| # @schema | |
| sha: | |
| # @schema | |
| # enum: ["Always", "Never", "IfNotPresent", null] | |
| # required: true | |
| # @schema |
There was a problem hiding this comment.
adding null to to the enum field does actually not end up in the final schema.
This would be no blocker for .Values.image.pullPolicy, because it is never null.
But .Values.services.<service>.image.pullPolicy needs to be nullable to fallback to .Values.image.pullPolicy
With your changes, liniting fails:
DEBU[2024-12-09T08:01:38+01:00] Using template files [charts/ocis/docs/templates/values-desc-table.adoc.gotmpl] for chart charts/ocis
/home/wkloucek/go/bin/gomplate-v3.11.8 --file=charts/ocis/docs/templates/values.adoc.yaml.gotmpl --out=charts/ocis/docs/values.adoc.yaml
/home/wkloucek/go/bin/helm-v3.16.2 lint charts/ocis -f 'charts/ocis/ci/absolute-minimum-values.yaml'
==> Linting charts/ocis
[INFO] Chart.yaml: icon is recommended
[ERROR] values.yaml: - services.activitylog.image.pullPolicy: services.activitylog.image.pullPolicy must be one of the following: "Always", "Never", "IfNotPresent"
As said, this is because the schema doesn't inherit the null:
"pullPolicy": {
"default": "",
"description": "Image pull policy",
+ "enum": [
+ "Always",
+ "Never",
+ "IfNotPresent"
+ ],
"required": [],
- "title": "pullPolicy",
- "type": [
- "string",
- "null"
- ]
+ "title": "pullPolicy"
},
charts/ocis/values.yaml
Outdated
| type: RollingUpdate | ||
| rollingUpdate: | ||
| # @schema | ||
| # type: string |
There was a problem hiding this comment.
This is defined as type IntOrString in Kubernetes API. Should we still restrict this to string only?
There was a problem hiding this comment.
Good point!
was applied in f793e61
charts/ocis/values.yaml
Outdated
| # Value can be an absolute number (ex: 5) or a percentage of desired pods (ex: 10%). | ||
| maxSurge: 25% | ||
| # @schema | ||
| # type: string |
There was a problem hiding this comment.
Good point!
was applied in f793e61
| # @schema | ||
| # type: array | ||
| # items: | ||
| # type: string |
There was a problem hiding this comment.
Do these nodes always have an <host>:<portnummer> nomenclature? If so we could use pattern here. Also applies for all oder nodes secure further down below.
There was a problem hiding this comment.
I think the nats client library probably has a port default, so this might also be possible without specifying a port.
Also I don't know if a protocol prefix like tcp:// could be allowed.
I'd like to keep this vague as long as there is no strict specification from the oCIS product
| # -- Create demo users on the first startup. | ||
| # Not recommended for production installations. | ||
| demoUsers: false | ||
| # Language related settings | ||
| language: | ||
| # @schema | ||
| # type: string |
There was a problem hiding this comment.
Shouldn't we use an enum with available languages here?
There was a problem hiding this comment.
I'm unsure if a exhaustive list about available / supported languages exists.
Even if it exists, we'd need to adapt it with every oCIS release? I'd like to avoid that in the current situation.
| # -- Enables email notifications. | ||
| enabled: false | ||
| smtp: | ||
| # @schema | ||
| # type: [string, null] |
There was a problem hiding this comment.
We could use https://json-schema.org/understanding-json-schema/reference/string#hostnames here.
| # type: boolean | ||
| # required: true | ||
| # uri: | ||
| # type: string |
There was a problem hiding this comment.
| # type: string | ||
| # required: false | ||
| # iconURI: | ||
| # type: string |
There was a problem hiding this comment.
| # hosts: | ||
| # type: array | ||
| # items: | ||
| # type: string |
There was a problem hiding this comment.
We could use https://json-schema.org/understanding-json-schema/reference/string#hostnames here.
|
I'm missing the commend here to generate the schema from the |
Description
Maintain a schema file to find misconfigurations.
There were already some misconfigurations found in the deployment examples (see the diff).
Related Issue
Motivation and Context
How Has This Been Tested?
helm lint charts/ocis -f ~/Projects/gitlab.xxx/exxx/ocis-infra/common/ocis/values.yaml -f ~/Projects/gitlab.xxx/exxx/ocis-infra/prod/apps/ocis/values.yamlScreenshots (if appropriate):
Types of changes
Checklist:
make docs) and committed