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

Feature/close draft pr #93

Merged
merged 21 commits into from
Jun 28, 2021
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions .wiby.json
Original file line number Diff line number Diff line change
@@ -1,13 +1,16 @@
{
"dependents": [
{
"repository": "https://github.com/wiby-test/partial"
"repository": "https://github.com/wiby-test/partial",
"pullRequest": false
},
{
"repository": "git://github.com/wiby-test/fail"
"repository": "git://github.com/wiby-test/fail",
"pullRequest": false
},
{
"repository": "git+https://github.com/wiby-test/pass"
"repository": "git+https://github.com/wiby-test/pass",
"pullRequest": true
}
]
}
35 changes: 35 additions & 0 deletions bin/commands/close-pr.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
'use strict'

const wiby = require('../..')

exports.desc = 'Use this command to close the PRs raised against your dependents. wiby will go off to the dependent’s repo and close the PRs raised that trigger jobs `package.json` pointing to your latest version (with the new changes) triggering the dependent’s CI to run.'

exports.builder = (yargs) => yargs
.option('dependent', {
desc: 'URL of a dependent',
type: 'string',
conflicts: 'config'
})
.option('close-pr', {
desc: 'Close a PR of a dependent raised in test',
alias: 'pr',
type: 'boolean',
conflicts: 'config'
})
ghinks marked this conversation as resolved.
Show resolved Hide resolved
.option('config', {
desc: 'Path to the configuration file. By default it will try to load the configuration from the first file it finds in the current working directory: `.wiby.json`, `.wiby.js`',
type: 'string'
})

exports.handler = async (params) => {
const config = params.dependent
? {
dependents: [{ repository: params.dependent, pullRequest: true }]
}
: wiby.validate({ config: params.config })

const result = await wiby.closePR(config)
// TODO, something more like the result process output
const output = `${result.length} PRs closed`
console.log(output)
}
4 changes: 3 additions & 1 deletion bin/commands/result.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@ exports.builder = (yargs) => yargs

exports.handler = async (params) => {
const config = params.dependent
? { dependents: [{ repository: params.dependent }] }
? {
dependents: [{ repository: params.dependent }]
}
: wiby.validate({ config: params.config })

const result = await wiby.result(config)
Expand Down
10 changes: 9 additions & 1 deletion bin/commands/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,22 @@ exports.builder = (yargs) => yargs
type: 'string',
conflicts: 'config'
})
.option('pull-request', {
desc: 'Raise a draft PR in addition to creating a branch',
alias: 'pr',
type: 'boolean',
conflicts: 'config'
})
.option('config', {
desc: 'Path to the configuration file. By default it will try to load the configuration from the first file it finds in the current working directory: `.wiby.json`, `.wiby.js`',
type: 'string'
})

exports.handler = (params) => {
const config = params.dependent
? { dependents: [{ repository: params.dependent }] }
? {
dependents: [{ repository: params.dependent, pullRequest: !!params['pull-request'] }]
}
: wiby.validate({ config: params.config })

return wiby.test(config)
Expand Down
48 changes: 48 additions & 0 deletions lib/closePR.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
'use strict'

const github = require('../lib/github')
const logger = require('./logger')
const context = require('./context')
const gitURLParse = require('git-url-parse')

const debug = logger('wiby:closepr')

module.exports = async ({ dependents }) => {
const closedPrs = []
for (const { repository: url, pullRequest } of dependents) {
if (pullRequest) {
const dependentPkgInfo = gitURLParse(url)
const parentBranchName = await context.getParentBranchName()
const branch = await context.getTestingBranchName(parentBranchName)
const resp = await github.getChecks(dependentPkgInfo.owner, dependentPkgInfo.name, branch)
const closedPR = await closeDependencyPR(dependentPkgInfo.owner, dependentPkgInfo.name, branch, resp.data.check_runs)
if (closedPR) {
closedPrs.push(closedPR)
}
}
}
return closedPrs
}

const closeDependencyPR = module.exports.closeDependencyPR = async function closeDependencyPR (owner, repo, branch, checkRuns) {
if (!checkRuns) {
return
}
const prsToClose = checkRuns.reduce((acc, check) => {
if (check.status === 'completed' &&
check.conclusion === 'success' &&
check.pull_requests &&
check.pull_requests.length !== 0) {
check.pull_requests.forEach((pr) => {
if (pr.head.ref === branch) {
acc.push({
number: pr.number
})
}
})
}
return acc
}, [])
debug(`Dependent module: ${JSON.stringify(prsToClose, null, 2)}`)
return await Promise.all(prsToClose.map((pr) => github.closePR(owner, repo, pr.number)))
}
3 changes: 2 additions & 1 deletion lib/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ const dependentSchema = joi.object({
'git',
'git+https'
]
})
}),
pullRequest: joi.boolean().optional()
}).unknown(false)

