Migrate CI from drone to GHA #383

ericpromislow · 2024-05-23T19:08:16Z

Related to rancher/rancher#45231

Actual PR at #394 (in order to access secrets for the publish and release actions)

tomleb

I've only been able to review the CI workflow and the change in the scripts.

Overall I think the approach of using k3d to start a cluster and Helm to start rancher is great.

Couple of things:

There's a lot of variables spread out in multiple scripts. I think it would be worth it to centralize them in a place to make it easy to change them. For example, the k3d version is in the k3d scripts, some versions are in the ci.yaml file, etc.
I'd get rid of the default values for most environment variables and just fail if they are unset. This way we avoid having things work with default (potentially stale) values if we remove the override by mistake.
The .hide-drone.yml file should be deleted instead of hidden.
Some scripts duplicate work (eg: scripts/package and scripts/package-ci). Fwiw, Dapper makes it difficult (it doesn't solve that in any way), and I'm slowly working on an alternative which hopefully would prevent this..

.github/workflows/ci.yaml

scripts/version

scripts/package-for-ci

scripts/ci

scripts/integration-test

tomleb · 2024-05-25T03:54:30Z

charts/rancher-webhook/Chart.yaml

@@ -11,4 +11,4 @@ annotations:
  catalog.cattle.io/os: linux
  catalog.cattle.io/permits-os: linux,windows
  catalog.cattle.io/rancher-version: ">= 2.9.0-0 < 2.10.0-0"
-  catalog.cattle.io/kube-version: ">= 1.23.0-0 < 1.29.0-0"
+  catalog.cattle.io/kube-version: ">= 1.23.0-0 < 1.30.0-0"


Imo this should be done as part of #366. The current version of webhook doesn't support 1.29 so I wouldn't expect the chart to say it does.

Same as above: this should be done as part of PR 366

@ericpromislow This is still part of the PR fyi, I still think it should be removed

scripts/integration-test

ericpromislow · 2024-05-28T03:47:12Z

The key to getting an image into the k3d cluster's registry is to not add a repository prefix to it. Just call it rancher/webhook and then when a pod controller tries to load it, it will look in the local registry first before looking at docker.io or any other configured repository.

ericpromislow · 2024-05-28T17:02:04Z

Also a note that I added set -u to most of the scripts so that they should fail when trying to access an unset variable.

MbolotSuse

Some minor issues, but overall a lot of good work so far.

package/Dockerfile

scripts/build

.github/workflows/ci.yaml

.github/workflows/publish.yaml

.github/workflows/release.yaml

MbolotSuse · 2024-06-04T18:22:29Z

.goreleaser.yaml

+release:
+  prerelease: auto
+  extra_files:
+    - glob : ./build/artifacts/*.tgz


Our current artifacts also contains sha256 files for the binaries, and a source code zip file example. Do we need to add extra patterns here to make sure this gets everything?

I'm not sure. From a debug run:

Run find dist/artifacts dist/artifacts dist/artifacts/webhook-linux-amd64 dist/artifacts/rancher-webhook-0.0.0-2bfcb08.tgz

I assume the goreleaser knows about webhook-linux-amd64 and needs to be told to save the .tgz file. And the checksum entry is based on the checksum file names in the current release.

I don't see a straightforward way to test this until we push the next release :(

The sha256 files seem to be missing from there. So I think even now we can say that something is missing. That being said, I agree that it's hard to test these things outside of a live run. Running an RC once this has merged should give you a way to test. Can you look at the sha256 issue and then we can run the live tests later after things have merged?

I'm going by the goreleaser documentation:

checksum: # You can change the name of the checksums file. # # Default: '{{ .ProjectName }}_{{ .Version }}_checksums.txt' # or, when split is set: '{{ .ArtifactName }}.{{ .Algorithm }}' # Templates: allowed name_template: "{{ .ProjectName }}_checksums.txt"

So I think that will give us what we want.

scripts/integration-test

scripts/package-for-ci

scripts/version

.github/workflows/publish.yaml

tomleb · 2024-06-05T14:54:41Z

.github/workflows/publish.yaml

+
+jobs:
+  ci :
+    uses: rancher/webhook/.github/workflows/ci.yaml@release/v0.5


Is there anyway to avoid hardcoding the branch? I suspect GHA has a variable for the "current branch" that we can use here.

github.base_ref returns the branch in ci.yaml, I'll need to verify this works in the other workloads (see Michael's comment on further testing of the other workloads still needed).

Here's what I've found about re-using a workflow that's part of the current repo. Have a look at https://docs.github.com/en/actions/using-workflows/reusing-workflows#calling-a-reusable-workflow:

If you use the second syntax option (without {owner}/{repo} and @{ref}) the called workflow is from the same commit as the caller workflow.

This would look like:

jobs: ci : uses: ./.github/workflows/ci.yml

This way no need to fiddle with any branch names/commits,etc.

tomleb · 2024-06-05T14:54:48Z

.github/workflows/release.yaml

+
+jobs:
+  ci:
+    uses: rancher/webhook/.github/workflows/ci.yaml@release/v0.5


Is there anyway to avoid hardcoding the branch? I suspect GHA has a variable for the "current branch" that we can use here.

tomleb · 2024-06-05T15:03:16Z

.goreleaser.yaml

+        - -extldflags
+        - -static
+        - -s
+        - -X main.Version={{.Version}} -X main.GitCommit={{.Commit}}


I guess my main concern is now we have 2 different binaries being built:

The one with gorelease on releases

The one with script/build

The binaries built in the latter are copied in the docker image and pushed to the container registry.

The binaries built in the former are pushed to the release/pre-release. I'm not sure what they're used for tbh, since afaik what's most important is the Helm chart (.tgz) for the charts repo and the docker image.

We're not trying to redo all our CI, so I'm not suggesting we drop the binaries from the release. However I wanna understand the reason to have two different ways to build the binaries, used in two different artifacts. Maintenance-wise, these two build mechanism will need to be kept in sync. I haven't used goreleaser before so I'd like to know how it benefits us for our use case.

There's a directive around somewhere that we should avoid storing binaries in artifacts directories for purposes of SLSA level-3 compliance, so I didn't see a way that we could test the same binaries we push to github without loading everything into one job.

Is there anything that would prevent replacing the goreleaser step with a make build step, or something like that?

I thought we want to use goreleaser because it automates more of the release process.

.github/workflows/release.yaml

.github/workflows/publish.yaml

MbolotSuse

Some small changes. Also, would be good to switch the actions to use a SHA instead of the tag to give better control to upgrades.

MbolotSuse · 2024-06-05T14:53:40Z

.github/workflows/publish.yaml

+on:
+  push:
+    tags:
+      - release/v*


This seems like it has a couple issues to me:

If the regex is trying to match a tag, I would expect this to be v* - the tags are the exact version like v0.5.1, not release/v0.5.1

Per this comment, I would expect this to need to be quoted

I don't think it needs to be quoted because the string value doesn't start with a * (in which case it would look like an anchor):

$ pbpaste | yq . { "on": { "push": { "tags": [ "release/v*" ] } } }

MbolotSuse · 2024-06-05T14:53:46Z

.github/workflows/release.yaml

+on:
+  push:
+    tags:
+      - release/v*


Similar comment here about the tag name.

ericpromislow · 2024-06-07T19:34:30Z

Actions now use SHAs

tomleb · 2024-06-11T13:43:02Z

.github/workflows/ci.yaml

+          # TODO: See wrangler for an example of how to run tests on arm64, not ready here
+          # - arm64


With wrangler supporting both, we should be able to make it work here as well.

ericpromislow · 2024-06-11T17:36:05Z

Continued at PR #394 in order to access secrets for the publish and release actions.

ericpromislow · 2024-06-14T22:26:28Z

We're going to move ahead with this PR and skip the failing test on ARM64 CI runs ... this is logged at
rancher/rancher#45837

tomleb · 2024-06-28T18:24:29Z

.goreleaser.yaml

@@ -0,0 +1,41 @@
+# Make sure to check the documentation at https://goreleaser.com


Is this still needed? I don't see anything using it.

It came from the boilerplate. I'll modify it because I like to have pointers to docs for third-party components, even when the link is obvious

I meant, do we still need goreleaser? I thought it wasn't being used anymore (the tool).

.github/workflows/ci.yaml

tests/integration/rkeMachineConfig_test.go

- Misc. changes to CI caught during further testing: 1. There should be two shell variables, HELM_VERSION for pulling down the right version of helm, and HELM_CHART_VERSION, for identifying generated helm charts. 2. Similarly, `TAG` and `IMAGE_TAG` are two different things. 3. Stop using make/dapper: env vars get lost 4. Skip the RKE Machine config unit test on CI/arm64 - Written up in issue 45837

MbolotSuse · 2024-07-02T20:45:33Z

LGTM in interest of functionality, some things we will want to follow up on.

ericpromislow requested a review from a team as a code owner May 23, 2024 19:08

ericpromislow force-pushed the gha-migration branch from 1ee8d0a to 38ec222 Compare May 25, 2024 00:27

tomleb reviewed May 25, 2024

View reviewed changes

tomleb reviewed May 27, 2024

View reviewed changes

scripts/integration-test Outdated Show resolved Hide resolved

scripts/integration-test Outdated Show resolved Hide resolved

scripts/integration-test Outdated Show resolved Hide resolved

scripts/integration-test Outdated Show resolved Hide resolved

ericpromislow force-pushed the gha-migration branch from 3c9fb13 to 3dd0c14 Compare May 27, 2024 23:49

ericpromislow requested a review from tomleb May 28, 2024 16:57

ericpromislow requested a review from a team May 28, 2024 17:41

ericpromislow force-pushed the gha-migration branch 7 times, most recently from 8a2f6cd to f71cdc3 Compare June 3, 2024 21:50

MbolotSuse reviewed Jun 4, 2024

View reviewed changes

ericpromislow force-pushed the gha-migration branch from cf68f8d to 57da271 Compare June 4, 2024 22:39

ericpromislow requested a review from MbolotSuse June 4, 2024 23:01

tomleb reviewed Jun 5, 2024

View reviewed changes

ericpromislow requested a review from tomleb June 5, 2024 21:06

MbolotSuse reviewed Jun 5, 2024

View reviewed changes

ericpromislow requested a review from MbolotSuse June 5, 2024 21:38

tomleb reviewed Jun 11, 2024

View reviewed changes

ericpromislow force-pushed the gha-migration branch from 796382b to b3c799c Compare June 11, 2024 17:21

ericpromislow mentioned this pull request Jun 11, 2024

Migrate webhook CI to GHA #394

Closed

ericpromislow closed this Jun 11, 2024

ericpromislow reopened this Jun 11, 2024

ericpromislow force-pushed the gha-migration branch 6 times, most recently from 97d6cf7 to 7b12d38 Compare June 14, 2024 22:02

ericpromislow requested a review from tomleb June 14, 2024 22:21

ericpromislow mentioned this pull request Jun 14, 2024

[BUG] Webhook GHA fails on ARM64 rancher/rancher#45837

Open

tomleb previously approved these changes Jun 14, 2024

View reviewed changes

ericpromislow dismissed tomleb’s stale review via c349706 June 17, 2024 22:14

ericpromislow force-pushed the gha-migration branch from c349706 to 53b7d94 Compare June 17, 2024 22:16

ericpromislow force-pushed the gha-migration branch 2 times, most recently from dbca647 to e8f3398 Compare June 27, 2024 21:26

ericpromislow requested a review from tomleb June 27, 2024 21:42

tomleb reviewed Jun 28, 2024

View reviewed changes

ericpromislow requested a review from tomleb June 28, 2024 19:03

ericpromislow force-pushed the gha-migration branch from e8f3398 to 8d50d8e Compare June 28, 2024 19:32

ericpromislow force-pushed the gha-migration branch from 1c57199 to 9f702b4 Compare June 28, 2024 22:47

ericpromislow added 2 commits July 2, 2024 11:37

Bring in Tom Lebreux's changes on image publishing.

198ac61

Add id-token permission for vault

91e7f18

tomleb previously approved these changes Jul 2, 2024

View reviewed changes

Pull in Tom's changes to use tag instead of docker metadata.

9ff5cce

ericpromislow dismissed tomleb’s stale review via 9ff5cce July 2, 2024 20:34

tomleb approved these changes Jul 2, 2024

View reviewed changes

MbolotSuse approved these changes Jul 2, 2024

View reviewed changes

ericpromislow merged commit 0a8155d into rancher:release/v0.5 Jul 2, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate CI from drone to GHA #383

Migrate CI from drone to GHA #383

ericpromislow commented May 23, 2024 •

edited

Loading

tomleb left a comment

tomleb May 25, 2024

ericpromislow May 28, 2024

tomleb Jun 11, 2024

ericpromislow commented May 28, 2024

ericpromislow commented May 28, 2024

MbolotSuse left a comment

MbolotSuse Jun 4, 2024

ericpromislow Jun 4, 2024

MbolotSuse Jun 5, 2024

ericpromislow Jun 5, 2024

tomleb Jun 5, 2024

ericpromislow Jun 5, 2024

tomleb Jun 6, 2024

tomleb Jun 5, 2024

ericpromislow Jun 5, 2024

tomleb Jun 5, 2024

ericpromislow Jun 5, 2024

tomleb Jun 6, 2024

ericpromislow Jun 7, 2024

MbolotSuse left a comment

MbolotSuse Jun 5, 2024

ericpromislow Jun 5, 2024 •

edited

Loading

MbolotSuse Jun 5, 2024

ericpromislow commented Jun 7, 2024

tomleb Jun 11, 2024

ericpromislow commented Jun 11, 2024 •

edited

Loading

ericpromislow commented Jun 14, 2024

tomleb Jun 28, 2024

ericpromislow Jun 28, 2024

tomleb Jun 28, 2024

MbolotSuse commented Jul 2, 2024

		# TODO: See wrangler for an example of how to run tests on arm64, not ready here
		# - arm64

		@@ -0,0 +1,41 @@
		# Make sure to check the documentation at https://goreleaser.com

Migrate CI from drone to GHA #383

Migrate CI from drone to GHA #383

Conversation

ericpromislow commented May 23, 2024 • edited Loading

tomleb 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

ericpromislow commented May 28, 2024

ericpromislow commented May 28, 2024

MbolotSuse 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MbolotSuse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericpromislow Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericpromislow commented Jun 7, 2024

Choose a reason for hiding this comment

ericpromislow commented Jun 11, 2024 • edited Loading

ericpromislow commented Jun 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MbolotSuse commented Jul 2, 2024

ericpromislow commented May 23, 2024 •

edited

Loading

ericpromislow Jun 5, 2024 •

edited

Loading

ericpromislow commented Jun 11, 2024 •

edited

Loading