Refactor PR checks

[edoardopirovano]

This PR does a refactoring of how our PR checks work to try and make it a bit easier to understand the different checks and to ensure we get good coverage of different versions in all our tests. In particular, each check is now specified in an individual YAML file in the pr-checks/checks folder. This file has a name and description of the check, as well as the steps that are unique to that check. If the check is not designed to run with all supported versions of the CLI and/or operating systems, this is specified by fields in this YAML file as well.

A script pr-checks/sync.py reads in all these test specification files and builds up a workflow using all of them, inserting some common steps at the start of each check and matrixing each one over the combination of CLIs and/or operating systems that they support. If the test specifications claims to support everything, we use the following:

• CLI: 2.3.1 (oldest supported release), 2.4.6, 2.5.9, toolcache version, latest release, latest nightly.
• OS: Ubuntu, MacOS, Windows

This is similar to what the VS code extension tests against (recent versions and latest patch version of old minor versions), however instead of testing against the newest version of the 2.3 series we test against specifically 2.3.1 since it is the oldest version that the Action claims support for. We also don't test against anything in the 2.2 series since the Action does not support this.

A new PR check verifies that the generated workflows are in sync with the YAML files specifying the various checks. This check together with some other checks that do not fit the generating model (in particular, unit tests and tests of the runner) are in a separate workflow. All integration tests of the Action have been migrated to being generated.

The required checks will not pass on this PR because they have changed name - it will require force merging and updating the branch protection rules afterwards.

[adityasharad]

Thanks for taking this on! A few initial thoughts below, and I'd like to spend some time looking over it together to see if we can make things simpler.

• I like the idea of factoring out composite step actions, and think we could go even further to use them to share code between the various check jobs.
• I think there is room to split out a few more workflows rather than have everything in a single PR checks workflow. The Runner tests for example could live on their own. Possibly the integration tests too. This makes life much easier if you have to re-run a workflow due to transient errors.
• I'm not yet convinced by the added complexity of generating the PR check workflow from specifications -- my preference would be to use first-class Actions features (like composite steps) wherever we can, but not have to create our own specification layers on top of it, since the total number of checks we're dealing with here is not that large.

[aeisenberg]

I would be leaning towards @adityasharad's suggestion that we try to avoid using a generator if possible. They are complex to maintain and it might be that we can create individual workflows for each of the generated actions.

That being said, I see 2 advantages with the generator:

1. A uniform way to declare matrix builds
2. Flexibility to add more complex, shared steps in the future (though, since the action is already fairly mature, I think there is a low likelihood of this needing to happen).

[aeisenberg]

I see this workflow is run on PRs to v1 branch, but the original workflow is not. Should they be the same?

[edoardopirovano]

Aha, yes, I think we can just omit this bit and run on all PRs like the original workflow does.

[edoardopirovano]

Thanks for the comments both, I've refactored the shared code into a step that the different checks can use.

I've also changed to having a workflow for each test rather than a workflow containing all of them so that things can be rerun individually in the case of transient errors.

I think there is some benefit to using a generator (which is now a bit simpler after the above changes). In particular:

1. It avoids repeating some boilerplate at the start of each check in which we'd have to specify the branches/PRs it runs on and some environment variables like GITHUB_TOKEN that we need in all the tests.
2. It provides a single place to update which versions of the CLI we are testing against by default (whilst still allowing some tests to override this).
3. Having a generator makes it easier to change how we run the tests. For instance, the change above to having a workflow per test instead of a workflow with all of them was fairly trivial to implement with just a few tweaks to the generator.

[aeisenberg]

Nice.

Next step is to change the required checks so that they match with the new names.

And all currently open PRs will need to be rebased.

[adityasharad]

Curious: does ruamel give us more functionality than the default pyyaml package, and are its load/dump functions equivalent to yaml.safe_load/dump?

[edoardopirovano]

I went for ruamel rather than pyyaml because pyyaml outputted multi-line strings (which we have a lot of since we use them for workflow steps that have more than one line) in a terrible way. In particular, instead of using | and then using line breaks, it would make a one-line string by replacing the line breaks with \n, i.e. instead of

steps: |
  line1
  line2

it would do:

steps: "line1\nline2"

This led to some very difficult to read workflows. I tried for a bit to get the right behaviour out of pyyaml but eventually gave up and decided to use ruamel instead.

---

Aha, a very good point. ruamel's load is actually equivalent to load rather than safe_load. We need to add a typ='safe' to get it to behave like safe_load.

[edoardopirovano]

Actually, scrap that last point, the default rt ("round-trip") loader is forked from the safe loader so is also safe (see here), and is needed to preserve line-breaks like we want.