Add release logic #35

miles-w-3 · 2025-01-04T04:32:10Z

Closes #30

A snapshot of what I did:

Create my pages branch

git checkout --orphan gh-pages
remove all files from this branch:
- find . -name ".*" -not -name "." -not -name ".." -not -name ".git" -maxdepth 1 -exec rm -rf {} \;

Add a readme, example:

# AWX Operator Helm Release

This branch is used to manage github-pages releases for the AWX Operator Helm Chart

git add README.md
git commit -am "chart releaser pages config"
- -a is important to ensure all files are removed
git push --set-upstream origin gh-pages

Create configs in main

go back to main and create the workflow configs: git switch main
- git switch -c feature/chart-releaser-to-main
Applied various fixes to Makefile to make it work as expected
add the release.yml action to .workflows
Added some documentation

Remaining design decisions

A huge consequence of this approach is that the charts directory can't be gitignored, since chart-releaser uses it to detect changes. A consequence of this is that contributors need to make sure that they don't commit into charts/ on release. I think a natural solution to this is to make a development make target for helm generation that goes to a /test folder, and then have a release make target which goes to charts and is used only in the release pipeline. That will keep this directory clean from accidental commits.

There's probably another approach we could take of permanently populating charts with enough stubs to fool chart-releaser into always wanting to make a release, but I'm not sure if we want to do that either - imo there is value in allowing non-chart-specific commits like readme and other top-level file updates which should not trigger a release.

Blockers for merge

I think that the current gh-pages branch is already in a good spot. It points at all of the old releases in the index file, and chart-releaser will seamlessly add releases onto it. @rooftopcellist can probably confirm, but I'm guessing the custom url for the page has already been set up.

Therefore, I don't think there are any remaining blockers beyond figuring out how to limit dumping in charts/. In this branch, I set the chart version at 3.0.0, since @oraNod as you pointed out helm recommends using semver.

One last thought, we may want a PR validation workflow that generates the chart and checks if there are file changes in charts/. If there are, it should make sure that the generated chart has a new version.

oraNod

Thanks for working on this one @miles-w-3 I'll find some time this week to go through and see these changes in action. It's great to see some progress made towards the next release though.

About protecting the charts directory from accidental commits, a .git/hooks/pre-commit hook could be a good option to prevent direct commits. Something like this might work:

#!/bin/bash
if git diff --cached --name-only | grep -q "^charts/"; then
    echo "Direct commits to the charts directory not allowed"
    echo "Use the release process instead"
    exit 1
fi

Cheers.

oraNod · 2025-01-06T13:59:32Z

.github/workflows/reusable-nox.yml

-              python-versions: "${{ matrix.python-versions }}"
-        - name: "Run nox -s ${{ matrix.session }}"
-          run: |
-            nox -s "${{ matrix.session }}"


@miles-w-3 Should we really delete this workflow? At the moment it only runs the docs build session but we might decide to add other nox sessions for things un-related to the release, like linting, spell checks, etc.

I'm fine to keep it, I'm not familiar with nox and wasn't sure if it was just a holdover from awx

oraNod · 2025-01-06T16:56:37Z

.gitignore

@@ -5,7 +5,7 @@ gh-pages/
 /bundle
 /bundle_tmp*
 /bundle.Dockerfile
-/charts
+


not sure we want an empty line here 😄

apologies, I thought you preferred new lines at the end of files

no worries. empty line at the end of file is good but this seems to insert an empty line in the middle of the file.

oraNod · 2025-01-06T17:02:03Z

Makefile

@@ -1,14 +1,18 @@
-# Include the Makefile from the ansible/awx-operator repository.
-# This Makefile is created with the clone-awx-operator.py script.
-include Makefile.awx-operator


@miles-w-3 I've only been able to take a bit of a cursory look at these changes but if we no longer need the makefile from the awx-operator repo should we also update the clone-awx-operator.py script so it doesn't create it?

yeah I think that would be fine. I removed the dependency on this makefile by adding in a few more variables. I think there's a chicken and egg problem on main right now, since the has stages that invoke clone-awx-operator.py, but the makefile can only run once Makefile.awx-operator has been created

miles-w-3 · 2025-01-09T02:16:00Z

Thanks for working on this one @miles-w-3 I'll find some time this week to go through and see these changes in action. It's great to see some progress made towards the next release though.

About protecting the charts directory from accidental commits, a .git/hooks/pre-commit hook could be a good option to prevent direct commits. Something like this might work:
#!/bin/bash
if git diff --cached --name-only | grep -q "^charts/"; then
    echo "Direct commits to the charts directory not allowed"
    echo "Use the release process instead"
    exit 1
fi
Cheers.

I think this hook could work, but wouldn't that prevent the workflow from committing to this directory as well during the release?
I think I addressed the rest of the comments

Signed-off-by: Miles Wilson <[email protected]>

oraNod

Some review comments for you @miles-w-3 Thanks again for working on this.

I noticed you mentioned the pre-commit hook to prevent direct commits to the charts/ directory and how that works with the release workflow. It should be possible to set up a pre-commit configuration that excludes the GHA bot. I think either that or we add a token with special permissions for the repo.

oraNod · 2025-01-09T15:31:33Z

.gitignore

@@ -5,7 +5,7 @@ gh-pages/
 /bundle
 /bundle_tmp*
 /bundle.Dockerfile
-/charts
+


no worries. empty line at the end of file is good but this seems to insert an empty line in the middle of the file.

oraNod · 2025-01-09T15:32:52Z

Makefile

