Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: Allow PR status checks to be managed external to safe settings #741

Open
wants to merge 9 commits into
base: main-enterprise
Choose a base branch
from
36 changes: 36 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:
jitran marked this conversation as resolved.
Show resolved Hide resolved
```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.

jitran marked this conversation as resolved.
Show resolved Hide resolved
⚠️ **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.

Expand Down
10 changes: 7 additions & 3 deletions lib/plugins/branches.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,19 @@
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 = [
'contexts',
]

module.exports = class Branches extends ErrorStash {
constructor (nop, github, repo, settings, log, errors) {
super(errors)
this.github = github
this.repo = repo
this.branches = settings
this.branches = structuredClone(settings)
this.log = log
this.nop = nop
}
Expand Down Expand Up @@ -49,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: branch.protection } })
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}`)

Expand All @@ -76,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, 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)
Expand Down
49 changes: 49 additions & 0 deletions lib/plugins/overrides.js
Original file line number Diff line number Diff line change
@@ -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)
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
}
}
8 changes: 8 additions & 0 deletions lib/plugins/rulesets.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
const Diffable = require('./diffable')
const NopCommand = require('../nopcommand')
const MergeDeep = require('../mergeDeep')
const Overrides = require('./overrides')
const ignorableFields = []
const overrides = [
'required_status_checks',
]

const version = {
'X-GitHub-Api-Version': '2022-11-28'
Expand Down Expand Up @@ -96,6 +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')
])
}
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)}`)
Expand All @@ -109,6 +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')
])
}
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)}`)
Expand All @@ -126,6 +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')
])
}
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)}`)
Expand All @@ -139,6 +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')
])
}
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)}`)
Expand Down
68 changes: 68 additions & 0 deletions test/unit/lib/plugins/branches.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,74 @@ describe('Branches', () => {
})
})

describe('when {{EXTERNALLY_DEFINED}} is present in "required_status_checks" and no status checks exists in GitHub', () => {
jitran marked this conversation as resolved.
Show resolved Hide resolved
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: 'bkeepers',
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: 'bkeepers',
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(
Expand Down
Loading
Loading