ci: optimise image building

[Joibel]

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]

Suggested change

	name: ${{matrix.image}}_image.tar
	name: ${{matrix.image}}

From the previous code, this should just be the name of the image. Not sure if this was a copy+paste typo?

[Joibel]

It's deliberate. We're pattern matching this with pattern: '*_image.tar' on line 164. This pattern would be '*' without the extra parts to the name.

[agilgur5]

Yes, I thought that would suffice, no? Or were you trying to be extra careful in case at some point other artifacts were added or something?

[agilgur5]

Follow-up to #13018 / #13018 (comment)

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]

I've reverted upload+download artifact actions again.

[Joibel]

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]

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]

@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:

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]

Merged

[terrytangyuan]

Updated settings too

[agilgur5]

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

Annnd that appears to be what we're using here since pattern is invalid in actions/download-artifact@v3.

This is getting a warning in CI for every E2E test in the matrix:

[E2E Tests (test-executor, minimal)](https://github.com/argoproj/argo-workflows/actions/runs/9249013492/job/25442227788#step:8:1)
Unexpected input(s) 'pattern', valid inputs are ['name', 'path']

But passes since "no name" == "download all"