From 1bd22611fd426a9e45a8927d5a7cba8605faa46a Mon Sep 17 00:00:00 2001
From: Rhys Arkins <rhys@keylocation.sg>
Date: Sat, 3 Jun 2017 09:40:13 +0200
Subject: [PATCH] Fix GitHub app merge (#234)
* Add installation debug
* Update initRepo merge detection
* mergePr tests
* guess squash
* Update tests
---
lib/api/github.js | 57 +++++-
lib/helpers/github-app.js | 1 +
test/api/__snapshots__/github.spec.js.snap | 38 ++--
test/api/github.spec.js | 191 ++++++++++++++++++++-
4 files changed, 258 insertions(+), 29 deletions(-)
diff --git a/lib/api/github.js b/lib/api/github.js
index d7e9033a54..c288c20e6c 100644
--- a/lib/api/github.js
+++ b/lib/api/github.js
@@ -140,8 +140,10 @@ async function initRepo(repoName, token, endpoint) {
config.mergeMethod = 'rebase';
} else if (res.body.allow_squash_merge) {
config.mergeMethod = 'squash';
- } else {
+ } else if (res.body.allow_merge_commit) {
config.mergeMethod = 'merge';
+ } else {
+ logger.debug('Could not find allowed merge methods for repo');
}
logger.debug(`${repoName} default branch = ${config.defaultBranch}`);
config.baseCommitSHA = await getBranchCommit(config.defaultBranch);
@@ -355,11 +357,54 @@ async function updatePr(prNo, title, body) {
}
async function mergePr(pr) {
- await ghGot.put(`repos/${config.repoName}/pulls/${pr.number}/merge`, {
- body: {
- merge_method: config.mergeMethod,
- },
- });
+ const url = `repos/${config.repoName}/pulls/${pr.number}/merge`;
+ const options = {
+ body: {},
+ };
+ if (config.mergeMethod) {
+ // This path is taken if we have auto-detected the allowed merge types from the repo
+ options.body.merge_method = config.mergeMethod;
+ try {
+ logger.debug(`mergePr: ${url}, ${JSON.stringify(options)}`);
+ await ghGot.put(url, options);
+ } catch (err) {
+ logger.error(
+ `Failed to ${options.body.merge_method} PR: ${JSON.stringify(err)}`
+ );
+ return;
+ }
+ } else {
+ // We need to guess the merge method and try squash -> rebase -> merge
+ options.body.merge_method = 'rebase';
+ try {
+ logger.debug(`mergePr: ${url}, ${JSON.stringify(options)}`);
+ await ghGot.put(url, options);
+ } catch (err1) {
+ logger.debug(
+ `Failed to ${options.body.merge_method} PR: ${JSON.stringify(err1)}`
+ );
+ try {
+ options.body.merge_method = 'squash';
+ logger.debug(`mergePr: ${url}, ${JSON.stringify(options)}`);
+ await ghGot.put(url, options);
+ } catch (err2) {
+ logger.debug(
+ `Failed to ${options.body.merge_method} PR: ${JSON.stringify(err2)}`
+ );
+ try {
+ options.body.merge_method = 'merge';
+ logger.debug(`mergePr: ${url}, ${JSON.stringify(options)}`);
+ await ghGot.put(url, options);
+ } catch (err3) {
+ logger.debug(
+ `Failed to ${options.body.merge_method} PR: ${JSON.stringify(err3)}`
+ );
+ logger.error('All merge attempts failed');
+ return;
+ }
+ }
+ }
+ }
// Delete branch
await ghGot.delete(`repos/${config.repoName}/git/refs/heads/${pr.head.ref}`);
}
diff --git a/lib/helpers/github-app.js b/lib/helpers/github-app.js
index 58a483e10c..92d0b5b99d 100644
--- a/lib/helpers/github-app.js
+++ b/lib/helpers/github-app.js
@@ -48,6 +48,7 @@ async function getRepositories(config) {
const installations = await ghApi.getInstallations(appToken);
logger.verbose(`Found installations for ${installations.length} users`);
for (const installation of installations) {
+ logger.debug(JSON.stringify(installation));
const installationRepos = await module.exports.getUserRepositories(
appToken,
installation.id
diff --git a/test/api/__snapshots__/github.spec.js.snap b/test/api/__snapshots__/github.spec.js.snap
index a20e9a5096..ee24817ab1 100644
--- a/test/api/__snapshots__/github.spec.js.snap
+++ b/test/api/__snapshots__/github.spec.js.snap
@@ -798,36 +798,36 @@ Object {
}
`;
-exports[`api/github initRepo should squash 1`] = `
+exports[`api/github initRepo should not guess at merge 1`] = `
Object {
"baseCommitSHA": "1234",
"baseTreeSHA": "5678",
"defaultBranch": "master",
- "mergeMethod": "squash",
"owner": "theowner",
"repoName": "some/repo",
}
`;
-exports[`api/github mergePr(prNo) should merge the PR 1`] = `
-Array [
- Array [
- "repos/some/repo/pulls/1234/merge",
- Object {
- "body": Object {
- "merge_method": "rebase",
- },
- },
- ],
-]
+exports[`api/github initRepo should rebase 1`] = `
+Object {
+ "baseCommitSHA": "1234",
+ "baseTreeSHA": "5678",
+ "defaultBranch": "master",
+ "mergeMethod": "rebase",
+ "owner": "theowner",
+ "repoName": "some/repo",
+}
`;
-exports[`api/github mergePr(prNo) should merge the PR 2`] = `
-Array [
- Array [
- "repos/some/repo/git/refs/heads/someref",
- ],
-]
+exports[`api/github initRepo should squash 1`] = `
+Object {
+ "baseCommitSHA": "1234",
+ "baseTreeSHA": "5678",
+ "defaultBranch": "master",
+ "mergeMethod": "squash",
+ "owner": "theowner",
+ "repoName": "some/repo",
+}
`;
exports[`api/github updatePr(prNo, title, body) should update the PR 1`] = `
diff --git a/test/api/github.spec.js b/test/api/github.spec.js
index 7f5ff97214..6abc21adbf 100644
--- a/test/api/github.spec.js
+++ b/test/api/github.spec.js
@@ -139,6 +139,7 @@ describe('api/github', () => {
default_branch: 'master',
allow_rebase_merge: true,
allow_squash_merge: true,
+ allow_merge_commit: true,
},
}));
// getBranchCommit
@@ -193,7 +194,7 @@ describe('api/github', () => {
'No token found for GitHub repository some/repo'
);
});
- it('should squash', async () => {
+ it('should rebase', async () => {
async function squashInitRepo(...args) {
// repo info
ghGot.mockImplementationOnce(() => ({
@@ -202,8 +203,9 @@ describe('api/github', () => {
login: 'theowner',
},
default_branch: 'master',
- allow_rebase_merge: false,
+ allow_rebase_merge: true,
allow_squash_merge: true,
+ allow_merge_commit: true,
},
}));
// getBranchCommit
@@ -227,6 +229,41 @@ describe('api/github', () => {
const config = await squashInitRepo('some/repo', 'token');
expect(config).toMatchSnapshot();
});
+ it('should squash', async () => {
+ async function mergeInitRepo(...args) {
+ // repo info
+ ghGot.mockImplementationOnce(() => ({
+ body: {
+ owner: {
+ login: 'theowner',
+ },
+ default_branch: 'master',
+ allow_rebase_merge: false,
+ allow_squash_merge: true,
+ allow_merge_commit: true,
+ },
+ }));
+ // getBranchCommit
+ ghGot.mockImplementationOnce(() => ({
+ body: {
+ object: {
+ sha: '1234',
+ },
+ },
+ }));
+ // getCommitTree
+ ghGot.mockImplementationOnce(() => ({
+ body: {
+ tree: {
+ sha: '5678',
+ },
+ },
+ }));
+ return github.initRepo(...args);
+ }
+ const config = await mergeInitRepo('some/repo', 'token');
+ expect(config).toMatchSnapshot();
+ });
it('should merge', async () => {
async function mergeInitRepo(...args) {
// repo info
@@ -238,6 +275,39 @@ describe('api/github', () => {
default_branch: 'master',
allow_rebase_merge: false,
allow_squash_merge: false,
+ allow_merge_commit: true,
+ },
+ }));
+ // getBranchCommit
+ ghGot.mockImplementationOnce(() => ({
+ body: {
+ object: {
+ sha: '1234',
+ },
+ },
+ }));
+ // getCommitTree
+ ghGot.mockImplementationOnce(() => ({
+ body: {
+ tree: {
+ sha: '5678',
+ },
+ },
+ }));
+ return github.initRepo(...args);
+ }
+ const config = await mergeInitRepo('some/repo', 'token');
+ expect(config).toMatchSnapshot();
+ });
+ it('should not guess at merge', async () => {
+ async function mergeInitRepo(...args) {
+ // repo info
+ ghGot.mockImplementationOnce(() => ({
+ body: {
+ owner: {
+ login: 'theowner',
+ },
+ default_branch: 'master',
},
}));
// getBranchCommit
@@ -619,8 +689,121 @@ describe('api/github', () => {
},
};
await github.mergePr(pr);
- expect(ghGot.put.mock.calls).toMatchSnapshot();
- expect(ghGot.delete.mock.calls).toMatchSnapshot();
+ expect(ghGot.put.mock.calls).toHaveLength(1);
+ expect(ghGot.delete.mock.calls).toHaveLength(1);
+ });
+ });
+ describe('mergePr(prNo)', () => {
+ it('should handle merge error', async () => {
+ await initRepo('some/repo', 'token');
+ const pr = {
+ number: 1234,
+ head: {
+ ref: 'someref',
+ },
+ };
+ ghGot.put.mockImplementationOnce(() => {
+ throw new Error('merge error');
+ });
+ await github.mergePr(pr);
+ expect(ghGot.put.mock.calls).toHaveLength(1);
+ expect(ghGot.delete.mock.calls).toHaveLength(0);
+ });
+ });
+ describe('mergePr(prNo) - autodetection', () => {
+ beforeEach(async () => {
+ async function guessInitRepo(...args) {
+ // repo info
+ ghGot.mockImplementationOnce(() => ({
+ body: {
+ owner: {
+ login: 'theowner',
+ },
+ default_branch: 'master',
+ },
+ }));
+ // getBranchCommit
+ ghGot.mockImplementationOnce(() => ({
+ body: {
+ object: {
+ sha: '1234',
+ },
+ },
+ }));
+ // getCommitTree
+ ghGot.mockImplementationOnce(() => ({
+ body: {
+ tree: {
+ sha: '5678',
+ },
+ },
+ }));
+ return github.initRepo(...args);
+ }
+ await guessInitRepo('some/repo', 'token');
+ ghGot.put = jest.fn();
+ });
+ it('should try rebase first', async () => {
+ const pr = {
+ number: 1235,
+ head: {
+ ref: 'someref',
+ },
+ };
+ await github.mergePr(pr);
+ expect(ghGot.put.mock.calls).toHaveLength(1);
+ expect(ghGot.delete.mock.calls).toHaveLength(1);
+ });
+ it('should try squash after rebase', async () => {
+ const pr = {
+ number: 1236,
+ head: {
+ ref: 'someref',
+ },
+ };
+ ghGot.put.mockImplementationOnce(() => {
+ throw new Error('no rebasing allowed');
+ });
+ await github.mergePr(pr);
+ expect(ghGot.put.mock.calls).toHaveLength(2);
+ expect(ghGot.delete.mock.calls).toHaveLength(1);
+ });
+ it('should try merge after squash', async () => {
+ const pr = {
+ number: 1237,
+ head: {
+ ref: 'someref',
+ },
+ };
+ ghGot.put.mockImplementationOnce(() => {
+ throw new Error('no rebasing allowed');
+ });
+ ghGot.put.mockImplementationOnce(() => {
+ throw new Error('no squashing allowed');
+ });
+ await github.mergePr(pr);
+ expect(ghGot.put.mock.calls).toHaveLength(3);
+ expect(ghGot.delete.mock.calls).toHaveLength(1);
+ });
+ it('should give up', async () => {
+ const pr = {
+ number: 1237,
+ head: {
+ ref: 'someref',
+ },
+ };
+ ghGot.put.mockImplementationOnce(() => {
+ throw new Error('no rebasing allowed');
+ });
+ ghGot.put.mockImplementationOnce(() => {
+ throw new Error('no squashing allowed');
+ });
+ ghGot.put.mockImplementationOnce(() => {
+ throw new Error('no merging allowed');
+ });
+ await github.mergePr(pr);
+ expect(ghGot.put.mock.calls).toHaveLength(3);
+ expect(ghGot.delete.mock.calls).toHaveLength(0);
});
});
describe('getFile(filePatch, branchName)', () => {
--
GitLab