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

[CI] Use fromJson to dynamically define native-tests matrix #13124

Merged
merged 3 commits into from
Nov 10, 2020

Conversation

zakkak
Copy link
Contributor

@zakkak zakkak commented Nov 4, 2020

Hello,

Today I learned that one can "dynamically" define a github action matrix using fromJson. This could replace #10763 with the following pros and cons.

Pros:

  • Workflows from other repositories (e.g. Mandrel) can fetch the native-tests.json and get the correct matrix for the Quarkus version they want to test without having to change the local workflow.
  • In maven profiles we still need to edit the workflow when a new "category" is added to the matrix, with fromJson this is no longer needed making much more flexible.

Cons

Alternatives

  1. There is nothing preventing us from combining the maven profiles with fromJson but it feels like an overkill. In this case the json file would define the groups and the corresponding timeout, while the maven profiles would define the tests belonging in each group.

  2. Another approach (combining the two) would be to not use a static json file and create one on the fly by parsing integration-tests/pom.xml and figuring out the available groups of tests (IT-* maven profiles), this way we would need to have a single timeout for all groups though and we would need to move to using -Ddocker for the services.

WDYT?

@zakkak
Copy link
Contributor Author

zakkak commented Nov 5, 2020

The Native-Tests Misc3 failure is interesting 👀
image

The same commit worked fine on my fork https://github.com/zakkak/quarkus/runs/1355280544

@ghost ghost added the area/infra-automation anything related to CI, bots, etc. that are used to automated our infrastructure label Nov 10, 2020
@zakkak
Copy link
Contributor Author

zakkak commented Nov 10, 2020

@gsmet @gastaldi I know that this is not even close to being a priority and that it's kind of hard to review but if it is something we would eventually want I think it would better be merged before 1.10 (to avoid exposing the IT-* profiles and then reverting them), so please have at least a high-level look before 1.10.

Thanks

@gastaldi gastaldi requested a review from gsmet November 10, 2020 13:33
@gsmet
Copy link
Member

gsmet commented Nov 10, 2020

I think it's an interesting idea if you can exploit it in the Mandrel build.

TBH I prefer this to the profile approach you used.

@zakkak
Copy link
Contributor Author

zakkak commented Nov 10, 2020

Please let me know if there is any way I can format the json file (or something else) to ease the review process for you since this requires going through the tests one by one and ensuring everything is still there :)

@gsmet
Copy link
Member

gsmet commented Nov 10, 2020

If you confirm this is your final PR, I'll do a thorough check that things are OK.

Just don't want to do it if you're in the process of changing things :).

@zakkak
Copy link
Contributor Author

zakkak commented Nov 10, 2020

@gsmet I just did a second pass to make sure everything looks OK. No further changes are expected so you may go ahead and review it :)

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

I checked the config file and also checked several builds and things looks good to me.

@gsmet gsmet merged commit 5086b2d into quarkusio:master Nov 10, 2020
@ghost ghost added this to the 1.11 - master milestone Nov 10, 2020
@zakkak zakkak deleted the ci-with-json branch November 10, 2020 15:54
@gsmet gsmet modified the milestones: 1.11 - master, 1.10.0.Final Nov 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/infra-automation anything related to CI, bots, etc. that are used to automated our infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants