Skip to content

Conversation

@junhaoliao
Copy link
Member

@junhaoliao junhaoliao commented Jan 22, 2026

Description

This PR adds a GitHub Actions workflow to lint and publish Helm charts to GitHub Pages, removing
the need for users to manually download and extract the chart from the repository.

Changes

Workflow (clp-package-helm)

  • Lint job: Runs task lint:check-helm on all PRs and pushes that modify the workflow,
    taskfile, or Helm chart files.
  • Publish job: Packages and publishes the Helm chart to the gh-pages branch. Only runs on
    pushes to main and v* release branches (not on PRs).

The workflow:

  1. Extracts the chart version from Chart.yaml
  2. Packages the chart using helm package
  3. Copies the .tgz file to the gh-pages branch
  4. Updates index.yaml using helm repo index --merge
  5. Commits and pushes to gh-pages

Taskfile (taskfiles/helm.yaml)

Adds a new taskfile with a helm:package task that:

  • Installs the Helm toolchain
  • Packages the chart to build/clp-package-helm/

Documentation updates

Updates the Kubernetes deployment docs to use the Helm repository instead of cloning/downloading
the chart:

# Before: Download and extract from repository
mkdir -p "$HOME/clp-package-helm"
curl ... | tar ...
cd "$HOME/clp-package-helm"
helm install clp .

# After: Use Helm repository
helm repo add clp https://y-scope.github.io/clp
helm repo update clp
helm install clp clp/clp DOCS_VAR_HELM_VERSION_FLAG ...

The DOCS_VAR_HELM_VERSION_FLAG placeholder is replaced at docs build time:

  • For stable releases (e.g., v0.8.0): --version 0.1.2
  • For dev versions (e.g., main): --devel

Checklist

  • The PR satisfies the contribution guidelines.
  • This is a breaking change and that has been indicated in the PR title, OR this isn't a
    breaking change.
  • Necessary docs have been updated, OR no docs need to be updated.

Validation performed

1. Workflow execution on main branch

Pushed to junhao/main and verified the workflow completed successfully:

✓ main clp-package-helm · 21231291674
Triggered via workflow_dispatch less than a minute ago

JOBS
✓ lint in 20s (ID 61090116820)
  ✓ Set up job
  ✓ Run actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8
  ✓ Install task
  ✓ Lint Helm charts
  ✓ Post Run actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8
  ✓ Complete job
✓ publish in 12s (ID 61090149929)
  ✓ Set up job
  ✓ Run actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8
  ✓ Install task
  ✓ Package Helm chart
  ✓ Checkout branch `gh-pages`
  ✓ Update Helm repository
  ✓ Push to gh-pages branch
  ✓ Post Checkout branch `gh-pages`
  ✓ Post Run actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8
  ✓ Complete job

Verified the commit message on gh-pages:

ci(helm): Publish clp-0.1.2-dev.19 from main (e424eb1edae23a5ac6f5f97ee61266b4af9ae7e1).

2. Workflow execution on v0.8.0 release branch

Created junhao/v0.8.0 with the workflow changes (rebased on top of version bump commit) and
verified publication of stable release:

✓ v0.8.0 clp-package-helm · 21231184364
Triggered via push about 3 minutes ago

JOBS
✓ lint in 23s (ID 61089798018)
  ✓ Set up job
  ✓ Run actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8
  ✓ Install task
  ✓ Lint Helm charts
  ✓ Post Run actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8
  ✓ Complete job
✓ publish in 17s (ID 61089828820)
  ✓ Set up job
  ✓ Run actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8
  ✓ Install task
  ✓ Package Helm chart
  ✓ Checkout branch `gh-pages`
  ✓ Update Helm repository
  ✓ Push to gh-pages branch
  ✓ Post Checkout branch `gh-pages`
  ✓ Post Run actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8
  ✓ Complete job

Verified the commit message on gh-pages:

