ci(*): merge build action #548

Mossaka · 2024-04-03T21:50:42Z

this commit merges the build step in the release action and the one in the ci action since they share a large overlap code.

the build step not only builds the binary, but signs it, and upload as action artifacts. this step is now running for every ci run, including PR and release pipeline.

this commit merges the build step in the release action and the one in the ci action since they share a large overlap code. the build step not only builds the binary, but signs it, and upload as action artifacts. this step is now running for every ci run, including PR and release pipeline. Signed-off-by: jiaxiao zhou <[email protected]>

jsturtevant · 2024-04-03T23:15:01Z

Are there implications for signing the binary every PR?

jsturtevant · 2024-04-03T23:23:50Z

.github/workflows/release.yml

+    uses: ./.github/workflows/action-build.yml
+    with:
+      os: "ubuntu-22.04"
+      runtime: ${{ matrix.runtime }}


there will be times when this value won't be common as the check in the action is expecting

Sorry, can you elaborate your concerns here?

This value in the release.yml will be something like oci-tar-builder

runwasi/.github/workflows/release.yml

Lines 66 to 69 in 8c98100

const crate = '${{ inputs.crate }}';

let runtime = crate.replace(/^containerd-shim-/, '');

core.setOutput('runtime', runtime);

const non_shim_crates = ['wasm', 'wasm-test-modules', 'oci-tar-builder'];

and I think signing will fail in the new signing step at https://github.com/containerd/runwasi/pull/548/files#diff-43035769cde349b680ebf72f18e67b049e730d0d0ec91174e6455a685b3c2accR61-R64

Good point! I have modified the logic of runtime_sub step to replace the name of non-shim crate to common in this commit: 61dc1ee

Mossaka · 2024-04-03T23:23:57Z

Are there implications for signing the binary every PR?

Since we are signing the artifacts that we publish on every build, I can imagine increased security and potentially degraded performance and increased complexity of managing keys.

Although stated in the release doc for how to verify the binaries, we probably want to make that even clearer by copying the exact verification command in the release doc everytime we release signed artifacts.

I am not sure if anyone would want to verify build artifacts for each PR / commit, but if they want, it's do-able.

.github/workflows/release.yml

jsturtevant · 2024-04-03T23:27:15Z

.github/workflows/action-test-smoke.yml

@@ -22,13 +22,13 @@ jobs:
      - name: Download artifacts
        uses: actions/download-artifact@master
        with:
-          name: containerd-shim-${{ inputs.runtime }}-linux-musl
+          name: containerd-shim-${{ inputs.runtime }}-x86_64-linux-musl


interesting, I don't see where this arch was being included in the inputs.runtime before. Or is this extra field new?

Before this PR, only the release pipeline runs both x86_64 and aarch64. The ci does not have a matrix for different archs. I added that to make the build job on par with the one in the release CI.

Signed-off-by: jiaxiao zhou <[email protected]>

marosset · 2024-04-05T16:18:35Z

If you add # yaml-language-server: $schema=https://json.schemastore.org/github-action.json to the top of all the github action files you'll get schema validation in a lot of editors like vs code

marosset · 2024-04-05T16:25:45Z

.github/workflows/action-test-smoke.yml

          path: dist
      - name: Unpack artifats
        shell: bash
        run: |
          mkdir -p dist/bin
-          tar -xzf dist/containerd-shim-${{ inputs.runtime }}-linux-musl.tar.gz -C dist/bin
+          tar -xzf dist/containerd-shim-${{ inputs.runtime }}-x86_64-linux-musl.tar.gz -C dist/bin


nit: Maybe make this a makefile target since the tar command is repeated in multiple workflows?

Mossaka · 2024-04-05T20:11:51Z

Are there implications for signing the binary every PR?

