-
Notifications
You must be signed in to change notification settings - Fork 286
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
Check issue for ZenHub release. #7427
Conversation
Build files for 92c4c3d have been deleted. |
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.
Testing approval condition to run job
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.
Thanks @tofumatt – this seems to do the trick but needs some reworking. See below.
- name: npm install | ||
run: | | ||
npm ci |
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 takes too long for this check to be useful. Installing all npm dependencies takes multiple minutes and somewhat defeats the purpose of the check since we can easily check and set this faster than this step will take.
We should be able to rework the script a little to avoid requiring added dependencies, since we're not really doing anything crazy there. A native fetch
would probably work just fine which we can get by setting up with Node 18+ which should be fine since we don't need any of our normal dependencies where this version would be more significant.
I took a quick look to see if there was a generic action for making Graph QL API requests that we could use here as well – and there is – but there're all geared at the GitHub API or doing some generic tests. We could of course see about creating our own custom action for this too but it seems a bit overkill for a single simple request.
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.
Arguably the point of the check is in case we forget, because we only set this after we've approved the PR, but that's still a fair point.
I'll see about a non-npm-install option 🙂
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.
In this case, since we're just using a few packages that can have a stable version, we can cache node_modules
directly and install just them. By doing this we can run the check in 28 seconds:
I don't think we'll get much faster, so if that isn't fast enough we either need our own action or to nix this issue 😅
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 think we'll get much faster, so if that isn't fast enough we either need our own action or to nix this issue 😅
I was thinking we can do without the dependencies altogether 😄 I've opened an alternative in #7725
This should be pretty much instant since it only needs to checkout and setup node. LMKWYT!
Co-authored-by: Evan Mattson <[email protected]>
…/site-kit-wp into release-github-actions-check-5554.
Closed in favour of #7725. 👍🏻 |
Summary
Addresses issue:
Relevant technical choices
@aaemnnosttv: I'm not sure if the
ZENHUB_GQL_API_TOKEN
secret is set yet; I tested this locally with my own ZenHub API token. Please make sure to add it as a secret before approving and testing the action 🙂I set up the action only to run when the PR is approved, because otherwise PRs that aren't yet attached to an issue (the default state until the reviewer sets them to be merged) will fail and show an X. That's going to be a misleading sign to developers working on the PR—PRs should fail when the tests fail, but not because we haven't assigned a release yet.
This way, you'll only see the reminder after you review a PR and mark it as approved 😄
Also: I omitted the "checkbox" check mentioned in the IB, because it's a bit too much—it's more complicated, but also I don't think it's required to have us check the checkbox :laugh:
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist