Skip to content

parallelize tests on GHA #9921

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.

Sign up for GitHub

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

Jump to bottom
Merged
evgeni merged 1 commit into from
Nov 24, 2023
Merged

parallelize tests on GHA #9921

evgeni merged 1 commit into from
Nov 24, 2023

Conversation

@evgeni
Copy link
Member

@evgeni evgeni commented Nov 24, 2023 •
edited
Loading

alt to #9920 and #9919

@evgeni evgeni mentioned this pull request Nov 24, 2023
Closed
evgeni
evgeni commented Nov 24, 2023
View reviewed changes
.github/workflows/foreman.yml
ruby: ${{ fromJson(needs.setup_matrix.outputs.matrix).ruby }}
node: ${{ fromJson(needs.setup_matrix.outputs.matrix).node }}
postgresql: ${{ fromJson(needs.setup_matrix.outputs.matrix).postgresql }}
task:
Copy link
Member Author

@evgeni evgeni Nov 24, 2023

Choose a reason for hiding this comment

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

I really wish GH would allow include to create a new axis…

Copy link
Member Author

@evgeni evgeni Nov 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I wonder if gha-matrix should gain a base-matrix param, which is used a base and then matrix.json is joined therein?

>>> import json
>>> base_matrix_input = '{"task":["a","b","c"]}'
>>> matrix_json = '{"ruby":["2.7","3.0"]}'
>>> matrix = json.loads(base_matrix_input)
>>> matrix.update(json.loads(matrix_json))
>>> matrix
{'task': ['a', 'b', 'c'], 'ruby': ['2.7', '3.0']}

ekohl
ekohl reviewed Nov 24, 2023
View reviewed changes
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Right now we set up node as well, but I don't think it's actually needed for these tasks. It is for some though, which we should add. Kind of related to what I started in theforeman/actions#18.

For foreman.git it'll be simple: we can assume that package.json is present. For plugins we can't, but your approach here leads to a thought: how would the matrix look if we take tasks and their dependencies into account. Would it be easier, or only add more complexity?

@ekohl
Copy link
Member

ekohl commented Nov 24, 2023

Another thought: in gha-puppet we have a dummy task at the end which we can add as a required step to verify the whole matrix. Today we don't have the Jenkins task as a required step, but we could add that. Having a single workflow makes that easier so I too prefer this approach to the other ones.

@evgeni
Copy link
Member Author

evgeni commented Nov 24, 2023

Right now we set up node as well, but I don't think it's actually needed for these tasks. It is for some though, which we should add. Kind of related to what I started in theforeman/actions#18.

For foreman.git it'll be simple: we can assume that package.json is present. For plugins we can't, but your approach here leads to a thought: how would the matrix look if we take tasks and their dependencies into account. Would it be easier, or only add more complexity?

Is the question "How do we avoid the cost of setting up Node and installing NPM dependencies for the branches that do not test JS things (= all of the existing ones, as we do not call webpack:compile and test:integration)?" OR "How do we avoid that cost for plugins that do not have Webpack/JS at all"?
The former requires knowledge about the dependencies of the task while the later requires knowledge about the dependencies of the plugin.
I don't think we can easily map task dependencies in the matrix, without it getting too wild. (We can hard code a few common task names, but not sure it's worth it)
For the plugin deps, going by package.json is probably a good thing as you did in actions#18.

@evgeni
Copy link
Member Author

evgeni commented Nov 24, 2023 •
edited
Loading

Another thought: in gha-puppet we have a dummy task at the end which we can add as a required step to verify the whole matrix. Today we don't have the Jenkins task as a required step, but we could add that. Having a single workflow makes that easier so I too prefer this approach to the other ones.

I can take a stab at this, let's see if GHA likes colons in ids.

Edit: no colons needs, phew

@ekohl
Copy link
Member

ekohl commented Nov 24, 2023

We could introduce rake tasks that fail if no package-lock.json file is present (or some other marker). IMHO it shouldn't call npm install because in CI you may want to ensure a cache, but at least be explicit about dependencies.

@evgeni evgeni force-pushed the parallel-workflow-alt-2 branch from cc05c9c to c855df2 Compare November 24, 2023 11:24
@evgeni
Copy link
Member Author

evgeni commented Nov 24, 2023

Looks like what you wanted?

image

ekohl
ekohl reviewed Nov 24, 2023
View reviewed changes
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I think we should include the task in the Gemfile.lock filename. It's likely it'll be exactly the same in each task, but not guaranteed. Other than that, 👍 on this.

I debated dropping Node and making these ruby-only tests, but that's for a follow up.

@evgeni evgeni force-pushed the parallel-workflow-alt-2 branch from c855df2 to e672559 Compare November 24, 2023 12:29
@evgeni
Copy link
Member Author

evgeni commented Nov 24, 2023 •
edited
Loading

Error: Artifact name is not valid: Gemfile-test:units-ruby-2.7-node-14-pg-12.lock. Contains the following character: Colon :

Meh

I've taken it out again. GH has no good way to do string substitution inside their templating language :/

@evgeni evgeni force-pushed the parallel-workflow-alt-2 branch from e672559 to 4a6ddc9 Compare November 24, 2023 12:35
@ekohl
Copy link
Member

ekohl commented Nov 24, 2023

I've taken it out again. GH has no good way to do string substitution inside their templating language :/

"yay"

@evgeni
Copy link
Member Author

evgeni commented Nov 24, 2023

But it also produces only one artifact:

image

@ekohl
Copy link
Member

ekohl commented Nov 24, 2023

I suspect you each one overwrites the other. https://github.com/theforeman/foreman/actions/runs/6980908724?pr=9923 does produce multiple, but that only has a simple definition right now.

@evgeni
Copy link
Member Author

evgeni commented Nov 24, 2023

Probably. Shall we merge as-is?

ekohl
ekohl previously approved these changes Nov 24, 2023
View reviewed changes
@evgeni evgeni force-pushed the parallel-workflow-alt-2 branch from 4a6ddc9 to 9778ba0 Compare November 24, 2023 13:32
@evgeni evgeni merged commit 1cfbb45 into theforeman:develop Nov 24, 2023
@evgeni evgeni deleted the parallel-workflow-alt-2 branch November 24, 2023 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ekohl ekohl ekohl left review comments

Assignees

No one assigned

Labels

Needs re-review Waiting on contributor

Projects

No open projects
EL 9 support
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@evgeni @ekohl