Now that I thought more about the implications, and realized that I missed one of the biggest implication in my previous comment - signing requires feature branches to be pushed to upstream, because they require id-token: write permission which will not grant to public forked repos (the max permission for id-token is read for forked repos, so even though you specified write permission, the pipeline won't grant you the context you want).

I am okay to put a condition on the signing to differentiate build / release pipelines so we can disable signing on build. Wdyt?

this commit makes the signing step optional in the build-action and then disable signing required in the build CI. it is only necessary for releasing the artifacts see more discussion on #548 (comment) Signed-off-by: jiaxiao zhou <[email protected]>

this commit replaces the name of non-shim runtime to "common" in the runtime_sub step under the release pipeline because "common" is used in action-build action to determine artifact signing, packaging and uploading. Signed-off-by: jiaxiao zhou <[email protected]>

.github/workflows/action-build.yml

Signed-off-by: jiaxiao zhou <[email protected]>

Mossaka · 2024-04-15T19:54:08Z

Could you please this PR again? @cpuguy83 @devigned 🙏

devigned

lgtm!

Nice work, @Mossaka!

Mossaka · 2024-04-15T23:14:21Z

Going to merge this now and any follow up can be addressed later

Mossaka mentioned this pull request Apr 3, 2024

ci(*): merge build action #546

Closed

Mossaka requested a review from jsturtevant April 3, 2024 23:13

jsturtevant reviewed Apr 3, 2024

View reviewed changes

.github/workflows/release.yml Outdated Show resolved Hide resolved

jsturtevant reviewed Apr 3, 2024

View reviewed changes

Mossaka added 5 commits April 3, 2024 23:33

ci(*): add arch to slug

8007d8b

Signed-off-by: jiaxiao zhou <[email protected]>

disable image signing on windows

8741d1e

Signed-off-by: jiaxiao zhou <[email protected]>

add id-token permission to ci

b205f19

Signed-off-by: jiaxiao zhou <[email protected]>

use the right artifact name for downloading

50df349

Signed-off-by: jiaxiao zhou <[email protected]>

remove permission for write contents in build action

2c0eaa5

Signed-off-by: jiaxiao zhou <[email protected]>

Mossaka force-pushed the opt-release branch from c449c7c to 2c0eaa5 Compare April 3, 2024 23:35

Mossaka mentioned this pull request Apr 5, 2024

ci(*): automate the version bump process #549

Closed

marosset reviewed Apr 5, 2024

View reviewed changes

Mossaka added 2 commits April 7, 2024 22:01

Mossaka force-pushed the opt-release branch from 2fc4053 to 61dc1ee Compare April 7, 2024 22:10

Mossaka closed this Apr 7, 2024

Mossaka reopened this Apr 7, 2024

cpuguy83 requested changes Apr 8, 2024

View reviewed changes

.github/workflows/action-build.yml Outdated Show resolved Hide resolved

.github/workflows/action-build.yml Show resolved Hide resolved

Mossaka added 2 commits April 11, 2024 01:48

feat(ci): use annotations for logs

df5f1f1

Signed-off-by: jiaxiao zhou <[email protected]>

feat(ci): extract signing to it's own action

9599c79

Signed-off-by: jiaxiao zhou <[email protected]>

Mossaka closed this Apr 11, 2024

Mossaka reopened this Apr 11, 2024

Mossaka requested review from cpuguy83 and devigned April 12, 2024 01:17

devigned approved these changes Apr 15, 2024

View reviewed changes

devigned enabled auto-merge (squash) April 15, 2024 20:23

Mossaka disabled auto-merge April 15, 2024 23:14

Mossaka merged commit 5f3317f into main Apr 15, 2024
80 of 85 checks passed

Mossaka deleted the opt-release branch April 15, 2024 23:14

Mossaka mentioned this pull request May 22, 2024

fix(release): syntax error in runtime_sub step #603

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci(*): merge build action #548

ci(*): merge build action #548

Mossaka commented Apr 3, 2024

jsturtevant commented Apr 3, 2024

jsturtevant Apr 3, 2024

Mossaka Apr 3, 2024

jsturtevant Apr 3, 2024

Mossaka Apr 7, 2024 •

edited

Loading

Mossaka commented Apr 3, 2024

jsturtevant Apr 3, 2024

Mossaka Apr 3, 2024

marosset commented Apr 5, 2024

marosset Apr 5, 2024

Mossaka commented Apr 5, 2024

Mossaka commented Apr 15, 2024

devigned left a comment

Mossaka commented Apr 15, 2024

	const crate = '${{ inputs.crate }}';
	let runtime = crate.replace(/^containerd-shim-/, '');
	core.setOutput('runtime', runtime);
	const non_shim_crates = ['wasm', 'wasm-test-modules', 'oci-tar-builder'];

ci(*): merge build action #548

ci(*): merge build action #548

Conversation

Mossaka commented Apr 3, 2024

jsturtevant commented Apr 3, 2024

jsturtevant Apr 3, 2024

Choose a reason for hiding this comment

Mossaka Apr 3, 2024

Choose a reason for hiding this comment

jsturtevant Apr 3, 2024

Choose a reason for hiding this comment

Mossaka Apr 7, 2024 • edited Loading

Choose a reason for hiding this comment

Mossaka commented Apr 3, 2024

jsturtevant Apr 3, 2024

Choose a reason for hiding this comment

Mossaka Apr 3, 2024

Choose a reason for hiding this comment

marosset commented Apr 5, 2024

marosset Apr 5, 2024

Choose a reason for hiding this comment

Mossaka commented Apr 5, 2024

Mossaka commented Apr 15, 2024

devigned left a comment

Choose a reason for hiding this comment

Mossaka commented Apr 15, 2024

Mossaka Apr 7, 2024 •

edited

Loading