ci(helm): Publish clp-0.1.2 from v0.8.0 (578de39c6c648df5b89739d2d87699b1bb2c238e).

3. Helm repository functionality

Add repository

$ helm repo add junhao-clp https://junhaoliao.github.io/clp
"junhao-clp" has been added to your repositories

Update repository

$ helm repo update junhao-clp
Hang tight while we grab the latest from your chart repositories...
...Successfully got an update from the "junhao-clp" chart repository
Update Complete. ⎈Happy Helming!

Search for available versions

$ helm search repo junhao-clp --versions --devel
NAME          	CHART VERSION	APP VERSION	DESCRIPTION
junhao-clp/clp	0.1.2        	0.8.0      	A Helm chart for CLP's (Compressed Log Processo...
junhao-clp/clp	0.1.2-dev.19 	0.7.1-dev  	A Helm chart for CLP's (Compressed Log Processo...

Pull stable version from v0.8.0 release branch

$ helm pull junhao-clp/clp --version 0.1.2
$ ls -la clp-0.1.2.tgz
-rw-r--r-- 1 junhao junhao 19026 Jan 21 19:36 clp-0.1.2.tgz

$ helm show chart junhao-clp/clp --version 0.1.2
apiVersion: v2
appVersion: 0.8.0
description: A Helm chart for CLP's (Compressed Log Processor) package deployment
...
version: 0.1.2

Pull dev version from main branch

$ helm pull junhao-clp/clp --version 0.1.2-dev.19
$ ls -la clp-0.1.2-dev.19.tgz
-rw-r--r-- 1 junhao junhao 19027 Jan 21 19:38 clp-0.1.2-dev.19.tgz

$ helm show chart junhao-clp/clp --version 0.1.2-dev.19
apiVersion: v2
appVersion: 0.7.1-dev
description: A Helm chart for CLP's (Compressed Log Processor) package deployment
...
version: 0.1.2-dev.19

Template chart (dry-run)

$ helm template clp junhao-clp/clp --version 0.1.2 | head -20
---
# Source: clp/templates/job-watcher-serviceaccount.yaml
apiVersion: "v1"
kind: "ServiceAccount"
metadata:
  name: clp-job-watcher
  labels:
    helm.sh/chart: clp-0.1.2
    app.kubernetes.io/name: clp
    app.kubernetes.io/instance: clp
    app.kubernetes.io/version: "0.8.0"
    app.kubernetes.io/managed-by: Helm
...

4. Published index.yaml

Verified the gh-pages branch contains both dev and stable versions:

apiVersion: v1
entries:
  clp:
  - apiVersion: v2
    appVersion: 0.8.0
    version: 0.1.2
    urls:
    - https://github.com/junhaoliao/clp/raw/gh-pages/clp-0.1.2.tgz
  - apiVersion: v2
    appVersion: 0.7.1-dev
    version: 0.1.2-dev.19
    urls:
    - https://github.com/junhaoliao/clp/raw/gh-pages/clp-0.1.2-dev.19.tgz
generated: "2026-01-22T00:38:30.123456789Z"

The Helm repository now contains:

  • Stable releases from v* branches (e.g., 0.1.2 with appVersion: 0.8.0)
  • Dev releases from main branch (e.g., 0.1.2-dev.19 with appVersion: 0.7.1-dev)

Summary by CodeRabbit

  • New Features

    • Automated Helm packaging and publishing pipeline added with CI publishing controls and duplicate-version checks.
  • Documentation

    • Kubernetes deployment and quick-start guides now install from the Helm repository and use a dynamic version flag.
    • Docs build now supports YAML parsing to populate Helm version flags.
  • Chores

    • CI workflows updated: removed Helm lint step and adjusted checkout behavior; introduced task-based Helm packaging support.

✏️ Tip: You can customize this high-level summary in your review settings.

@junhaoliao junhaoliao requested a review from a team as a code owner January 22, 2026 00:51
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 22, 2026

Walkthrough

