Improve helm chart workflow.

[mikedrpc]

No description provided.

[mxssl]

Findings from review:

1. chart/nodecore/CODEOWNERS will not be enforced by GitHub. CODEOWNERS files are only loaded from .github/CODEOWNERS, /CODEOWNERS, or /docs/CODEOWNERS. Also, the owner token should be @drpcorg/infra (team slug), not @drpcorg/teams/infra. As written, the intended required-review protection for chart changes is effectively not active.

2. .github/workflows/publish-chart.yml now gates the publish job with if: github.event_name == 'push' && github.ref == 'refs/heads/main' while still defining workflow_dispatch inputs. This makes manual dispatch unable to publish (the job is skipped on workflow_dispatch), which is a behavioral regression from the previous workflow.\n\n3) The PR trigger path filter only includes chart/nodecore/**. Workflow and composite-action changes under .github/** (including this chart publish workflow/action) won’t run this CI in future PRs, so chart-publish pipeline changes can merge unvalidated.

[Copilot]

Pull request overview

This PR improves the NodeCore Helm chart workflow by adding stricter CI validation (linting + version checks), introducing a values JSON schema for Helm validation, and adding chart ownership/metadata documentation.

Changes:

• Added values.schema.json for Helm values validation and updated values.yaml formatting/types.
• Updated the GitHub Actions workflow to lint on PRs and validate chart version increments.
• Added chart maintainers metadata plus contribution and ownership files.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
chart/nodecore/values.yaml	Normalizes YAML formatting and quotes CPU values; restructures example nodecoreConfig.
chart/nodecore/values.schema.json	Introduces a JSON schema for Helm values validation.
chart/nodecore/Chart.yaml	Adds maintainers metadata to the chart.
chart/nodecore/CODEOWNERS	Attempts to define code ownership for chart changes.
CONTRIBUTING.md	Documents schema update expectations and CI behavior for chart PRs.
.github/workflows/publish-chart.yml	Adds PR path triggers, stricter linting, and a chart version check action; adjusts publishing conditions.
.github/actions/check-chart-version/action.yml	Adds a composite action to enforce chart version > latest nodecore-* tag.

Comments suppressed due to low confidence (2)

.github/workflows/publish-chart.yml:113

• The publish job packages and pushes the chart without running any chart validation (lint/schema render) in the same job. If something lands on main without the PR checks (e.g., direct pushes, reruns, or future workflow changes), an invalid chart could be published; consider running helm lint --strict (and/or ct lint) in the publish job as a gate before helm package/helm push.

      - name: Package & push nodecore chart
        run: |
          CHART_VERSION_TAG=$(yq e '.version' ${{ env.CHART_DIR }}/Chart.yaml)
          echo "CHART_VERSION_TAG=${CHART_VERSION_TAG}" >> $GITHUB_ENV
      
          helm package "${{ env.CHART_DIR }}" \
            --version="${CHART_VERSION_TAG}" \
            --app-version="${CHART_VERSION_TAG}"
      

.github/workflows/publish-chart.yml:73

• actions/checkout@v5 is not a valid release of actions/checkout (the latest stable major is v4). This will make the publish job fail to start; use actions/checkout@v4 (or pin to a commit SHA) instead.

      - name: Checkout
        uses: actions/checkout@v5
        with:
          ref: ${{ inputs.ref != '' && inputs.ref || github.ref_name }}

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[mxssl]

Code Review for PR #119: Improve helm chart workflow

Summary

This PR adds CI validation (linting, version checks), a values.schema.json for Helm validation, chart metadata, and contribution/ownership docs. The direction is good — stricter CI for chart changes is valuable — but there are several issues that should be addressed before merging.

---

Critical Issues

1. CODEOWNERS file is in the wrong location
chart/nodecore/CODEOWNERS will not be picked up by GitHub. CODEOWNERS must live in one of: /.github/CODEOWNERS, /CODEOWNERS, or /docs/CODEOWNERS. Additionally, the owner syntax @drpcorg/teams/infra is incorrect — it should be @drpcorg/infra (the team slug without /teams/). As-is, the intended required-review gate for chart changes is completely non-functional.

2. workflow_dispatch is broken for publishing
The new guard on the publish job:

if: github.event_name == 'push' && github.ref == 'refs/heads/main'

excludes workflow_dispatch entirely. Previously, manual dispatch could trigger a publish (useful for hotfixes or re-publishes). This is a behavioral regression. Fix by including workflow_dispatch:

if: (github.event_name == 'push' && github.ref == 'refs/heads/main') || github.event_name == 'workflow_dispatch'

3. actions/checkout@v5 does not exist
Line 58 of publish-chart.yml (in the publish job) uses actions/checkout@v5, which is not a released version. The latest stable major is v4. This will cause the publish job to fail. While this was pre-existing, the PR should fix it since it's modifying this workflow.

---

Medium Issues

4. PR path trigger is too narrow

pull_request:
  paths:
    - 'chart/nodecore/**'

Changes to .github/workflows/publish-chart.yml or .github/actions/check-chart-version/** won't trigger the lint/validate job on PRs, so pipeline changes can merge without CI validation. Add:

paths:
  - 'chart/nodecore/**'
  - '.github/workflows/publish-chart.yml'
  - '.github/actions/check-chart-version/**'

5. Redundant checkout in composite action
The composite action .github/actions/check-chart-version/action.yml includes its own actions/checkout@v3 step. The calling workflow already checks out the repo (with fetch-depth: 0), so this second checkout is redundant and also uses an older version (v3 vs v4). Remove the checkout step from the composite action and rely on the caller's checkout.

6. yq is installed twice independently
yq is installed in both the composite action and the publish job, with the same version pinned in two places. If this tool is needed in multiple jobs, consider either:

• Installing it once in the workflow and passing it down, or
• At minimum, extracting the version to a workflow-level env var so it's maintained in one place.

7. ct lint --check-version-increment overlaps with the custom version check action
ct lint with --check-version-increment already validates that the chart version is bumped vs. the target branch. The new check-chart-version composite action does something slightly different (compares against git tags), but the overlap is confusing. Consider documenting why both are needed or removing one.

---

Minor / Nits

8. Commented-out branch trigger

# - helm-chart

If the helm-chart branch trigger is no longer needed, remove it entirely rather than leaving it commented out.

9. additionalProperties: false in schema top-level
The values.schema.json sets "additionalProperties": false at the root. This is quite strict — any values key not explicitly defined in the schema will cause helm lint --strict to fail. This may cause friction if subchart overrides or ad-hoc values are needed in the future. Consider whether this is intentional.

10. Publish job lacks validation gate
The publish job (publish-helm-chart-nodecore) doesn't run helm lint or any validation before packaging and pushing. If something lands on main without the PR checks (e.g., direct push, admin merge), an invalid chart could be published. Consider adding a lint step in the publish job as a safety net.

11. No needs: ct-lint on publish job
The ct-lint job and publish-helm-chart-nodecore job are independent. On push to main, ct-lint is skipped (it has if: github.event_name == 'pull_request') and publish runs unconditionally. This is likely intentional but worth confirming — a failed lint in a PR won't block a direct push to main from publishing.

---

What looks good

• Adding values.schema.json is a solid improvement for catching misconfigurations early.
• Normalizing values.yaml indentation (4-space to 2-space) and quoting CPU values as strings is correct — Helm/YAML can interpret bare 500m as milliunits.
• Adding helm lint --strict in the PR workflow is valuable.
• Adding maintainers to Chart.yaml is good practice.
• CONTRIBUTING.md is a nice touch for documenting the schema update expectation.

---

Overall: The intent is good but there are functional issues (broken CODEOWNERS, broken workflow_dispatch, invalid checkout version) that need fixing before this is safe to merge.