PR check generator: add excludeOsAndVersionCombination
#2350
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
angelapwen
force-pushed
the
angelapwen/add-exclude-pr-check-param
branch
from
62fb0a9
to
9ff3492
Compare
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.
angelapwen
force-pushed
the
angelapwen/add-exclude-pr-check-param
branch
from
9ff3492
to
462c756
Compare
henrymercer
previously approved these changes
Jun 24, 2024
pr-checks/sync.py
Outdated
| 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
pr-checks/sync.py
Outdated
|
|
||
| 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.
There was a problem hiding this comment.
Good point, I actually misordered it myself when testing initially 😆
henrymercer
reviewed
Jun 24, 2024
pr-checks/sync.py
Outdated
| @@ -27,6 +27,12 @@ | |||
| "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):
henrymercer
approved these changes
Jun 24, 2024
|
Updated the branch protection rules accordingly for |
This was referenced Jun 28, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
excludeOsAndVersionCombinationparameter 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, andnightly-latestare 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