-
Notifications
You must be signed in to change notification settings - Fork 147
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
fix: handle error when branch already exists #48
Changes from 2 commits
865b982
33a231a
542b5a9
b179e9b
eddf278
62f61d9
f00625d
91e49bc
f55efae
2a6bfc6
a218994
7378180
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,16 +1,25 @@ | ||
const { ResourceNotFoundError } = require('../utils/errors') | ||
const { | ||
ResourceNotFoundError, | ||
AllContributorBotError, | ||
} = require('../utils/errors') | ||
|
||
class Repository { | ||
constructor({ repo, owner, github }) { | ||
constructor({ repo, owner, github, defaultBranch }) { | ||
this.github = github | ||
this.repo = repo | ||
this.owner = owner | ||
this.defaultBranch = defaultBranch | ||
this.basedBranch | ||
} | ||
|
||
getFullname() { | ||
return `${this.owner}/${this.repo}` | ||
} | ||
|
||
setBasedBranch(branchName) { | ||
this.basedBranch = branchName | ||
} | ||
|
||
async getFile(filePath) { | ||
// https://octokit.github.io/rest.js/#api-Repos-getContents | ||
let file | ||
|
@@ -19,6 +28,7 @@ class Repository { | |
owner: this.owner, | ||
repo: this.repo, | ||
path: filePath, | ||
ref: this.basedBranch, | ||
}) | ||
} catch (error) { | ||
if (error.code === 404) { | ||
|
@@ -61,17 +71,17 @@ class Repository { | |
return multipleFilesByPath | ||
} | ||
|
||
async getHeadRef(defaultBranch) { | ||
async getHeadRef(branchName) { | ||
sinchang marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const result = await this.github.git.getRef({ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of checking 404 above when called. Can we throw an error here? (BranchAlreadyExists) and catch that |
||
owner: this.owner, | ||
repo: this.repo, | ||
ref: `heads/${defaultBranch}`, | ||
ref: `heads/${branchName}`, | ||
}) | ||
return result.data.object.sha | ||
} | ||
|
||
async createBranch({ branchName, defaultBranch }) { | ||
const fromSha = await this.getHeadRef(defaultBranch) | ||
async createBranch(branchName) { | ||
const fromSha = await this.getHeadRef(this.defaultBranch) | ||
|
||
// https://octokit.github.io/rest.js/#api-Git-createRef | ||
await this.github.git.createRef({ | ||
|
@@ -140,40 +150,44 @@ class Repository { | |
await Promise.all(createOrUpdateFilesMultiple) | ||
} | ||
|
||
async createPullRequest({ title, body, branchName, defaultBranch }) { | ||
const result = await this.github.pulls.create({ | ||
owner: this.owner, | ||
repo: this.repo, | ||
title, | ||
body, | ||
head: branchName, | ||
base: defaultBranch, | ||
maintainer_can_modify: true, | ||
}) | ||
return result.data.html_url | ||
async createPullRequest({ title, body, branchName }) { | ||
try { | ||
const result = await this.github.pulls.create({ | ||
owner: this.owner, | ||
repo: this.repo, | ||
title, | ||
body, | ||
head: branchName, | ||
base: this.defaultBranch, | ||
maintainer_can_modify: true, | ||
}) | ||
return { pullRequestURL: result.data.html_url, result: true } | ||
} catch (error) { | ||
// pull request is already open | ||
if (error.code === 422) return { pullRequestURL: '', result: false } | ||
throw error | ||
} | ||
} | ||
|
||
async createPullRequestFromFiles({ | ||
title, | ||
body, | ||
filesByPath, | ||
branchName, | ||
defaultBranch, | ||
}) { | ||
await this.createBranch({ branchName, defaultBranch }) | ||
async createPullRequestFromFiles({ title, body, filesByPath, branchName }) { | ||
if (this.basedBranch === this.defaultBranch) | ||
sinchang marked this conversation as resolved.
Show resolved
Hide resolved
|
||
this.createBranch(branchName) | ||
|
||
await this.createOrUpdateFiles({ | ||
filesByPath, | ||
branchName, | ||
}) | ||
|
||
const pullRequestURL = await this.createPullRequest({ | ||
const { pullRequestURL, result } = await this.createPullRequest({ | ||
title, | ||
body, | ||
branchName, | ||
defaultBranch, | ||
}) | ||
|
||
// TODO | ||
if (!result) | ||
throw new AllContributorBotError('Pull request is already open') | ||
|
||
return pullRequestURL | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,7 +20,6 @@ async function processAddContributor({ | |
optionsConfig, | ||
who, | ||
contributions, | ||
defaultBranch, | ||
}) { | ||
const { name, avatar_url, profile } = await getUserDetails({ | ||
github: context.github, | ||
|
@@ -56,7 +55,6 @@ async function processAddContributor({ | |
)}.\n\nThis was requested by ${commentReply.replyingToWho()} [in this comment](${commentReply.replyingToWhere()})`, | ||
filesByPath: filesByPathToUpdate, | ||
branchName: `all-contributors/add-${who}`, | ||
defaultBranch, | ||
}) | ||
|
||
commentReply.reply( | ||
|
@@ -65,14 +63,29 @@ async function processAddContributor({ | |
} | ||
|
||
async function processIssueComment({ context, commentReply }) { | ||
const commentBody = context.payload.comment.body | ||
const { who, action, contributions } = parseComment(commentBody) | ||
const branchName = `all-contributors/add-${who}` | ||
const defaultBranch = context.payload.repository.default_branch | ||
const repository = new Repository({ | ||
...context.repo(), | ||
github: context.github, | ||
defaultBranch, | ||
}) | ||
|
||
try { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This block should happen after fetching config IMO There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This block should happen inside the |
||
await repository.getHeadRef(branchName) | ||
repository.setBasedBranch(branchName) | ||
} catch (error) { | ||
if (error.code !== 404) throw error | ||
repository.setBasedBranch(defaultBranch) | ||
} | ||
|
||
const optionsConfig = new OptionsConfig({ | ||
repository, | ||
commentReply, | ||
}) | ||
|
||
try { | ||
await optionsConfig.fetch() | ||
} catch (error) { | ||
|
@@ -83,18 +96,14 @@ async function processIssueComment({ context, commentReply }) { | |
} | ||
} | ||
|
||
const commentBody = context.payload.comment.body | ||
const parsedComment = parseComment(commentBody) | ||
|
||
if (parsedComment.action === 'add') { | ||
if (action === 'add') { | ||
await processAddContributor({ | ||
context, | ||
commentReply, | ||
repository, | ||
optionsConfig, | ||
who: parsedComment.who, | ||
contributions: parsedComment.contributions, | ||
defaultBranch: context.payload.repository.default_branch, | ||
who, | ||
contributions, | ||
}) | ||
return | ||
} | ||
|
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.
I wonder how we can take the latest master file and overwrite the current branch :think: like a rebase
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.
We probably shouldn't address this here. This is probably a whole ticket in itself. #62.
I'm interested in hearing how you would solve this problem tho.