Skip to content
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.

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

Improve GitHub Actions #329

Closed

Conversation

itskingori
Copy link

@itskingori itskingori commented Oct 16, 2024

Thought I'd help improve things a little bit, basically:

Reviewing

Follow the commits for an easier review, each commit is an idea.

Motivations

I wanted linux/arm64 support on Docker Hub but saw that it's just linux/amd64 (though it's on GItHub Container Registry). Found these two related issues & PRs trying to understand the current status:

Initially, I found the current organisation of GitHub Actions across multiple different workflows confusing because it wasn't clear which ones one is test or release jobs.

My belief is that most jobs can be categorised along those two broad categories. So this PR is a very subjective view of how I think it should be i.e. different triggers for test and release jobs on just two workflows then those that trigger the relevant jobs.

Preview

Screenshot 2024-10-16 at 19 59 27

References

@itskingori itskingori force-pushed the improve_github_actions branch 2 times, most recently from 498110c to 75b30c9 Compare October 16, 2024 17:56
@danihodovic
Copy link
Owner

cc @adinhodovic

@itskingori itskingori force-pushed the improve_github_actions branch from 75b30c9 to 8106c50 Compare October 16, 2024 18:05
@itskingori itskingori force-pushed the improve_github_actions branch from 8106c50 to d75f703 Compare October 16, 2024 18:13
@itskingori itskingori force-pushed the improve_github_actions branch from d75f703 to 353ce08 Compare October 16, 2024 18:19
@itskingori itskingori marked this pull request as ready for review October 16, 2024 18:22
Copy link
Author

@itskingori itskingori left a comment

Choose a reason for hiding this comment

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

I've updated the description to give more context. I am willing to make modifications and explain any part that isn't clear.

Copy link
Author

@itskingori itskingori left a comment

Choose a reason for hiding this comment

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

Some commentary for even more context ... 💬

Comment on lines 9 to 22
github:
runs-on: ubuntu-latest
steps:
- name: Checkout
uses: actions/checkout@v4
- name: Create Release
uses: actions/create-release@v1
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
with:
tag_name: ${{ github.ref }}
release_name: Version ${{ github.ref }}
draft: false
prerelease: false
Copy link
Author

Choose a reason for hiding this comment

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

This is new. It creates GitHub release after a tag is created. To note, the other release jobs are dependent on this on since they have needs: github set.

Comment on lines +49 to +50
username: ${{ secrets.DOCKER_HUB_USERNAME }}
password: ${{ secrets.DOCKER_HUB_TOKEN }}
Copy link
Author

Choose a reason for hiding this comment

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

You'll need to set DOCKER_HUB_USERNAME and DOCKER_HUB_TOKEN as GitHub Actions secrets.

Comment on lines 57 to 66
- name: Build, tag, and push image to Docker Hub
uses: docker/build-push-action@v6
with:
cache-from: type=gha
cache-to: type=gha,mode=max
context: .
labels: ${{ steps.metadata.outputs.labels }}
platforms: linux/amd64,linux/arm64
push: true
tags: ${{ steps.metadata.outputs.tags }}
Copy link
Author

Choose a reason for hiding this comment

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

The idea here is the metadata action will create tags for the multiple images set here and since we're authenticated into both registries (GitHub Container Registry & Docker Hub) at this point, both pushes should work.

on:
push:
tags:
- '[0-9]+.[0-9]+.[0-9]+'
Copy link
Author

Choose a reason for hiding this comment

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

