Skip to content

feat(helm): AS-706 helm values schema#396

Closed
afoley587 wants to merge 16 commits into
release/v2.9.0from
afoley/as-706-helm-values-schema
Closed

feat(helm): AS-706 helm values schema#396
afoley587 wants to merge 16 commits into
release/v2.9.0from
afoley/as-706-helm-values-schema

Conversation

@afoley587

@afoley587 afoley587 commented Jun 12, 2025

Copy link
Copy Markdown
Contributor

Rationale

A helm values schema allows us to validate/verify a user's values.yaml file entries before installing. It allows us to then display helpful messages about type mismatches / expected values / etc.

This, hopefully, makes installations easier for customers and allows us to provide a smoother experience.

Changes

  1. Adds a new pre-commit hook for helm-schema

  2. Adds a new makefile target for installing helm plugins

  3. Updates pre-commit workflow to install helm-schema for the new pre-commit hook

  4. Creates the new values.schema.json file with the following settings:

    ---
    # https://github.com/losisin/helm-values-schema-json/tree/main
    
    # https://json-schema.org/draft/2020-12
    draft: 2020
    
    indent: 4
    
    output: helm/fiftyone-teams-app/values.schema.json
    
    # Allow additional properties such as env vars
    # and additional maps
    noAdditionalProperties: false
    
    useHelmDocs: true
    
    values:
      - helm/fiftyone-teams-app/values.yaml
  5. Also disables a new markdown lint finding:

    # MD059/descriptive-link-text Link text should be descriptive
    # This rule marks things like:
    # `Example nginx configurations can be found [here](../)`
    # as errors.
    # The context comes before the link so this seems
    # like a fair rule to disable.
    MD059: false

Checklist

  • This PR maintains parity between Docker Compose and Helm

Testing

Test 1: Inserting a map where an array is expected

Using this test, we should throw an error because topologySpreadConstraints should be a list

apiSettings:
  topologySpreadConstraints:
      maxSkew: 
        topologyKey: 
        whenUnsatisfiable: 
        labelSelector:
          matchLabels:
            app.kubernetes.io/name: teams-api-foo
            app.kubernetes.io/instance: dev-fiftyone-ai

Running the below shows it does!

 % helm install --dry-run dev-fiftyone-ai ./helm/fiftyone-teams-app --values ../cloud-build-and-deploy/helm-configs/dev-values.yaml --namespace dev-fiftyone-ai
coalesce.go:301: warning: destination for fiftyone-teams-app.apiSettings.topologySpreadConstraints is a table. Ignoring non-table value ([])
Error: INSTALLATION FAILED: values don't meet the specifications of the schema(s) in the following chart(s):
fiftyone-teams-app:
- apiSettings.topologySpreadConstraints: Invalid type. Expected: array, given: object
Test 2: Inserting a string where an int is expected

Using this test, we should throw an error because podDisruptionBudget.minAvailable" should be an int`

casSettings:
  podDisruptionBudget:
    enabled: true
    minAvailable: "1"

Running the below shows it does!

 % helm install --dry-run dev-fiftyone-ai ./helm/fiftyone-teams-app --values ../cloud-build-and-deploy/helm-configs/dev-values.yaml --namespace dev-fiftyone-ai
Error: INSTALLATION FAILED: values don't meet the specifications of the schema(s) in the following chart(s):
fiftyone-teams-app:
- casSettings.podDisruptionBudget.minAvailable: Invalid type. Expected: integer, given: string
Test 3: Making sure additional keys are allowed

Using this test, we should not throw an error

casSettings:
  env:
    CAS_DATABASE_NAME: dev-cas
    CAS_DEFAULT_USER_ROLE: ADMIN
    FIFTYONE_APP_BANNER_COLOR: "orange"
    FIFTYONE_APP_BANNER_TEXT_COLOR: "purple"
    FIFTYONE_APP_BANNER_TEXT: "You are currently viewing the Dev Deployment"
    THIS_IS_SPARTA: yay <---- additional key, not in schema

Running the below shows it does!

 % helm install --dry-run dev-fiftyone-ai ./helm/fiftyone-teams-app --values ../cloud-build-and-deploy/helm-configs/dev-values.yaml --namespace dev-fiftyone-ai

@afoley587 afoley587 requested a review from a team as a code owner June 12, 2025 20:10
kevin-dimichel
kevin-dimichel previously approved these changes Jun 12, 2025

@kevin-dimichel kevin-dimichel left a comment

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.

Nice!

findtopher
findtopher previously approved these changes Jun 12, 2025

@findtopher findtopher left a comment

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.

It'd be nice to see some documentation about how a customer might use this; right now it just looks like another thing that will frustrate me when I put a percentage in where an integer was tested....

@afoley587

Copy link
Copy Markdown
Contributor Author

It'd be nice to see some documentation about how a customer might use this; right now it just looks like another thing that will frustrate me when I put a percentage in where an integer was tested....

Customers won't use this directly. When they run a helm install -f some-values.yaml, helm will look at their settings in the some-values.yaml file and compare the types to the schema. If there is a type mismatch (i.e. they're using a string instead of an int), helm will tell them the error:

Error: INSTALLATION FAILED: values don't meet the specifications of the schema(s) in the following chart(s):
fiftyone-teams-app:
- casSettings.podDisruptionBudget.minAvailable: Invalid type. Expected: integer, given: string

But we can talk offline to discuss how to make sure no one has a negative experience!

@findtopher findtopher left a comment

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.

It's unfortunate that this can't be used ahead of time to validate the config file... oh well!

@afoley587

Copy link
Copy Markdown
Contributor Author

Closing for now. I am going to use a new tool which gives some more freedom over the union of types and will need to retest.

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