Skip to content

Minify Helm chart values JSON schema#595

Merged
soloio-bulldozer[bot] merged 9 commits intomainfrom
marcogschmidt/20411-minify-json-schema
Apr 18, 2025
Merged

Minify Helm chart values JSON schema#595
soloio-bulldozer[bot] merged 9 commits intomainfrom
marcogschmidt/20411-minify-json-schema

Conversation

@marcogschmidt
Copy link
Copy Markdown
Contributor

@marcogschmidt marcogschmidt commented Apr 15, 2025

Motivation

With this change, the values.schema.json file generated as part of the Helm chart generation is no longer prettified. This drastically reduces the size of the file and minimizes the changes of the new maximum file size limit introduced by Helm in v3.17.3. There are no functional changes. Human users can still prettify the file using external tools (e.g. jq) if necessary.

Testing

You can test that this fixes the GME issue by following the instructions on https://github.com/solo-io/gloo-mesh-enterprise/pull/20443.

@solo-changelog-bot
Copy link
Copy Markdown

Issues linked to changelog:
https://github.com/solo-io/gloo-mesh-enterprise/issues/20411

Comment thread codegen/render/funcs.go
}

return indentJson(string(jsonSchema))
return string(jsonSchema)
Copy link
Copy Markdown
Contributor Author

@marcogschmidt marcogschmidt Apr 15, 2025

Choose a reason for hiding this comment

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

This is the only actual change.

jenshu
jenshu previously approved these changes Apr 16, 2025
@jenshu
Copy link
Copy Markdown
Contributor

jenshu commented Apr 16, 2025

looks like some tests are failing

@birkland
Copy link
Copy Markdown
Contributor

Would it be possible to bump helm in CI to v3.17.3? That would difinitively prove GME can be installed with helm v3.17.3 with the minified json.

Also, while you're at it, could you bump the helm dependency in go.mod to helm.sh/helm/v3 v3.17.3? It's on v3.17.0, and all versions less than v3.17.3 will register as a CVE

}

// minifyJSON is a helper function for tests to remove whitespace from JSON strings
func minifyJSON(jsonStr string) (string, error) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

json.Compact might work here https://pkg.go.dev/encoding/json#Compact

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.

Thanks for the pointer, I wasn't aware of that function! Just tested and it works like a charm 🎉

Copy link
Copy Markdown
Member

@Sodman Sodman left a comment

Choose a reason for hiding this comment

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

LGTM although I agree with Aaron's feedback that we should bump ourselves out of CVEs with this change too. Also agree with Jenny that if there's a stdlib way to do the same thing, I'd prefer that over a DIY solution.

@marcogschmidt
Copy link
Copy Markdown
Contributor Author

marcogschmidt commented Apr 17, 2025

Would it be possible to bump helm in CI to v3.17.3? That would difinitively prove GME can be installed with helm v3.17.3 with the minified json.

Also, while you're at it, could you bump the helm dependency in go.mod to helm.sh/helm/v3 v3.17.3? It's on v3.17.0, and all versions less than v3.17.3 will register as a CVE

@birkland These considerations totally make sense and I was planning on bumping the dependency, but we need to do that in the GME repo. We don't have any Go dependency on helm here. Did you perchance think this PR is in the GME repo?

@soloio-bulldozer soloio-bulldozer Bot merged commit ab8ad1c into main Apr 18, 2025
3 checks passed
@soloio-bulldozer soloio-bulldozer Bot deleted the marcogschmidt/20411-minify-json-schema branch April 18, 2025 13:10
jmcguire98 added a commit that referenced this pull request Apr 22, 2025
* Don't prettify chart values JSON schema

* Changelog

* Make linter happy

Remove unused functions
Add default switch clause

* Move changelog

* Codegen

* Update unit tests assertions

* Tests: use `json.Compact` to minify

* Un-focus test

* Remove unused function
# Conflicts:
#	codegen/test/chart/values.schema.json
soloio-bulldozer Bot pushed a commit that referenced this pull request Apr 22, 2025
….x (#596)

* Update SetClusterName and GetClusterName to use generateName instead of annotations (#593)

* add backwards compatibility in skv2

* add changelog

* update skv2 to not fallback to annotations

* try backwards compatibility fallback on gets, but not sets

* clean up skv2 unit tests

* fix changelog

* fix issuelink

* address nit

* move changelog

* move changelog into 0.36.7 since it was not cut yet

* Revert "move changelog into 0.36.7 since it was not cut yet"

This reverts commit 341c6c2.

* Minify Helm chart values JSON schema (#595)

* Don't prettify chart values JSON schema

* Changelog

* Make linter happy

Remove unused functions
Add default switch clause

* Move changelog

* Codegen

* Update unit tests assertions

* Tests: use `json.Compact` to minify

* Un-focus test

* Remove unused function
# Conflicts:
#	codegen/test/chart/values.schema.json

* move minify helm changelog

* merge changelog files

* fix codegen
marcogschmidt added a commit that referenced this pull request Apr 23, 2025
* Don't prettify chart values JSON schema

* Changelog

* Make linter happy

Remove unused functions
Add default switch clause

* Move changelog

* Codegen

* Update unit tests assertions

* Tests: use `json.Compact` to minify

* Un-focus test

* Remove unused function

(cherry picked from commit ab8ad1c)
soloio-bulldozer Bot pushed a commit that referenced this pull request Apr 23, 2025
* Minify Helm chart values JSON schema (#595)

* Don't prettify chart values JSON schema

* Changelog

* Make linter happy

Remove unused functions
Add default switch clause

* Move changelog

* Codegen

* Update unit tests assertions

* Tests: use `json.Compact` to minify

* Un-focus test

* Remove unused function

(cherry picked from commit ab8ad1c)

* Move changelog
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.

4 participants