search

argoproj / argo-workflows

Workflow Engine for Kubernetes
https://argo-workflows.readthedocs.io/
Apache License 2.0
14.43k stars 3.09k forks source link

ci: optimise image building #13027

Closed Joibel closed 1 week ago

Joibel commented 3 weeks ago

Build the argoexec and argocli images in a matrix github action.

Download and install the images into k3d in one action each.

This commit will require changes to the Required steps in github which I cannot do myself. I'm not sure why that one argoexec-image is listed as required at all, it's a dependency of testing rather than a proper part of the test or CI.

Also updates upload and download artifact actions to latest v4, which seem to be working OK now. v3 will deprecate by November. edit by agilgur5: this was to support the merge-multiple param per below

agilgur5 commented 3 weeks ago

Follow-up to #13018 / https://github.com/argoproj/argo-workflows/pull/13018#discussion_r1594188099

This commit will require changes to the Required steps in github which I cannot do myself.

Neither can I. Only @terrytangyuan and @sarabala1979 can change the required steps in the branch protection rules.

I'm not sure why that one argoexec-image is listed as required at all, it's a dependency of testing rather than a proper part of the test or CI.

All of them were required prior to #12006. Technically speaking, the image build could have a different set of changed files than E2Es although it is currently unconditional. Making sure it passes is also safer just in case -- PRs shouldn't break image builds.

Also updates upload and download artifact actions to latest v4, which seem to be working OK now. v3 will deprecate by November.

We have a follow-up issue for this #12455 where there were some lingering problems last we checked. There's also one more place that uses upload-artifact v3, the docs build. And if working we should remove the dependabot ignore as well. So I'd probably suggest splitting that into a separate PR more generally

Joibel commented 2 weeks ago

I've reverted upload+download artifact actions again.

Joibel commented 2 weeks ago

I've reverted upload+download artifact actions again.

merge-multiple doesn't work in v3. The docs are not versioned, so it's hard to discover how stuff should work. So that's part of why I'd updated, I remember now.

Also, I'm talking to myself again.

agilgur5 commented 2 weeks ago

merge-multiple doesn't work in v3. The docs are not versioned, so it's hard to discover how stuff should work. So that's part of why I'd updated, I remember now.

Ah that makes more sense. I edited your PR description to mention that.

It seems like "download all" is available if you don't specify name and use a directory.

But let's keep v4 regardless so we don't have to change that again in the future

Joibel commented 1 week ago

@terrytangyuan or @sarabala1979, this commit changes the name of a CI step. The ci step argoexec-image is in the required list, and this replaces it with the matrix steps: image I assume these are called argo-images (argoexec) and argo-images (argocli), but maybe the required can just be argo-images

This will need updating. This PR doesn't actually change anything that happens in the CI, that happened in #13018

terrytangyuan commented 1 week ago

Merged

terrytangyuan commented 1 week ago

Updated settings too