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.

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

Rewrite to be purely driven by external state #10

Merged
merged 51 commits into from Jun 5, 2019

Conversation

Projects
None yet
3 participants
@chrisdothtml
Copy link
Contributor

commented May 24, 2019

Fixes #9

Instead of being driven by hooks, write it to more generically check for changes based on the current state of the repos/PRs. This makes it a lot more reliable, and is able to recover from things like changes happening while the bot is offline

In addition to that, also adds some features:

  • Can recover from being offline
    • checks open PRs of all repos on startup and syncs if needed
  • Syncs PR title/body
  • Has ability to control public PR title/body from metadata in private PR body
  • Ability to generate sync PR from private PRs
    • only opens PR in the public repo if its files are changed
  • Can now sync merge commits
  • Can now handle forced pushes
    • does this by constantly keeping 1:1 mirror of commits and comparing whenever changes happen
  • Only ever uses generic, non-descriptive commit message if commit changes both public and private files

@chrisdothtml chrisdothtml referenced this pull request May 24, 2019

Closed

PR title syncing #9

@chrisdothtml chrisdothtml changed the title Rewrite to be purely driven by external state [WIP] Rewrite to be purely driven by external state May 24, 2019

Show resolved Hide resolved lib/cache.js
Show resolved Hide resolved lib/graphql.js Outdated
Show resolved Hide resolved lib/graphql.js Outdated
import {throttleWebhook} from './utils.js';

async function init(repoNames) {
for (const repoName of repoNames) {

This comment has been minimized.

Copy link
@ganemone

ganemone Jun 3, 2019

Does this need to execute in serial? if not we could speed this up by running in parallel.

Promise.all(repoNames.filter(r => hasRelationship(r)).map(r => ... etc)))

This comment has been minimized.

Copy link
@chrisdothtml

chrisdothtml Jun 3, 2019

Author Contributor

In a general sense it could run in parallel. However with the way we're using it (deployed as 2 separate apps on each org) I think I'm actually gonna need to modify this to avoid the 2 deployments doing double work (since syncPR actually syncs both repos involved in the pr).

Longer term I think I'm gonna see if I can fix the issues we had with just using one deployment for both orgs

This comment has been minimized.

Copy link
@chrisdothtml

chrisdothtml Jun 4, 2019

Author Contributor

Nevermind, actually fixing this issue in fusionjs/probot-app-workflow#147

export function getChildrenNames(repoName) {
return getChildren(repoName).map(child => child.name);
}

/**
* @param {string} parentRepoName

This comment has been minimized.

Copy link
@ganemone

ganemone Jun 3, 2019

Why are we using jsdoc instead of flow here?

This comment has been minimized.

Copy link
@chrisdothtml

chrisdothtml Jun 3, 2019

Author Contributor

It started off as just being quicker than adding flow (since vscode supports out of the box) but kinda expanded to the whole repo. I can switch to flow

This comment has been minimized.

Copy link
@ganemone

ganemone Jun 3, 2019

Feel free to do that latter - doesn't need to be included in this PR

chrisdothtml added some commits Jun 3, 2019

@chrisdothtml chrisdothtml changed the title [WIP] Rewrite to be purely driven by external state Rewrite to be purely driven by external state Jun 3, 2019

@KevinGrandon
Copy link

left a comment

It's a big one, and a little hard to make sense of it all, but if you tested and it works it sounds good to me.


// throttle this since it's sometimes triggered more than 20 times within
// a window of ~10 seconds
const handler = throttleWebhook(payload.sha, 10000, async payload => {

This comment has been minimized.

Copy link
@KevinGrandon

KevinGrandon Jun 4, 2019

I didn't see a callsite for this function. Should we remove it if it's not used?

This comment has been minimized.

Copy link
@chrisdothtml
const github = await probot.auth();
const repoLists = await github
.request('GET /app/installations')
.then(res => res.data.map(install => install.id))

This comment has been minimized.

Copy link
@KevinGrandon

KevinGrandon Jun 4, 2019

Nit: seems a bit strange to see a mix of async/await and promises here. Should we stick with async await?

This comment has been minimized.

Copy link
@chrisdothtml

chrisdothtml Jun 4, 2019

Author Contributor

I occasionally do it to avoid making temporary variables for an eventual result. Basically, of these two options, I went with Option 2:

Option 1

const response = await request(...)
const ids = response.data.map(...)
const repoLists = await Promise.all(ids.map(...))

Option 2

const repoLists = await request(...)
  .then(response => response.data.map(...))
  .then(ids => Promise.all(ids.map(...))

I can see how it could look a bit weird though

@uber-workflow-bot

This comment has been minimized.

Copy link

commented Jun 5, 2019

Found TODOs without GitHub issues:

// TODO: currently using an actual user token (instead of the github app)

// TODO: currently using an actual user for this instead of github app;

// TODO: handle non-file-types (e.g. submodule pointer)

// TODO: handle pagination to support PRs with more than 100 changed files

// TODO: make commits independent of syncState (i.e. have it lookup the state on its own)

// TODO: close/re-open depending on whether a parent repo pr currently contains changes to child repo files; also should NOT sync merge if child pr is closed because of this

@chrisdothtml chrisdothtml merged commit 3e1b2d9 into master Jun 5, 2019

5 of 7 checks passed

probot/migrations PRs with breaking changes must provide a migration guide
probot/todos TODO without open GitHub issue
Details
buildkite/probot-app-monorepo-sync Build #59 passed (1 minute, 37 seconds)
Details
license/cla Contributor License Agreement is signed.
Details
probot/label-docs-pr Docs label has been set (or unset)
probot/pr-label At least one required semver-related label exists
probot/pr-title PR title is valid

@uber-workflow-bot uber-workflow-bot bot deleted the state-driven branch Jun 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.