Adds a Helm packaging-and-publishing GitHub Actions workflow, removes an explicit full-fetch and a Helm lint step from an existing lint workflow, adds Taskfile Helm packaging tasks, updates docs to install from the published Helm repo, and exposes a Helm version flag to docs via Chart.yaml.

Changes

Cohort / File(s) Summary
CI/CD workflows
.github/workflows/clp-lint.yaml, .github/workflows/clp-package-helm.yaml
Removed fetch-depth: 0 and the "Lint Helm charts" step from the existing lint workflow; added clp-package-helm.yaml workflow with lint and publish jobs that run task-based Helm checks, package charts, derive chart version from tools/.../Chart.yaml, update/merge index.yaml, and push packaged charts to gh-pages (skips publish if version already exists).
Task configuration
taskfile.yaml, taskfiles/helm.yaml
Added helm include in taskfile.yaml and new package task in taskfiles/helm.yaml that prepares a Helm build directory, sources the Helm toolchain env, and runs helm package into the output directory.
Docs build / helper
docs/conf/conf.py, docs/requirements.txt
Added PyYAML>=6.0.3 to docs requirements and introduced _get_helm_version_flag() in docs/conf/conf.py to read Chart.yaml and populate DOCS_VAR_HELM_VERSION_FLAG (returns --version <ver> or --devel).
User documentation
docs/src/user-docs/guides-k8s-deployment.md, docs/src/user-docs/quick-start/clp-json.md, docs/src/user-docs/quick-start/clp-text.md
Replaced local-chart download/extract and local-path install instructions with repository-based Helm commands (helm repo add / helm repo update) and helm install clp/clp using DOCS_VAR_HELM_VERSION_FLAG; updated examples and removed tarball/extract steps.

Sequence Diagram(s)

sequenceDiagram
    actor Dev as Developer
    participant GH as GitHub Actions
    participant Task as Task Runner
    participant Helm as Helm CLI
    participant Pages as gh-pages

    Dev->>GH: Push to main or v* branch / open PR
    GH->>Task: Run lint job (on PRs)
    Task->>Helm: Run task lint (helm checks)
    Helm-->>Task: Lint results
    Task-->>GH: Lint job completes

    GH->>Task: Run publish job (non-PR)
    Task->>Task: Read tools/.../Chart.yaml → derive version
    Task->>Helm: Run helm package -> produce .tgz
    Helm-->>Task: Packaged chart artifact
    Task->>Pages: Checkout gh-pages, merge/update index.yaml, copy .tgz
    Pages-->>GH: Push gh-pages with chart artifacts (unless skipped)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically describes the main change: adding a GitHub Actions workflow for publishing Helm charts to GitHub Pages, which aligns with the primary objective and substantial changes throughout the PR.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
docs/src/user-docs/guides-k8s-deployment.md (1)

296-302: Update helm template command to use repository chart reference.

Line 299 still uses a local path (.) which is inconsistent with the repository-based approach introduced in this PR. The command should reference the repository chart or clarify if local development is intended.

📝 Suggested fix
-helm template clp . -f custom-values.yaml
+helm template clp clp/clp -f custom-values.yaml

Or if previewing with local values before installation:

 helm template clp . -f custom-values.yaml
+# Or preview from the repository:
+helm template clp clp/clp DOCS_VAR_HELM_VERSION_FLAG -f custom-values.yaml
🤖 Fix all issues with AI agents
In @.github/workflows/clp-package-helm.yaml:
- Around line 98-101: The workflow currently always runs git commit -m
"ci(helm): Publish clp-${{env.CHART_VERSION}} ..." which fails when there are no
staged changes; modify the step to detect staged changes before committing by
running a check (e.g., git diff --cached --quiet or git status --porcelain) and
only run git commit when that check indicates there are changes, otherwise skip
the commit/push steps to avoid a non‑zero exit on "nothing to commit".
- Around line 66-70: The current version extraction (the shell command that sets
the variable version via grep on Chart.yaml and a sed) only handles quoted
versions and is brittle; change the pipeline that assigns version (the command
using grep '^version:' and sed) to a more robust extractor that strips the
leading "version:" prefix and removes optional surrounding quotes (e.g., use a
sed/awk pattern that accepts quoted or unquoted values and returns only the
version token), so CHART_VERSION gets the raw version string regardless of
whether Chart.yaml uses quotes.

