Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make matrix script process any matrix items #11446

Merged
merged 3 commits into from
Mar 16, 2022

Conversation

nineinchnick
Copy link
Member

@nineinchnick nineinchnick commented Mar 13, 2022

Description

When #10323 introduced a script to filter out non-impacted modules from the the CI test matrix, this script was intentionally very simple and only handled the currently existing matrix. This PR extends it to:

  • allow processing any keys in the include section, not just currently used ones
  • process any matrix items, instead of only processing the include section

This allows future contributors to adjust the CI matrix as they see fit, without having to figure out how it's different than the regular GHA matrix.

Is this change a fix, improvement, new feature, refactoring, or other?
CI tools improvement

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)
CI scripts

How would you describe this change to a non-technical end user or system administrator?
n/a

Related issues, pull requests, and links

Documentation

(x) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

(x) No release notes entries required.
( ) Release notes entries required with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Is it necessary to expand the matrix? Can we just passthrough the matrix as it is and let GHA handle the expansion?

e.g. in current impl we don't correctly handle include where it matches an already existing entry in the matrix. We append instead of overriding exisitng entry in this impl.

i.e. would it work if we just converted the matrix.yaml into a matrix.json and let it be?

@nineinchnick
Copy link
Member Author

Question: Is it necessary to expand the matrix? Can we just passthrough the matrix as it is and let GHA handle the expansion?

e.g. in current impl we don't correctly handle include where it matches an already existing entry in the matrix. We append instead of overriding exisitng entry in this impl.

i.e. would it work if we just converted the matrix.yaml into a matrix.json and let it be?

Yes, that'll work, we just have to filter by modules in both the matrix and include sections. Thanks for pointing this out, I'm also not very confident that our matrix expansion would work exactly the same as GHA does it.

@nineinchnick nineinchnick force-pushed the generify-matrix-script branch from 1856b57 to df0b7b4 Compare March 14, 2022 09:15
@hashhar
Copy link
Member

hashhar commented Mar 14, 2022

@nineinchnick Can you "test" by modifying the matrix to use in a commit:

  • include that overwrites existing entry
  • include that adds new entry
  • exclude that removes an entry
  • standalone entry in matrix without include

The impl looks good to me % the case where includes overrides an existing module (a test would confirm).

@nineinchnick nineinchnick force-pushed the generify-matrix-script branch from df0b7b4 to ed9410f Compare March 15, 2022 09:03
@nineinchnick
Copy link
Member Author

@nineinchnick Can you "test" by modifying the matrix to use in a commit:

The impl looks good to me % the case where includes overrides an existing module (a test would confirm).

I added tests in the script itself, PTAL. I'm not sure what you mean with overrides though.

@nineinchnick nineinchnick changed the title Make matrix script process any matrix items and expand matrix Make matrix script process any matrix items Mar 15, 2022
Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@hashhar
Copy link
Member

hashhar commented Mar 15, 2022

I'm not sure what you mean with overrides though.

I meant the case you added for # include entry overwrites one in matrix.

@nineinchnick nineinchnick force-pushed the generify-matrix-script branch from ed9410f to 3f72391 Compare March 15, 2022 14:57
@hashhar hashhar merged commit 64686a8 into trinodb:master Mar 16, 2022
@github-actions github-actions bot added this to the 374 milestone Mar 16, 2022
@nineinchnick nineinchnick deleted the generify-matrix-script branch March 16, 2022 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants