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

use the values from matrix.json in plugin react tests #9881

Merged
merged 1 commit into from
Oct 31, 2023

Conversation

evgeni
Copy link
Member

@evgeni evgeni commented Oct 27, 2023

No description provided.

@evgeni evgeni requested a review from a team as a code owner October 27, 2023 07:05
@evgeni evgeni force-pushed the load-matrix-in-plugin-react branch 2 times, most recently from 6115392 to 3622535 Compare October 27, 2023 07:14
@evgeni
Copy link
Member Author

evgeni commented Oct 27, 2023

Why does this only trigger the "last" entry in the include matrix…

@evgeni evgeni marked this pull request as draft October 27, 2023 07:16
@evgeni evgeni force-pushed the load-matrix-in-plugin-react branch from 3622535 to c2b308f Compare October 27, 2023 07:18
@evgeni
Copy link
Member Author

evgeni commented Oct 27, 2023

According to the logs, it's correct:

##[debug]..=> 'matrix'
##[debug]=> '{"ruby": ["2.7"], "node": ["14"], "include": [{"repo": "foreman-tasks", "org": "theforeman"}, {"repo": "katello", "org": "Katello"}]}'
##[debug]Result: '{"ruby": ["2.7"], "node": ["14"], "include": [{"repo": "foreman-tasks", "org": "theforeman"}, {"repo": "katello", "org": "Katello"}]}'

@evgeni evgeni force-pushed the load-matrix-in-plugin-react branch 2 times, most recently from 7ea2708 to 547302c Compare October 27, 2023 07:40
@evgeni evgeni force-pushed the load-matrix-in-plugin-react branch 3 times, most recently from aab4866 to 4ab18e5 Compare October 30, 2023 18:03
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"}]
Copy link
Member

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.

@evgeni
Copy link
Member Author

evgeni commented Oct 30, 2023

so turns out, include on an existing matrix, does overwrite things if it can (see https://docs.github.com/en/actions/using-jobs/using-a-matrix-for-your-jobs#expanding-or-adding-matrix-configurations), so my intent to have

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.

@evgeni evgeni force-pushed the load-matrix-in-plugin-react branch 2 times, most recently from 362fd3b to 1d45f5b Compare October 30, 2023 18:33
@evgeni
Copy link
Member Author

evgeni commented Oct 30, 2023

well, this at least works…

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.

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.

.github/workflows/plugins_react_tests.yml Outdated Show resolved Hide resolved
@evgeni
Copy link
Member Author

evgeni commented Oct 31, 2023

Given we read the matrix multiple times, any thoughts on converging to a single large ci.yaml file?

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.
But preferably not in this PR.

@evgeni evgeni force-pushed the load-matrix-in-plugin-react branch 3 times, most recently from 6a3039b to 6357e10 Compare October 31, 2023 06:30
@evgeni
Copy link
Member Author

evgeni commented Oct 31, 2023

@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"
Copy link
Member Author

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.

Copy link
Member

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:

Suggested change
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.

Copy link
Member Author

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.

Copy link
Member

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

Copy link
Member Author

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

Copy link
Member

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
Copy link
Member

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?..

Copy link
Member Author

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.

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.

From testing this looks OK. Just a few small things.

.github/workflows/plugins_react_tests.yml Outdated Show resolved Hide resolved
.github/workflows/plugins_react_tests.yml Outdated Show resolved Hide resolved
ekohl
ekohl previously approved these changes Oct 31, 2023
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.

This now looks good to me.

@evgeni evgeni marked this pull request as ready for review October 31, 2023 16:12
@evgeni evgeni force-pushed the load-matrix-in-plugin-react branch from 595de55 to e650849 Compare October 31, 2023 16:40
@evgeni evgeni merged commit a84793d into theforeman:develop Oct 31, 2023
7 of 9 checks passed
@evgeni evgeni deleted the load-matrix-in-plugin-react branch October 31, 2023 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants