use composite action to avoid configuring versions/downloads multiple times

[rajatjindal]

this basically move setting up of rust and other dependencies to a separate action file. (composite actions).

one downside is that we are setting up superset of dependencies, so we may install some which are not required in a particular step.

the upside is that:

• build.yml is much more readable now
• the version config is moved to a single file now (.github/actions/spin-tests-dependencies)

I've not changed release action yet to use these composite action (I can spend some more time to do that if this looks like an ok approach)

Signed-off-by: Rajat Jindal rajatjindal83@gmail.com

[itowlson]

I don't quite understand what's happening in this PR, sorry (and to be clear I think this is my ignorance rather than the PR itself!). My first reaction on reading it was that we now had three copies of the build workflow instead of one with a matrix. I do like how lots of the noise is removed from build and the centralisation of the parameters but I feel like I am not really understanding how it works or how it will be maintained. Could you share GH documentation links if those would help, or expand on how you see this working over time please? Thanks!

[rajatjindal]

there are two ways Github supports using reusing actions repeated at multiple places.

1. Reusable workflows
2. Composite actions

One major difference I observed between them is that reusable workflow runs as a job (and anything that needs this, will have to add needs: reusable-job step to wait for it to complete. This also means that they run on different Github runner. so any artifacts produced by reusable job have to to be downloaded when needed (using cache or artifacts)

with composite action, it runs as a single step in the same job, so use same github runner and so any setup done is available for next steps.

About having copying the same workflow 3 times, we can potentially have them all in same file with if conditions for OS specific steps. e,g. there will still be 3 steps of download nomad, one for each of ubuntu, macos and windows with if: ${{ runner.os == 'OS_NAME'}} conditionals

How will it work in future:

• when we have to upgrade Rust version, we update version in .github/actions/spin-test-dependencies and it will reflect everywhere we are using this action.
• About testing a specific rust version before actually making changes, I think it can work with either opening PR for new rust version OR we can add a workflow dispatch trigger which can accept a rust version and will override defaults in .github/actions/spin-test-dependencies

e.g.

      - name: setup dependencies
        uses: ./.github/actions/spin-tests-dependencies
        with:
           rust-version: ${{ github.event.input.rust-version }}

I hope it makes sense.

@vdice can you please also chime in and provide your feedback.

[itowlson]

Thanks @rajatjindal - that makes things a lot clearer. It definitely sounds like composite actions are the way to go, I think I just need to get my head around how we decompose things.

It seems like currently we take advantage of a lot of overlap between the different OSes in the build action; it would be great to see if we can maintain that and so ensure the overall structure remains the same across all targets. But I reckon I need to do some more reading to understand it.

Thanks again for the detailed explanation, it was super useful!

[rajatjindal]

Thanks @rajatjindal - that makes things a lot clearer. It definitely sounds like composite actions are the way to go, I think I just need to get my head around how we decompose things.

It seems like currently we take advantage of a lot of overlap between the different OSes in the build action; it would be great to see if we can maintain that and so ensure the overall structure remains the same across all targets. But I reckon I need to do some more reading to understand it.

is there any scenario in your mind where this may cause trouble for us in future? I would love to hear it so that we can address it while we are still early in this refactor.

[itowlson]

Mostly just thinking if we need to add and remove steps e.g. if we used a local transient OCI registry, needed to build a new SDK, installed a custom linter, etc. I guess it would be fairly obvious if we missed something / got something wrong though.

[vdice]

I see the points made in the description above (build.yml is cleaner, dependencies updated in one spot), but at the expense of cognitive overhead of the additional files (and scenarios where all three need updating, say, if a dependency download steps need to change). It seems like either approach brings different pros/cons.

However, I'm not sure if the current setup was causing any big pain points for us -- but perhaps @rajatjindal you've anticipated or experienced some, hence this PR?

I think if we do go this direction, we'd want consistency among applicable workflows to follow the same approach. So, it may be informative to see how release.yml would change.

Another item to note: With the e2e effort, were we planning on migrating all tests that required deployment platform dependencies (like bindle, nomad, hippo) into the e2e framework? Then, presumably, the corresponding downloads here in these GH action files would go away, replaced by the Dockerfile/containerized approach that the e2e framework uses... which would mean much less for the separate os-based composite actions to do and perhaps decreasing the value of their presence.

[itowlson]

@vdice It bit me when I was nailing down the Rust version - in one place Rust has been installed slightly differently so the Wasm target install didn't work correctly. The problem we have now is not necessarily any one pain point, but gradual rot as different workflows or jobs are updated in subtly different ways which work the same... until they don't. A 'death by a thousand cuts' kind of pain rather than a "I just trod on a nail" kind of pain if that makes sense...?

[rajatjindal]

I see the points made in the description above (build.yml is cleaner, dependencies updated in one spot), but at the expense of cognitive overhead of the additional files (and scenarios where all three need updating, say, if a dependency download steps need to change). It seems like either approach brings different pros/cons.

I added them to 3 different files only to keep it readable when user have a question like "my CI is failing on Windows, what are the things we are setting up as dependencies on Windows". In current setup they have to parse all the different if conditions to reach a conclusion of what all is installed on a specific OS.

Having said that, it is possible to move all of the setup to just one file (but unfortunately we can't use matrix here, however we can keep matrix in build.yml and just pass along the calculated values to the reusable composite action as inputs).

pls let me know if you prefer that. (@endocrimes also mentioned that she has opinions about this change)

However, I'm not sure if the current setup was causing any big pain points for us -- but perhaps @rajatjindal you've anticipated or experienced some, hence this PR?

repeated setup steps was the big pain point. until we move all the existing tests to new e2e-framework, we have multiple setups to include bindle, hippo etc. Also the fact that Ivan has been running into failures quite frequently triggered my interest in making the actions simple and readable and as a possible outcome more robust.

I think if we do go this direction, we'd want consistency among applicable workflows to follow the same approach. So, it may be informative to see how release.yml would change.

I've not spent time on changing release.yml with these reusable actions, but i would think it would remain more or less same (most of the steps are about how to package and release the artifacts, which are not changing as part of this PR).

Do you have any specific step in mind about release.yml?

Another item to note: With the e2e effort, were we planning on migrating all tests that required deployment platform dependencies (like bindle, nomad, hippo) into the e2e framework? Then, presumably, the corresponding downloads here in these GH action files would go away, replaced by the Dockerfile/containerized approach that the e2e framework uses... which would mean much less for the separate os-based composite actions to do and perhaps decreasing the value of their presence.

Last time I tried running Docker on macos and Windows, I ran into some issues with using containers on Windows/Macos, need to revisit this and see if that works.

[rajatjindal]

one more question I want to ask is, how do you think this is different from setting up dependencies like we do today. e.g.

      - name: Install Rust toolchain
        uses: actions-rs/toolchain@v1
        with:
          toolchain: 1.66
          default: true

vs

    - name: setup dependencies
      uses: ./.github/actions/spin-tests-dependencies
      with:
        rust-version: ${{ inputs.rust-version }}
        bindle-version: ${{ inputs.bindle-version }}
        nomad-version: ${{ inputs.nomad-version }}
        hippo-version: ${{ inputs.hippo-version }}

[vdice]

Thanks for all of the context @rajatjindal and @itowlson. I'm good with giving this a go and seeing how it works for us. The composite action does seem to be a powerful approach 👍

[endocrimes]

👋 I've been holding off giving this a review while I think it over - but I don't think this puts us in a better place than we are today.

We still need to define everything in three places - but now the produced diff when changing versions or adding a new dependency would span 3-4 files, and be less convenient to sed or find and replace in your editor.

At the very least, we should keep passing versions explicitly from the workflow.

(also setting up the cache shouldn't happen from within the nested actions - it's not used by them, it's used by the build itself).

[endocrimes]

Not every ubuntu job needs Go - we shouldn't include it in the "default" dependencies

[rajatjindal]

👋 I've been holding off giving this a review while I think it over - but I don't think this puts us in a better place than we are today.

Thank you for taking out time to provide your feedback.

We still need to define everything in three places - but now the produced diff when changing versions or adding a new dependency would span 3-4 files, and be less convenient to sed or find and replace in your editor.

the version change would be to only one file https://github.com/rajatjindal/spin/blob/reuseable-steps/.github/actions/spin-tests-dependencies/action.yml

but may be moving them all to one file will be better (as suggested by Vaughn as well)

At the very least, we should keep passing versions explicitly from the workflow.

I am thinking we should keep the versions in the reusable action. can we do something like:

• all dependencies disabled by default
• default - dependency enabled & default version specified by reusable action
• x.y.z - dependency enabled & version override

(I'll have to try this out to see if there will be some issues with this approach)

(also setting up the cache shouldn't happen from within the nested actions - it's not used by them, it's used by the build itself).

If my understanding is correct, In this case the reusable action will run in context of current workflow, and should behave similar to how it would have behaved if the step was included in the workflow directly.

[rajatjindal]

Hi @endocrimes

I've made few changes:

• Moved all setup to one file. we can't use matrix in composite action, so you would still see some steps reused with different urls
• by default all dependencies are disabled. you need to explicitly request any dependency installation
• by default (if enabled), it will install a default version of dependency. you can override the version from specific steps if needed.

If this still does not look like a reasonable change, I will go ahead and close this PR and will try to think of a better way to achieve this.

[endocrimes]

I'd probably prefer forcing consumers to choose versions, but this is looking much better 👍

[rajatjindal]

I just found out about endocrimes/setup-nomad. I can include that here and that will reduce duplication.

also we can setup similar actions for hippo and bindle. so it will make it much better than what we have now (in the PR i mean).

I will try setting up those actions tomorrow and will update PR.

[rajatjindal]

Hi @vdice @endocrimes

I have implemented actions similar to setup-nomad and used them here in the composite action.

Could u pls take a look and see if we can go ahead with this.

Kindly feel free to bring up any concerns that u might have (with new actions or otherwise)

Thanks