-
Notifications
You must be signed in to change notification settings - Fork 213
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
Remove the "2 Reviews Needed" column automation from the PR board #4657
Conversation
Co-authored-by: Dhruv Bhanushali <[email protected]>
@dhruvkb is the reasoning for your suggestions that having 1 approving review is the same as being "APPROVED", now? I just want to make sure I understand how this is working; thanks! |
Yeah, that's exactly it @zackkrida. PRs with one approval will already have |
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.
LGTM!
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 logic looks correct now.
We could rename the column to "Needs review" instead of "Needs 1 review" for brevity.
How did the file pass the linting step in CI before? Are these files not linted?
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 logic looks correct now.
We could rename the column to "Needs review" instead of "Needs 1 review" for brevity.
How did the file pass the linting step in CI before? Are these files not linted?
The PR is failing the linting check. Do we use a different set of rules for these files?
Adding mjs
here should fix the CI error:
openverse/.pre-commit-config.yaml
Line 128 in b96fc34
files: (frontend|automations|packages/js).*?\.(js|ts|vue|json5|json)$ |
@obulat automations have their separate Prettier config file for some reason. |
Great find! I guess we need to update the pre-commit to include |
…o include mjs files
Thanks for the suggestions! I've now:
As for the actual code itself:
I'll merge once CI passes. |
Description
This PR removes JavaScript logic from the
automations/js/src/project_automation/prs.mjs
file which moves cards into the "Needs 2 Reviews" column of our PR project board.This PR also updates the entire file to conform to our prettier config.
Testing Instructions
Review the logic and hope it works!
Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin