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.
Financial-Times · Mar 30, 2022 · 91245b1 · 91245b1
1 parent 4cc843f
commit 91245b1
Show file tree

Hide file tree

Showing 5 changed files with 112 additions and 38 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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',