definitelytyped / dt-mergebot Goto Github PK

View Code? Open in Web Editor NEW

114.0 14.0 44.0 4.99 MB

The bot which handles auto-merging your PRs

Home Page: https://devblogs.microsoft.com/typescript/changes-to-how-we-manage-definitelytyped/

License: MIT License

JavaScript 4.94% TypeScript 95.06%

azure-functions github-bot

dt-mergebot's Introduction

This is the bot which controls the workflow of Definitely Typed PRs.

Setup

# Clone it
git clone https://github.com/DefinitelyTyped/dt-mergebot.git
cd dt-mergebot

# Deps
npm install

# Validate it works
npm test

How the app works

There are three main stages once the app has a PR number:

Query the GitHub GraphQL API for PR metadata (pr-info)
Create a PR actions metadata object (compute-pr-actions)
Do work based on the resulting actions (execute-pr-actions)

How the bot works

There is an Azure function in PR-Trigger which receives webhooks; this function's job is to find the PR number then it runs the above steps.

Running Locally

You probably don't need to do this. Use test to validate any change inside the src dir against integration tests.

However, you need to have a GitHub API access key in either: DT_BOT_AUTH_TOKEN, BOT_AUTH_TOKEN or AUTH_TOKEN. Ask Ryan for the bot's auth token (TypeScript team members: Look in the team OneNote). Don't run the bot under your own auth token as this will generate a bunch of spam from duplicate comments.

# Windows
set BOT_AUTH_TOKEN=xxxxxxxxxxxxxxxxxxxxxxxxxxxx

# *nix
export BOT_AUTH_TOKEN=xxxxxxxxxxxxxxxxxxxxxxxxxxxx

Then to run locally you'll need to install the Azure Functions cli.

Development

# Build
npm run build

# Run the CLI to see what would happen to an existing PR
npm run single-info -- [PR_NUM]
# or
npm run single-info-debug -- [PR_NUM]

If you update any queries

Run this to update the generate types:

# Code-gen the schema
npm run graphql-schema

If you change project columns or labels

Run this to update the cached values:

# Regenerate src/_tests/cachedQueries.json
npm run update-test-data

Tests

# Run tests, TypeScript is transpiled at runtime
npm test

Most of the tests run against a fixtured PR, these are high level integration tests which store the PR info and then re-run the latter two phases of the app.

To create fixtures of a current PR:

# To create a fixture for PR 43161
npm run create-fixture -- 43161

Then you can work against these fixtures offline with:

# Watch mode for all tests
npm test -- --watch
# Just run fixtures for one PR
npm test -- --testNamePattern 44299

Run a test with the debugger:

node --inspect --inspect-brk ./node_modules/.bin/jest -i --runInBand --testNamePattern 44299

Then use "Attach to Process ID" to connect to that test runner

If your changes require re-creating all fixtures:

npm run update-all-fixtures

Be careful with this, because PRs may now be in a different state e.g. it's now merged and it used to be a specific weird state.

Running with real webhooks

You need a tool like ngrok to expose a URL from the webhooks section on DT.

Start two terminal sessions with:

yarn watch (for TypeScript changes)
yarn start (for the app)

Then start a third with your localhost router like ngrok:

ngrok http 7071

dt-mergebot's People

Contributors

Stargazers

Watchers

dt-mergebot's Issues

`lastEditedAt` unused

There's no need to get lastEditedAt in pr-query.ts, since it's unused now, and unlikely to be useful.

'Ready to merge' not working

See DefinitelyTyped/DefinitelyTyped#46229

A PR with new packages should always end up in 'needs maintainer review';

DefinitelyTyped/DefinitelyTyped#44831 wasn't in the right section

Bot is quite eagerly switching columns around

What I think is happening is that we get a lot of updates as CI statuses change due to normal processes, which could be triggering switching the columns around.

Consider allowing auto‑merging when the only edited `tsconfig.json` is in the `scripts` directory

The scripts directory doesn’t affect the published package, so I’d expect that changes to it are not considered when determining whether module config files were changed.

Code Reviews

Because you edited one package and updated the tests (👏), I can merge this once someone else signs off on it.

Status

✅ No merge conflicts
✅ Continuous integration tests have passed
❌ Most recent commit is approved by type definition owners, DT maintainers or others

All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes.

Diagnostic Information: What the bot saw about this PR

