From 91245b118bd67c58ec2091be2af8f2fb540b393c Mon Sep 17 00:00:00 2001 From: per1234 Date: Tue, 29 Mar 2022 19:27:49 -0700 Subject: [PATCH] Communicate lack of 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 As an attempt to work around the lack of a merge capability, "GitHub Label Sync" arbitrarily selects the first current label matching the configuration for processing and silently ignores the other matching labels. Under specific conditions, the workaround does as intended. Under other conditions, even the single matching current label it deigned to recognize would require a merge. Since it is treated as a normal "changed" entry, the attempt to change the name of the current label to the name of another current label is rejected by the GitHub API with a cryptic "422: Validation Failed" error, since it does not support merging labels in this manner. Neither behavior (silently ignoring a configuration or failing cryptically) provides a good user experience. The ideal would be to provide the required merge capability. Second best is to clearly communicate the situation to the user. The latter is implemented here. When a configuration requires a merge, GitHub Label Sync prints a clear error message identifying the problematic configuration element(s) and exits with status 1. The repository maintainer can then manually adjust the unsupported labels in order to begin using "GitHub Label Sync" successfully. This is done by adding a new "merge" diff entry type, which also lays the groundwork for the addition of a merge capability. --- lib/calculate-label-diff.js | 45 +++++++++---- lib/github-label-sync.js | 9 +++ lib/stringify-label-diff.js | 2 +- test/unit/lib/calculate-label-diff.test.js | 76 +++++++++++++++------- test/unit/lib/stringify-label-diff.test.js | 18 +++++ 5 files changed, 112 insertions(+), 38 deletions(-) diff --git a/lib/calculate-label-diff.js b/lib/calculate-label-diff.js index cf8510b..2fb48ff 100644 --- a/lib/calculate-label-diff.js +++ b/lib/calculate-label-diff.js @@ -7,35 +7,48 @@ function calculateLabelDiff(currentLabels, configuredLabels) { const resolvedLabels = []; configuredLabels.forEach((configuredLabel) => { - // Get current labels which match the configured label + // Get current labels which match the configured label name const matches = currentLabels.filter((currentLabel) => { if (currentLabel.name.toLowerCase() === configuredLabel.name.toLowerCase()) { return true; } + }); + // Get current labels which match an alias of the configured label + const aliasMatches=currentLabels.filter((currentLabel) => { if (configuredLabel.aliases && configuredLabel.aliases.map(label => label.toLowerCase()).indexOf(currentLabel.name.toLowerCase()) !== -1) { return true; } }); + // Alias matches must be processed after a name match in order to determine when a "merge" entry is appropriate + matches.push(...aliasMatches); + + resolvedLabels.push(...matches); // If we have no matches, the configured label is missing if (matches.length === 0) { return diff.push(createMissingEntry(configuredLabel)); } - // Always take the first match - const matchedLabel = matches[0]; - resolvedLabels.push(...matches); + matches.forEach((matchedLabel, index) => { - const matchedDescription = getLabelDescription(matchedLabel); - const configuredDescription = getLabelDescription(configuredLabel, matchedDescription); + const matchedDescription = getLabelDescription(matchedLabel); + const configuredDescription = getLabelDescription(configuredLabel, matchedDescription); - // If we have a match, but properties are not equal - if (configuredLabel.name !== matchedLabel.name || - configuredLabel.color !== matchedLabel.color || - configuredDescription !== matchedDescription - ) { - return diff.push(createChangedEntry(matchedLabel, configuredLabel)); - } + // If we have a match, but properties are not equal + if (configuredLabel.name !== matchedLabel.name || + configuredLabel.color !== matchedLabel.color || + configuredDescription !== matchedDescription + ) { + if (index === 0) { + // The first match can be handled as a simple change + return diff.push(createChangedEntry(matchedLabel, configuredLabel)); + } + + // Additional matches require a merge operation + return diff.push(createMergeEntry(matchedLabel, configuredLabel)); + } + + }); }); currentLabels.filter(label => resolvedLabels.indexOf(label) === -1).forEach((currentLabel) => { @@ -93,6 +106,12 @@ function createChangedEntry(actualLabel, expectedLabel) { return changedEntry; } +function createMergeEntry(actualLabel, expectedLabel) { + const mergeEntry = createChangedEntry(actualLabel, expectedLabel); + mergeEntry.type = 'merge'; + return mergeEntry; +} + function createAddedEntry(actualLabel) { const addedEntry = { name: actualLabel.name, diff --git a/lib/github-label-sync.js b/lib/github-label-sync.js index fd39572..47cad02 100644 --- a/lib/github-label-sync.js +++ b/lib/github-label-sync.js @@ -74,6 +74,15 @@ 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/lib/stringify-label-diff.js b/lib/stringify-label-diff.js index 5da7050..10c3f5b 100644 --- a/lib/stringify-label-diff.js +++ b/lib/stringify-label-diff.js @@ -7,7 +7,7 @@ function stringifyLabelDiff(labelDiff) { if (diffEntry.type === 'missing') { return `Missing: the "${diffEntry.name}" label is missing from the repo. It will be created.`; } - if (diffEntry.type === 'changed') { + if (diffEntry.type === 'changed' || diffEntry.type === 'merge') { const description = diffEntry.expected.description; return `Changed: the "${diffEntry.name}" label in the repo is out of date.` + diff --git a/test/unit/lib/calculate-label-diff.test.js b/test/unit/lib/calculate-label-diff.test.js index 111c24f..58f63a4 100644 --- a/test/unit/lib/calculate-label-diff.test.js +++ b/test/unit/lib/calculate-label-diff.test.js @@ -321,7 +321,7 @@ describe('lib/calculate-label-diff', () => { }); - describe('when a configured label alias exists in the current labels, and the configured label exists in the current labels', () => { + describe('when a configured label exists in the current labels but has changes, and the configured label\'s alias exists in the current labels', () => { beforeEach(() => { currentLabels = [ @@ -346,20 +346,34 @@ describe('lib/calculate-label-diff', () => { diff = calculateLabelDiff(currentLabels, configuredLabels); }); - it('should only add a "changed" entry for the first match to the returned diff', () => { - assert.lengthEquals(diff, 1); - assert.deepEqual(diff[0], { - name: 'bar', - type: 'changed', - actual: { - name: 'bar', - color: '00ff00' - }, - expected: { + it('should add a "changed" entry for the configured label and a "merge" entry for the alias to the returned diff', () => { + assert.lengthEquals(diff, 2); + assert.deepEqual(diff, [ + { name: 'foo', - color: '0000ff' + type: 'changed', + actual: { + name: 'foo', + color: 'ff0000' + }, + expected: { + name: 'foo', + color: '0000ff' + } + }, + { + name: 'bar', + type: 'merge', + actual: { + name: 'bar', + color: '00ff00' + }, + expected: { + name: 'foo', + color: '0000ff' + }, } - }); + ]); }); }); @@ -390,20 +404,34 @@ describe('lib/calculate-label-diff', () => { diff = calculateLabelDiff(currentLabels, configuredLabels); }); - it('should only add a "changed" entry for the first match to the returned diff', () => { - assert.lengthEquals(diff, 1); - assert.deepEqual(diff[0], { - name: 'bar', - type: 'changed', - actual: { + it('should add a "changed" entry for one alias and a "merge" entry for the other alias to the returned diff', () => { + assert.lengthEquals(diff, 2); + assert.deepEqual(diff, [ + { name: 'bar', - color: '00ff00' + type: 'changed', + actual: { + name: 'bar', + color: '00ff00' + }, + expected: { + name: 'baz', + color: '0000ff' + } }, - expected: { - name: 'baz', - color: '0000ff' + { + name: 'foo', + type: 'merge', + actual: { + name: 'foo', + color: 'ff0000' + }, + expected: { + name: 'baz', + color: '0000ff' + } } - }); + ]); }); }); diff --git a/test/unit/lib/stringify-label-diff.test.js b/test/unit/lib/stringify-label-diff.test.js index 368d6c3..b06e66c 100644 --- a/test/unit/lib/stringify-label-diff.test.js +++ b/test/unit/lib/stringify-label-diff.test.js @@ -58,6 +58,24 @@ describe('lib/stringify-label-diff', () => { assert.strictEqual(stringifiedDiff[0], 'Changed: the "foo" label in the repo is out of date. It will be updated to "bar" with color "#00ff00".'); }); + it('should stringify "merge" diff entries', () => { + labelDiff.push({ + name: 'foo', + type: 'merge', + actual: { + name: 'foo', + color: 'ff0000' + }, + expected: { + name: 'bar', + color: '00ff00' + } + }); + stringifiedDiff = stringifyLabelDiff(labelDiff); + assert.lengthEquals(stringifiedDiff, 1); + assert.strictEqual(stringifiedDiff[0], 'Changed: the "foo" label in the repo is out of date. It will be updated to "bar" with color "#00ff00".'); + }); + it('should stringify "added" diff entries', () => { labelDiff.push({ name: 'foo',