-
Notifications
You must be signed in to change notification settings - Fork 67
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
Add custom GitHub action and pull request template to enforce that documentation is updated #1694
Conversation
|
✅ Deploy Preview for compiled-css-in-js canceled.
|
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 didn't exactly validate the logic of the regex's and all that, but generally looks good to me! Some minor, very minor suggestions to freely ignore around logging mostly.
.github/workflows/task-checklist.yml
Outdated
steps: | ||
- uses: actions/checkout@v4 | ||
- name: Install dependencies | ||
run: cd .github/actions/task-checklist && yarn install |
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 feel like you can just put this into .github/actions/task-checklist/action.yml
, but … it's been 2 years since I build Github Actions a lot and I don't have any examples in repos I have access to nowadays; doesn't really matter.
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.
had a quick look into this - doesn't look like javascript actions support calling shell commands out of the box
composite actions seems like a potential way to make this possible, will try it out
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.
Yeah, that was it, composite actions…you do you though, no dramas here, it's clear what you're doing.
on: | ||
check_run: | ||
pull_request: | ||
types: [opened, edited] |
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.
These types make sense to me! Good job here.
'False positive? Insert <!-- task-checklist-ignore-start --> and <!-- task-checklist-ignore-end --> in the section(s) of your PR where you want to skip the check.' | ||
); | ||
console.log('However, with great power comes great responsibility...'); | ||
console.log('---'); |
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 the suggestion is to use core.info
instead of console.log
and core.debug
for anything you want hidden behind the debug flag.
I'd suggest changing console.log
=> core.info
, but to be honest I can't tell you the difference.
} | ||
|
||
console.log('---'); | ||
console.log( |
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.
Could be a core.warning
if you care at all.
const run = () => { | ||
const body = github.context.payload.pull_request?.body; | ||
if (!body) { | ||
console.log('PR description empty, skipping this check.'); |
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.
Could be a core.warning
or even core.notice
(to annotate) if you care at all.
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.
ah looks like the core
functions https://github.com/actions/toolkit/tree/main/packages/core#annotations have better integrations with GitHub's UI, good idea
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.
Yeah, I just don't exactly know what core.info
does over console.log
, but the rest is helpful!
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.
Atlassian NPM mirror
@@ -0,0 +1 @@ | |||
registry="https://registry.yarnpkg.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.
ensure we don't accidentally use atlassian NPM mirror
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.
Just looking at changes since last review, I think you covered it (and the NPM issue), LGTM.
Added a pull request template with a checklist that asks that documentation be updated for every PR.
This is enforced by a custom GitHub action that checks whether any tasks in the checklist are marked as incomplete. If any are incomplete, the GitHub action will give an error.
You can exempt parts the PR description from the incomplete task check by adding
<!-- task-checklist-ignore-start -->
and<!-- task-checklist-ignore-end -->
around the sections where you want to disable the check.Test it out here!
Alternative methods considered for finding incomplete tasks in the PR description: