-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
[bitnami/apisix] Updated jsonschema to allow string values for fields passed to tpl #27441
[bitnami/apisix] Updated jsonschema to allow string values for fields passed to tpl #27441
Conversation
Signed-off-by: James Riley McHugh <[email protected]>
Signed-off-by: Bitnami Containers <[email protected]>
Signed-off-by: James Riley McHugh <[email protected]>
…ames-mchugh/bitnami-charts into fix-apisix-jsonschema-for-extraenvvars
…schema-for-extraenvvars # Conflicts: # bitnami/apisix/CHANGELOG.md
f8625b1
to
a4b15c0
Compare
Signed-off-by: James Riley McHugh <[email protected]>
a4b15c0
to
a5b52a7
Compare
Signed-off-by: Bitnami Containers <[email protected]>
@migruiz4 thank you for taking the review! Please let me know if any updates are needed or if you'd like me to scope down the changes. I'm hoping we can get this merged soon so my team doesn't have to continue to fork the chart. |
…schema-for-extraenvvars # Conflicts: # bitnami/apisix/CHANGELOG.md # bitnami/apisix/Chart.yaml
Signed-off-by: James Riley McHugh <[email protected]>
…schema-for-extraenvvars
Signed-off-by: James Riley McHugh <[email protected]>
4079f53
to
6e9cb59
Compare
Signed-off-by: Bitnami Containers <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @james-mchugh,
I'm sorry for the late response.
Changes look good to me, but I want to leave a note for any user who may have any questions related to this.
Although the chart value allows 'string' format, the kubernetes schema needs to be honored after rendering.
For example, kubernetes schema determines the format 'array' for the container 'command' key in its manifest. Therefore, at values.yaml
:
# This would be wrong
command: "tail -f {{ .Values.myPath }}"
# This is would be correct
command: |-
- tail
- -f
- {{ .Values.myPath }}
Although the value type is string
, when rendered it will become array
resulting in a correct Kubernetes manifest.
Thank you very much for your contribution.
… passed to tpl (bitnami#27441) * Updated jsonschema to allow string values for fields passed to tpl Signed-off-by: James Riley McHugh <[email protected]> * Update CHANGELOG.md Signed-off-by: Bitnami Containers <[email protected]> * Removed extra newlines added to type fields Signed-off-by: James Riley McHugh <[email protected]> * Bumped chart version Signed-off-by: James Riley McHugh <[email protected]> * Update CHANGELOG.md Signed-off-by: Bitnami Containers <[email protected]> * Bumped chart version Signed-off-by: James Riley McHugh <[email protected]> * Bumped chart version Signed-off-by: James Riley McHugh <[email protected]> * Update CHANGELOG.md Signed-off-by: Bitnami Containers <[email protected]> --------- Signed-off-by: James Riley McHugh <[email protected]> Signed-off-by: Bitnami Containers <[email protected]> Co-authored-by: James Riley McHugh <[email protected]> Co-authored-by: Bitnami Containers <[email protected]>
Description of the change
Updates all fields in values.schema.json that are directly passed to
tpl
to also allow for string fields.Benefits
Chart users can take full advantage of the
tpl
function being used to do advanced templating from custom values.yaml files. For example,Possible drawbacks
Making strings an allowed type for many of these fields increases the risk that a user may input an invalid value.
However, I've done a quick audit of the Bitnami charts and how they utilize the values.schema.json file. Of the 110 Bitnami charts, 23 use a values.schema.json file. Of those 23, only 5 (including Apisix) have over 400 lines. Most schemas used for Bitnami charts are pretty sparse, and it is not typical for all fields to be included with strict validation.
Applicable issues
Additional information
If we want to scope this PR down a bit, we can scope it to only include the
extraEnvVars
fields that were mentioned in the linked issue.Checklist
Chart.yaml
according to semver. This is not necessary when the changes only affect README.md files.README.md
using readme-generator-for-helm