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

Add JSON schemas for values.yaml #95

Merged
merged 34 commits into from
Sep 12, 2023
Merged

Add JSON schemas for values.yaml #95

merged 34 commits into from
Sep 12, 2023

Conversation

iblutrifork
Copy link
Contributor

@iblutrifork iblutrifork commented Aug 22, 2023

For now, I've added only a JSON chart for the cheetah-application/values.yaml
The plan is to add JSON charts for the other values.yaml files as well

Implementing https://jira.trifork.com/browse/KAMDP-341

The current json schema fails only for 1 helm release - that of platypus hello world:
https://github.com/trifork/cheetah-example-platypus-team/blob/a68182349c985ccd44fb680bc7c6d15bd5d6ec8a/systems/hello/base/hello-world.yaml#L20C1-L20C1
The tag here is a number, when usually it's a string

Here is a script to verify the charts in k8 (it assumes there are <20 releases that use the cheetah-application chart):

kubectl get helmreleases.helm.toolkit.fluxcd.io --all-namespaces -o yaml > releases.tmp
yq '.items | map(select(.spec.chart.spec.chart == "cheetah-application")) | .[].spec.values | split_doc' releases.tmp > filtered.tmp
for i in `seq 0 20` ; do yq "select(di == ${i})" filtered.tmp | yq 'load("values.yaml") *= .' > "${i}.tmp" ; done
for i in `seq 0 20` ; do if [ `cat "${i}.tmp" | wc -l` -gt 1 ] ; then helm lint . --values "${i}.tmp" ; fi ; done

for i in `seq 0 20` ; do rm "${i}.tmp" ; done
rm filtered.tmp
rm releases.tmp

Using this tool for automatic schema generation: https://jsonformatter.org/yaml-to-jsonschema

@iblutrifork iblutrifork requested a review from a team as a code owner August 22, 2023 08:32
@cthtrifork
Copy link
Contributor

Is there any way to avoid drift? Some CI?

@AndersBennedsgaard
Copy link
Contributor

Is there any way to avoid drift? Some CI?

@cthtrifork what drift?

@cthtrifork
Copy link
Contributor

Is there any way to avoid drift? Some CI?

@cthtrifork what drift?

When we make changes to the helm chart - how do we ensure the schema is up-to-date

@AndersBennedsgaard
Copy link
Contributor

I would guess that the ct lint command here verifies that the values.yaml conforms to the values.schema.json - and potentially all test-values in ci/*-values.yaml - but we should verify this

@iblutrifork
Copy link
Contributor Author

iblutrifork commented Aug 24, 2023

I would guess that the ct lint command here verifies that the values.yaml conforms to the values.schema.json - and potentially all test-values in ci/*-values.yaml - but we should verify this

It does indeed lint the chart and fail the CI upon any discrepancies, e.g. https://github.com/trifork/cheetah-charts/actions/runs/5962171340/job/16172838585.
This applies for the files in the ci/ directory as well https://github.com/trifork/cheetah-charts/actions/runs/5962911160/job/16175034406?pr=95

Copy link
Contributor

@AndersBennedsgaard AndersBennedsgaard left a comment

Choose a reason for hiding this comment

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

I think that we need some more required values depending on **.enabled and **.create.
If they are true several keys under that object should probably also be required.
For example, if ingress.enabled=true, we should make ingress.tls, ingress.hosts, and ingress.paths required

charts/cheetah-application/values.schema.json Outdated Show resolved Hide resolved
charts/cheetah-application/values.schema.json Outdated Show resolved Hide resolved
charts/cheetah-application/values.schema.json Outdated Show resolved Hide resolved
charts/cheetah-application/values.schema.json Outdated Show resolved Hide resolved
charts/cheetah-application/values.schema.json Outdated Show resolved Hide resolved
charts/cheetah-application/values.schema.json Outdated Show resolved Hide resolved
charts/cheetah-application/values.schema.json Outdated Show resolved Hide resolved
charts/cheetah-application/values.schema.json Outdated Show resolved Hide resolved
charts/cheetah-application/values.schema.json Outdated Show resolved Hide resolved
charts/cheetah-application/values.schema.json Show resolved Hide resolved
iblutrifork and others added 9 commits August 30, 2023 13:03
Co-authored-by: Anders Bennedsgaard <abbennedsgaard@gmail.com>
Co-authored-by: Anders Bennedsgaard <abbennedsgaard@gmail.com>
Co-authored-by: Anders Bennedsgaard <abbennedsgaard@gmail.com>
Co-authored-by: Anders Bennedsgaard <abbennedsgaard@gmail.com>
Co-authored-by: Anders Bennedsgaard <abbennedsgaard@gmail.com>
Co-authored-by: Anders Bennedsgaard <abbennedsgaard@gmail.com>
Co-authored-by: Anders Bennedsgaard <abbennedsgaard@gmail.com>
Co-authored-by: Anders Bennedsgaard <abbennedsgaard@gmail.com>
@iblutrifork
Copy link
Contributor Author

I think that we need some more required values depending on **.enabled and **.create. If they are true several keys under that object should probably also be required. For example, if ingress.enabled=true, we should make ingress.tls, ingress.hosts, and ingress.paths required

I have added the changes. Can you double-check if the required fields make sense? Also, I have not added a required field for serviceAccount because based on the field descriptions, none of them should be required.

charts/cheetah-application/values.schema.json Outdated Show resolved Hide resolved
charts/cheetah-application/values.schema.json Show resolved Hide resolved
charts/cheetah-application/values.schema.json Outdated Show resolved Hide resolved
charts/cheetah-application/values.schema.json Outdated Show resolved Hide resolved
charts/cheetah-application/values.schema.json Outdated Show resolved Hide resolved
charts/cheetah-application/values.schema.json Outdated Show resolved Hide resolved
iblutrifork and others added 8 commits September 5, 2023 09:15
Co-authored-by: Anders Bennedsgaard <abbennedsgaard@gmail.com>
Co-authored-by: Anders Bennedsgaard <abbennedsgaard@gmail.com>
Co-authored-by: Anders Bennedsgaard <abbennedsgaard@gmail.com>
Co-authored-by: Anders Bennedsgaard <abbennedsgaard@gmail.com>
Co-authored-by: Anders Bennedsgaard <abbennedsgaard@gmail.com>
Copy link
Contributor

@AndersBennedsgaard AndersBennedsgaard left a comment

Choose a reason for hiding this comment

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

I think this is as ready as we can make it.

As the charts isn't very DRY with this (lot of duplication between values.yaml and this values.schema.json such as the description), we might want to create a task on how to reduce this duplication.
We could keep the values.yaml very minimal by only including defaults for the required fields, remove the description from the schema such that we only have it in values.yaml, move defaults to the schema and remove the values.yaml entirely, or something else entirely. 🤔

For now, let's merge it in and start testing the new feature 👍

@iblutrifork iblutrifork merged commit 1f249e6 into main Sep 12, 2023
4 checks passed
@iblutrifork iblutrifork deleted the json-schema branch September 12, 2023 07:35
This was referenced Sep 12, 2023
iblutrifork added a commit that referenced this pull request Sep 26, 2023
Add JSON schema for opensearch

Continuation of #95
Implementing https://jira.trifork.com/browse/KAMDP-341
iblutrifork added a commit that referenced this pull request Oct 17, 2023
Add JSON schema for flink-job

Continuation of #95
Implementing https://jira.trifork.com/browse/KAMDP-341
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants