Skip to content

Add reusable workflows - #11

Open
arolos wants to merge 2 commits into
8-ci-upgrade-github-actions-to-latest-major-versionsfrom
10-centralize-shared-ci-workflows-into-reusable-workflows
Open

Add reusable workflows#11
arolos wants to merge 2 commits into
8-ci-upgrade-github-actions-to-latest-major-versionsfrom
10-centralize-shared-ci-workflows-into-reusable-workflows

Conversation

@arolos

@arolos arolos commented Jul 15, 2026

Copy link
Copy Markdown
Member

Close #10

@devin-ai-integration devin-ai-integration Bot 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.

Devin Review found 4 potential issues.

Open in Devin Review

Comment on lines +80 to +82
key: kubeconform-schemas-${{ inputs.kubeconform_version }}-${{ hashFiles('charts/**/Chart.yaml') }}
restore-keys: |
kubeconform-schemas-${{ inputs.kubeconform_version }}-

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.

📝 Info: kubeconform schema cache key is keyed only on Chart.yaml, not chart_glob

The cache key kubeconform-schemas-${{ inputs.kubeconform_version }}-${{ hashFiles('charts/**/Chart.yaml') }} hardcodes the charts/**/Chart.yaml glob, independent of the configurable chart_glob input. This works for both documented callers (single-chart charts/charts/Chart.yaml, and monorepo charts/*/charts/*/Chart.yaml, both matched by charts/**/Chart.yaml). It would only break for a caller whose charts live outside charts/, where hashFiles returns empty and the key collapses to a constant. Not a correctness issue because kubeconform re-downloads any missing schemas and restore-keys provides prefix fallback; noting for awareness in case chart layouts diverge from the two documented shapes.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +190 to +194
# Inject short commit SHA for CDN immutable path (matched by upload prefix).
if [[ "${INJECT_CDN_SHA}" == "true" ]]; then
SHORT_SHA="$(git rev-parse --short HEAD)"
yq -i ".cdn.commitSha = \"${SHORT_SHA}\"" "${CHART_DIR}/values.yaml"
fi

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.

🔍 cdn.commitSha injected without the 'g' prefix used by the RC version

helm-publish injects cdn.commitSha as the raw git rev-parse --short HEAD (helm-publish.yaml:192-193), whereas oci-build.yaml:214 prefixes the short SHA with g for the RC version segment. These are intentionally different values (the RC version needs the g for SemVer §9 numeric-identifier validity, while the CDN path presumably uses the plain short SHA to match the upload prefix). Worth confirming the CDN upload destination_prefix in caller repos uses the plain (un-prefixed) short SHA so the chart's cdn.commitSha resolves to a path that actually exists.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

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.

The g was added to prevent release-please actions being blocked by short_sha's that contain only numbers.

Comment on lines +181 to +182
OCI_BASE: oci://${{ env.REGISTRY }}/${{ github.repository }}/charts
IMAGE_BASE: ${{ env.REGISTRY }}/${{ github.repository }}/charts

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.

📝 Info: OCI paths rely on github.repository already being lowercase

OCI_BASE/IMAGE_BASE interpolate ${{ github.repository }} directly (helm-publish.yaml:181-182), and release-retag/oci-build do the same for image refs. GHCR requires lowercase repository paths. This is safe for the bcit-tlu org (already lowercase) and matches the pre-existing oci-build.yaml convention, but any caller repo/org containing uppercase letters would produce an invalid OCI reference. Noting the shared assumption rather than flagging as a bug.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +119 to +121
- name: Run extra command
if: inputs.extra_command != ''
run: ${{ inputs.extra_command }}

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.

🟨 Reusable workflow input interpolated directly into a shell run block

The extra_command input is expanded directly into the run: script via template interpolation (.github/workflows/helm-lint.yaml:121), so whatever string a caller supplies is executed verbatim as shell. Because the value is substituted before the shell sees it (rather than passed via an environment variable), any shell metacharacters in the input are interpreted, giving arbitrary command execution in the job. Callers of a reusable workflow are generally trusted, so impact is limited, but the pattern is fragile hardening-wise.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@kphunter

Copy link
Copy Markdown
Contributor

IMHO, I'd prefer to see an independent https://github.com/bcit-tlu/sugar-suite CI/CD pipeline until it is proven to work with CDN asset replacement. It would also be nice to be able to develop a solution for branch-based deployment, because local "previews" of features or fixes is less convenient (and problematic for mobile testing).

I feel that there are too many moving parts at the moment ,and that before we standardize and consolidate, we should have a scalable solution that works really well that we can then use as the "golden record" for replication to other repos.

@kphunter
kphunter removed their request for review July 23, 2026 23:16
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