-
Notifications
You must be signed in to change notification settings - Fork 1k
use the values from matrix.json in plugin react tests #9881
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
Conversation
6115392 to
3622535
Compare
|
Why does this only trigger the "last" entry in the include matrix… |
3622535 to
c2b308f
Compare
|
According to the logs, it's correct: |
7ea2708 to
547302c
Compare
aab4866 to
4ab18e5
Compare
| run: | | ||
| curl --silent --show-error --fail --output matrix.json https://raw.githubusercontent.com/theforeman/foreman/${{ github.base_ref }}/.github/matrix.json | ||
| cat >matrix_include.json <<EOF | ||
| [{"repo": "foreman-tasks", "org": "theforeman", "ruby": "2.7"}, {"repo": "katello", "org": "Katello"}] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think include won't do here. Isn't this essentially adding another vector to the matrix?
Perhaps it would also be easier if we simply had repo as fully qualified. So theforeman/foreman-tasks and Katello/katello. For the exact paths it doesn't matter and makes other things easier.
|
so turns out, matrix:
ruby: 2.7
node: 14
include:
- repo: katello
- repo: tasksdoesn't work, as the last entry in the include list wins. this behaviour is different from when you have only the include statement (see https://docs.github.com/en/actions/using-jobs/using-a-matrix-for-your-jobs#example-adding-configurations) this is apparently totally intentional and expected. |
362fd3b to
1d45f5b
Compare
|
well, this at least works… |
ekohl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given we read the matrix multiple times, any thoughts on converging to a single large ci.yaml file? Also keeping #9871 in mind.
I've been thinking about moving all Foreman & Foreman plugins CI tests to GHA and write up an RFC for that.
Certainly an optimization we can think of. Ritght now it wouldn't work as the triggers are different for the existing workflows (they filter on different paths), but we could question whether this is required or whether we should "always" run those. |
6a3039b to
6357e10
Compare
|
@ekohl needs theforeman/gha-matrix-builder#1 to work :) |
| id: build_matrix | ||
| uses: ekohl/gha-matrix-builder@v0 | ||
| with: | ||
| base_matrix_url: "https://raw.githubusercontent.com/${{ github.repository }}/${{ github.base_ref }}/.github/matrix.json" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't get it to run against my fork.
github.repository points at the target, so my branch name (github.head_ref) doesn't resolve here.
Maybe github.sha would work? But IMHO something we can fix later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird, in my testing this worked:
| base_matrix_url: "https://raw.githubusercontent.com/${{ github.repository }}/${{ github.base_ref }}/.github/matrix.json" | |
| base_matrix_url: "https://raw.githubusercontent.com/${{ github.repository }}/${{ github.ref_name }}/.github/matrix.json" |
But maybe I was only testing on push in my own repo so I didn't notice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ref_name for this pr is 9881, I tried.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://docs.github.com/en/actions/learn-github-actions/variables#default-environment-variables is a bit ambiguous. Perhaps this needs to be extracted (using non-trivial logic) from the PR. I'm more and more leaning to using actions/checkout by using https://github.com/actions/checkout#fetch-only-a-single-file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, this is also my preference, I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've opened theforeman/gha-matrix-builder#2 to start trying this out.
| path: ./projects/${{ matrix.repo }} | ||
| - name: Generate ${{ matrix.repo }} npm dependencies package-lock | ||
| repository: ${{ matrix.plugin }} | ||
| path: ${{ github.workspace }}/projects/plugin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a stupid question, but shouldn't be plugin a variable here?..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you mean {{ matrix.plugin }}? that contains a slash now and I kinda didn't want to find out which parts of the GHA Octopus do and which do not understand how to create folders correctly.
Given we're only testing one plugin at a time, there is nothing that prevents us from having it in a common folder.
ekohl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From testing this looks OK. Just a few small things.
ekohl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This now looks good to me.
Co-authored-by: Ewoud Kohl van Wijngaarden <ewoud@kohlvanwijngaarden.nl>
595de55 to
e650849
Compare
No description provided.