Skip to content

[helm] Restructure helm chart of Fluss#2871

Open
nhuantho wants to merge 6 commits intoapache:mainfrom
nhuantho:helm/2865
Open

[helm] Restructure helm chart of Fluss#2871
nhuantho wants to merge 6 commits intoapache:mainfrom
nhuantho:helm/2865

Conversation

@nhuantho
Copy link
Contributor

Purpose

Linked issue: close #2865

Brief change log

  • Restructured values.yaml
  • Created values.schema.json: schema of values.yaml and standard to valid value.yaml.
  • Created values.schema.schema.json: schema of values.schema.json and standard to valid values.schema.json.
  • Created tools/ci/helm_ci/validate_helm.py to validate helm.
  • Add ci to validate helm.
  • Add ignore python in .gitignore

Tests

API and Format

Documentation

@nhuantho
Copy link
Contributor Author

@affo Can you review for me?

@nhuantho
Copy link
Contributor Author

@swuferhong can you review for me?

# Conflicts:
#	helm/values.yaml

# Conflicts:
#	helm/templates/sts-coordinator.yaml
#	helm/templates/sts-tablet.yaml
#	helm/values.yaml
Copy link
Contributor

@affo affo left a comment

Choose a reason for hiding this comment

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

I left some punctual comments 🤝

I think this PR tries to do 2 tasks:

  1. refactor values
  2. enforce schema on values.

I suggest to now let #2870 land before refactoring the values, and later address any remaining change that you want to address.

I think this PR should be only dedicated to task 2 🤝

About that, I don't exactly get why we have the Python script, as I think helm should directly pickup the values.schema.json for validation.

Maintaining the schema itself will be very hard, hence I suggest to lean for an helm plugin such as https://github.com/dadav/helm-schema for auto-generating the schema.
We can add a CI step such as:

- name: Check schema is up to date
  run: |
    helm schema --dry-run > /tmp/generated-schema.json
    diff helm/values.schema.json /tmp/generated-schema.json

to verify that the schema is up-to-date.

At this point, values should be turned into something like this for helm-schema to work:

# @schema
# type: integer
# minimum: 1
# @schema
# Number of coordinator replicas
coordinatorServer:
  replicas: 1

  # @schema
  # type: object
  # additionalProperties: false
  # @schema
  persistence:
    # @schema
    # type: boolean
    # @schema
    enabled: false

    # @schema
    # type: string
    # pattern: ^[0-9]+(Gi|Ti)$
    # @schema
    size: 8Gi

    # @schema
    # type: string
    # @schema
    storageClass: ""

For the x-docsSection property and to enforce all properties are active, we can just create a plain python script to validate they exist:

# Instead of a meta-schema, just validate the rules explicitly
for prop, schema in values_schema["properties"].items():
    assert "description" in schema, f"{prop} missing description"
    assert "default" in schema, f"{prop} missing default"
    assert "x-docsSection" in schema, f"{prop} missing x-docsSection"

No dependency, just validating the schema and easier to reason about than a schema validating a schema 🤝

In any case, I would bootstrap this without x-docsSection.

I would separate that into another PR to address automatic documentation generation 🤝

- name: Set up Python
uses: actions/setup-python@v5
with:
python-version: "3.11"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
python-version: "3.11"
python-version: "3.12"

Just better to point to that one if we don't break anything

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to unify the Python version with .github/workflows/unstable-test-reporter.yaml

python-version: "3.11"

- name: Install dependencies
run: pip install jsonschema==4.26.0 PyYAML==6.0.3
Copy link
Contributor

Choose a reason for hiding this comment

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

I would favor a requirements.txt under tools/ci/helm_ci.
It would make it easier to validate locally.

The command would become pip install -r tools/ci/helm_ci/requirements.txt

- name: Install dependencies
run: pip install jsonschema==4.26.0 PyYAML==6.0.3

- name: Run unstable test reporter
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- name: Run unstable test reporter
- name: Validate values

@@ -0,0 +1,89 @@
# Licensed to the Apache Software Foundation (ASF) under one
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this script ?

Isn't helm lint already picking values.schema.json for validation?
Is this only for the meta schema?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@affo, from my side, when everyone updated helm. Could they please explain why it needs to be updated and describe the changes in the schema?
For example:
image

@affo
Copy link
Contributor

affo commented Mar 16, 2026

@nhuantho by the way, thanks for the contribution and to light the spotlight on this defect 🤝

@affo
Copy link
Contributor

affo commented Mar 16, 2026

@nhuantho as a side note, one can already structure schema annotations by adding comments prefixed with -- to pair with helm-docs which is a tool we may add for generating the docs https://github.com/norwoodj/helm-docs

@nhuantho
Copy link
Contributor Author

@affo, I will answer some confusion:

  • I think it will confuse users who use Helm
# @schema
  # type: object
  # additionalProperties: false
  # @schema

Focus pr:

  • We have the first verified value change to avoid affecting the helm -> [Helm] Chart Component Configuration Isolation #2472
  • Suggest defining value with a standard (airflow).
  • Build a schema and describe all values of helm, and the first step for auto-generating helm docs.

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.

[Helm] Restructure helm chart of Fluss

2 participants