Skip to content
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.

Sign up for GitHub

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

Jump to bottom

PROD-29281: Replace Mergeable with a custom tool for label and title enforcement #3862

Merged
merged 6 commits into from
Apr 23, 2024
Merged

PROD-29281: Replace Mergeable with a custom tool for label and title enforcement #3862

merged 6 commits into from
Apr 23, 2024

Conversation

Kingdutch
Copy link
Member

@Kingdutch Kingdutch commented Apr 14, 2024 •
edited
Loading

Problem

The message that Mergeable adds is usually not very helpful and it can be difficult to figure out without opening the workflows what needs to be changed. Additionally Mergeable doesn’t really provide us with many ways to extend its capabilities and build on top of it.

Solution

In we’ve found that we can use GitHub Scripts to create a tool that can post and update comments on a PR to provide more targetted and helpful feedback.

Issue tracker

https://getopensocial.atlassian.net/browse/PROD-29281

Theme issue tracker

How to test

  • Create a PR with different combinations of missing labels, a missing milestone and malformed title

The business logic of the PR manager is tested using Jest, (see *.test.js) we assume that GitHub's API works as described and don't have tests that talk to the REST API directly.

Definition of done

Before merge

  • Code/peer review is completed
  • All commit messages are clear and clean. If applicable a rebase was performed
  • All automated tests are green
  • Functional/manual tests of the acceptance criteria are approved
  • All acceptance criteria were met
  • New features or changes to existing features are covered by tests, either unit (preferably) or behat
  • Update path is tested. New hook_updates should respect update order, right naming convention and consider hook_post_update code
  • Module can be safely uninstalled. Update/implement hook_uninstall and make sure that removed configuration or dependencies are removed/uninstalled
  • This pull request has all required labels (team/type/priority)
  • This pull request has a milestone
  • This pull request has an assignee (if applicable)
  • Any front end changes are tested on all major browsers
  • New UI elements, or changes on UI elements are approved by the design team
  • New features, or feature changes are approved by the product owner

After merge

  • Code is tested on all branches that it has been cherry-picked
  • Update hook number might need adjustment, make sure they have the correct order
  • The Drupal.org ticket(s) are updated according to this pull request status

Screenshots

When nothing is provided.

The feedback message posted by the PR Manager on a pull request without any labels or milestone.

Duplicate labels or a title mixing the Drupal and Jira format

image

Release notes

Pull Requests will no longer be validated by Mergeable and instead use a newly introduced custom built JavaScript solution under the name "PR Manager". It aims to provide more helpful messages in case PRs violate the requirements we set.

One additional change is that PRs for Jira tickets should no longer start with Issue # but should start with the Jira ticket directly.

Change Record

Translations

@Kingdutch Kingdutch added status: needs review This pull request is waiting for a requested review prio: medium type: repository Improvements to working with the repository (e.g. templates, README files, or workflows) team: guardians labels Apr 14, 2024
@Kingdutch Kingdutch added this to the 13.0.0 milestone Apr 14, 2024
@mergeable Mergeable
Copy link

mergeable bot commented Apr 14, 2024

Thanks for contributing towards Open Social! A maintainer from the @goalgorilla/maintainers group might not review all changes from all teams/contributors. Please don't be discouraged if it takes a while. In the meantime, we have some automated checks running and it might be that you will see our comments with some tips or requests to speed up the review process. 😊

@Kingdutch Kingdutch force-pushed the internal/PROD-29281-replace-mergeable branch 2 times, most recently from cfcea92 to 6746c28 Compare April 14, 2024 09:16
@Kingdutch Kingdutch mentioned this pull request Apr 14, 2024
Closed
17 tasks
@Kingdutch Kingdutch force-pushed the internal/PROD-29281-replace-mergeable branch from 6746c28 to 2b707f9 Compare April 14, 2024 09:47
ronaldtebrake
ronaldtebrake approved these changes Apr 22, 2024
View reviewed changes
Copy link
Contributor

@ronaldtebrake ronaldtebrake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one!

@Kingdutch
Add the initial version of the PR Manager
ee7fea6 
The initial version of the PR Manager will provide feedback on
incorrectly labeled pull requests.

We've created a GitHub application that will receive an authenticated
`octokit` instance (named `github` in our script) when it's executed by
GitHub script actions. We've added a simple markdown parser which can
help us find links and other required items in the pull request body.

The script will provide feedback based on the errors it finds, creating
a new comment if one doesn't exist, updating an existing one if there is
one and removing a created comment once all issues are fixed.
@Kingdutch
Ignore dependabot in PR Manager
c3aec26 
We currently ignore dependabot in Mergeable and we port that behaviour
to the PR manager until we decide differently.
@Kingdutch
Add PR title validation to PR manager
5a46ae1 
This was the only validation that Mergeable still had that the PR
manager was missing.

We've also changed it slightly so that we get rid of `Issue #PROD` which
is now very prolific but which is pushing Drupal standards onto our Jira
issues. We provide a specific message for that case to guide people to
the new format.

The benefit of making sure we properly distinguish is that other tools
looking at titles know whether to check Jira or Drupal.org for the
source of truth.
@Kingdutch
Remove mergeable configuration
1ec9c45 
The final step is to uninstall the app but this completes the
replacement of mergeable by our own JavaScript based tool for which we
can provide more friendly messaging.
@Kingdutch
Ensure PR Manager tests are run in CI
b533ab5
@Kingdutch
Make duplication messages consistent
5c2bd54 
We were using "Duplicate" in one and "Multiple X" in the other. We now
have them all consistently showing "multiple x" to aid recognizability
for the users.
@Kingdutch Kingdutch force-pushed the internal/PROD-29281-replace-mergeable branch from c0428e7 to 5c2bd54 Compare April 22, 2024 07:23
@Kingdutch Kingdutch merged commit 4c00ae8 into main Apr 23, 2024
191 checks passed
@Kingdutch Kingdutch deleted the internal/PROD-29281-replace-mergeable branch April 23, 2024 07:40
@ronaldtebrake ronaldtebrake removed this from the 13.0.0 milestone Apr 23, 2024
@ronaldtebrake ronaldtebrake added this to the 13.0.0-alpha1 milestone Apr 23, 2024
@Kingdutch Kingdutch added the scope: PR manager This change is limited in scope to Open Social's PR manager. label Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ronaldtebrake ronaldtebrake ronaldtebrake approved these changes

@bramtenhove bramtenhove Awaiting requested review from bramtenhove

Assignees
No one assigned
Labels
prio: medium scope: PR manager This change is limited in scope to Open Social's PR manager. status: needs review This pull request is waiting for a requested review team: guardians type: repository Improvements to working with the repository (e.g. templates, README files, or workflows)
Milestone
13.0.0-alpha1
Development

Successfully merging this pull request may close these issues.
2 participants
@Kingdutch @ronaldtebrake