Fix label color padding and report invalid label colors

lib/plugins/labels.js

@@ -21,12 +21,12 @@ module.exports = class Labels extends Diffable {
 
     if (this.entries) {
       this.entries.forEach(label => {
-        // Force color to string since some hex colors can be numerical (e.g. 999999)
-        if (label.color) {
+        // Force color to string since some hex colors can be numerical (e.g. 999999).
+        // Numbers lose leading zeros in YAML (000001 becomes 1), so only they are padded.
+        if (typeof label.color === 'number') {
+          label.color = String(label.color).padStart(6, '0')
+        } else if (label.color !== undefined && label.color !== null) {
           label.color = String(label.color).replace(/^#/, '')
-          if (label.color.length < 6) {
-            label.color.padStart(6, '0')
-          }
         }
       })
     }
@@ -61,6 +61,9 @@ module.exports = class Labels extends Diffable {
     this.log.debug(`Updating labels for ${JSON.stringify(attrs, null, 4)}   ${JSON.stringify(this.repo, null, 4)}`)
     attrs.current_name = attrs.oldname || attrs.name
     delete attrs.oldname
+    if (this.hasInvalidColor(attrs)) {
+      return this.invalidColorError(attrs)
+    }
     if (this.nop) {
       return Promise.resolve([
         new NopCommand(this.constructor.name, this.repo, this.github.rest.issues.updateLabel.endpoint(this.wrapAttrs(attrs)), 'Update label')
@@ -70,6 +73,9 @@ module.exports = class Labels extends Diffable {
   }
 
   add (attrs) {
+    if (this.hasInvalidColor(attrs)) {
+      return this.invalidColorError(attrs)
+    }
     if (this.nop) {
       return Promise.resolve([
         new NopCommand(this.constructor.name, this.repo, this.github.rest.issues.createLabel.endpoint(this.wrapAttrs(attrs)), 'Create label')
@@ -98,4 +104,18 @@ module.exports = class Labels extends Diffable {
   isExcluded (label) {
     return this.exclude.some(rx => rx.test(label.name))
   }
+
+  hasInvalidColor (label) {
+    return label.color !== undefined && !/^[0-9a-f]{6}$/i.test(label.color)
+  }
+
+  // YAML reads unquoted colors like 5319e7 as numbers (5319 × 10^7), which can't be recovered here.
+  invalidColorError (label) {
+    const msg = `Label '${label.name}' has invalid color '${label.color}'. Colors must be 6 hex characters, e.g. '5319e7'. Quote the value in YAML so it isn't read as a number.`
+    if (this.nop) {
+      return Promise.resolve([new NopCommand(this.constructor.name, this.repo, null, msg, 'ERROR')])
+    }
+    this.logError(msg)
+    return Promise.resolve([])
+  }
 }

test/unit/lib/plugins/labels.test.js

@@ -148,5 +148,70 @@ describe('Labels', () => {
         expect(github.rest.issues.createLabel).toHaveBeenCalledTimes(1)
       })
     })
+
+    it('pads colors that YAML read as numbers', () => {
+      github.paginate.mockReturnValueOnce(Promise.resolve([]))
+
+      const plugin = configure([
+        { name: 'black', color: 0, description: '' }, // YAML treats `color: 000000` as 0
+        { name: 'almost-black', color: 1, description: '' } // YAML treats `color: 000001` as 1
+      ])
+
+      return plugin.sync().then(() => {
+        const createdColors = github.rest.issues.createLabel.mock.calls.map(([params]) => params.color)
+        expect(createdColors).toEqual(['000000', '000001'])
+      })
+    })
+
+    describe('invalid colors', () => {
+      // YAML treats `color: 5319e7` as 5319 × 10^7
+      const config = [{ name: 'hello-world', color: 5319e7, description: '' }]
+      const expectedMessage = "Label 'hello-world' has invalid color '53190000000'. Colors must be 6 hex characters, e.g. '5319e7'. Quote the value in YAML so it isn't read as a number."
+
+      it('reports an error instead of creating the label', () => {
+        github.paginate.mockReturnValueOnce(Promise.resolve([]))
+        log.error = jest.fn()
+        const errors = []
+        const plugin = new Labels(false, github, { owner: 'bkeepers', repo: 'test' }, config, log, errors)
+
+        return plugin.sync().then(() => {
+          expect(github.rest.issues.createLabel).not.toHaveBeenCalled()
+          expect(errors).toEqual([expect.objectContaining({ msg: expectedMessage, plugin: 'Labels' })])
+        })
+      })
+
+      it('reports an error instead of updating the label', () => {
+        github.paginate.mockReturnValueOnce(Promise.resolve([{ name: 'hello-world', color: '5319e7', description: '' }]))
+        log.error = jest.fn()
+        const errors = []
+        const plugin = new Labels(false, github, { owner: 'bkeepers', repo: 'test' }, config, log, errors)
+
+        return plugin.sync().then(() => {
+          expect(github.rest.issues.updateLabel).not.toHaveBeenCalled()
+          expect(errors).toEqual([expect.objectContaining({ msg: expectedMessage })])
+        })
+      })
+
+      it('shows the error in the dry-run', () => {
+        github.paginate.mockReturnValueOnce(Promise.resolve([]))
+        const plugin = new Labels(true, github, { owner: 'bkeepers', repo: 'test' }, config, log, [])
+
+        return plugin.sync().then(results => {
+          expect(results.flat()).toContainEqual(expect.objectContaining({ type: 'ERROR', action: expect.objectContaining({ msg: expectedMessage }) }))
+        })
+      })
+
+      it('reports a quoted non-hex color as written, without padding it', () => {
+        github.paginate.mockReturnValueOnce(Promise.resolve([]))
+        log.error = jest.fn()
+        const errors = []
+        const plugin = new Labels(false, github, { owner: 'bkeepers', repo: 'test' }, [{ name: 'hello-world', color: 'red', description: '' }], log, errors)
+
+        return plugin.sync().then(() => {
+          expect(github.rest.issues.createLabel).not.toHaveBeenCalled()
+          expect(errors).toEqual([expect.objectContaining({ msg: expect.stringContaining("invalid color 'red'. Colors must be 6 hex characters") })])
+        })
+      })
+    })
   })
 })