From 65b6359b52f7f572e0358854f1858b67e1764776 Mon Sep 17 00:00:00 2001 From: per1234 Date: Sun, 3 Apr 2022 20:34:30 -0700 Subject: [PATCH] Add support for label merge There are two scenarios where handling a label configuration would require merging one label into another: - The configured label exists in the current labels and its alias exists in the current labels - Multiple aliases of a configured label exist in the current labels Previously, "GitHub Label Sync" did not have support for label merge, and would return an error when the user's configuration would have required a merge operation. The merge capability is added here. The GitHub labels API does not provide a merge capability, so merging labels requires the use of the issues API to add the "expected" label to each issue/PR that has the "actual" label. From the user's perspective, the outcome of this "merge" operation is no different from any simple "changed" operation: the "actual" label has been replaced by the "expected" label in the repository labels and all issues/PRs the label was applied to. --- lib/action-label-diff.js | 22 +++ lib/github-label-api.js | 48 ++++++ lib/github-label-sync.js | 9 -- test/unit/lib/action-label-diff.test.js | 115 ++++++++++++++ test/unit/lib/github-label-api.test.js | 198 ++++++++++++++++++++++++ test/unit/mock/github-label-api.js | 2 + 6 files changed, 385 insertions(+), 9 deletions(-) diff --git a/lib/action-label-diff.js b/lib/action-label-diff.js index ed08a7f..18246e5 100644 --- a/lib/action-label-diff.js +++ b/lib/action-label-diff.js @@ -14,6 +14,28 @@ function actionLabelDiff(options) { if (diffEntry.type === 'changed') { return apiClient.updateLabel(repo, diffEntry.name, diffEntry.expected); } + if (diffEntry.type === 'merge') { + return apiClient.getLabeledIssues(repo, diffEntry.name) // Get all issues and PRs that have the "actual" label + .then((issues) => { + const mergeIssues = issues.filter((issue) => { + // Per-issue label application is needed where the "expected" label is not already present + return !issue.labels.some((label) => { + return label.name === diffEntry.expected.name; + }); + }); + + // Add the "expected" label to the issues + const issueActions = mergeIssues.map((issue) => { + return apiClient.labelIssue(repo, issue.number, diffEntry.expected.name); + }); + + return Promise.all(issueActions); + }) + .then(() => { + // Now that all issues with "actual" label have "expected" label, "actual" label can be deleted + return apiClient.deleteLabel(repo, diffEntry.name); + }); + } if (diffEntry.type === 'added') { return apiClient.deleteLabel(repo, diffEntry.name); } diff --git a/lib/github-label-api.js b/lib/github-label-api.js index 78d9988..bf87814 100644 --- a/lib/github-label-api.js +++ b/lib/github-label-api.js @@ -87,6 +87,54 @@ class ApiClient { }); } + getLabeledIssues (repo, labelName) { + return new Promise((resolve, reject) => { + const allIssues = []; + const endpoint = `/repos/${repo}/issues`; + const params = { + labels: labelName, + page: 1, + per_page: 100, + state: 'all' + }; + const getCallback = (error, status, issues) => { + if (error) { + error.method = 'GET'; + error.endpoint = endpoint; + return reject(error); + } + if (status !== 200) { + return reject(new Error(`API responded with ${status} status`)); + } + allIssues.push.apply(allIssues, issues); + if (issues.length === params.per_page) { + params.page += 1; + this.apiClient.get(endpoint, params, getCallback); + } else { + resolve(allIssues); + } + }; + this.apiClient.get(endpoint, params, getCallback); + }); + } + + labelIssue (repo, issueNumber, labelName) { + return new Promise((resolve, reject) => { + const endpoint = `/repos/${repo}/issues/${issueNumber}/labels`; + this.apiClient.post(endpoint, {labels: [labelName]}, (error, status) => { + if (error) { + error.method = 'POST'; + error.endpoint = endpoint; + return reject(error); + } + if (status !== 200) { + return reject(new Error(`API responded with ${status} status`)); + } + resolve(); + }); + }); + } + deleteLabel (repo, labelName) { labelName = encodeURIComponent(labelName); return new Promise((resolve, reject) => { diff --git a/lib/github-label-sync.js b/lib/github-label-sync.js index 47cad02..fd39572 100644 --- a/lib/github-label-sync.js +++ b/lib/github-label-sync.js @@ -74,15 +74,6 @@ function githubLabelSync(options) { stringifyLabelDiff(labelDiff).forEach((diffLine) => { log.info(format.diff(diffLine)); }); - - const labelDiffMerges = labelDiff.filter((diff) => { - return diff.type === 'merge'; - }); - if (labelDiffMerges.length) { - const messages = labelDiffMerges.map(diff => `Configuration of the "${diff.expected.name}" label requires unsupported merge operation`); - return Promise.reject({ message: messages.join('\n\n') }); - } - return labelDiff; }) .then((labelDiff) => { diff --git a/test/unit/lib/action-label-diff.test.js b/test/unit/lib/action-label-diff.test.js index 3eadc2d..d602b64 100644 --- a/test/unit/lib/action-label-diff.test.js +++ b/test/unit/lib/action-label-diff.test.js @@ -77,6 +77,121 @@ describe('lib/action-label-diff', () => { assert.strictEqual(actions[0], updatePromise); }); + it('should convert "merge" diff entries to label API delete promises', () => { + const issues = [ + { + number: 11, + labels: [ + { + name: 'bar' + } + ] + }, + { + number: 42, + labels: [ + { + name: 'bar' + }, + { + name: 'baz' + } + ] + } + ]; + apiClient.getLabeledIssues.returns(Promise.resolve(issues)); + apiClient.labelIssue.returns(Promise.resolve()); + const deletePromiseValue = 'asdf'; + apiClient.deleteLabel.returns(Promise.resolve(deletePromiseValue)); + options.diff.push({ + name: 'foo', + type: 'merge', + actual: { + name: 'bar', + color: 'ff0000', + description: 'baz' + }, + expected: { + name: 'foo', + color: '00ff00', + description: 'baz' + } + }); + actions = actionLabelDiff(options); + assert.lengthEquals(actions, 1); + return actions[0] + .then((value) => { + assert.calledOnce(apiClient.getLabeledIssues); + assert.calledWithExactly(apiClient.getLabeledIssues, options.repo, options.diff[0].name); + assert.calledTwice(apiClient.labelIssue); + assert.calledWithExactly(apiClient.labelIssue.getCall(0), options.repo, issues[0].number, options.diff[0].expected.name); + assert.calledWithExactly(apiClient.labelIssue.getCall(1), options.repo, issues[1].number, options.diff[0].expected.name); + assert.calledOnce(apiClient.deleteLabel); + assert.calledWithExactly(apiClient.deleteLabel, options.repo, options.diff[0].name); + assert.strictEqual(value, deletePromiseValue); + }); + }); + + it('should not attempt redundant issue labeling for "merge" diff entries', () => { + const issues = [ + { + number: 11, + labels: [ + { + name: 'bar' + } + ] + }, + { + number: 42, + labels: [ + { + name: 'bar' + }, + { + name: 'foo' + } + ] + }, + { + number: 123, + labels: [ + { + name: 'bar' + }, + { + name: 'baz' + } + ] + } + ]; + apiClient.getLabeledIssues.returns(Promise.resolve(issues)); + apiClient.labelIssue.returns(Promise.resolve()); + apiClient.deleteLabel.returns(Promise.resolve()); + options.diff.push({ + name: 'foo', + type: 'merge', + actual: { + name: 'bar', + color: 'ff0000', + description: 'baz' + }, + expected: { + name: 'foo', + color: '00ff00', + description: 'baz' + } + }); + actions = actionLabelDiff(options); + assert.lengthEquals(actions, 1); + return actions[0] + .then(() => { + assert.calledTwice(apiClient.labelIssue); + assert.calledWithExactly(apiClient.labelIssue.getCall(0), options.repo, issues[0].number, options.diff[0].expected.name); + assert.calledWithExactly(apiClient.labelIssue.getCall(1), options.repo, issues[2].number, options.diff[0].expected.name); + }); + }); + it('should convert "added" diff entries to label API delete promises', () => { const deletePromise = Promise.resolve(); apiClient.deleteLabel.returns(deletePromise); diff --git a/test/unit/lib/github-label-api.test.js b/test/unit/lib/github-label-api.test.js index 2b196ce..ef96ec1 100644 --- a/test/unit/lib/github-label-api.test.js +++ b/test/unit/lib/github-label-api.test.js @@ -350,6 +350,204 @@ describe('lib/github-label-api', () => { }); + it('should have a `getLabeledIssues` method', () => { + assert.isFunction(instance.getLabeledIssues); + }); + + describe('.getLabeledIssues(repo, labelName)', () => { + const issues = ['foo', 'bar', 'baz']; + const repo = 'foo/bar'; + const labelName = 'baz qux'; + let returnedPromise; + + beforeEach(() => { + instance.apiClient.get.yieldsAsync(null, 200, issues); + returnedPromise = instance.getLabeledIssues(repo, labelName); + }); + + it('should return a promise', () => { + assert.instanceOf(returnedPromise, Promise); + }); + + it('should make a GET request to the repo issues endpoint', () => { + assert.calledOnce(instance.apiClient.get); + assert.calledWith(instance.apiClient.get, `/repos/${repo}/issues`, {labels: labelName, page: 1, per_page: 100, state: 'all'}); + }); + + describe('.then()', () => { + let resolvedValue; + + beforeEach((done) => { + returnedPromise.then((value) => { + resolvedValue = value; + done(); + }).catch(done); + }); + + it('should resolve with the repo issues', () => { + assert.deepEqual(resolvedValue, issues); + }); + + }); + + describe('when the API client errors', () => { + const apiClientError = new Error('API client error'); + + beforeEach(() => { + instance.apiClient.get.yieldsAsync(apiClientError); + returnedPromise = instance.getLabeledIssues(repo, labelName); + }); + + describe('.catch()', () => { + let caughtError; + + beforeEach((done) => { + returnedPromise.then(done).catch((error) => { + caughtError = error; + done(); + }); + }); + + it('should fail with the API client error', () => { + assert.strictEqual(caughtError, apiClientError); + }); + + it('should augment the API client error with request information', () => { + assert.strictEqual(caughtError.method, 'GET'); + assert.strictEqual(caughtError.endpoint, `/repos/${repo}/issues`); + }); + + }); + + }); + + describe('when the API response with a non-200 status', () => { + + beforeEach(() => { + instance.apiClient.get.yieldsAsync(null, 404); + returnedPromise = instance.getLabeledIssues(repo, labelName); + }); + + describe('.catch()', () => { + let caughtError; + + beforeEach((done) => { + returnedPromise.then(done).catch((error) => { + caughtError = error; + done(); + }); + }); + + it('should fail with a status error', () => { + assert.instanceOf(caughtError, Error); + assert.strictEqual(caughtError.message, 'API responded with 404 status'); + }); + + }); + + }); + + }); + + it('should have a `labelIssue` method', () => { + assert.isFunction(instance.labelIssue); + }); + + describe('.labelIssue(repo, issueNumber, labelName)', () => { + const issueNumber = '42'; + const labelName = 'baz qux'; + const repo = 'foo/bar'; + let returnedPromise; + + beforeEach(() => { + instance.apiClient.post.yieldsAsync(null, 200); + returnedPromise = instance.labelIssue(repo, issueNumber, labelName); + }); + + it('should return a promise', () => { + assert.instanceOf(returnedPromise, Promise); + }); + + it('should make a POST request to the repo issue labels endpoint (encoding `issueNumber`)', () => { + assert.calledOnce(instance.apiClient.post); + assert.calledWith(instance.apiClient.post, `/repos/${repo}/issues/${issueNumber}/labels`, {labels: [labelName]}); + }); + + describe('.then()', () => { + let resolvedValue; + + beforeEach((done) => { + returnedPromise.then((value) => { + resolvedValue = value; + done(); + }).catch(done); + }); + + it('should resolve with no value', () => { + assert.isUndefined(resolvedValue); + }); + + }); + + describe('when the API client errors', () => { + const apiClientError = new Error('API client error'); + + beforeEach(() => { + instance.apiClient.post.yieldsAsync(apiClientError); + returnedPromise = instance.labelIssue(repo, issueNumber, labelName); + }); + + describe('.catch()', () => { + let caughtError; + + beforeEach((done) => { + returnedPromise.then(done).catch((error) => { + caughtError = error; + done(); + }); + }); + + it('should fail with the API client error', () => { + assert.strictEqual(caughtError, apiClientError); + }); + + it('should augment the API client error with request information', () => { + assert.strictEqual(caughtError.method, 'POST'); + assert.strictEqual(caughtError.endpoint, `/repos/${repo}/issues/${issueNumber}/labels`); + }); + + }); + + }); + + describe('when the API response with a non-200 status', () => { + + beforeEach(() => { + instance.apiClient.post.yieldsAsync(null, 422); + returnedPromise = instance.labelIssue(repo, issueNumber, labelName); + }); + + describe('.catch()', () => { + let caughtError; + + beforeEach((done) => { + returnedPromise.then(done).catch((error) => { + caughtError = error; + done(); + }); + }); + + it('should fail with a status error', () => { + assert.instanceOf(caughtError, Error); + assert.strictEqual(caughtError.message, 'API responded with 422 status'); + }); + + }); + + }); + + }); + it('should have a `deleteLabel` method', () => { assert.isFunction(instance.deleteLabel); }); diff --git a/test/unit/mock/github-label-api.js b/test/unit/mock/github-label-api.js index 0c9c4ce..2ee903a 100644 --- a/test/unit/mock/github-label-api.js +++ b/test/unit/mock/github-label-api.js @@ -7,7 +7,9 @@ module.exports = sinon.stub(); module.exports.mockClient = { createLabel: sinon.stub(), deleteLabel: sinon.stub(), + getLabeledIssues: sinon.stub(), getLabels: sinon.stub(), + labelIssue: sinon.stub(), updateLabel: sinon.stub() }; module.exports.returns(module.exports.mockClient);