-	$(HELM) repo index .cr-release-packages --url https://github.com/$(CHART_OWNER)/$(CHART_REPO)/releases/download/ --merge $(CHART_DIR)/index.yaml
-
-	mv .cr-release-packages/index.yaml $(CHART_DIR)/index.yaml
+	@echo "Helm chart successfully configured for $(CHART_NAME)"


here it actually looks like there is a missing empty line at the end of the file.

oraNod · 2025-01-09T15:41:59Z

.github/workflows/release.yml

+        run: |
+          make helm-chart-generate


Suggested change

run: |

make helm-chart-generate

run: make helm-chart-generate

oraNod · 2025-01-09T15:54:31Z

.github/workflows/release.yml

+        run: |
+          git status --porcelain charts/ | grep -q "." && echo "changes=true" >> $GITHUB_OUTPUT || echo "changes=false" >> $GITHUB_OUTPUT


Suggested change

run: |

git status --porcelain charts/ | grep -q "." && echo "changes=true" >> $GITHUB_OUTPUT || echo "changes=false" >> $GITHUB_OUTPUT

run: >-

git diff --quiet charts/

&& echo "changes=true" >> $GITHUB_OUTPUT

|| echo "changes=false" >> $GITHUB_OUTPUT

I'd recommend using a scalar to break this into multiple lines for readability. I also think we can simplify it by using git diff --quiet instead of the porcelain option and the grep pipe.

I'm not too stuck on it but I also wonder if we could s/changes/chart_changes to make that output variable more descriptive.

oraNod · 2025-01-09T16:00:24Z

.github/workflows/release.yml

+      # We need to do this step because our helm chart is not the source of truth, it's generated from starter
+      # Note that failing to do this doesn't break this release, but it can break future ones and make the release state confusing
+      - name: Commit generated chart
+        id: update_source


Is this step called somewhere? If not, there's probably no need for an ID.

oraNod · 2025-01-09T16:15:57Z

.github/workflows/release.yml

+      - name: Commit generated chart
+        id: update_source
+        run: |
+          git pull


is it necessary to do a pull here? it seems unnecessary to me given we already have the full history and the workflow runs on a push to main.

if you feel strongly about it, how about git pull --rebase origin main instead of just a plain pull?

oraNod · 2025-01-09T16:17:52Z

.github/workflows/release.yml

+    branches:
+      - main


Suggested change

branches:

- main

branches:

- main

paths:

- 'charts/**'

Does it make sense to run only when chart files change?

oraNod · 2025-01-09T16:22:48Z

.github/workflows/release.yml

+          git add charts/
+          git commit -m "Generated chart for release"
+          git push
+        if: steps.check_changes.outputs.changes == 'true'


In this step and the one below, I feel like the condition should be placed after the name for readability instead of at the end.

oraNod · 2025-01-09T16:27:34Z

.github/workflows/release.yml

+        uses: helm/[email protected]
+        env:
+          CR_TOKEN: "${{ secrets.GITHUB_TOKEN }}"
+        if: steps.check_changes.outputs.changes == 'true'


What about also adding the setup-helm action to do some validation? That should allow a task to run: helm lint charts/* maybe also helm template charts/* if that would be useful?

miles-w-3 changed the title ~~Add release logic - updated pr for posterity~~ Add release logic Jan 4, 2025

miles-w-3 requested a review from oraNod January 4, 2025 04:34

miles-w-3 mentioned this pull request Jan 4, 2025

Helm chart release process #30

Open

oraNod mentioned this pull request Jan 6, 2025

Bump helm/kind-action from 1.11.0 to 1.12.0 in the dependencies group #34

Merged

oraNod reviewed Jan 6, 2025

View reviewed changes

miles-w-3 force-pushed the feature/chart-releaser-to-main branch from b8a62f5 to f715b8b Compare January 9, 2025 02:15

miles-w-3 force-pushed the feature/chart-releaser-to-main branch 3 times, most recently from fc1d4a0 to 50f55dc Compare January 9, 2025 03:34

Add release logic

7bf6463

Signed-off-by: Miles Wilson <[email protected]>

miles-w-3 force-pushed the feature/chart-releaser-to-main branch from 50f55dc to 7bf6463 Compare January 9, 2025 03:36

oraNod reviewed Jan 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add release logic #35

Add release logic #35

miles-w-3 commented Jan 4, 2025 •

edited

Loading

oraNod left a comment

oraNod Jan 6, 2025

miles-w-3 Jan 9, 2025

oraNod Jan 6, 2025

miles-w-3 Jan 9, 2025

oraNod Jan 9, 2025

oraNod Jan 6, 2025

miles-w-3 Jan 9, 2025

miles-w-3 commented Jan 9, 2025

oraNod left a comment

oraNod Jan 9, 2025

oraNod Jan 9, 2025

oraNod Jan 9, 2025

oraNod Jan 9, 2025

oraNod Jan 9, 2025

oraNod Jan 9, 2025

oraNod Jan 9, 2025

oraNod Jan 9, 2025

oraNod Jan 9, 2025

oraNod Jan 9, 2025

oraNod Jan 9, 2025

		run: \|
		git status --porcelain charts/ \| grep -q "." && echo "changes=true" >> $GITHUB_OUTPUT \|\| echo "changes=false" >> $GITHUB_OUTPUT

Add release logic #35

Are you sure you want to change the base?

Add release logic #35

Conversation

miles-w-3 commented Jan 4, 2025 • edited Loading

A snapshot of what I did:

Create my pages branch

Create configs in main

Remaining design decisions

Blockers for merge

oraNod left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miles-w-3 commented Jan 9, 2025

oraNod left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miles-w-3 commented Jan 4, 2025 •

edited

Loading