exports.schema = joi.object({
Expand Down
21 changes: 21 additions & 0 deletions lib/github.js
Original file line number Diff line number Diff line change
Expand Up @@ -136,3 +136,24 @@ module.exports.getCommitStatusesForRef = async function getCommitStatusesForRef
ref: branch
})
}

module.exports.createPR = async function createPR (owner, repo, title, head, base, draft, body) {
return octokit.pulls.create({
owner,
repo,
title,
head,
base,
draft,
body
})
}

module.exports.closePR = async function closePR (owner, repo, pullNumber) {
return octokit.rest.pulls.update({
owner,
repo,
pull_number: pullNumber,
state: 'closed'
})
}
2 changes: 2 additions & 0 deletions lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,5 @@ exports.test = require('./test')
exports.result = require('./result')

exports.validate = require('./config').validate

exports.closePR = require('./closePR')
17 changes: 16 additions & 1 deletion lib/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ module.exports = async function ({ dependents }) {
const parentDependencyLink = await context.getDependencyLink(parentRepositoryInfo.owner, parentRepositoryInfo.name, parentBranchName)
debug('Commit URL to test:', parentDependencyLink)

for (const { repository: url } of dependents) {
for (const { repository: url, pullRequest } of dependents) {
const dependentRepositoryInfo = gitURLParse(url)
const dependentPkgJson = await github.getPackageJson(dependentRepositoryInfo.owner, dependentRepositoryInfo.name)
debug(`Dependent module: ${dependentRepositoryInfo.owner}/${dependentRepositoryInfo.name}`)
Expand All @@ -32,6 +32,9 @@ module.exports = async function ({ dependents }) {

const patchedPackageJSON = applyPatch(parentDependencyLink, parentPkgJSON.name, dependentPkgJson, parentPkgJSON.name)
await pushPatch(patchedPackageJSON, dependentRepositoryInfo.owner, dependentRepositoryInfo.name, parentPkgJSON.name, parentBranchName)
if (pullRequest) {
Copy link
Member

Choose a reason for hiding this comment

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

Updated #95 (comment) - if we think that's the right approach, then we should check if the PR is already open here (we need to open a new one if it's closed).

I can't recall if the GH API returns the PR list as part of the "get branch" API call, or if there's a separate API call for that...

Copy link
Contributor Author

@ghinks ghinks Apr 28, 2021

Choose a reason for hiding this comment

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

Ok. I agree that clean needs it to. I'll merge the latest changes back into my branch and carry on.

One other thing occurred to me. We may want to be using the Promise.allSettled() module as if we checking upon multiple checks. But that can be for later.

Copy link
Member

Choose a reason for hiding this comment

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

Promise.allSettled()

Yeah, that probably makes sense.

Copy link
Contributor Author

@ghinks ghinks Apr 29, 2021

Choose a reason for hiding this comment

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

After looking at the needs of the clean command. I do not think we need to do anything. If the branch is deleted during a clean then the PR that was raised against that branch to trigger an action is also deleted.

If you agree with my opinion I'll look at issue 95 with respect to the close-pr command.

await createPR(dependentRepositoryInfo.owner, dependentRepositoryInfo.name, parentBranchName, parentDependencyLink)
}
}
}

Expand All @@ -58,3 +61,15 @@ async function pushPatch (dependentPkgJson, dependentOwner, dependentRepo, paren
await github.createBranch(dependentOwner, dependentRepo, commitSha, branch)
debug(`Changes pushed to https://github.com/${dependentOwner}/${dependentRepo}/blob/${branch}/package.json`)
}

const createPR = module.exports.createPR = async function createPR (dependentOwner, dependentRepo, parentBranchName, parentDependencyLink) {
const title = `Wiby changes to ${parentDependencyLink}`
const body = `Wiby has raised this PR to kick off automated jobs.
You are dependent upon ${parentDependencyLink}
`
const head = context.getTestingBranchName(parentBranchName)
const draft = true
const result = await github.createPR(dependentOwner, dependentRepo, title, head, parentBranchName, draft, body)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to set the base to parentBranchName here? I do have a note in the issues, that if the branch exists in the dependent repo as well as in parent repo, we should try to use that, but that's not implemented yet, so at this point parentBranchName likely does not even exist - we should probably keep it undefined (if the API allows it - otherwise we need to fetch the default branch name...)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was not sure on this comment, but its late afternoon so I'm taking it easy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dominykas ok, I came back to look at this comment and confess to not understanding at first you are referring to this I think issue 95. If so I think I understand.

Copy link
Member

@dominykas dominykas Apr 28, 2021

Choose a reason for hiding this comment

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

It's kind of related to that issue, but that's not exactly it.

When opening a PR, we want to merge head into base. So most of the time base should be main / master (i.e. default branch), not parentBranchName?

  1. Suppose you're working on my-feature
  2. You wiby test
  3. A wiby-my-feature branch is created in the dependent repo
  4. You should be opening a PR on the dependent repo to merge wiby-my-feature into main

i.e. my-feature (parentBranchName) will not even exist in the dependent, most of the time. When it does - we do have the "existing branches" case, which needs to be thought through - most likely we'll want to create a wiby-my-feature in the dependent, but we will not start it at main / master, but at my-feature instead. In that case, there could be some DX wins if the PR on the dependent tried to merge wiby-my-feature into my-feature, but at the same time - if it tries to merge wiby-my-feature into main, it's probably no big deal either?

This is hard 😅

Copy link
Member

Choose a reason for hiding this comment

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

To simplify this - I think initially the base param should always be empty (if GH will accept that), and the rest should be addressed as we tackle the existing branches case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not quite done yet, I'm going for the merging into the default branch main or whatever the default is set to. ( the better DX experience needs another change which I'm going to try for tomorrow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dominykas , I altered the code to raise the PR against the default branch and added changes to the tests to cover this most simple scenario. I too want to get this PR merged and maybe covering the other edge cases should be handled in another follow up PR?

debug(`PR raised upon ${result.data.html_url}`)
return result
}
43 changes: 43 additions & 0 deletions test/cli/closePR.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
'use strict'
const tap = require('tap')
const gitFixture = require('../fixtures/git')
const childProcess = require('child_process')
const nock = require('nock')
const path = require('path')

const wibyCommand = path.join(__dirname, '..', '..', 'bin', 'wiby')
const fixturesPath = path.resolve(path.join(__dirname, '..', 'fixtures'))

tap.only('closePRs command', async (t) => {
t.beforeEach(async () => {
nock.disableNetConnect()
gitFixture.init()
})

t.test('close-pr should fail if config and dependent provided', async (t) => {
try {
childProcess.execSync(`${wibyCommand} close-pr --config=.wiby.json --dependent="https://github.com/wiby-test/fakeRepo"`)
tap.fail()
} catch (err) {
tap.equal(true, err.message.includes('Arguments dependent and config are mutually exclusive'))
}
})
t.test('close-pr should call and close the PR on command with dependent argument', async (t) => {
const result = childProcess.execSync(`${wibyCommand} close-pr --dependent="https://github.com/wiby-test/fakeRepo"`, {
env: {
...process.env,
NODE_OPTIONS: `-r ${fixturesPath}/http/close-pr-command-positive.js`
}
}).toString()
t.match(result, '1 PRs closed\n')
})
t.only('close-pr should call and close the PR on command using wiby.json settings', async (t) => {
const result = childProcess.execSync(`${wibyCommand} close-pr`, {
env: {
...process.env,
NODE_OPTIONS: `-r ${fixturesPath}/http/close-pr-command-positive.js`
}
}).toString()
t.match(result, '1 PRs closed\n')
})
})
2 changes: 1 addition & 1 deletion test/cli/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ tap.test('test command', async (tap) => {
})

tap.test('test command should call test module with dependent URI', async (tap) => {
const result = childProcess.execSync(`${wibyCommand} test --dependent="https://github.com/wiby-test/fakeRepo"`, {
const result = childProcess.execSync(`${wibyCommand} test --dependent="https://github.com/wiby-test/fakeRepo" --pull-request=true`, {
env: {
...process.env,
NODE_OPTIONS: `-r ${fixturesPath}/http/test-command-positive.js`
Expand Down
Loading