{
  "type": "info",
  "pr_number": 44282,
  "author": "fishcharlie",
  "owners": [],
  "dangerLevel": "ScopedAndTested",
  "headCommitAbbrOid": "2579430",
  "headCommitOid": "25794304acf8c1fd70712bd068beb07b0e09755e",
  "mergeIsRequested": false,
  "stalenessInDays": 0,
  "lastCommitDate": "2020-04-27T23:58:24.000Z",
  "reviewLink": "https://github.com/DefinitelyTyped/DefinitelyTyped/pull/44282/files",
  "hasMergeConflict": false,
  "authorIsOwner": false,
  "isFirstContribution": true,
  "popularityLevel": "Well-liked by everyone",
  "anyPackageIsNew": false,
  "packages": [
    "benchmark"
  ],
  "files": [
    {
      "filePath": "types/benchmark/benchmark-tests.ts",
      "kind": "test",
      "package": "benchmark"
    },
    {
      "filePath": "types/benchmark/index.d.ts",
      "kind": "definition",
      "package": "benchmark"
    }
  ],
  "otherApprovalCount": 0,
  "ownerApprovalCount": 0,
  "maintainerApprovalCount": 0,
  "hasDismissedReview": false,
  "travisResult": "pass",
  "reviewersWithStaleReviews": [],
  "approvalFlags": 0,
  "isChangesRequested": false
}

Delete old travis update messages

These are probably force pushes - DefinitelyTyped/DefinitelyTyped#44299

All states where the bot closes a PR should remove the PR from projects

DefinitelyTyped/DefinitelyTyped#44266 (comment)

For example, I needed to edit the pr to trigger moving

Don’t close PRs that were just reviewed

DefinitelyTyped/DefinitelyTyped#43695 (comment)

Better workflow for "Author is Owner" / "No Other Owners"

Since recently, the demand for updating single owner definitions outstrips the capacity for DT maintainers to perform reviews. It used to be that I could count on adding features to my typings and it be no longer than 2 or 3 days and it would be merged but it's changed since the bot has taken over more responsibility. I am looking for a better workflow for single owner definitions.

The single owner scenario should be treated with responsibility of maintaining the quality of the package, not a DT maintainer. It takes me about 8 hours to bring a certain definition up to date with a new JS release. A DT maintainer is not going to put as much effort into the review as I, the single owner, did and it's half insulting to assume so.

If the single owner is good enough for reviewing and allowing commits from others then they should surely be good enough for their own changes. They should be treated as if they own that part of the repo. In fact maybe having a way to allow git submodules to pull in definitions wouldn't be a bad idea (it would speed up the DT CI build).

Looking at the current DT board. There are ~300 recently merged PRs...none of which are tagged "No Other Owners". Those are all waiting in other boards.

My current options are...

Add another "owner" simply to allow the bot to ping someone else to review/approve the changes. Who is qualified? Who cares? The NPM Credits section shows "These definitions were written by" which would be technically incorrect unless it starts showing these definitions were Written By: Owners , Reviewed By: Everyone that's ever approved changes on the file.
Wait for some unknown period of time until a volunteer comes along (~3 weeks?)
Go try to hunt down a DT maintainer, and request their review. (Tried it and did not work)
Become a DT maintainer? Does that allow me to approve my own changes?
Forget using DT and maintain my own private repo.

Verify scheduled task isn't working as expected

doesn't look like it from the boards

"Ready to merge" should work on an approving review

This didn’t trigger a merge: DefinitelyTyped/DefinitelyTyped#46734 (review)

Always put npm/unpkg links in comment

They currently only appear for new definitions.

Bot can do duplicate work because of concurrent incoming web hooks from GH

Azure/azure-functions-host#912 (comment) looks like the answer

What should we do with mega-PRs?

E.g. DefinitelyTyped/DefinitelyTyped#44272

e.g. I don't think we should be pinging almost all contributors - is there a 'this PR affects too many packages' limit?

Use Danger to make the CI give feedback to the PR instead of forcing click-throughs

I think this probably needs changes in types-publisher to also make a JSON dump of errors, then danger reads it and posts it up.

Look at mutation responses to know if something pass/failed

The bot doesn't ping dt module owners

Doh! Examples:

Make time-out a month, not a week

Don't do work on closed PRs

See: DefinitelyTyped/DefinitelyTyped#44105

People should not be able to self-merge until maintainers of all affected packages have approved a change.

