-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
feat: run GHA build job locally with act #23959
Conversation
dd0602b
to
98487fc
Compare
I usually use Makefiles although left it out of this project due to the number of Windows users. We face this challenge:
So @lmilbaum is there any strong benefit from this? |
6bee32f
to
c135ef8
Compare
@rarkins Thank you for your feedback. I'm testing something with the PR. It is in draft. Can we review its value or/and alternative options in a later stage? |
0937501
to
d583fb7
Compare
a2ffb29
to
532dc83
Compare
- name: Install pnpm | ||
uses: pnpm/[email protected] | ||
with: |
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.
why this is required?
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.
Setup node uses pnpm but the corepack is not enabled yet so the binary is missing.
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.
then enable corepack instead
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.
It is installed with the node setup
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, but corepack is also available if nodejs is already available
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.
Do we want to merge this before then? That PR is conflicted and waiting for someone to resume it and then hopefully the maintainers to merge it
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 do think it has value in merging it. Waiting for a general approval and then I'll rebase.
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'll need a regex manager added to keep it updated though. It's already out of date :)
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.
Done
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 would prefer to leave the corepack enable when not on act. maybe we should build our own act image to not need the huge default act image.
4dead74
to
2324171
Compare
@viceice ping |
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.
Add regex manager for updating pnpm version in action
@@ -462,6 +462,7 @@ jobs: | |||
|
|||
- name: Upload | |||
uses: actions/upload-artifact@0b7f8abb1508181956e8e162db84b466c27e18ce # v3.1.2 | |||
if: ${{ !env.ACT }} |
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.
? 🤔
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 step should not be enabled when running the pipeline with act
- name: Install pnpm | ||
uses: pnpm/[email protected] | ||
with: |
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 would prefer to leave the corepack enable when not on act. maybe we should build our own act image to not need the huge default act image.
|
||
- name: Setup Node | ||
uses: actions/setup-node@5e21ff4d9bc1a8cf6de233a3057d20ec6b3fb69d # v3.8.1 | ||
with: | ||
node-version: ${{ inputs.node-version }} | ||
cache: ${{ env.CACHE_HIT != 'true' && 'pnpm' || '' }} | ||
|
||
- name: Enable corepack | ||
shell: bash | ||
run: corepack enable |
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 will probably not work when there is a pnpm in place
## Run GitHub Action build job locally with act | ||
|
||
1. Install act - [installation instructions](https://github.com/nektos/act#installation) | ||
1. Run (Pick the Large image) |
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.
why do we need the large image?
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 don't remember now why the large image is required. I do remember that the smaller image doesn't work.
Can this be moved forward to merging or should it be closed? |
Thanks @rarkins for reminding me of this PR. Closing. |
Changes
Context
Documentation (please check one with an [x])
How I've tested my work (please select one)
I have verified these changes via: