-
Notifications
You must be signed in to change notification settings - Fork 993
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
use the values from matrix.json in plugin react tests #9881
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: tasks doesn'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… |
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.
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.
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 <[email protected]>
595de55
to
e650849
Compare
No description provided.