For example, in this PR a maintainer of the react-native-material-ripple package approved the changes, but the bulk of the changes were actually in react-native, which the author was able to self-merge before a react-native maintainer was able to review the changes.

Merge commits from accepting a PR causes a 500

This means the PR lookup will fail because the PR is closed and the returns nothing

Bot saying it needs tests when it had tests

PR: DefinitelyTyped/DefinitelyTyped#44439

A PR without tests which has reviews from a module owner should be in 'maintainer review'

Duplicate processing still happens when events happen in quick succession

#43 is not entirely fixed because sometimes events can happen in quick succession: DefinitelyTyped/DefinitelyTyped#43695 (comment)

When I added a review, the webhook fired both a "review submitted" and a "comment created" event simultaneously. Coincidentally, the PR author also commented at about the same time. @typescript-bot commented 4 times. I don’t know where the fourth came from.

Unmangle scoped package names in npm/unpkg links

PRs to e.g. types/hapi__joi should create links to npmjs.com/package/@hapi/joi, not npmjs.com/package/hapi__joi.

Allow for regular contributors to get away with 'infractions'

For modules which aren't too popular, and for users who have submitted a lot of PRs (like 20-30+) we could

Allow with no tests
Allow other reviewers
Allow PRs which touch tsconfig or tslint for that project

We could say only allow 1 of these at a time per PR, and to give at least a day before self merging is allowed.

Fix ready-to-merge detection for email reply

I think if the comment body starts with "ready to merge," that’s probably sufficient:

DefinitelyTyped/DefinitelyTyped#44420 (comment)

Make the schedules task cull "Recently Merged" to the most recent 50

like a fixed length queue

Something is up with the status update webhook

We get a webook like this:

But our response is this:

 \"travisResult\": \"unknown\"

Add unpkg link to welcome comment for new packages

The npm link is useful for gauging popularity, but I also use unpkg on every new definition review to check module format.

Bot doesn't get web hooks for new issue comments (nor new reviews)

In DefinitelyTyped/DefinitelyTyped#43696 - I edited the body of the PR to trigger the final merge

Delay on processing of new PRs or alternative processing

After I added information when failing to find a PR, I saw a failure that had the information below for a pull_request / opened event. The PR does exist, and its URL is even mentioned in the event payload. So I'm guessing that there might be some processing delays (maybe there's a separate db that the gql api uses that needs to update) -- and therefore it's probably best to delay the processing of such events by a few seconds.

It's probably best to do this in addition to some manual per-pr sequentialization of handling incoming requests, since there are probably a small outburst of events when a pr is created.

Another possibility, since there are other cases when several events fire at the same time is, and since the processing always looks at the whole state so there's no need to process each and every event for the same PR:

When an event is received, figure out the PR number, and immediately return some "OK" answer.
Wait for several seconds for each of these (after returning the OK).
In this period, if there are new events for the same PR, then ignore them (or maybe extend the waiting period for the existing one).
Then do one processing of the PR.

Failed because of: No PR with this number exists, ({
  "data":{"repository":{"pullRequest":null,"__typename":"Repository"}},
  "loading":false,
  "networkStatus":7,
  "stale":false,
  "errors":[{
    "type":"NOT_FOUND",
    "path":["repository","pullRequest"],
    "locations":[{"line":3,"column":5}],
    "message":"Could not resolve to a PullRequest with the number of 46330."
  }]
})

There's no way to declare that module is auto generated

It's a follow-up to discussion with @orta
and it's related to Maxim-Mazurok/google-api-typings-generator#245

I'm maintaining a repo that is generating d.ts for Google client-side APIs (here's why: Google API in NodeJS VS Client)

Google APIs are updated constantly. Almost every hour at least one API is changed. Sometimes it's just revision number bump. Sometimes it's change of wording in comments. Sometimes it's bigger.
So, we're using Github Actions in order to generate new typings and put them in types branch.
Another action compares our types branch with typings in DT and if it's different - opens/updates PR: example PR.
You can find more info about this, including some flow-charts in this PR and linked issues.

There are currently a few problems:

When we're changing comments - bot still wants tests to be changed.
Another problem is when someone reviews my PR. Good example. As you can see, bot kept moving it form one column one the project board to another, making it fo to the bottom of list every time.

Since we don't really care about revision-only updates, I disabled them.

But still, it takes quiet some time to merge these changes and I'm feeling bad about wasting reviewers time.

Can we come up with some solution that would auto-merge auto-generated modules? Any ideas?