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

Invalid values.schema.json passes lint #12994

Open
michael-todorovic opened this issue Apr 30, 2024 · 4 comments
Open

Invalid values.schema.json passes lint #12994

michael-todorovic opened this issue Apr 30, 2024 · 4 comments
Labels
feature help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines.

Comments

@michael-todorovic
Copy link

Hello,
We have helm 3.14.4

version.BuildInfo{Version:"v3.14.4", GitCommit:"81c902a123462fd4052bc5e9aa9c513c4c8fc142", GitTreeState:"clean", GoVersion:"go1.21.9"}

We have built a values.schema.json which works very well, this is super useful! However, recently, we introduced a bug (double curly brace) in it which makes the json invalid. Helm is happy with it:

╰ helm lint -f values.yaml -f tests/values/linter_global.yaml -f tests/values/container.yaml -f tests/values/network_mounts.yaml
==> Linting .
[INFO] Chart.yaml: icon is recommended

1 chart(s) linted, 0 chart(s) failed

However, the json is invalid:

╰ jsonlint values.schema.json 
Error: Parse error on line 1609:
...   }        }    },    "if": {      
---------------------^
Expecting 'EOF', got ','

This makes helm lint/helm template misleading as they report valid values, whereas it's not.

When I fix the jsonschema, I get failures that were hidden by the wrong status before:

╰ helm lint -f values.yaml -f tests/values/linter_global.yaml -f tests/values/container.yaml -f tests/values/network_mounts.yaml 
==> Linting .
[INFO] Chart.yaml: icon is recommended
[ERROR] values.yaml: - networkMounts.patrick: Additional property bob is not allowed

[ERROR] templates/: values don't meet the specifications of the schema(s) in the following chart(s):
workload-server-chart:
- networkMounts.patrick: Additional property bob is not allowed


Error: 1 chart(s) linted, 1 chart(s) failed

Could you check please?

@banjoh
Copy link

banjoh commented Apr 30, 2024

Are you able to provide a minimal example we can use to reproduce? I using a default chart generated using helm create and then used helm-schema to generate the json schema. After injecting some extra curly braces in several places, helm lint fails cause the json schema is invalid

@michael-todorovic
Copy link
Author

Hello, indeed I forgot the example 🤦
I ran helm create foobar and then injected this values.schema.json:

{
    "type": "object",
    "required": [
        "image"
    ],
    "properties": {
        "image": {
            "type": "object",
            "required": [
                "repository",
                "pullPolicy"
            ],
            "properties": {
                "repository": {
                    "type": "string",
                    "pattern": "^([-_/.a-z0-9]+)$"
                },
                "pullPolicy": {
                    "type": "string",
                    "pattern": "^(Always|Never|IfNotPresent)$"
                }
            }}
        }
    }
}

You can see the double curly brace after pullPolicy, which makes the json invalid. However, helm lint is happy:

╰ helm lint . -f values.yaml
==> Linting .
[INFO] Chart.yaml: icon is recommended

1 chart(s) linted, 0 chart(s) failed

helm template likely uses the same code path so it's happy as well.

We found out we broke our json thanks to helm package:

╰ helm package .                         
Error: failed to save: Invalid JSON in values.schema.json

I think we can expect lint+template to fail like package does 😄 For example, our CI lints on dev branches but packages only on release/pre-release

Thanks for your help 🙏

@banjoh
Copy link

banjoh commented May 1, 2024

I managed to reproduce the error as well. This is a valid issue

@gjenkins8 gjenkins8 added feature help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. labels May 9, 2024
@gjenkins8
Copy link
Contributor

gjenkins8 commented May 9, 2024

I've marked this as a feature, with the presumption that helm lint doesn't currently validate the chart's values.schema.json validity today. And as such, that would be be a useful feature.

If that presumption is wrong (and helm is validating values.schema.json), but somehow is failing to report something invalid. Then this would be a bug.

Either way, PR is welcome please.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines.
Projects
None yet
Development

No branches or pull requests

3 participants