Skip to content

Changed version and publish chart workflow #29

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Changed version and publish chart workflow #29

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
be078d4
Modified the build_push_chart so it gets the version from chart/Chart…
MarkBogelund May 22, 2025
e2d0d13
modified the publish chart workflow to only run on manual trigger or …
MarkBogelund May 22, 2025
156f81e
added suggestion from coderabbitai for error handling in the build_pu…
MarkBogelund May 22, 2025
7d189ae
Simplified manual check and use yq for version collection
MarkBogelund May 27, 2025
77924a0
changed the paths on pr and push, so they trigger workflow when somet…
MarkBogelund May 27, 2025
8ce0096
removed trailing white spaces
MarkBogelund May 27, 2025
03de636
added step to install yq
MarkBogelund May 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 46 additions & 13 deletions .github/workflows/publish-chart.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,41 +1,74 @@
name: "Publish Chart"

on:
workflow_dispatch:
pull_request:
paths:
- .github/workflows/publish_chart.yml
- chart/**
- "chart/**"
- ".github/workflows/publish-chart.yml"
push:
paths:
- .github/workflows/publish_chart.yml
- chart/**
- "chart/**"
- ".github/workflows/publish-chart.yml"
branches:
- main

jobs:
check-version-change:
runs-on: ubuntu-latest
outputs:
should_publish: ${{ steps.check-version.outputs.changed || steps.manual-flag.outputs.changed }}
steps:
- name: Checkout Code
uses: actions/checkout@v3
with:
ref: ${{ github.head_ref || github.ref_name }}
fetch-depth: 0
- name: Check if Chart Version Changed
id: check-version
if: github.event_name != 'workflow_dispatch' # Skip if manually triggered
run: |
if git diff --name-only HEAD^ HEAD | grep -q 'chart/Chart.yaml'; then
Copy link

@cubic-dev-ai cubic-dev-ai bot May 27, 2025

Choose a reason for hiding this comment

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

Using HEAD^ assumes the previous commit exists; on the first commit of a branch or repository this command fails, causing the entire job to error out.

# Check if Chart.yaml existed in previous commit
Comment on lines +36 to +38
Copy link

@coderabbitai coderabbitai bot May 27, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid missing version changes on multi-commit pushes
Using git diff HEAD^ HEAD inspects only the last commit. If chart/Chart.yaml was modified earlier in a multi-commit push, this will be missed.

Switch to comparing the full push range provided by GitHub Actions:

- if git diff --name-only HEAD^ HEAD | grep -q 'chart/Chart.yaml'; then
+ if git diff --name-only ${{ github.event.before }} ${{ github.sha }} | grep -q 'chart/Chart.yaml'; then

This ensures all changes in the push are evaluated.

📝 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
run: |
if git diff --name-only HEAD^ HEAD | grep -q 'chart/Chart.yaml'; then
# Check if Chart.yaml existed in previous commit
run: |
if git diff --name-only ${{ github.event.before }} ${{ github.sha }} | grep -q 'chart/Chart.yaml'; then
# Check if Chart.yaml existed in previous commit
🤖 Prompt for AI Agents 
In .github/workflows/publish-chart.yml around lines 31 to 33, the script uses
'git diff HEAD^ HEAD' which only checks the last commit for changes to
'chart/Chart.yaml', missing modifications in earlier commits of a multi-commit
push. Update the git diff command to use the full push range provided by GitHub
Actions, such as 'git diff ${{ github.event.before }} ${{ github.event.after
}}', to ensure all changes in the entire push are evaluated.

if git cat-file -e HEAD^:chart/Chart.yaml 2>/dev/null; then
PREVIOUS_VERSION=$(git show HEAD^:chart/Chart.yaml | yq -r '.version')
CURRENT_VERSION=$(yq -r '.version' chart/Chart.yaml)
if [ "$PREVIOUS_VERSION" != "$CURRENT_VERSION" ]; then
echo "Version changed from $PREVIOUS_VERSION to $CURRENT_VERSION"
echo "changed=true" >> $GITHUB_OUTPUT
else
echo "Version didn't change ($CURRENT_VERSION)"
echo "changed=false" >> $GITHUB_OUTPUT
fi
else
echo "Chart.yaml is newly added, treating as version change"
echo "changed=true" >> $GITHUB_OUTPUT
fi
else
echo "Chart.yaml wasn't modified"
echo "changed=false" >> $GITHUB_OUTPUT
fi
- name: Set Publish Flag for Manual Runs
if: github.event_name == 'workflow_dispatch'
id: manual-flag
run: echo "changed=true" >> $GITHUB_OUTPUT
publish-chart:
needs: check-version-change
if: needs.check-version-change.outputs.should_publish == 'true'
Comment on lines +64 to +65
Copy link

@coderabbitai coderabbitai bot May 27, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

⚠️ Potential issue

Prevent publishing on pull_request events

As written, a version bump in a PR will trigger the publish-chart job and push artifacts for unmerged code. Restrict the publish step to pushes (e.g., on main) by extending the condition:

- if: needs.check-version-change.outputs.should_publish == 'true'
+ if: needs.check-version-change.outputs.should_publish == 'true' && github.event_name == 'push'
📝 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
needs: check-version-change
if: needs.check-version-change.outputs.should_publish == 'true'
needs: check-version-change
if: needs.check-version-change.outputs.should_publish == 'true' && github.event_name == 'push'
🤖 Prompt for AI Agents 
In .github/workflows/publish-chart.yml at lines 64 to 65, the current condition
allows the publish-chart job to run on pull_request events, which can cause
unmerged code to be published. Modify the if condition to also check that the
event is a push event, for example by adding a condition to ensure
github.event_name == 'push' along with the existing should_publish check, so
publishing only occurs on pushes like to the main branch.

runs-on: ubuntu-latest
steps:
- name: Checkout Code
uses: actions/checkout@v3
with:
ref: ${{ github.head_ref }}
ref: ${{ github.head_ref || github.ref_name }}
fetch-depth: 0

- name: Install Helm
uses: azure/setup-helm@v3
with:
version: v3.14.0

- name: Copy README.md to Chart Directory
run: cp README.md chart/README.md

- name: Create Chart Package
run: helm package chart -d ./tmp

- name: Login to Registry
run: echo "${{ secrets.REGISTRY_PASSWORD }}" | helm registry login ${{ vars.REGISTRY_ADDR }} --username ${{ vars.REGISTRY_USERNAME }} --password-stdin

- name: Push Packaged Chart to Registry
run: helm push ./tmp/* oci://${{ vars.REGISTRY_ADDR }}/library

15 changes: 12 additions & 3 deletions do.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,21 @@ build_push_image() {
}

build_push_chart() {
version=$(cat VERSION)
# Ensure prerequisites are available
if ! command -v yq >/dev/null 2>&1; then
echo "Error: 'yq' is required but not installed." >&2
exit 1
fi
if [ ! -f chart/Chart.yaml ]; then
echo "Error: chart/Chart.yaml not found." >&2
exit 1
fi
version=$(yq -r '.version' chart/Chart.yaml)
Copy link

@cubic-dev-ai cubic-dev-ai bot May 22, 2025

Choose a reason for hiding this comment

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

The yq command is invoked with the -r flag but without the required sub-command (e|eval) that newer versions of mikefarah/yq expect; this exits with "unknown shorthand flag" and stops the build script (set -e).

Copy link

@cubic-dev-ai cubic-dev-ai bot May 27, 2025

Choose a reason for hiding this comment

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

Using yq without the "eval" (or "e") sub-command fails with modern versions of mikefarah/yq (v4+): running yq -r returns an unknown flag error, causing the script to exit. Use yq e -r (or yq eval -r) so the command works across common yq versions.

Suggested change
version=$(yq -r '.version' chart/Chart.yaml)
version=$(yq e -r '.version' chart/Chart.yaml)

helm package chart
helm push helm-charts-oci-proxy-$version.tgz oci://8gears.container-registry.com/library
helm push "helm-charts-oci-proxy-${version}.tgz" \
"oci://8gears.container-registry.com/library"
}


deploy() {
helm upgrade -i --namespace ocip-staging --create-namespace ocip-staging ./chart
}
Expand Down