Repository navigation
feat(helm): Add workflow for publishing Helm charts to GitHub Pages. #1891
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,115 @@ | ||
| name: "clp-package-helm" | ||
|
|
||
| on: | ||
| pull_request: | ||
| paths: | ||
| - ".github/workflows/clp-package-helm.yaml" | ||
| - "taskfiles/helm.yaml" | ||
| - "tools/deployment/package-helm/**" | ||
| push: | ||
| paths: | ||
| - ".github/workflows/clp-package-helm.yaml" | ||
| - "taskfiles/helm.yaml" | ||
| - "tools/deployment/package-helm/**" | ||
| workflow_dispatch: | ||
|
|
||
| permissions: {} | ||
|
|
||
| concurrency: | ||
| group: "${{github.workflow}}-${{github.ref}}" | ||
|
|
||
| # Cancel in-progress jobs for efficiency | ||
| cancel-in-progress: true | ||
|
Comment on lines
+18
to
+22
Contributor
There was a problem hiding this comment. 🧩 Analysis chain🏁 Script executed: cat -n .github/workflows/clp-package-helm.yamlRepository: y-scope/clp Length of output: 4480 🌐 Web query:
💡 Result: In GitHub Actions, only runs that resolve to the exact same What
|
||
|
|
||
| jobs: | ||
| lint: | ||
| runs-on: "ubuntu-24.04" | ||
| steps: | ||
| - uses: "actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8" # v6.0.1 | ||
| with: | ||
| submodules: "recursive" | ||
|
|
||
| # Fetch all history for all branches; otherwise, `helm lint` would complain about not | ||
| # finding the `origin/main` branch. | ||
| fetch-depth: 0 | ||
|
|
||
| - name: "Install task" | ||
| shell: "bash" | ||
| run: "npm install -g @go-task/cli@3.44.0" | ||
|
|
||
| - name: "Lint Helm charts" | ||
| shell: "bash" | ||
| run: "task lint:check-helm" | ||
|
|
||
| publish: | ||
| # Publish from `main` and `vA.B.C` release branches. | ||
| if: >- | ||
| github.event_name != 'pull_request' | ||
| && (github.ref == 'refs/heads/main' || startsWith(github.ref, 'refs/heads/v')) | ||
| needs: "lint" | ||
| runs-on: "ubuntu-24.04" | ||
| permissions: | ||
| # To push to the `gh-pages` branch. | ||
| contents: "write" | ||
| steps: | ||
| - uses: "actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8" # v6.0.1 | ||
| with: | ||
| submodules: "recursive" | ||
|
|
||
| - name: "Install task" | ||
| shell: "bash" | ||
| run: "npm install -g @go-task/cli@3.44.0" | ||
|
|
||
| - name: "Package Helm chart" | ||
| shell: "bash" | ||
| run: |- | ||
| version=$( | ||
| grep '^version:' "tools/deployment/package-helm/Chart.yaml" \ | ||
| | sed 's/version: *"\(.*\)"/\1/' | ||
| ) | ||
|
Member
There was a problem hiding this comment. How about doing this with
Member
Author
There was a problem hiding this comment. i thought about it but it's not available by default on GH action runners. that said, i did find this action provided by the same maintainer: https://github.com/marketplace/actions/yq-portable-yaml-processor . alternatively, i suppose we can use just wanna make sure whether we are fine with the extra dependency
Member
There was a problem hiding this comment. Yeah, I'm fine with the extra dependency. It's just in the workflow anyway? |
||
| echo "CHART_VERSION=$version" >> "$GITHUB_ENV" | ||
|
coderabbitai[bot] marked this conversation as resolved.
Resolved
|
||
|
|
||
| task helm:package | ||
|
|
||
| - name: "Checkout branch `gh-pages`" | ||
| uses: "actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8" # v6.0.1 | ||
| with: | ||
| path: "gh-pages" | ||
| ref: "gh-pages" | ||
|
|
||
| - name: "Update Helm repository" | ||
| id: "update-helm-repo" | ||
| shell: "bash" | ||
| run: |- | ||
| chart_tgz="clp-${{env.CHART_VERSION}}.tgz" | ||
|
|
||
| # Skip if this chart version already exists. | ||
| if [[ -f "gh-pages/${chart_tgz}" ]]; then | ||
| echo "Chart ${chart_tgz} already exists, skipping publish." | ||
| echo "skip_publish=true" >> "$GITHUB_OUTPUT" | ||
| exit 0 | ||
| fi | ||
|
|
||
| . "build/toolchains/helm/env" | ||
| cp "build/clp-package-helm"/*.tgz "gh-pages/" | ||
|
|
||
| # Update index.yaml, merging with existing index if present. | ||
|
junhaoliao marked this conversation as resolved.
Resolved
|
||
| url="${{github.server_url}}/${{github.repository}}/raw/gh-pages" | ||
| if [[ -f "gh-pages/index.yaml" ]]; then | ||
| helm repo index "gh-pages" --merge "gh-pages/index.yaml" --url "${url}" | ||
| else | ||
| helm repo index "gh-pages" --url "${url}" | ||
| fi | ||
|
|
||
| - name: "Push to gh-pages branch" | ||
| if: "steps.update-helm-repo.outputs.skip_publish != 'true'" | ||
| shell: "bash" | ||
| working-directory: "gh-pages" | ||
| run: |- | ||
| git config user.name "$GITHUB_ACTOR" | ||
| git config user.email "$GITHUB_ACTOR@users.noreply.github.com" | ||
| git add "*.tgz" "index.yaml" | ||
| commit_message="ci(helm): Publish clp-${{env.CHART_VERSION}}" | ||
| commit_message+=" from ${{github.ref_name}} (${{github.sha}})." | ||
| git commit -m "$commit_message" | ||
| git push | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,9 @@ | |
| """Sphinx configuration file for CLP documentation.""" | ||
|
|
||
| from datetime import datetime, timezone | ||
| from pathlib import Path | ||
|
|
||
| import yaml | ||
| from sphinx.application import Sphinx | ||
|
|
||
| # Constants | ||
|
|
@@ -120,6 +122,24 @@ def setup(app: Sphinx) -> None: | |
| app.connect("source-read", _replace_variable_placeholders) | ||
|
|
||
|
|
||
| def _get_helm_version_flag() -> str: | ||
| """ | ||
| Reads the Helm chart version from Chart.yaml and returns the appropriate version flag. | ||
|
|
||
| :return: "--version <version>" for stable releases, or "--devel" for dev versions. | ||
| """ | ||
|
coderabbitai[bot] marked this conversation as resolved.
Resolved
|
||
| chart_yaml_path = ( | ||
| Path(__file__).parent.parent.parent / "tools/deployment/package-helm/Chart.yaml" | ||
|
junhaoliao marked this conversation as resolved.
Resolved
|
||
| ) | ||
| chart = yaml.safe_load(chart_yaml_path.read_text()) | ||
|
|
||
| version = chart.get("version") | ||
| if version is not None and "-dev" not in version: | ||
| return f"--version {version}" | ||
|
|
||
| return "--devel" | ||
|
Comment on lines
+131
to
+140
Contributor
There was a problem hiding this comment. 🧩 Analysis chain🏁 Script executed: # First, check if Chart.yaml exists at the specified path
find . -name "Chart.yaml" -type fRepository: y-scope/clp Length of output: 166 🏁 Script executed: # Check the current state of the _get_helm_version_flag function and its context
cat -n docs/conf/conf.py | sed -n '120,150p'Repository: y-scope/clp Length of output: 1224 🏁 Script executed: # Look for error handling patterns and how yaml is imported/used
rg "yaml\." docs/conf/conf.py -A 2 -B 2Repository: y-scope/clp Length of output: 241 🏁 Script executed: # Find where _get_helm_version_flag is called
rg "_get_helm_version_flag" docs/conf/conf.py -B 2 -A 2Repository: y-scope/clp Length of output: 399 🏁 Script executed: # Verify Chart.yaml exists at the expected location
find . -path "*/tools/deployment/package-helm/Chart.yaml" -type f -exec head -5 {} \;Repository: y-scope/clp Length of output: 211 🏁 Script executed: # Check for any yaml imports and their usage patterns
rg "import yaml" docs/conf/conf.pyRepository: y-scope/clp Length of output: 66 Add error handling to prevent doc build failures when Chart.yaml is missing or invalid. The function is called during Sphinx setup to populate documentation variables. If Additionally, the docstring states the function returns an empty string 🛠️ Proposed fix- chart = yaml.safe_load(chart_yaml_path.read_text())
-
- version = chart.get("version")
+ try:
+ chart = yaml.safe_load(chart_yaml_path.read_text()) or {}
+ version = chart.get("version")
+ except (FileNotFoundError, yaml.YAMLError, AttributeError):
+ return "--devel"🤖 Prompt for AI Agents
Member
Author
There was a problem hiding this comment. The
Contributor
There was a problem hiding this comment.
One minor note: the docstring currently states the function returns :return: "--version <version>" for stable releases, or "--devel" for dev versions.This is just a documentation accuracy fix and doesn't affect the fail-fast behavior. ✏️ Learnings added
🧠 Learnings used |
||
|
|
||
|
|
||
| def _replace_variable_placeholders(_app: Sphinx, _docname: str, content: list[str]) -> None: | ||
| """ | ||
| Replaces each variable placeholder in the docs with the relevant value. | ||
|
|
@@ -130,6 +150,7 @@ def _replace_variable_placeholders(_app: Sphinx, _docname: str, content: list[st | |
| """ | ||
| placeholder_to_value = { | ||
| "DOCS_VAR_CLP_GIT_REF": CLP_GIT_REF, | ||
| "DOCS_VAR_HELM_VERSION_FLAG": _get_helm_version_flag(), | ||
| } | ||
| for placeholder, value in placeholder_to_value.items(): | ||
| content[0] = content[0].replace(placeholder, value) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -113,13 +113,14 @@ Once your cluster is ready, you can install CLP using the Helm chart. | |
|
|
||
| ### Getting the chart | ||
|
|
||
| The CLP Helm chart is published to a [Helm repository][clp-helm-repo] hosted on GitHub Pages. | ||
|
|
||
| ```bash | ||
| # If you haven't already, add the CLP Helm repository. | ||
|
Member
There was a problem hiding this comment. What happens if you had already added the repo?
Member
Author
There was a problem hiding this comment. harmless unless the user have added another Helm repo also with name "clp" locally:
Member
Author
There was a problem hiding this comment. actually, their prompts are quite intuitive:
Member
Author
There was a problem hiding this comment. i guess users may blindly run the command then. i can see the docstring may confuse / cause mental barrier for them to the run the command, though i can't think of better wordings. do you have any suggestions?
Member
There was a problem hiding this comment. Yeah, I feel like we can simply remove the comment? |
||
| helm repo add clp https://y-scope.github.io/clp | ||
|
|
||
| # Update the repository to get the latest chart versions. | ||
| helm repo update clp | ||
| ``` | ||
|
|
||
| #### Production cluster requirements (optional) | ||
|
|
@@ -204,7 +205,7 @@ export CLP_COMPRESSION_WORKER_REPLICAS=1 | |
| export CLP_QUERY_WORKER_REPLICAS=1 | ||
| export CLP_REDUCER_REPLICAS=1 | ||
|
|
||
| helm install clp clp/clp DOCS_VAR_HELM_VERSION_FLAG \ | ||
| --set clpConfig.data_directory="$CLP_HOME/var/data" \ | ||
| --set clpConfig.logs_directory="$CLP_HOME/var/log" \ | ||
| --set clpConfig.tmp_directory="$CLP_HOME/var/tmp" \ | ||
|
|
@@ -225,7 +226,7 @@ For multi-node clusters with shared storage mounted on all nodes (e.g., NFS/Ceph | |
| `/etc/fstab`), enable distributed storage mode and configure multiple worker replicas: | ||
|
|
||
| ```bash | ||
| helm install clp clp/clp DOCS_VAR_HELM_VERSION_FLAG \ | ||
| --set distributedDeployment=true \ | ||
| --set compressionWorker.replicas=3 \ | ||
| --set queryWorker.replicas=3 \ | ||
|
|
@@ -288,7 +289,7 @@ credentials: | |
| Install with custom values: | ||
|
|
||
| ```bash | ||
| helm install clp clp/clp DOCS_VAR_HELM_VERSION_FLAG -f custom-values.yaml | ||
| ``` | ||
|
|
||
| ::::{tip} | ||
|
|
@@ -324,6 +325,8 @@ To run compression workers, query workers, and reducers in separate node pools: | |
| ```{code-block} yaml | ||
| :caption: dedicated-scheduling.yaml | ||
|
|
||
| distributedDeployment: true | ||
|
Member
There was a problem hiding this comment. Hm, this seems unrelated to the PR?
Member
Author
There was a problem hiding this comment. right, i was trying to fix an length violation on this line, then i found the unnecessary cmdline override, so i just fixed it. anyways i agree it's a separate issue and should be fixed in another PR. @coderabbitai so you may see that i moved the distributedDeployment setting from the command-line into the
Contributor
There was a problem hiding this comment. 🧩 Analysis chain
🏁 Script executed: Length of output: 42
The issue documents the goal of moving 🧠 Learnings used |
||
|
|
||
| compressionWorker: | ||
| replicas: 2 | ||
| scheduling: | ||
|
|
@@ -346,7 +349,8 @@ To run compression workers, query workers, and reducers in separate node pools: | |
| 3. Install: | ||
|
|
||
| ```bash | ||
| helm install clp clp/clp DOCS_VAR_HELM_VERSION_FLAG \ | ||
| -f dedicated-scheduling.yaml | ||
|
|
||
| ``` | ||
|
|
||
| #### Shared node pool | ||
|
|
@@ -363,6 +367,8 @@ To run all worker types in the same node pool: | |
|
|
||
| ```{code-block} yaml | ||
| :caption: shared-scheduling.yaml | ||
|
|
||
| distributedDeployment: true | ||
|
|
||
|
|
||
| compressionWorker: | ||
| replicas: 2 | ||
|
|
@@ -393,7 +399,7 @@ To run all worker types in the same node pool: | |
| 3. Install: | ||
|
|
||
| ```bash | ||
| helm install clp clp/clp DOCS_VAR_HELM_VERSION_FLAG -f shared-scheduling.yaml | ||
| ``` | ||
|
|
||
| --- | ||
|
|
@@ -520,7 +526,11 @@ kubectl exec -it <pod-name> -- /bin/bash | |
| To debug Helm chart issues: | ||
|
|
||
| ```bash | ||
| # For debugging the published chart from the repository | ||
| helm install clp clp/clp DOCS_VAR_HELM_VERSION_FLAG --dry-run --debug | ||
|
|
||
| # For debugging local chart changes during development | ||
| helm install clp /path/to/local/chart --dry-run --debug | ||
| ``` | ||
|
|
||
| --- | ||
|
|
@@ -586,7 +596,7 @@ To tear down a `kubeadm` cluster: | |
| [aks]: https://azure.microsoft.com/en-us/products/kubernetes-service | ||
| [api-server]: guides-using-the-api-server.md | ||
| [Cilium]: https://cilium.io/ | ||
| [clp-helm-repo]: https://y-scope.github.io/clp | ||
| [clp-releases]: https://github.com/y-scope/clp/releases | ||
| [design-orchestration]: ../dev-docs/design-deployment-orchestration.md | ||
| [docker-compose-deployment]: guides-docker-compose-deployment.md | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| version: "3" | ||
|
|
||
| includes: | ||
| toolchains: "toolchains.yaml" | ||
|
|
||
| vars: | ||
| G_PACKAGE_HELM_BUILD_DIR: "{{.G_BUILD_DIR}}/clp-package-helm" | ||
|
|
||
| tasks: | ||
| package: | ||
| vars: | ||
| OUTPUT_DIR: "{{.G_PACKAGE_HELM_BUILD_DIR}}" | ||
| deps: ["toolchains:helm"] | ||
| cmds: | ||
| - "rm -rf '{{.OUTPUT_DIR}}'" | ||
| - "mkdir -p '{{.OUTPUT_DIR}}'" | ||
| - |- | ||
| . "{{.G_HELM_TOOLCHAIN_ENV_FILE}}" | ||
| helm package "{{.ROOT_DIR}}/tools/deployment/package-helm" --destination "{{.OUTPUT_DIR}}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
Consider using YAML anchor for duplicate paths.
The paths array is identical for both
pull_requestandpushtriggers. Per repository conventions, YAML anchors are preferred to avoid duplication.♻️ Suggested refactor
Based on learnings, YAML anchors/aliases are acceptable and preferred in this repository's workflows.
📝 Committable suggestion
🤖 Prompt for AI Agents