To note, I removed the preceding v for better flexibility in the jobs. This way, in the downstream jobs you can manually prefix where applicable rather than assume it will be everywhere (which it isn't e.g. Docker image tags).

Comment on lines +67 to +72
- name: Update Description On Docker Hub Description
uses: peter-evans/dockerhub-description@v4
with:
username: ${{ secrets.DOCKER_HUB_USERNAME }}
password: ${{ secrets.DOCKER_HUB_TOKEN }}
repository: ${{ env.IMAGE }}
Copy link
Author

Choose a reason for hiding this comment

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

Automates the overview on Docker Hub, which is currently empty ...

Screenshot 2024-10-16 at 20 40 46

Comment on lines +1 to +11
name: Test

on:
push:
branches:
- master
pull_request:
branches:
- master

jobs:
Copy link
Author

Choose a reason for hiding this comment

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

With all these test jobs as part of one workflow it will look great. Obviously right now the workflow isn't approved so it isn't running but it should look something like this ...

Screenshot 2024-10-16 at 19 59 27

Copy link
Author

@itskingori itskingori left a comment

Choose a reason for hiding this comment

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

Updated PR description and updated release workflow to have the Helm Chart packaged and uploaded as an assets of the release instead of having two releases (one for the app and another for the chart).

Warning

It's worth noting that the release is untested as it only runs when a tag happens. I've done my best to review the logic and I am willing to fix any issues if it fails.

Comment on lines +92 to +103
- name: Get Chart Version
id: chart-version
run: echo "::set-output name=version::$(grep 'version:' ./charts/celery-exporter/Chart.yaml | cut -d ' ' -f 2)"
- name: Package Helm chart
run: helm package ./charts/celery-exporter --app-version=${{ github.ref_name }}
- name: Upload To Github Release
uses: softprops/action-gh-release@v2
with:
files: |
celery-exporter-${{ steps.chart-version.outputs.version }}.tgz
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
Copy link
Author

Choose a reason for hiding this comment

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

Instead of creating a release for the Helm Chart like this ...

- name: Run chart-releaser
uses: helm/[email protected]
env:
CR_TOKEN: "${{ secrets.GITHUB_TOKEN }}"
CR_RELEASE_NAME_TEMPLATE: "{{ .Name }}-chart-{{ .Version }}"

Have it uploaded as an asset to the GitHub release (on tag), because each time we tag we'll get two releases which can be cleaner, here's what we have now ...

Screenshot 2024-10-17 at 04 50 49

version: v3.14.4
- name: Get Chart Version
id: chart-version
run: echo "::set-output name=version::$(grep 'version:' ./charts/celery-exporter/Chart.yaml | cut -d ' ' -f 2)"
Copy link
Author

Choose a reason for hiding this comment

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

Basic test that this works ...

$ ls -l ./charts/* 
total 24
-rw-r--r--   1 Kingori  staff   333 Oct 17 04:22 Chart.yaml
-rw-r--r--   1 Kingori  staff  2790 Oct 16 18:57 README.md
drwxr-xr-x   3 Kingori  staff    96 Oct 16 18:57 ci
drwxr-xr-x  10 Kingori  staff   320 Oct 16 18:57 templates
-rw-r--r--   1 Kingori  staff  2640 Oct 16 18:57 values.yaml

$ grep 'version:' ./charts/celery-exporter/Chart.yaml | cut -d ' ' -f 2
0.7.0

id: chart-version
run: echo "::set-output name=version::$(grep 'version:' ./charts/celery-exporter/Chart.yaml | cut -d ' ' -f 2)"
- name: Package Helm chart
run: helm package ./charts/celery-exporter --app-version=${{ github.ref_name }}
Copy link
Author

Choose a reason for hiding this comment

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

Local test that this works ...

$ helm package ./charts/celery-exporter --app-version=0.10.13          
Successfully packaged chart and saved it to: /Users/Kingori/Repositories/itskingori/celery-exporter/celery-exporter-0.7.0.tgz

uses: softprops/action-gh-release@v2
with:
files: |
celery-exporter-${{ steps.chart-version.outputs.version }}.tgz
Copy link
Author

Choose a reason for hiding this comment

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

Should be available after packaging ...

$ helm package ./charts/celery-exporter --app-version=0.10.13          
Successfully packaged chart and saved it to: /Users/Kingori/Repositories/itskingori/celery-exporter/celery-exporter-0.7.0.tgz

$ ls -l celery-exporter-0.7.0.tgz 
-rw-r--r--  1 Kingori  staff  4981 Oct 17 04:53 celery-exporter-0.7.0.tgz

Comment on lines 15 to 17

version: 0.7.0
appVersion: 0.9.2
Copy link
Author

Choose a reason for hiding this comment

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

Preferring to set the app-version dynamically i.e. with the --app-version flag available on helm package ...

$ helm package --help | grep '\-version'
      --app-version string       set the appVersion on the chart to this version
      --version string           set the version on the chart to this semver version

@danihodovic
Copy link
Owner

This will take some time to review

@itskingori
Copy link
Author

No problem. I'll keep an eye out for notifications whenever you get round to looking at it.

version: 0.7.0
version: 0.7.1
Copy link
Author

Choose a reason for hiding this comment

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

Bumped because the test/helm-chart job is failing due no version bump detected despite the removal of appVersion in b697d34 ...

Screenshot 2024-10-17 at 21 03 46

@@ -12,6 +12,4 @@ sources:
maintainers:
- name: danihodovic
- name: adinhodovic

version: 0.7.0
appVersion: 0.9.2

Choose a reason for hiding this comment

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

Did we intend to remove this line?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, in preference to dynamically setting it as done below with the --app-version flag ...

run: helm package ./charts/celery-exporter --app-version=${{ github.ref_name }}

As it should track the app version which is only determined during the actual release.

@itskingori
Copy link
Author

itskingori commented Nov 19, 2024

I should have probably filed an issue to confirm if the scope of my suggestions is acceptable (and reviewable). Been waiting a bit for this but now opting to package my own image in Intellection/docker-celery-exporter as it's urgent ATM.

These ideas have been implemented in the below PRs:

@danihodovic
Copy link
Owner

I should have probably filed an issue to confirm if the scope of my suggestions is acceptable (and reviewable). Been waiting a bit for this but now opting to package my own image in Intellection/docker-celery-exporter as it's urgent ATM.

Sounds good. I still haven't gotten around to looking at the change as work has been hectic and I release a version of celery-exporter about once a month on average.

@itskingori
Copy link
Author

I understand. My changes were also not easily digestible. I kinda struggled with needing approvals for the workflows to run each time I pushed, so I can test.

Feel free to have a look at the workflows in Intellection/docker-celery-exporter (and copy if you want, MIT licensed), they pretty much just check out the code here and do what I was aiming to do in totality (at least on the Docker build front).

Got an ARM based build finally, so I'm unblocked for now ...

$ docker buildx imagetools inspect zappi/celery-exporter:0.10.14
Name:      docker.io/zappi/celery-exporter:0.10.14
MediaType: application/vnd.oci.image.index.v1+json
Digest:    sha256:2226d248f957af47d47476315c8dd83d5ace3796470d96b90870bba286b00549

Manifests:
  Name:        docker.io/zappi/celery-exporter:0.10.14@sha256:1c514308647461b97d1630372842140e562e3101eaf20639c3703f61385c9804
  MediaType:   application/vnd.oci.image.manifest.v1+json
  Platform:    linux/amd64

  Name:        docker.io/zappi/celery-exporter:0.10.14@sha256:400845b8bbf89fcaf4b61ca709420a0aba807964397ecaecd4c88da3369bcf51
  MediaType:   application/vnd.oci.image.manifest.v1+json
  Platform:    linux/arm64

  Name:        docker.io/zappi/celery-exporter:0.10.14@sha256:0c89f89db4b97b37bc46f49580430932053f98740bdab4323d6ec86bd9cb30a6
  MediaType:   application/vnd.oci.image.manifest.v1+json
  Platform:    unknown/unknown
  Annotations:
    vnd.docker.reference.digest: sha256:1c514308647461b97d1630372842140e562e3101eaf20639c3703f61385c9804
    vnd.docker.reference.type:   attestation-manifest

  Name:        docker.io/zappi/celery-exporter:0.10.14@sha256:dc4e1cdee61c8ff571a907488eada65db5d681092fb1191487025e8784e9ade3
  MediaType:   application/vnd.oci.image.manifest.v1+json
  Platform:    unknown/unknown
  Annotations:
    vnd.docker.reference.digest: sha256:400845b8bbf89fcaf4b61ca709420a0aba807964397ecaecd4c88da3369bcf51
    vnd.docker.reference.type:   attestation-manifest

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.

multi platform - ARM64 release Add version tagged Docker image
3 participants