From 286fa028ecde0f32a61c9be13db0119654ff4b9b Mon Sep 17 00:00:00 2001 From: Ji Tran Date: Fri, 20 Dec 2024 12:35:24 +1100 Subject: [PATCH 1/9] Allow branch protection settings to be overridden outside of safe settings. --- lib/plugins/branches.js | 35 +++++++++++++++++++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/lib/plugins/branches.js b/lib/plugins/branches.js index cac0aadf..a53a2e94 100644 --- a/lib/plugins/branches.js +++ b/lib/plugins/branches.js @@ -3,6 +3,9 @@ const NopCommand = require('../nopcommand') const MergeDeep = require('../mergeDeep') const ignorableFields = [] const previewHeaders = { accept: 'application/vnd.github.hellcat-preview+json,application/vnd.github.luke-cage-preview+json,application/vnd.github.zzzax-preview+json' } +const overrides = [ + 'required_status_checks.contexts', +] module.exports = class Branches extends ErrorStash { constructor (nop, github, repo, settings, log, errors) { @@ -49,7 +52,7 @@ module.exports = class Branches extends ErrorStash { const params = Object.assign({}, p) return this.github.repos.getBranchProtection(params).then((result) => { const mergeDeep = new MergeDeep(this.log, this.github, ignorableFields) - const changes = mergeDeep.compareDeep({ branch: { protection: this.reformatAndReturnBranchProtection(result.data) } }, { branch: { protection: branch.protection } }) + const changes = mergeDeep.compareDeep({ branch: { protection: this.reformatAndReturnBranchProtection(result.data) } }, { branch: { protection: this.removeOverrides(branch.protection, result.data) } }) const results = { msg: `Followings changes will be applied to the branch protection for ${params.branch.name} branch`, additions: changes.additions, modifications: changes.modifications, deletions: changes.deletions } this.log.debug(`Result of compareDeep = ${results}`) @@ -76,7 +79,7 @@ module.exports = class Branches extends ErrorStash { return this.github.repos.updateBranchProtection(params).then(res => this.log(`Branch protection applied successfully ${JSON.stringify(res.url)}`)).catch(e => { this.logError(`Error applying branch protection ${JSON.stringify(e)}`); return [] }) }).catch((e) => { if (e.status === 404) { - Object.assign(params, branch.protection, { headers: previewHeaders }) + Object.assign(params, this.removeOverrides(branch.protection, {}), { headers: previewHeaders }) if (this.nop) { resArray.push(new NopCommand(this.constructor.name, this.repo, this.github.repos.updateBranchProtection.endpoint(params), 'Add Branch Protection')) return Promise.resolve(resArray) @@ -122,4 +125,32 @@ module.exports = class Branches extends ErrorStash { } return protection } + + getLastKey (obj, property) { + const keys = property.split('.') + for (let i = 0; i < keys.length - 1; i++) { + if (!obj[keys[i]]) return [obj, keys[i]] + obj = obj[keys[i]] + } + return [obj, keys[keys.length - 1]] + } + + removeOverrides (source, target) { + if (source) { + overrides.forEach(override => { + let [obj, lastKey] = this.getLastKey(source, override) + if ((Array.isArray(obj[lastKey]) || typeof obj[lastKey] === 'string') && obj[lastKey].includes('{{EXTERNALLY_DEFINED}}')) { + let [obj2, lastKey2] = this.getLastKey(target, override) + if (obj2[lastKey2]) { + obj[lastKey] = obj2[lastKey2] + } else if (Array.isArray(obj[lastKey])) { + obj[lastKey] = [] + } else { + obj[lastKey] = '' + } + } + }) + } + return source + } } From 6582bb22a2921f2b3e5d42892f554f6c905352bd Mon Sep 17 00:00:00 2001 From: Ji Tran Date: Tue, 7 Jan 2025 11:06:11 +1100 Subject: [PATCH 2/9] Allow ruleset settings to be overridden outside of safe settings. --- lib/plugins/branches.js | 7 +++--- lib/plugins/rulesets.js | 52 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 3 deletions(-) diff --git a/lib/plugins/branches.js b/lib/plugins/branches.js index a53a2e94..3148c116 100644 --- a/lib/plugins/branches.js +++ b/lib/plugins/branches.js @@ -129,18 +129,19 @@ module.exports = class Branches extends ErrorStash { getLastKey (obj, property) { const keys = property.split('.') for (let i = 0; i < keys.length - 1; i++) { - if (!obj[keys[i]]) return [obj, keys[i]] + if (!obj[keys[i]]) return [null, null] obj = obj[keys[i]] } return [obj, keys[keys.length - 1]] } - removeOverrides (source, target) { + removeOverrides (source, existing) { if (source) { overrides.forEach(override => { let [obj, lastKey] = this.getLastKey(source, override) + if (!obj) return if ((Array.isArray(obj[lastKey]) || typeof obj[lastKey] === 'string') && obj[lastKey].includes('{{EXTERNALLY_DEFINED}}')) { - let [obj2, lastKey2] = this.getLastKey(target, override) + let [obj2, lastKey2] = this.getLastKey(existing, override) if (obj2[lastKey2]) { obj[lastKey] = obj2[lastKey2] } else if (Array.isArray(obj[lastKey])) { diff --git a/lib/plugins/rulesets.js b/lib/plugins/rulesets.js index 6846e1ce..cb78b8b8 100644 --- a/lib/plugins/rulesets.js +++ b/lib/plugins/rulesets.js @@ -2,6 +2,9 @@ const Diffable = require('./diffable') const NopCommand = require('../nopcommand') const MergeDeep = require('../mergeDeep') const ignorableFields = [] +const overrides = [ + 'required_status_checks', +] const version = { 'X-GitHub-Api-Version': '2022-11-28' @@ -88,6 +91,51 @@ module.exports = class Rulesets extends Diffable { return merged.hasChanges } + getObjectRef (data, dataKey) { + const results = [] + const traverse = (obj) => { + for (const key in obj) { + if (key === dataKey) { + results.push(obj) + } else if (Array.isArray(obj[key])) { + obj[key].forEach(element => traverse(element)) + } else if (typeof obj[key] === 'object' && obj[key]) { + traverse(obj[key]) + } + } + } + traverse(data) + return results + } + + // When {{EXTERNALLY_DEFINED}} is found in the override value, retain the + // existing value from GitHub. + // Note: + // - The admin settings could define multiple overrides, but the GitHub API + // retains one only. + // - The PUT method for rulesets (update) allows for multiple overrides. + // - The POST method for rulesets (create) allows for one override only. + removeOverrides (source, existing) { + overrides.forEach(override => { + let sourceRefs = this.getObjectRef(source, override) + let data = JSON.stringify(sourceRefs, null, 2) + if (data.includes('{{EXTERNALLY_DEFINED}}')) { + let existingRefs = this.getObjectRef(existing, override) + sourceRefs.forEach(sourceRef => { + if (existingRefs[0]) { + sourceRef[override] = existingRefs[0][override] + } else if (Array.isArray(sourceRef[override])) { + sourceRef[override] = [] + } else if (typeof sourceRef[override] === 'object' && sourceRef[override]) { + sourceRef[override] = {} + } else { + sourceRef[override] = '' + } + }) + } + }) + } + update (existing, attrs) { const parms = this.wrapAttrs(Object.assign({ id: existing.id }, attrs)) if (this.scope === 'org') { @@ -96,6 +144,7 @@ module.exports = class Rulesets extends Diffable { new NopCommand(this.constructor.name, this.repo, this.github.request.endpoint('PUT /orgs/{org}/rulesets/{id}', parms), 'Update Ruleset') ]) } + this.removeOverrides(parms, existing) this.log.debug(`Updating Ruleset with the following values ${JSON.stringify(parms, null, 2)}`) return this.github.request('PUT /orgs/{org}/rulesets/{id}', parms).then(res => { this.log(`Ruleset updated successfully ${JSON.stringify(res.url)}`) @@ -109,6 +158,7 @@ module.exports = class Rulesets extends Diffable { new NopCommand(this.constructor.name, this.repo, this.github.request.endpoint('PUT /repos/{owner}/{repo}/rulesets/{id}', parms), 'Update Ruleset') ]) } + this.removeOverrides(parms, existing) this.log.debug(`Updating Ruleset with the following values ${JSON.stringify(parms, null, 2)}`) return this.github.request('PUT /repos/{owner}/{repo}/rulesets/{id}', parms).then(res => { this.log(`Ruleset updated successfully ${JSON.stringify(res.url)}`) @@ -126,6 +176,7 @@ module.exports = class Rulesets extends Diffable { new NopCommand(this.constructor.name, this.repo, this.github.request.endpoint('POST /orgs/{org}/rulesets', this.wrapAttrs(attrs)), 'Create Ruleset') ]) } + this.removeOverrides(attrs, {}) this.log.debug(`Creating Rulesets with the following values ${JSON.stringify(attrs, null, 2)}`) return this.github.request('POST /orgs/{org}/rulesets', this.wrapAttrs(attrs)).then(res => { this.log(`Ruleset created successfully ${JSON.stringify(res.url)}`) @@ -139,6 +190,7 @@ module.exports = class Rulesets extends Diffable { new NopCommand(this.constructor.name, this.repo, this.github.request.endpoint('POST /repos/{owner}/{repo}/rulesets', this.wrapAttrs(attrs)), 'Create Ruleset') ]) } + this.removeOverrides(attrs, {}) this.log.debug(`Creating Rulesets with the following values ${JSON.stringify(attrs, null, 2)}`) return this.github.request('POST /repos/{owner}/{repo}/rulesets', this.wrapAttrs(attrs)).then(res => { this.log(`Ruleset created successfully ${JSON.stringify(res.url)}`) From 927536ebfa0bb91b52319d498a2f96f87eac0922 Mon Sep 17 00:00:00 2001 From: Ji Tran Date: Tue, 7 Jan 2025 12:54:57 +1100 Subject: [PATCH 3/9] Created Overrides module for handling externally defined settings. --- lib/plugins/branches.js | 36 +++------------------------ lib/plugins/overrides.js | 49 ++++++++++++++++++++++++++++++++++++ lib/plugins/rulesets.js | 54 ++++------------------------------------ 3 files changed, 58 insertions(+), 81 deletions(-) create mode 100644 lib/plugins/overrides.js diff --git a/lib/plugins/branches.js b/lib/plugins/branches.js index 3148c116..6e21ad30 100644 --- a/lib/plugins/branches.js +++ b/lib/plugins/branches.js @@ -1,10 +1,11 @@ const ErrorStash = require('./errorStash') const NopCommand = require('../nopcommand') const MergeDeep = require('../mergeDeep') +const Overrides = require('./overrides') const ignorableFields = [] const previewHeaders = { accept: 'application/vnd.github.hellcat-preview+json,application/vnd.github.luke-cage-preview+json,application/vnd.github.zzzax-preview+json' } const overrides = [ - 'required_status_checks.contexts', + 'contexts', ] module.exports = class Branches extends ErrorStash { @@ -52,7 +53,7 @@ module.exports = class Branches extends ErrorStash { const params = Object.assign({}, p) return this.github.repos.getBranchProtection(params).then((result) => { const mergeDeep = new MergeDeep(this.log, this.github, ignorableFields) - const changes = mergeDeep.compareDeep({ branch: { protection: this.reformatAndReturnBranchProtection(result.data) } }, { branch: { protection: this.removeOverrides(branch.protection, result.data) } }) + const changes = mergeDeep.compareDeep({ branch: { protection: this.reformatAndReturnBranchProtection(result.data) } }, { branch: { protection: Overrides.removeOverrides(overrides, branch.protection, result.data) } }) const results = { msg: `Followings changes will be applied to the branch protection for ${params.branch.name} branch`, additions: changes.additions, modifications: changes.modifications, deletions: changes.deletions } this.log.debug(`Result of compareDeep = ${results}`) @@ -79,7 +80,7 @@ module.exports = class Branches extends ErrorStash { return this.github.repos.updateBranchProtection(params).then(res => this.log(`Branch protection applied successfully ${JSON.stringify(res.url)}`)).catch(e => { this.logError(`Error applying branch protection ${JSON.stringify(e)}`); return [] }) }).catch((e) => { if (e.status === 404) { - Object.assign(params, this.removeOverrides(branch.protection, {}), { headers: previewHeaders }) + Object.assign(params, Overrides.removeOverrides(overrides, branch.protection, {}), { headers: previewHeaders }) if (this.nop) { resArray.push(new NopCommand(this.constructor.name, this.repo, this.github.repos.updateBranchProtection.endpoint(params), 'Add Branch Protection')) return Promise.resolve(resArray) @@ -125,33 +126,4 @@ module.exports = class Branches extends ErrorStash { } return protection } - - getLastKey (obj, property) { - const keys = property.split('.') - for (let i = 0; i < keys.length - 1; i++) { - if (!obj[keys[i]]) return [null, null] - obj = obj[keys[i]] - } - return [obj, keys[keys.length - 1]] - } - - removeOverrides (source, existing) { - if (source) { - overrides.forEach(override => { - let [obj, lastKey] = this.getLastKey(source, override) - if (!obj) return - if ((Array.isArray(obj[lastKey]) || typeof obj[lastKey] === 'string') && obj[lastKey].includes('{{EXTERNALLY_DEFINED}}')) { - let [obj2, lastKey2] = this.getLastKey(existing, override) - if (obj2[lastKey2]) { - obj[lastKey] = obj2[lastKey2] - } else if (Array.isArray(obj[lastKey])) { - obj[lastKey] = [] - } else { - obj[lastKey] = '' - } - } - }) - } - return source - } } diff --git a/lib/plugins/overrides.js b/lib/plugins/overrides.js new file mode 100644 index 00000000..400b2295 --- /dev/null +++ b/lib/plugins/overrides.js @@ -0,0 +1,49 @@ +const ErrorStash = require('./errorStash') + +module.exports = class Overrides extends ErrorStash { + static getObjectRef (data, dataKey) { + const results = [] + const traverse = (obj) => { + for (const key in obj) { + if (key === dataKey) { + results.push(obj) + } else if (Array.isArray(obj[key])) { + obj[key].forEach(element => traverse(element)) + } else if (typeof obj[key] === 'object' && obj[key]) { + traverse(obj[key]) + } + } + } + traverse(data) + return results + } + + // When {{EXTERNALLY_DEFINED}} is found in the override value, retain the + // existing value from GitHub. + // Note: + // - The admin settings could define multiple overrides, but the GitHub API + // retains one only. + // - The PUT method for rulesets (update) allows for multiple overrides. + // - The POST method for rulesets (create) allows for one override only. + static removeOverrides (overrides, source, existing) { + overrides.forEach(override => { + let sourceRefs = Overrides.getObjectRef(source, override) + let data = JSON.stringify(sourceRefs, null, 2) + if (data.includes('{{EXTERNALLY_DEFINED}}')) { + let existingRefs = Overrides.getObjectRef(existing, override) + sourceRefs.forEach(sourceRef => { + if (existingRefs[0]) { + sourceRef[override] = existingRefs[0][override] + } else if (Array.isArray(sourceRef[override])) { + sourceRef[override] = [] + } else if (typeof sourceRef[override] === 'object' && sourceRef[override]) { + sourceRef[override] = {} + } else { + sourceRef[override] = '' + } + }) + } + }) + return source + } +} diff --git a/lib/plugins/rulesets.js b/lib/plugins/rulesets.js index cb78b8b8..9e80836a 100644 --- a/lib/plugins/rulesets.js +++ b/lib/plugins/rulesets.js @@ -1,6 +1,7 @@ const Diffable = require('./diffable') const NopCommand = require('../nopcommand') const MergeDeep = require('../mergeDeep') +const Overrides = require('./overrides') const ignorableFields = [] const overrides = [ 'required_status_checks', @@ -91,51 +92,6 @@ module.exports = class Rulesets extends Diffable { return merged.hasChanges } - getObjectRef (data, dataKey) { - const results = [] - const traverse = (obj) => { - for (const key in obj) { - if (key === dataKey) { - results.push(obj) - } else if (Array.isArray(obj[key])) { - obj[key].forEach(element => traverse(element)) - } else if (typeof obj[key] === 'object' && obj[key]) { - traverse(obj[key]) - } - } - } - traverse(data) - return results - } - - // When {{EXTERNALLY_DEFINED}} is found in the override value, retain the - // existing value from GitHub. - // Note: - // - The admin settings could define multiple overrides, but the GitHub API - // retains one only. - // - The PUT method for rulesets (update) allows for multiple overrides. - // - The POST method for rulesets (create) allows for one override only. - removeOverrides (source, existing) { - overrides.forEach(override => { - let sourceRefs = this.getObjectRef(source, override) - let data = JSON.stringify(sourceRefs, null, 2) - if (data.includes('{{EXTERNALLY_DEFINED}}')) { - let existingRefs = this.getObjectRef(existing, override) - sourceRefs.forEach(sourceRef => { - if (existingRefs[0]) { - sourceRef[override] = existingRefs[0][override] - } else if (Array.isArray(sourceRef[override])) { - sourceRef[override] = [] - } else if (typeof sourceRef[override] === 'object' && sourceRef[override]) { - sourceRef[override] = {} - } else { - sourceRef[override] = '' - } - }) - } - }) - } - update (existing, attrs) { const parms = this.wrapAttrs(Object.assign({ id: existing.id }, attrs)) if (this.scope === 'org') { @@ -144,7 +100,7 @@ module.exports = class Rulesets extends Diffable { new NopCommand(this.constructor.name, this.repo, this.github.request.endpoint('PUT /orgs/{org}/rulesets/{id}', parms), 'Update Ruleset') ]) } - this.removeOverrides(parms, existing) + Overrides.removeOverrides(overrides, parms, existing) this.log.debug(`Updating Ruleset with the following values ${JSON.stringify(parms, null, 2)}`) return this.github.request('PUT /orgs/{org}/rulesets/{id}', parms).then(res => { this.log(`Ruleset updated successfully ${JSON.stringify(res.url)}`) @@ -158,7 +114,7 @@ module.exports = class Rulesets extends Diffable { new NopCommand(this.constructor.name, this.repo, this.github.request.endpoint('PUT /repos/{owner}/{repo}/rulesets/{id}', parms), 'Update Ruleset') ]) } - this.removeOverrides(parms, existing) + Overrides.removeOverrides(overrides, parms, existing) this.log.debug(`Updating Ruleset with the following values ${JSON.stringify(parms, null, 2)}`) return this.github.request('PUT /repos/{owner}/{repo}/rulesets/{id}', parms).then(res => { this.log(`Ruleset updated successfully ${JSON.stringify(res.url)}`) @@ -176,7 +132,7 @@ module.exports = class Rulesets extends Diffable { new NopCommand(this.constructor.name, this.repo, this.github.request.endpoint('POST /orgs/{org}/rulesets', this.wrapAttrs(attrs)), 'Create Ruleset') ]) } - this.removeOverrides(attrs, {}) + Overrides.removeOverrides(overrides, attrs, {}) this.log.debug(`Creating Rulesets with the following values ${JSON.stringify(attrs, null, 2)}`) return this.github.request('POST /orgs/{org}/rulesets', this.wrapAttrs(attrs)).then(res => { this.log(`Ruleset created successfully ${JSON.stringify(res.url)}`) @@ -190,7 +146,7 @@ module.exports = class Rulesets extends Diffable { new NopCommand(this.constructor.name, this.repo, this.github.request.endpoint('POST /repos/{owner}/{repo}/rulesets', this.wrapAttrs(attrs)), 'Create Ruleset') ]) } - this.removeOverrides(attrs, {}) + Overrides.removeOverrides(overrides, attrs, {}) this.log.debug(`Creating Rulesets with the following values ${JSON.stringify(attrs, null, 2)}`) return this.github.request('POST /repos/{owner}/{repo}/rulesets', this.wrapAttrs(attrs)).then(res => { this.log(`Ruleset created successfully ${JSON.stringify(res.url)}`) From f5fb78137b62e45bb322753db28f87d5c1396902 Mon Sep 17 00:00:00 2001 From: Ji Tran Date: Thu, 9 Jan 2025 00:08:46 +1100 Subject: [PATCH 4/9] On top level ruleset updates, it would trigger a sync all that will trigger top level branch protection rules across all repos. Make a clone of the branch settings before modifying it since it's a shared object. --- lib/plugins/branches.js | 2 +- lib/plugins/overrides.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/plugins/branches.js b/lib/plugins/branches.js index 6e21ad30..57b1610a 100644 --- a/lib/plugins/branches.js +++ b/lib/plugins/branches.js @@ -13,7 +13,7 @@ module.exports = class Branches extends ErrorStash { super(errors) this.github = github this.repo = repo - this.branches = settings + this.branches = structuredClone(settings) this.log = log this.nop = nop } diff --git a/lib/plugins/overrides.js b/lib/plugins/overrides.js index 400b2295..26a5c55f 100644 --- a/lib/plugins/overrides.js +++ b/lib/plugins/overrides.js @@ -28,7 +28,7 @@ module.exports = class Overrides extends ErrorStash { static removeOverrides (overrides, source, existing) { overrides.forEach(override => { let sourceRefs = Overrides.getObjectRef(source, override) - let data = JSON.stringify(sourceRefs, null, 2) + let data = JSON.stringify(sourceRefs) if (data.includes('{{EXTERNALLY_DEFINED}}')) { let existingRefs = Overrides.getObjectRef(existing, override) sourceRefs.forEach(sourceRef => { From 7ac1a1e7af1b063ac3aacd07b642a8bbc6aa1127 Mon Sep 17 00:00:00 2001 From: Ji Tran Date: Fri, 10 Jan 2025 17:31:07 +1100 Subject: [PATCH 5/9] Updated readme. --- README.md | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/README.md b/README.md index b3137a37..47ff79e7 100644 --- a/README.md +++ b/README.md @@ -123,6 +123,42 @@ overridevalidators: A sample of `deployment-settings` file is found [here](docs/sample-settings/sample-deployment-settings.yml). +### Custom Status Checks +For branch protection rules and rulesets, you can allow for status checks to be defined outside of safe-settings together with your usual safe settings. + +This can be defined at the org, sub-org, and repo level. + +To configure this for branch protection rules, specifiy `{{EXTERNALLY_DEFINED}}` under the `contexts` keyword: +```yaml +branches: + - name: main + protection: + ... + required_status_checks: + contexts: + - "{{EXTERNALLY_DEFINED}}" +``` + +For rulesets, specify `{{EXTERNALLY_DEFINED}}` under the `required_status_checks` keyword: +```yaml +rulesets: + - name: Status Checks + ... + rules: + - type: required_status_checks + parameters: + required_status_checks: + - context: "{{EXTERNALLY_DEFINED}}" +``` + +Notes: + - For branch protection rules, contexts defined at the org level are merged together with the sub-org and repo level contexts. + - When `{{EXTERNALLY_DEFINED}}` is defined for a new branch protection rule or ruleset configuration, they will be deployed with no status checks. + - When an existing branch protection rule or ruleset configuration is ammended with `{{EXTERNALLY_DEFINED}}`, the status checks in the existing rules in GitHub will remain as is. + +⚠️ **Warning:** +When `{{EXTERNALLY_DEFINED}}` is removed from an existing branch protection rule or ruleset configuration, the status checks in the existing rules in GitHub will revert to the checks that are defined in safe-settings. + ### Performance When there are 1000s of repos to be managed -- and there is a global settings change -- safe-settings will have to work efficiently and only make the necessary API calls. From 938631a46087551d90c07c40a2f14fb6dd37f8f2 Mon Sep 17 00:00:00 2001 From: Ji Tran Date: Wed, 15 Jan 2025 20:16:13 +1100 Subject: [PATCH 6/9] Added unit tests. --- test/unit/lib/plugins/branches.test.js | 68 +++++++ test/unit/lib/plugins/rulesets.test.js | 257 +++++++++++++++++++++++++ 2 files changed, 325 insertions(+) create mode 100644 test/unit/lib/plugins/rulesets.test.js diff --git a/test/unit/lib/plugins/branches.test.js b/test/unit/lib/plugins/branches.test.js index 68290ea4..6a41aac7 100644 --- a/test/unit/lib/plugins/branches.test.js +++ b/test/unit/lib/plugins/branches.test.js @@ -165,6 +165,74 @@ describe('Branches', () => { }) }) + describe('when {{EXTERNALLY_DEFINED}} is present in "required_status_checks" and no status checks exists in GitHub', () => { + it('it initialises the status checks with an empty list', () => { + const plugin = configure( + [{ + name: 'main', + protection: { + required_status_checks: { + strict: true, + contexts: ['{{travis-ci', '{{EXTERNALLY_DEFINED}}'] + } + } + }] + ) + + return plugin.sync().then(() => { + expect(github.repos.updateBranchProtection).toHaveBeenCalledWith({ + owner: 'jitran', + repo: 'test', + branch: 'main', + required_status_checks: { + strict: true, + contexts: [] + }, + headers: { accept: 'application/vnd.github.hellcat-preview+json,application/vnd.github.luke-cage-preview+json,application/vnd.github.zzzax-preview+json' } + }) + }) + }) + }) + + describe('when {{EXTERNALLY_DEFINED}} is present in "required_status_checks" and status checks exists in GitHub', () => { + it('it retains the status checks from GitHub', () => { + github.repos.getBranchProtection = jest.fn().mockResolvedValue({ + data: { + enforce_admins: { enabled: false }, + protection: { + required_status_checks: { + contexts: ['check-1', 'check-2'] + } + } + } + }) + const plugin = configure( + [{ + name: 'main', + protection: { + required_status_checks: { + strict: true, + contexts: ['{{travis-ci', '{{EXTERNALLY_DEFINED}}'] + } + } + }] + ) + + return plugin.sync().then(() => { + expect(github.repos.updateBranchProtection).toHaveBeenCalledWith({ + owner: 'jitran', + repo: 'test', + branch: 'main', + required_status_checks: { + strict: true, + contexts: ['check-1', 'check-2'] + }, + headers: { accept: 'application/vnd.github.hellcat-preview+json,application/vnd.github.luke-cage-preview+json,application/vnd.github.zzzax-preview+json' } + }) + }) + }) + }) + describe('when multiple branches are configured', () => { it('updates them each appropriately', () => { const plugin = configure( diff --git a/test/unit/lib/plugins/rulesets.test.js b/test/unit/lib/plugins/rulesets.test.js new file mode 100644 index 00000000..6b6415cd --- /dev/null +++ b/test/unit/lib/plugins/rulesets.test.js @@ -0,0 +1,257 @@ +/* eslint-disable no-undef */ + +const { when } = require('jest-when') +const Rulesets = require('../../../../lib/plugins/rulesets') +const version = { + 'X-GitHub-Api-Version': '2022-11-28' +} + +function generateRequestRuleset(id, name, checks) { + return { + id: id, + name: name, + source_type: 'Repository', + target: 'branch', + enforcement: 'active', + conditions: { + ref_name: { + include: ['~ALL'], + exclude: [] + } + }, + rules: [ + { + type: 'required_status_checks', + parameters: { + strict_required_status_checks_policy: true, + required_status_checks: checks + } + } + ] + } +} + +function generateResponseRuleset(id, name, checks) { + return { + id: id, + name: name, + source_type: 'Repository', + target: 'branch', + enforcement: 'active', + conditions: { + ref_name: { + include: ['~ALL'], + exclude: [] + } + }, + owner: 'jitran', + repo: 'test', + rules: [ + { + type: 'required_status_checks', + parameters: { + strict_required_status_checks_policy: true, + required_status_checks: checks + } + } + ], + headers: version, + } +} + +describe('Rulesets', () => { + let github + const log = jest.fn() + log.debug = jest.fn() + log.error = jest.fn() + + function configure (config) { + const noop = false + const errors = [] + return new Rulesets(noop, github, { owner: 'jitran', repo: 'test' }, config, log, errors) + } + + beforeEach(() => { + github = { + repos: { + get: jest.fn().mockResolvedValue({ + data: { + default_branch: 'main' + } + }) + }, + request: jest.fn().mockImplementation(() => Promise.resolve('request')), + } + + github.request.endpoint = { + merge: jest.fn().mockReturnValue({ + method: 'GET', + url: '/repos/jitran/test/rulesets', + headers: version + }) + } + }) + + describe('sync', () => { + it('syncs ruleset settings', () => { + // Mock the GitHub API response + github.paginate = jest.fn().mockResolvedValue([]) + + // Initialise safe-settings + const plugin = configure( + [ + generateRequestRuleset( + 1, + 'All branches', + [ + { context: 'Status Check 1' }, + { context: 'Status Check 2' } + ] + ) + ] + ) + + return plugin.sync().then(() => { + expect(github.request).toHaveBeenLastCalledWith( + 'POST /repos/{owner}/{repo}/rulesets', + generateResponseRuleset( + 1, + 'All branches', + [ + { context: 'Status Check 1' }, + { context: 'Status Check 2' } + ] + ) + ) + }) + }) + }) + + describe('when {{EXTERNALLY_DEFINED}} is present in "required_status_checks" and no status checks exists in GitHub', () => { + it('it initialises the status checks with an empty list', () => { + // Mock the GitHub API response + github.paginate = jest.fn().mockResolvedValue([]) + + // Initialise safe-settings + const plugin = configure( + [ + generateRequestRuleset( + 1, + 'All branches', + [ + { context: 'Status Check 1' }, + { context: '{{EXTERNALLY_DEFINED}}' } + ] + ) + ] + ) + + return plugin.sync().then(() => { + expect(github.request).toHaveBeenLastCalledWith( + 'POST /repos/{owner}/{repo}/rulesets', + generateResponseRuleset( + 1, + 'All branches', + [] + ) + ) + }) + }) + }) + + describe('when {{EXTERNALLY_DEFINED}} is present in "required_status_checks" and status checks exists in GitHub', () => { + it('it retains the status checks from GitHub and everything else is reset to the safe-settings', () => { + // Mock the GitHub API response + github.paginate = jest.fn().mockResolvedValue([ + generateRequestRuleset( + 1, + 'All branches 1', + [ + { context: 'Custom Check 1' }, + { context: 'Custom Check 2' } + ] + ), + generateRequestRuleset( + 2, + 'All branches 2', + [ + { context: 'Custom Check 3' }, + { context: 'Custom Check 4' } + ] + ), + generateRequestRuleset( + 3, + 'All branches 3', + [ + { context: 'Custom Check 5' }, + { context: 'Custom Check 6' } + ] + ) + ]) + + // Initialise safe-settings + const plugin = configure( + [ + generateRequestRuleset( + 1, + 'All branches 1', + [ + { context: 'Status Check 1' }, + { context: '{{EXTERNALLY_DEFINED}}' } + ] + ), + generateRequestRuleset( + 2, + 'All branches 2', + [ + { context: 'Status Check 1' }, + { context: 'Status Check 2' } + ] + ), + generateRequestRuleset( + 3, + 'All branches 3', + [] + ) + ] + ) + + return plugin.sync().then(() => { + expect(github.request).toHaveBeenNthCalledWith( + 1, + 'PUT /repos/{owner}/{repo}/rulesets/{id}', + generateResponseRuleset( + 1, + 'All branches 1', + [ + { context: 'Custom Check 1' }, + { context: 'Custom Check 2' } + ] + ) + ) + expect(github.request).toHaveBeenNthCalledWith( + 2, + 'PUT /repos/{owner}/{repo}/rulesets/{id}', + generateResponseRuleset( + 2, + 'All branches 2', + [ + { context: 'Status Check 1' }, + { context: 'Status Check 2' } + ] + ) + ) + expect(github.request).toHaveBeenNthCalledWith( + 3, + 'PUT /repos/{owner}/{repo}/rulesets/{id}', + generateResponseRuleset( + 3, + 'All branches 3', + [] + ) + ) + }) + }) + }) + // TODO: Write tests for org rulesets +}) From 3e4250c512f5282747dbc37d674206a8072bbc70 Mon Sep 17 00:00:00 2001 From: Ji Tran Date: Thu, 16 Jan 2025 09:45:26 +1100 Subject: [PATCH 7/9] Added unit tests for organisation rulesets. --- test/unit/lib/plugins/branches.test.js | 4 +- test/unit/lib/plugins/rulesets.test.js | 193 ++++++++++++++++++++++--- 2 files changed, 173 insertions(+), 24 deletions(-) diff --git a/test/unit/lib/plugins/branches.test.js b/test/unit/lib/plugins/branches.test.js index 6a41aac7..62da1448 100644 --- a/test/unit/lib/plugins/branches.test.js +++ b/test/unit/lib/plugins/branches.test.js @@ -181,7 +181,7 @@ describe('Branches', () => { return plugin.sync().then(() => { expect(github.repos.updateBranchProtection).toHaveBeenCalledWith({ - owner: 'jitran', + owner: 'bkeepers', repo: 'test', branch: 'main', required_status_checks: { @@ -220,7 +220,7 @@ describe('Branches', () => { return plugin.sync().then(() => { expect(github.repos.updateBranchProtection).toHaveBeenCalledWith({ - owner: 'jitran', + owner: 'bkeepers', repo: 'test', branch: 'main', required_status_checks: { diff --git a/test/unit/lib/plugins/rulesets.test.js b/test/unit/lib/plugins/rulesets.test.js index 6b6415cd..3a9f18a5 100644 --- a/test/unit/lib/plugins/rulesets.test.js +++ b/test/unit/lib/plugins/rulesets.test.js @@ -5,20 +5,31 @@ const Rulesets = require('../../../../lib/plugins/rulesets') const version = { 'X-GitHub-Api-Version': '2022-11-28' } +const repo_conditions = { + ref_name: { + include: ['~ALL'], + exclude: [] + }, +} +const org_conditions = { + ref_name: { + include: ['~ALL'], + exclude: [] + }, + repository_name: { + include: ["~ALL"], + exclude: ["admin"] + } +} -function generateRequestRuleset(id, name, checks) { - return { +function generateRequestRuleset(id, name, conditions, checks) { + response = { id: id, name: name, source_type: 'Repository', target: 'branch', enforcement: 'active', - conditions: { - ref_name: { - include: ['~ALL'], - exclude: [] - } - }, + conditions: conditions, rules: [ { type: 'required_status_checks', @@ -29,23 +40,17 @@ function generateRequestRuleset(id, name, checks) { } ] } + return response } -function generateResponseRuleset(id, name, checks) { - return { +function generateResponseRuleset(id, name, conditions, checks, org=false) { + response = { id: id, name: name, source_type: 'Repository', target: 'branch', enforcement: 'active', - conditions: { - ref_name: { - include: ['~ALL'], - exclude: [] - } - }, - owner: 'jitran', - repo: 'test', + conditions: conditions, rules: [ { type: 'required_status_checks', @@ -57,6 +62,13 @@ function generateResponseRuleset(id, name, checks) { ], headers: version, } + if (org) { + response.org = 'jitran' + } else { + response.owner = 'jitran' + response.repo = 'test' + } + return response } describe('Rulesets', () => { @@ -65,10 +77,10 @@ describe('Rulesets', () => { log.debug = jest.fn() log.error = jest.fn() - function configure (config) { + function configure (config, scope='repo') { const noop = false const errors = [] - return new Rulesets(noop, github, { owner: 'jitran', repo: 'test' }, config, log, errors) + return new Rulesets(noop, github, { owner: 'jitran', repo: 'test' }, config, log, errors, scope) } beforeEach(() => { @@ -82,7 +94,7 @@ describe('Rulesets', () => { }, request: jest.fn().mockImplementation(() => Promise.resolve('request')), } - + github.request.endpoint = { merge: jest.fn().mockReturnValue({ method: 'GET', @@ -103,6 +115,7 @@ describe('Rulesets', () => { generateRequestRuleset( 1, 'All branches', + repo_conditions, [ { context: 'Status Check 1' }, { context: 'Status Check 2' } @@ -117,6 +130,7 @@ describe('Rulesets', () => { generateResponseRuleset( 1, 'All branches', + repo_conditions, [ { context: 'Status Check 1' }, { context: 'Status Check 2' } @@ -138,6 +152,7 @@ describe('Rulesets', () => { generateRequestRuleset( 1, 'All branches', + repo_conditions, [ { context: 'Status Check 1' }, { context: '{{EXTERNALLY_DEFINED}}' } @@ -152,6 +167,7 @@ describe('Rulesets', () => { generateResponseRuleset( 1, 'All branches', + repo_conditions, [] ) ) @@ -166,6 +182,7 @@ describe('Rulesets', () => { generateRequestRuleset( 1, 'All branches 1', + repo_conditions, [ { context: 'Custom Check 1' }, { context: 'Custom Check 2' } @@ -174,6 +191,7 @@ describe('Rulesets', () => { generateRequestRuleset( 2, 'All branches 2', + repo_conditions, [ { context: 'Custom Check 3' }, { context: 'Custom Check 4' } @@ -182,6 +200,7 @@ describe('Rulesets', () => { generateRequestRuleset( 3, 'All branches 3', + repo_conditions, [ { context: 'Custom Check 5' }, { context: 'Custom Check 6' } @@ -195,6 +214,7 @@ describe('Rulesets', () => { generateRequestRuleset( 1, 'All branches 1', + repo_conditions, [ { context: 'Status Check 1' }, { context: '{{EXTERNALLY_DEFINED}}' } @@ -203,6 +223,7 @@ describe('Rulesets', () => { generateRequestRuleset( 2, 'All branches 2', + repo_conditions, [ { context: 'Status Check 1' }, { context: 'Status Check 2' } @@ -211,6 +232,7 @@ describe('Rulesets', () => { generateRequestRuleset( 3, 'All branches 3', + repo_conditions, [] ) ] @@ -223,6 +245,7 @@ describe('Rulesets', () => { generateResponseRuleset( 1, 'All branches 1', + repo_conditions, [ { context: 'Custom Check 1' }, { context: 'Custom Check 2' } @@ -235,6 +258,7 @@ describe('Rulesets', () => { generateResponseRuleset( 2, 'All branches 2', + repo_conditions, [ { context: 'Status Check 1' }, { context: 'Status Check 2' } @@ -247,11 +271,136 @@ describe('Rulesets', () => { generateResponseRuleset( 3, 'All branches 3', + repo_conditions, [] ) ) }) }) }) - // TODO: Write tests for org rulesets + + describe('[org] sync', () => { + it('syncs ruleset settings', () => { + // Mock the GitHub API response + github.paginate = jest.fn().mockResolvedValue([]) + + // Initialise safe-settings + const plugin = configure( + [ + generateRequestRuleset( + 1, + 'All branches', + org_conditions, + [ + { context: 'Status Check 1' }, + { context: 'Status Check 2' } + ] + ) + ], + 'org' + ) + + return plugin.sync().then(() => { + expect(github.request).toHaveBeenLastCalledWith( + 'POST /orgs/{org}/rulesets', + generateResponseRuleset( + 1, + 'All branches', + org_conditions, + [ + { context: 'Status Check 1' }, + { context: 'Status Check 2' } + ], + true + ) + ) + }) + }) + }) + + describe('[org] when {{EXTERNALLY_DEFINED}} is present in "required_status_checks" and no status checks exists in GitHub', () => { + it('it initialises the status checks with an empty list', () => { + // Mock the GitHub API response + github.paginate = jest.fn().mockResolvedValue([]) + + // Initialise safe-settings + const plugin = configure( + [ + generateRequestRuleset( + 1, + 'All branches', + org_conditions, + [ + { context: 'Status Check 1' }, + { context: '{{EXTERNALLY_DEFINED}}' } + ] + ) + ], + 'org' + ) + + return plugin.sync().then(() => { + expect(github.request).toHaveBeenLastCalledWith( + 'POST /orgs/{org}/rulesets', + generateResponseRuleset( + 1, + 'All branches', + org_conditions, + [], + true + ) + ) + }) + }) + }) + + describe('[org] when {{EXTERNALLY_DEFINED}} is present in "required_status_checks" and status checks exists in GitHub', () => { + it('it retains the status checks from GitHub', () => { + // Mock the GitHub API response + github.paginate = jest.fn().mockResolvedValue([ + generateRequestRuleset( + 1, + 'All branches 1', + org_conditions, + [ + { context: 'Custom Check 1' }, + { context: 'Custom Check 2' } + ] + ) + ]) + + // Initialise safe-settings + const plugin = configure( + [ + generateRequestRuleset( + 1, + 'All branches 1', + org_conditions, + [ + { context: 'Status Check 1' }, + { context: '{{EXTERNALLY_DEFINED}}' } + ] + ) + ], + 'org' + ) + + return plugin.sync().then(() => { + expect(github.request).toHaveBeenNthCalledWith( + 1, + 'PUT /orgs/{org}/rulesets/{id}', + generateResponseRuleset( + 1, + 'All branches 1', + org_conditions, + [ + { context: 'Custom Check 1' }, + { context: 'Custom Check 2' } + ], + true + ) + ) + }) + }) + }) }) From 77bcf3e0cdcbd4e7b735a255d9456dae2469e34f Mon Sep 17 00:00:00 2001 From: Ji Tran Date: Thu, 16 Jan 2025 19:50:00 +1100 Subject: [PATCH 8/9] Updated tests to reflect latest ruleset data type changes. --- README.md | 2 +- test/unit/lib/plugins/branches.test.js | 4 +-- test/unit/lib/plugins/rulesets.test.js | 36 ++++++++++++++++---------- 3 files changed, 26 insertions(+), 16 deletions(-) diff --git a/README.md b/README.md index 47ff79e7..0cc1c94b 100644 --- a/README.md +++ b/README.md @@ -154,7 +154,7 @@ rulesets: Notes: - For branch protection rules, contexts defined at the org level are merged together with the sub-org and repo level contexts. - When `{{EXTERNALLY_DEFINED}}` is defined for a new branch protection rule or ruleset configuration, they will be deployed with no status checks. - - When an existing branch protection rule or ruleset configuration is ammended with `{{EXTERNALLY_DEFINED}}`, the status checks in the existing rules in GitHub will remain as is. + - When an existing branch protection rule or ruleset configuration is amended with `{{EXTERNALLY_DEFINED}}`, the status checks in the existing rules in GitHub will remain as is. ⚠️ **Warning:** When `{{EXTERNALLY_DEFINED}}` is removed from an existing branch protection rule or ruleset configuration, the status checks in the existing rules in GitHub will revert to the checks that are defined in safe-settings. diff --git a/test/unit/lib/plugins/branches.test.js b/test/unit/lib/plugins/branches.test.js index 62da1448..62b50cb1 100644 --- a/test/unit/lib/plugins/branches.test.js +++ b/test/unit/lib/plugins/branches.test.js @@ -165,7 +165,7 @@ describe('Branches', () => { }) }) - describe('when {{EXTERNALLY_DEFINED}} is present in "required_status_checks" and no status checks exists in GitHub', () => { + describe('when {{EXTERNALLY_DEFINED}} is present in "required_status_checks" and no status checks exist in GitHub', () => { it('it initialises the status checks with an empty list', () => { const plugin = configure( [{ @@ -194,7 +194,7 @@ describe('Branches', () => { }) }) - describe('when {{EXTERNALLY_DEFINED}} is present in "required_status_checks" and status checks exists in GitHub', () => { + describe('when {{EXTERNALLY_DEFINED}} is present in "required_status_checks" and status checks exist in GitHub', () => { it('it retains the status checks from GitHub', () => { github.repos.getBranchProtection = jest.fn().mockResolvedValue({ data: { diff --git a/test/unit/lib/plugins/rulesets.test.js b/test/unit/lib/plugins/rulesets.test.js index 3a9f18a5..1f28eea5 100644 --- a/test/unit/lib/plugins/rulesets.test.js +++ b/test/unit/lib/plugins/rulesets.test.js @@ -22,11 +22,10 @@ const org_conditions = { } } -function generateRequestRuleset(id, name, conditions, checks) { +function generateRequestRuleset(id, name, conditions, checks, org=false) { response = { id: id, name: name, - source_type: 'Repository', target: 'branch', enforcement: 'active', conditions: conditions, @@ -40,6 +39,11 @@ function generateRequestRuleset(id, name, conditions, checks) { } ] } + if (org) { + response.source_type = 'Organization' + } else { + response.source_type = 'Repository' + } return response } @@ -47,7 +51,6 @@ function generateResponseRuleset(id, name, conditions, checks, org=false) { response = { id: id, name: name, - source_type: 'Repository', target: 'branch', enforcement: 'active', conditions: conditions, @@ -63,8 +66,10 @@ function generateResponseRuleset(id, name, conditions, checks, org=false) { headers: version, } if (org) { + response.source_type = 'Organization' response.org = 'jitran' } else { + response.source_type = 'Repository' response.owner = 'jitran' response.repo = 'test' } @@ -100,8 +105,9 @@ describe('Rulesets', () => { method: 'GET', url: '/repos/jitran/test/rulesets', headers: version - }) - } + } + ) + } }) describe('sync', () => { @@ -141,7 +147,7 @@ describe('Rulesets', () => { }) }) - describe('when {{EXTERNALLY_DEFINED}} is present in "required_status_checks" and no status checks exists in GitHub', () => { + describe('when {{EXTERNALLY_DEFINED}} is present in "required_status_checks" and no status checks exist in GitHub', () => { it('it initialises the status checks with an empty list', () => { // Mock the GitHub API response github.paginate = jest.fn().mockResolvedValue([]) @@ -175,7 +181,7 @@ describe('Rulesets', () => { }) }) - describe('when {{EXTERNALLY_DEFINED}} is present in "required_status_checks" and status checks exists in GitHub', () => { + describe('when {{EXTERNALLY_DEFINED}} is present in "required_status_checks" and status checks exist in GitHub', () => { it('it retains the status checks from GitHub and everything else is reset to the safe-settings', () => { // Mock the GitHub API response github.paginate = jest.fn().mockResolvedValue([ @@ -294,7 +300,8 @@ describe('Rulesets', () => { [ { context: 'Status Check 1' }, { context: 'Status Check 2' } - ] + ], + true ) ], 'org' @@ -318,7 +325,7 @@ describe('Rulesets', () => { }) }) - describe('[org] when {{EXTERNALLY_DEFINED}} is present in "required_status_checks" and no status checks exists in GitHub', () => { + describe('[org] when {{EXTERNALLY_DEFINED}} is present in "required_status_checks" and no status checks exist in GitHub', () => { it('it initialises the status checks with an empty list', () => { // Mock the GitHub API response github.paginate = jest.fn().mockResolvedValue([]) @@ -333,7 +340,8 @@ describe('Rulesets', () => { [ { context: 'Status Check 1' }, { context: '{{EXTERNALLY_DEFINED}}' } - ] + ], + true ) ], 'org' @@ -354,7 +362,7 @@ describe('Rulesets', () => { }) }) - describe('[org] when {{EXTERNALLY_DEFINED}} is present in "required_status_checks" and status checks exists in GitHub', () => { + describe('[org] when {{EXTERNALLY_DEFINED}} is present in "required_status_checks" and status checks exist in GitHub', () => { it('it retains the status checks from GitHub', () => { // Mock the GitHub API response github.paginate = jest.fn().mockResolvedValue([ @@ -365,7 +373,8 @@ describe('Rulesets', () => { [ { context: 'Custom Check 1' }, { context: 'Custom Check 2' } - ] + ], + true ) ]) @@ -379,7 +388,8 @@ describe('Rulesets', () => { [ { context: 'Status Check 1' }, { context: '{{EXTERNALLY_DEFINED}}' } - ] + ], + true ) ], 'org' From 18c44cf7856866e539e588cb9c0e37bf8857ae01 Mon Sep 17 00:00:00 2001 From: Ji Tran Date: Fri, 17 Jan 2025 10:28:51 +1100 Subject: [PATCH 9/9] Updated documentation. --- README.md | 2 +- lib/plugins/overrides.js | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 0cc1c94b..7d480a16 100644 --- a/README.md +++ b/README.md @@ -128,7 +128,7 @@ For branch protection rules and rulesets, you can allow for status checks to be This can be defined at the org, sub-org, and repo level. -To configure this for branch protection rules, specifiy `{{EXTERNALLY_DEFINED}}` under the `contexts` keyword: +To configure this for branch protection rules, specify `{{EXTERNALLY_DEFINED}}` under the `contexts` keyword: ```yaml branches: - name: main diff --git a/lib/plugins/overrides.js b/lib/plugins/overrides.js index 26a5c55f..9b9ea00a 100644 --- a/lib/plugins/overrides.js +++ b/lib/plugins/overrides.js @@ -21,8 +21,8 @@ module.exports = class Overrides extends ErrorStash { // When {{EXTERNALLY_DEFINED}} is found in the override value, retain the // existing value from GitHub. // Note: - // - The admin settings could define multiple overrides, but the GitHub API - // retains one only. + // - The admin settings could define multiple status check rules for a + // ruleset, but the GitHub API retains one only, i.e. the last one. // - The PUT method for rulesets (update) allows for multiple overrides. // - The POST method for rulesets (create) allows for one override only. static removeOverrides (overrides, source, existing) {