-
-
Notifications
You must be signed in to change notification settings - Fork 832
GHA: make element-web and cypress workflows reusable #10969
Conversation
9618bb1
to
7a32452
Compare
... so that we can call it from the js-sdk repo
... so that we can call it from js-sdk
7a32452
to
2603ab9
Compare
react-sdk-repository: | ||
type: string | ||
required: true | ||
description: "The name of the github repository to check out and build." |
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.
Surely this could be driven by just choosing that repository to workflow_call an action upon?
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.
Sorry, I don't understand?
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.
To be clear: I don't believe there is any way to figure out which repo this workflow file has been sourced from, from within the workflow.
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.
Sure, and each repo/fork could just hardcode their own repo - https://github.com/matrix-org/matrix-react-sdk/blob/develop/.github/workflows/tests.yml#LL36C21-L36C21
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.
we've discussed this at some length in the chat, but to summarise my position:
One way or the other, we have to hardcode matrix-org/matrix-react-sdk
twice. We can either:
- Put it in the calling workflow twice (as matrix-js-sdk#3412 does), or:
- Put it once in the calling workflow, and once in the target workflow.
There are a couple of approaches to the latter, but whichever approach we take, my contention is that, given the need for duplication, it's better that the duplicates be next to each other than at a distance. Primarily, doing so makes it much clearer what needs to be changed when setting up a fork.
Looking at this from the PoV of the calling workflow, we either have:
-
@t3chguy's proposal:
jobs: build-element-web: uses: matrix-org/matrix-react-sdk/.github/workflows/element-web.yaml@HEAD with: matrix-js-sdk-sha: ${{ github.sha }}
It is unclear to me, looking at that incantation, that even if I change the reference in
uses
, it will still build the react-sdk frommatrix-org/matrix-react-sdk
. -
@richvdh's proposal:
jobs: build-element-web: uses: matrix-org/matrix-react-sdk/.github/workflows/element-web.yaml@HEAD with: matrix-js-sdk-sha: ${{ github.sha }} react-sdk-repository: matrix-org/matrix-react-sdk
Yes there is duplication there, but it saves hardcoding the repository in the target workflow, and it is pretty obvious what I need to change if I want to use a fork of the react-sdk.
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.
a third approach is to split out into a composite action which the js-sdk and other callers can call, and it'll have access to its own repository via github.action_repository
where no duplication or hard-coding is needed
react-sdk-repository: | ||
type: string | ||
required: true | ||
description: "The name of the github repository to check out and build." |
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.
Same as above
Co-authored-by: Michael Telatynski <7t3chguy@gmail.com>
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.
The repetition Rich proposes looks almost 100% harmless, and the code looks good otherwise.
Part of element-hq/element-web#25313.
The idea here is to allow us to fire off a Cypress test run against a rust-crypto-enabled Element Web R after each js-sdk PR. These need to run in the context of a js-sdk PR, so we could just copy-paste the workflows into the js-sdk; however a more pleasant alternative is to make the existing workflows re-usable.
This change is marked as an internal change (Task), so will not be included in the changelog.