In `@docs/conf/conf.py`:
- Around line 125-130: The docstring for _get_helm_version_flag is inaccurate:
it claims dev returns an empty string but the function actually returns
"--devel"; update the docstring to reflect the real behavior (describe that it
returns "--version <version>" for stable releases and "--devel" for development
versions) so the documentation matches the implementation.
- Around line 131-140: The current logic that reads Chart.yaml (variables
chart_yaml_path, chart, version) can raise if the file is missing or contains
invalid YAML; wrap the chart_yaml_path.read_text() and yaml.safe_load(...) in a
try/except that catches IOError, OSError, yaml.YAMLError (or a broad Exception)
and on any error return "--devel" to avoid failing the Sphinx build; also update
the function docstring to state that for dev or on error it returns "--devel"
(not an empty string) so the docs match the implementation.

In `@docs/src/user-docs/guides-k8s-deployment.md`:
- Around line 529-530: Replace the generic placeholder in the helm install
example (the line containing "helm install clp <repo-or-path/to/chart> --dry-run
--debug") with the repository-style chart reference now that the chart is
published; update it to use the "<repo-name>/clp" form (and if necessary add a
preceding note to run "helm repo add <repo-name> <url>" before installation) so
readers install from the chart repo rather than a local path.

Comment on lines 98 to 101
git config user.name "$GITHUB_ACTOR"
git config user.email "$GITHUB_ACTOR@users.noreply.github.com"
git add "*.tgz" "index.yaml"
git commit -m "ci(helm): Publish clp-${{env.CHART_VERSION}} from ${{github.ref_name}} (${{github.sha}})."
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Handle no-op publishes to avoid failing on “nothing to commit”. A re-run with identical artifacts will exit non‑zero on git commit.

🛠️ Proposed fix
           git config user.name "$GITHUB_ACTOR"
           git config user.email "$GITHUB_ACTOR@users.noreply.github.com"
           git add "*.tgz" "index.yaml"
-          git commit -m "ci(helm): Publish clp-${{env.CHART_VERSION}} from ${{github.ref_name}} (${{github.sha}})."
-          git push
+          if git diff --cached --quiet; then
+            echo "No Helm repo changes to publish."
+            exit 0
+          fi
+          git commit -m "ci(helm): Publish clp-${{env.CHART_VERSION}} from ${{github.ref_name}} (${{github.sha}})."
+          git push
🤖 Prompt for AI Agents
In @.github/workflows/clp-package-helm.yaml around lines 98 - 101, The workflow
currently always runs git commit -m "ci(helm): Publish
clp-${{env.CHART_VERSION}} ..." which fails when there are no staged changes;
modify the step to detect staged changes before committing by running a check
(e.g., git diff --cached --quiet or git status --porcelain) and only run git
commit when that check indicates there are changes, otherwise skip the
commit/push steps to avoid a non‑zero exit on "nothing to commit".

Comment on lines 131 to 140
chart_yaml_path = (
Path(__file__).parent.parent.parent / "tools/deployment/package-helm/Chart.yaml"
)
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"
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 22, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, check if Chart.yaml exists at the specified path
find . -name "Chart.yaml" -type f

Repository: 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 2

Repository: 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 2

Repository: 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.py

Repository: 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 Chart.yaml is missing, unreadable, or contains invalid YAML, the entire documentation build fails with no fallback. Wrap the file read and YAML parse in a try-except to gracefully return "--devel" on error.

Additionally, the docstring states the function returns an empty string "" for dev versions, but it actually returns "--devel". Update the docstring to match the implementation.

🛠️ 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
In `@docs/conf/conf.py` around lines 131 - 140, The current logic that reads
Chart.yaml (variables chart_yaml_path, chart, version) can raise if the file is
missing or contains invalid YAML; wrap the chart_yaml_path.read_text() and
yaml.safe_load(...) in a try/except that catches IOError, OSError,
yaml.YAMLError (or a broad Exception) and on any error return "--devel" to avoid
failing the Sphinx build; also update the function docstring to state that for
dev or on error it returns "--devel" (not an empty string) so the docs match the
implementation.

Copy link
Member Author

Choose a reason for hiding this comment

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

The Chart.yaml is not expected to be missing. If we ever move it, it is actually better to let the docs building fail so that developers can be reminded to update the docs.

Copy link
Contributor

Choose a reason for hiding this comment

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

@junhaoliao, understood! That's a reasonable approach—letting the build fail serves as an explicit signal when the file structure changes, which is consistent with the fail-fast philosophy I see in the CLP project.

One minor note: the docstring currently states the function returns "" for dev versions, but the implementation actually returns "--devel". You might want to update line 129 to say:

    :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
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1891
File: docs/conf/conf.py:131-140
Timestamp: 2026-01-22T01:18:27.853Z
Learning: In the CLP project's documentation build (docs/conf/conf.py), when _get_helm_version_flag() reads Chart.yaml, the team prefers to let the build fail if the file is missing or moved, rather than adding error handling with fallbacks. This fail-fast approach serves as a reminder to developers to update the documentation when file structures change.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1261
File: .github/workflows/clp-core-build.yaml:294-332
Timestamp: 2025-08-25T06:29:59.610Z
Learning: In the CLP project, Bill-hbrhbr prefers a "fail fast" approach for CI workflows - allowing potential command availability issues (like getconf in musllinux) to surface through CI failures rather than preemptively adding fallback logic, as they will fix issues when they occur.

Learnt from: junhaoliao
Repo: y-scope/clp PR: 1152
File: components/clp-package-utils/clp_package_utils/general.py:0-0
Timestamp: 2025-08-19T14:41:28.901Z
Learning: In the CLP codebase, prefer explicit failures over automatic directory creation in utility functions like dump_config. The user junhaoliao prefers to let file operations fail when parent directories don't exist, as this helps catch implementation errors during development rather than masking setup issues with automatic directory creation.

Learnt from: kirkrodrigues
Repo: y-scope/clp PR: 1885
File: docs/src/user-docs/guides-k8s-deployment.md:586-589
Timestamp: 2026-01-19T12:06:21.961Z
Learning: In the y-scope/clp project documentation built with Sphinx, relative paths work for linking to files within the repository, but directory links require GitHub URLs. When linking to directories (e.g., the Helm chart at tools/deployment/package-helm/), use GitHub URLs with DOCS_VAR_CLP_GIT_REF to keep them version-agnostic.

Learnt from: junhaoliao
Repo: y-scope/clp PR: 1466
File: .github/workflows/clp-rust-checks.yaml:14-15
Timestamp: 2025-10-22T21:14:12.225Z
Learning: Repository y-scope/clp: In GitHub Actions workflows (e.g., .github/workflows/clp-rust-checks.yaml), YAML anchors/aliases are acceptable and preferred to avoid duplication; if actionlint flags an alias node (e.g., on push.paths) as an error, treat it as a tool limitation and do not require inlining unless the team asks to silence the warning.

Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-10-22T21:02:31.113Z
Learning: Repository y-scope/clp: Maintain deterministic CI/builds for Rust; add a check to verify Cargo.lock is in sync with Cargo.toml without updating dependencies (non-mutating verification in clp-rust-checks workflow).

@junhaoliao junhaoliao added this to the February 2026 milestone Jan 22, 2026
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In @.github/workflows/clp-package-helm.yaml:
- Around line 3-14: The pull_request and push trigger blocks both duplicate the
same paths array; replace the duplicated arrays with a YAML anchor/alias: define
an anchor (e.g., &common_paths) for the shared paths list and reference it under
both pull_request.paths and push.paths using the alias (*common_paths) so both
triggers reuse the same paths definition while keeping the existing entries
(".github/workflows/clp-package-helm.yaml", "taskfiles/helm.yaml",
"tools/deployment/package-helm/**").
♻️ Duplicate comments (1)
.github/workflows/clp-package-helm.yaml (1)

66-70: Chart version extraction only handles quoted values.

This concern was previously raised. The sed regex requires quotes around the version value. If Chart.yaml ever uses an unquoted version (e.g., version: 0.1.2), the extraction will fail.

Comment on lines +3 to +14
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:
Copy link
Contributor

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_request and push triggers. Per repository conventions, YAML anchors are preferred to avoid duplication.

♻️ Suggested refactor
 on:
   pull_request:
-    paths:
+    paths: &helm_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/**"
+    paths: *helm_paths
   workflow_dispatch:

Based on learnings, YAML anchors/aliases are acceptable and preferred in this repository's workflows.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
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:
on:
pull_request:
paths: &helm_paths
- ".github/workflows/clp-package-helm.yaml"
- "taskfiles/helm.yaml"
- "tools/deployment/package-helm/**"
push:
paths: *helm_paths
workflow_dispatch:
🤖 Prompt for AI Agents
In @.github/workflows/clp-package-helm.yaml around lines 3 - 14, The
pull_request and push trigger blocks both duplicate the same paths array;
replace the duplicated arrays with a YAML anchor/alias: define an anchor (e.g.,
&common_paths) for the shared paths list and reference it under both
pull_request.paths and push.paths using the alias (*common_paths) so both
triggers reuse the same paths definition while keeping the existing entries
(".github/workflows/clp-package-helm.yaml", "taskfiles/helm.yaml",
"tools/deployment/package-helm/**").

@junhaoliao junhaoliao requested review from hoophalab and kirkrodrigues and removed request for kirkrodrigues January 22, 2026 02:36
Comment on lines 66 to 69
version=$(
grep '^version:' "tools/deployment/package-helm/Chart.yaml" \
| sed 's/version: *"\(.*\)"/\1/'
)
Copy link
Member

Choose a reason for hiding this comment

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

How about doing this with yq?

Copy link
Member Author

Choose a reason for hiding this comment

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

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 go install to install it oo.

just wanna make sure whether we are fine with the extra dependency

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I'm fine with the extra dependency. It's just in the workflow anyway?

```{code-block} yaml
:caption: dedicated-scheduling.yaml
distributedDeployment: true
Copy link
Member

Choose a reason for hiding this comment

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

Hm, this seems unrelated to the PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

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 custom-values.yaml for consistency. the improvements are not strictly related to the purpose of this PR though. i'll revert the changes in this PR but please create an issue to track this potential improvement.

Copy link
Contributor

Choose a reason for hiding this comment

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

🧩 Analysis chain

@junhaoliao, I'll create an issue to track this improvement.


🏁 Script executed:

#!/bin/bash
# Create a GitHub issue to track the improvement

gh issue create \
  --repo y-scope/clp \
  --title "docs: Move distributedDeployment setting to YAML examples for consistency" \
  --body "## Context

In the Kubernetes deployment guide, some examples use command-line \`--set distributedDeployment=true\` while others could benefit from having this setting in the YAML example files for consistency.

## Improvement

Move the \`distributedDeployment\` setting from command-line overrides into the YAML example files (e.g., \`dedicated-scheduling.yaml\`, \`shared-scheduling.yaml\`) for better consistency and clarity.

## References

- PR: https://github.com/y-scope/clp/pull/1891
- Comment: https://github.com/y-scope/clp/pull/1891#discussion_r2718465254
- Requested by: `@junhaoliao`" \
  --assignee junhaoliao

Length of output: 42


@junhaoliao, I've created issue #1900 to track this improvement: #1900

The issue documents the goal of moving distributedDeployment settings from command-line overrides into the YAML example files for better consistency in the Kubernetes deployment guide.


🧠 Learnings used
Learnt from: kirkrodrigues
Repo: y-scope/clp PR: 1885
File: docs/src/user-docs/guides-k8s-deployment.md:586-589
Timestamp: 2026-01-19T12:06:21.961Z
Learning: In the y-scope/clp project documentation built with Sphinx, relative paths work for linking to files within the repository, but directory links require GitHub URLs. When linking to directories (e.g., the Helm chart at tools/deployment/package-helm/), use GitHub URLs with DOCS_VAR_CLP_GIT_REF to keep them version-agnostic.

Learnt from: quinntaylormitchell
Repo: y-scope/clp PR: 968
File: docs/src/user-guide/quick-start/overview.md:73-109
Timestamp: 2025-06-18T20:39:05.899Z
Learning: The CLP project team prefers to use video content to demonstrate detailed procedural steps (like tarball extraction) rather than including every step in the written documentation, keeping the docs focused on conceptual guidance.

Learnt from: junhaoliao
Repo: y-scope/clp PR: 1466
File: .github/workflows/clp-rust-checks.yaml:14-15
Timestamp: 2025-10-22T21:14:12.225Z
Learning: Repository y-scope/clp: In GitHub Actions workflows (e.g., .github/workflows/clp-rust-checks.yaml), YAML anchors/aliases are acceptable and preferred to avoid duplication; if actionlint flags an alias node (e.g., on push.paths) as an error, treat it as a tool limitation and do not require inlining unless the team asks to silence the warning.

Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-10-22T21:02:31.113Z
Learning: Repository y-scope/clp: Maintain deterministic CI/builds for Rust; add a check to verify Cargo.lock is in sync with Cargo.toml without updating dependencies (non-mutating verification in clp-rust-checks workflow).

Comment on lines 352 to 353
helm install clp clp/clp DOCS_VAR_HELM_VERSION_FLAG \
-f dedicated-scheduling.yaml
Copy link
Member

Choose a reason for hiding this comment

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

Ditto

Comment on lines 370 to 371
distributedDeployment: true
Copy link
Member

Choose a reason for hiding this comment

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

Ditto

# Clone the repository (if you haven't already)
git clone --branch DOCS_VAR_CLP_GIT_REF https://github.com/y-scope/clp.git
cd clp/tools/deployment/package-helm
# If you haven't already, add the CLP Helm repository.
Copy link
Member

Choose a reason for hiding this comment

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

What happens if you had already added the repo?

Copy link
Member Author

Choose a reason for hiding this comment

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

harmless unless the user have added another Helm repo also with name "clp" locally:

> helm repo add clp https://junhaoliao.github.io/clp
"clp" has been added to your repositories

> helm repo add clp https://junhaoliao.github.io/clp
"clp" already exists with the same configuration, skipping

Copy link
Member Author

@junhaoliao junhaoliao Jan 22, 2026

Choose a reason for hiding this comment

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

actually, their prompts are quite intuitive:

# ... following above commands
> helm repo add clp https://y-scope.github.io/clp
Error: repository name (clp) already exists, please specify a different name

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I feel like we can simply remove the comment?

junhaoliao and others added 3 commits January 23, 2026 02:40
…view

Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
… from code review

Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
docs/src/user-docs/guides-k8s-deployment.md (1)

296-302: Clarify the helm template command context.

The tip uses helm template clp . which references the current directory (local chart), but the surrounding section demonstrates repository-based installation. This is inconsistent with the debugging section (lines 528-532), which explicitly distinguishes between repository and local chart scenarios.

Consider updating to match the repository-based flow or providing both options with clear context:

::::{tip}
To preview the generated Kubernetes manifests before installing:

**For the published chart:**
```bash
helm template clp clp/clp DOCS_VAR_HELM_VERSION_FLAG -f custom-values.yaml

For local chart development:

helm template clp . -f custom-values.yaml

::::


</blockquote></details>

</blockquote></details>

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In @.github/workflows/clp-package-helm.yaml:
- Around line 18-22: The workflow's global concurrency group uses "${{
github.ref }}" which allows runs from different branches to execute in parallel
and causes gh-pages push races; update the publishing job (the job named
"publish") to use its own concurrency block (or change the global block) so the
group is unique for gh-pages pushes (e.g., a publish-specific group string) and
keep cancel-in-progress: true; modify the concurrency.group value referenced in
the workflow to a fixed publish-only identifier for the publish job instead of
including "${{ github.ref }}" so publish runs are serialized and
non-fast-forward failures are avoided.

Comment on lines +18 to +22
concurrency:
group: "${{github.workflow}}-${{github.ref}}"

# Cancel in-progress jobs for efficiency
cancel-in-progress: true
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

cat -n .github/workflows/clp-package-helm.yaml

Repository: y-scope/clp

Length of output: 4480


🌐 Web query:

GitHub Actions concurrency group github.ref multiple branches parallel execution same job

💡 Result:

In GitHub Actions, only runs that resolve to the exact same concurrency.group value will block/cancel each other. So whether the “same job” can run in parallel across branches depends entirely on what string your group expression produces. (docs.github.com)

What github.ref does for multiple branches

  • On push events, github.ref is the full branch ref (for example refs/heads/main, refs/heads/feature-x). That means different branches produce different group keys, so they can run in parallel when you use group: ${{ github.ref }} (or include it in a longer string). (docs.github.com)
  • On pull_request events, github.ref is not the source branch; it’s typically a PR ref like refs/pull/<n>/merge, so concurrency will be “per PR ref,” not “per branch name.” (docs.github.com)

Recommended patterns

1) Parallel across branches, but serialized per-branch (common)

concurrency:
  group: ${{ github.workflow }}-${{ github.ref }}
  cancel-in-progress: true

This allows branch A and branch B to run in parallel, but new runs on the same branch cancel/queue each other. (docs.github.com)

2) “Per source branch” behavior for PRs + pushes (avoid PR github.ref mismatch)

concurrency:
  group: ${{ github.workflow }}-${{ github.head_ref || github.ref_name }}
  cancel-in-progress: true

github.head_ref exists only for PR events, so the fallback avoids errors and keeps a stable “branch-like” key across event types. (docs.github.com)

If you share your trigger (push, pull_request, etc.) and your current concurrency: block, I can tell you exactly why your runs are (or aren’t) parallelizing.

Citations:


Serialise publishing to avoid gh-pages push races.

The concurrency group includes ${{ github.ref }}, which generates different group values for different branches. This allows the publish job on main and v* release branches to run in parallel. Since both push to the same gh-pages branch, simultaneous pushes create non-fast-forward failures. Add a publish-specific concurrency group to serialise updates.

🛠️ Suggested fix (publish-only concurrency)
   publish:
+    concurrency:
+      group: "${{github.workflow}}-gh-pages"
+      cancel-in-progress: true
     # 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'))
🤖 Prompt for AI Agents
In @.github/workflows/clp-package-helm.yaml around lines 18 - 22, The workflow's
global concurrency group uses "${{ github.ref }}" which allows runs from
different branches to execute in parallel and causes gh-pages push races; update
the publishing job (the job named "publish") to use its own concurrency block
(or change the global block) so the group is unique for gh-pages pushes (e.g., a
publish-specific group string) and keep cancel-in-progress: true; modify the
concurrency.group value referenced in the workflow to a fixed publish-only
identifier for the publish job instead of including "${{ github.ref }}" so
publish runs are serialized and non-fast-forward failures are avoided.

Copy link
Member

@kirkrodrigues kirkrodrigues left a comment

Choose a reason for hiding this comment

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

New changes look good apart from the comments I posted in replies.

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.

2 participants