PR check generator: add excludeOsAndVersionCombination - #2350
angelapwen merged 4 commits into
Conversation
Sorry, something went wrong.
Now that CLI v2.17.4+ are available, we can switch this job back to `ubuntu`. As a result, we can also bring back testing on the older CLI versions (which did not work on MacOS). CLI v.2.16.6 has a known failure on Linux so we exclude it from this workflow. This change is orthogonal to the PR check generator change as the check doesn't use the generator.
henrymercer
left a comment
There was a problem hiding this comment.
Just a small suggestion to reduce the likelihood of an argument ordering mismatch
Sorry, something went wrong.
| checkSpecification = yaml.load(checkStream) | ||
|
|
||
| matrix = [] | ||
| excludedVersionsAndOses = checkSpecification.get('excludeOsAndVersionCombination', []) |
There was a problem hiding this comment.
We could rename to excludedOsesAndVersions for clarity since it's OSes first and versions second
Sorry, something went wrong.
|
|
||
| for runnerImage in runnerImagesForOs: | ||
| # Skip appending this combination to the matrix if it is explicitly excluded. | ||
| if is_version_and_os_excluded(version, operatingSystem, excludedVersionsAndOses): |
There was a problem hiding this comment.
Similarly here consider using the same order of OS and version as in the YAML.
Sorry, something went wrong.
There was a problem hiding this comment.
Good point, I actually misordered it myself when testing initially 😆
Sorry, something went wrong.
| "nightly-latest" | ||
| ] | ||
|
|
||
| def is_os_and_version_excluded(version, os, exclude_params): |
There was a problem hiding this comment.
Final suggestion: match the order of the arguments too, e.g. is_os_and_version_excluded(os, version, exclude_params):
Sorry, something went wrong.
There was a problem hiding this comment.
Good call ✨
Sorry, something went wrong.
|
Updated the branch protection rules accordingly for main, releases/v2, and releases/v3. |
Sorry, something went wrong.
We often need to exclude specific combinations of CLI versions + operating systems. Previously there was no easy way to do this: we would run all CLI versions specified against all operating systems specified. This change adds the excludeOsAndVersionCombination parameter to do just that!
To test, I re-enabled Swift on Linux for CLI v >= 2.17.4, which remained as a to-do item after we disabled those checks in #2299. Note that default, linked, and nightly-latest are all now current enough to re-enable those. The new generator script allows us to exclude all older CLI versions on Linux.
I'll update the required PR checks once this PR is approved ✅
Merge / deployment checklist