From 84229c738216c4f286ddaa4c2d4dbfb3185f5dfa Mon Sep 17 00:00:00 2001 From: Jonathan Morley Date: Wed, 19 Mar 2025 11:48:49 -0400 Subject: [PATCH 1/2] feat: enable some settings to be marked unsafe, settable in the repositories themselves --- .../sample-deployment-settings.yml | 5 + index.js | 59 +++--- lib/configManager.js | 29 ++- lib/deploymentConfig.js | 10 + lib/settings.js | 70 +++++-- package-lock.json | 23 ++ package.json | 2 + test/unit/index.test.js | 197 ++++++++++-------- test/unit/lib/settings.test.js | 77 ++++++- 9 files changed, 330 insertions(+), 142 deletions(-) diff --git a/docs/sample-settings/sample-deployment-settings.yml b/docs/sample-settings/sample-deployment-settings.yml index 4244b45e8..93869bdfb 100644 --- a/docs/sample-settings/sample-deployment-settings.yml +++ b/docs/sample-settings/sample-deployment-settings.yml @@ -27,3 +27,8 @@ overridevalidators: Some error script: | return true +unsafeFields: + # You can specify the fields that are allowed to be controlled by individual repositories + - /repository/description + - /repository/allow_auto_merge + \ No newline at end of file diff --git a/index.js b/index.js index 40e057391..842b50592 100644 --- a/index.js +++ b/index.js @@ -258,41 +258,48 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) => const { payload } = context const { repository } = payload - const adminRepo = repository.name === env.ADMIN_REPO - if (!adminRepo) { - return - } - const defaultBranch = payload.ref === 'refs/heads/' + repository.default_branch if (!defaultBranch) { robot.log.debug('Not working on the default branch, returning...') return } - const settingsModified = payload.commits.find(commit => { - return commit.added.includes(Settings.FILE_NAME) || - commit.modified.includes(Settings.FILE_NAME) - }) - if (settingsModified) { - robot.log.debug(`Changes in '${Settings.FILE_NAME}' detected, doing a full synch...`) - return syncAllSettings(false, context) - } + const adminRepo = repository.name === env.ADMIN_REPO + if (adminRepo) { + const settingsModified = payload.commits.find(commit => { + return commit.added.includes(Settings.FILE_NAME) || + commit.modified.includes(Settings.FILE_NAME) + }) + if (settingsModified) { + robot.log.debug(`Changes in '${Settings.FILE_NAME}' detected, doing a full synch...`) + return syncAllSettings(false, context) + } - const repoChanges = getAllChangedRepoConfigs(payload, context.repo().owner) - if (repoChanges.length > 0) { - return Promise.all(repoChanges.map(repo => { - return syncSettings(false, context, repo) - })) - } + const repoChanges = getAllChangedRepoConfigs(payload, context.repo().owner) + if (repoChanges.length > 0) { + return Promise.all(repoChanges.map(repo => { + return syncSettings(false, context, repo) + })) + } - const changes = getAllChangedSubOrgConfigs(payload) - if (changes.length) { - return Promise.all(changes.map(suborg => { - return syncSubOrgSettings(false, context, suborg) - })) - } + const changes = getAllChangedSubOrgConfigs(payload) + if (changes.length) { + return Promise.all(changes.map(suborg => { + return syncSubOrgSettings(false, context, suborg) + })) + } - robot.log.debug(`No changes in '${Settings.FILE_NAME}' detected, returning...`) + robot.log.debug(`No changes in '${Settings.FILE_NAME}' detected, returning...`) + } else { + const settingsModified = payload.commits.find(commit => { + return commit.added.includes('.github/settings.yml') || + commit.modified.includes('.github/settings.yml') + }) + if (settingsModified) { + robot.log.debug(`Changes in '.github/settings.yml' detected, doing a sync for ${repository.name}...`) + return syncSettings(false, context) + } + } }) robot.on('create', async context => { diff --git a/lib/configManager.js b/lib/configManager.js index 58f5bb436..4fdf3bcf3 100644 --- a/lib/configManager.js +++ b/lib/configManager.js @@ -10,25 +10,20 @@ module.exports = class ConfigManager { } /** -* Loads a file from GitHub -* -* @param params Params to fetch the file with -* @return The parsed YAML file -*/ - async loadYaml (filePath) { + * Loads a file from GitHub + * + * @param params Params to fetch the file with + * @return The parsed YAML file + */ + static async loadYaml (octokit, params) { try { - const repo = { owner: this.context.repo().owner, repo: env.ADMIN_REPO } - const params = Object.assign(repo, { path: filePath, ref: this.ref }) - const response = await this.context.octokit.repos.getContent(params).catch(e => { - this.log.error(`Error getting settings ${e}`) - }) + const response = await octokit.repos.getContent(params) // Ignore in case path is a folder // - https://developer.github.com/v3/repos/contents/#response-if-content-is-a-directory if (Array.isArray(response.data)) { return null } - // we don't handle symlinks or submodule // - https://developer.github.com/v3/repos/contents/#response-if-content-is-a-symlink // - https://developer.github.com/v3/repos/contents/#response-if-content-is-a-submodule @@ -45,14 +40,18 @@ module.exports = class ConfigManager { } /** - * Loads a file from GitHub + * Loads the settings file from GitHub * - * @param params Params to fetch the file with * @return The parsed YAML file */ async loadGlobalSettingsYaml () { const CONFIG_PATH = env.CONFIG_PATH const filePath = path.posix.join(CONFIG_PATH, env.SETTINGS_FILE_PATH) - return this.loadYaml(filePath) + return ConfigManager.loadYaml(this.context.octokit, { + owner: this.context.repo().owner, + repo: env.ADMIN_REPO, + path: filePath, + ref: this.ref + }) } } diff --git a/lib/deploymentConfig.js b/lib/deploymentConfig.js index e515c91b1..7a07ab9b3 100644 --- a/lib/deploymentConfig.js +++ b/lib/deploymentConfig.js @@ -11,6 +11,7 @@ class DeploymentConfig { // static config static configvalidators = {} static overridevalidators = {} + static unsafeFields = [] static { const deploymentConfigPath = process.env.DEPLOYMENT_CONFIG_FILE ? process.env.DEPLOYMENT_CONFIG_FILE : 'deployment-settings.yml' @@ -36,6 +37,15 @@ class DeploymentConfig { this.configvalidators[validator.plugin] = { isValid: f, error: validator.error } } } + + const unsafeFields = this.config.unsafeFields + if (unsafeFields) { + if (this.isIterable(unsafeFields)) { + this.unsafeFields = unsafeFields + } else { + throw new Error('unsafeFields must be an array') + } + } } static isNonEmptyArray (obj) { diff --git a/lib/settings.js b/lib/settings.js index 96a8cf962..cd3fb671b 100644 --- a/lib/settings.js +++ b/lib/settings.js @@ -1,7 +1,10 @@ const path = require('path') const { Eta } = require('eta') +const jsonPointer = require('json-pointer') +const lodashSet = require('lodash.set') const commetMessageTemplate = require('./commentmessage') const errorTemplate = require('./error') +const ConfigManager = require('./configManager') const Glob = require('./glob') const NopCommand = require('./nopcommand') const MergeDeep = require('./mergeDeep') @@ -298,11 +301,6 @@ ${this.results.reduce((x, y) => { async updateRepos(repo) { this.subOrgConfigs = this.subOrgConfigs || await this.getSubOrgConfigs() - let repoConfig = this.config.repository - if (repoConfig) { - repoConfig = Object.assign(repoConfig, { name: repo.repo, org: repo.owner }) - } - const subOrgConfig = this.getSubOrgConfig(repo.repo) // If suborg config has been updated then only restrict to the repos for that suborg @@ -313,6 +311,14 @@ ${this.results.reduce((x, y) => { this.log.debug(`Process normally... Not a SubOrg config change or SubOrg config was changed and this repo is part of it. ${JSON.stringify(repo)} suborg config ${JSON.stringify(this.subOrgConfigMap)}`) + let repoConfig = this.config.repository + + // Overlay with repo information + if (repoConfig) { + repoConfig = Object.assign(repoConfig, { name: repo.repo, org: repo.owner }) + } + + // Overlay with suborg if (subOrgConfig) { let suborgRepoConfig = subOrgConfig.repository if (suborgRepoConfig) { @@ -321,19 +327,26 @@ ${this.results.reduce((x, y) => { } } - // Overlay repo config + // Overlay with centralized repo config // RepoConfigs should be preloaded but checking anyway const overrideRepoConfig = this.repoConfigs[`${repo.repo}.yml`]?.repository || this.repoConfigs[`${repo.repo}.yaml`]?.repository if (overrideRepoConfig) { repoConfig = this.mergeDeep.mergeDeep({}, repoConfig, overrideRepoConfig) } + + // Overlay with decentralized repo config + const unsafeRepoOverrideConfig = (await this.getUnsafeRepoConfig(repo))?.repository + if (unsafeRepoOverrideConfig) { + repoConfig = this.mergeDeep.mergeDeep({}, repoConfig, unsafeRepoOverrideConfig) + } + const {shouldContinue, nopCommands} = await new Archive(this.nop, this.github, repo, repoConfig, this.log).sync() if (nopCommands) this.appendToResults(nopCommands) if (shouldContinue) { if (repoConfig) { try { this.log.debug(`found a matching repoconfig for this repo ${JSON.stringify(repoConfig)}`) - const childPlugins = this.childPluginsList(repo) + const childPlugins = await this.childPluginsList(repo) const RepoPlugin = Settings.PLUGINS.repository return new RepoPlugin(this.nop, this.github, repo, repoConfig, this.installation_id, this.log, this.errors).sync().then(res => { this.appendToResults(res) @@ -356,7 +369,7 @@ ${this.results.reduce((x, y) => { } } else { this.log.debug(`Didnt find any a matching repoconfig for this repo ${JSON.stringify(repo)} in ${JSON.stringify(this.repoConfigs)}`) - const childPlugins = this.childPluginsList(repo) + const childPlugins = await this.childPluginsList(repo) return Promise.all(childPlugins.map(([Plugin, config]) => { return new Plugin(this.nop, this.github, repo, config, this.log, this.errors).sync().then(res => { this.appendToResults(res) @@ -379,26 +392,59 @@ ${this.results.reduce((x, y) => { for (const k of Object.keys(this.subOrgConfigs)) { const repoPattern = new Glob(k) if (repoName.search(repoPattern) >= 0) { - return this.subOrgConfigs[k] + const subOrgConfig = this.subOrgConfigs[k] + // Coerce 'repositories' to 'repository' + subOrgConfig.repository = subOrgConfig.repositories + delete subOrgConfig.repositories + return subOrgConfig } } } return undefined } + async getUnsafeRepoConfig (repo) { + const repoConfig = await ConfigManager.loadYaml(this.github, { + ...repo, + path: '.github/settings.yml' + }) + + const { unsafeFields } = this.config + + const result = {} + for (const unsafeField of unsafeFields) { + let value + try { + value = jsonPointer.get(repoConfig, unsafeField) + } catch {} + + if (value !== undefined) { + lodashSet(result, jsonPointer.parse(unsafeField), value) + } + } + + return result + } + // Remove Org specific configs from the repo config returnRepoSpecificConfigs(config) { const newConfig = Object.assign({}, config) // clone delete newConfig.rulesets + + // Coerce 'repositories' to 'repository' + newConfig.repository = newConfig.repositories + delete newConfig.repositories + return newConfig } - childPluginsList(repo) { + async childPluginsList(repo) { const repoName = repo.repo const subOrgOverrideConfig = this.getSubOrgConfig(repoName) - this.log.debug(`suborg config for ${repoName} is ${JSON.stringify(subOrgOverrideConfig)}`) + this.log.debug(`suborg config for ${repoName} is ${JSON.stringify(subOrgOverrideConfig)}`) const repoOverrideConfig = this.getRepoOverrideConfig(repoName) - const overrideConfig = this.mergeDeep.mergeDeep({}, this.returnRepoSpecificConfigs(this.config), subOrgOverrideConfig, repoOverrideConfig) + const unsafeRepoOverrideConfig = await this.getUnsafeRepoConfig(repo) + const overrideConfig = this.mergeDeep.mergeDeep({}, this.returnRepoSpecificConfigs(this.config), subOrgOverrideConfig, repoOverrideConfig, unsafeRepoOverrideConfig) this.log.debug(`consolidated config is ${JSON.stringify(overrideConfig)}`) diff --git a/package-lock.json b/package-lock.json index 8823564d6..6c191463f 100644 --- a/package-lock.json +++ b/package-lock.json @@ -14,7 +14,9 @@ "deepmerge": "^4.3.1", "eta": "^3.5.0", "js-yaml": "^4.1.0", + "json-pointer": "^0.6.2", "lodash": "^4.17.21", + "lodash.set": "^4.3.2", "node-cron": "^3.0.2", "octokit": "^4.1.2", "probot": "^13.4.4" @@ -6334,6 +6336,12 @@ "is-callable": "^1.1.3" } }, + "node_modules/foreach": { + "version": "2.0.6", + "resolved": "https://registry.npmjs.org/foreach/-/foreach-2.0.6.tgz", + "integrity": "sha512-k6GAGDyqLe9JaebCsFCoudPPWfihKu8pylYXRlqP1J7ms39iPoTtk2fviNglIeQEwdh0bQeKJ01ZPyuyQvKzwg==", + "license": "MIT" + }, "node_modules/forwarded": { "version": "0.2.0", "resolved": "https://registry.npmjs.org/forwarded/-/forwarded-0.2.0.tgz", @@ -9098,6 +9106,15 @@ "integrity": "sha512-xyFwyhro/JEof6Ghe2iz2NcXoj2sloNsWr/XsERDK/oiPCfaNhl5ONfp+jQdAZRQQ0IJWNzH9zIZF7li91kh2w==", "dev": true }, + "node_modules/json-pointer": { + "version": "0.6.2", + "resolved": "https://registry.npmjs.org/json-pointer/-/json-pointer-0.6.2.tgz", + "integrity": "sha512-vLWcKbOaXlO+jvRy4qNd+TI1QUPZzfJj1tpJ3vAXDych5XJf93ftpUKe5pKCrzyIIwgBJcOcCVRUfqQP25afBw==", + "license": "MIT", + "dependencies": { + "foreach": "^2.0.4" + } + }, "node_modules/json-schema-traverse": { "version": "0.4.1", "resolved": "https://registry.npmjs.org/json-schema-traverse/-/json-schema-traverse-0.4.1.tgz", @@ -9418,6 +9435,12 @@ "integrity": "sha512-Sb487aTOCr9drQVL8pIxOzVhafOjZN9UU54hiN8PU3uAiSV7lx1yYNpbNmex2PK6dSJoNTSJUUswT651yww3Mg==", "license": "MIT" }, + "node_modules/lodash.set": { + "version": "4.3.2", + "resolved": "https://registry.npmjs.org/lodash.set/-/lodash.set-4.3.2.tgz", + "integrity": "sha512-4hNPN5jlm/N/HLMCO43v8BXKq9Z7QdAGc/VGrRD61w8gN9g/6jF9A4L1pbUgBLCffi0w9VsXfTOij5x8iTyFvg==", + "license": "MIT" + }, "node_modules/loose-envify": { "version": "1.4.0", "resolved": "https://registry.npmjs.org/loose-envify/-/loose-envify-1.4.0.tgz", diff --git a/package.json b/package.json index c1f2f5c50..7832aefde 100644 --- a/package.json +++ b/package.json @@ -30,7 +30,9 @@ "deepmerge": "^4.3.1", "eta": "^3.5.0", "js-yaml": "^4.1.0", + "json-pointer": "^0.6.2", "lodash": "^4.17.21", + "lodash.set": "^4.3.2", "node-cron": "^3.0.2", "octokit": "^4.1.2", "probot": "^13.4.4" diff --git a/test/unit/index.test.js b/test/unit/index.test.js index feae42d95..ade4f5f2a 100644 --- a/test/unit/index.test.js +++ b/test/unit/index.test.js @@ -1,8 +1,8 @@ const { Probot } = require('probot') const plugin = require('../../index') -describe.skip('plugin', () => { - let app, event, sync, github +describe('plugin', () => { + let app, event, sync, syncAll, github beforeEach(() => { class Octokit { @@ -17,6 +17,9 @@ describe.skip('plugin', () => { this.repos = { getContent: jest.fn(() => Promise.resolve({ data: { content: '' } })) } + this.hook = { + before: jest.fn() + } } auth () { @@ -26,129 +29,153 @@ describe.skip('plugin', () => { app = new Probot({ secret: 'abcdef', Octokit }) github = { + apps: { + listInstallations: { + endpoint: { + merge: jest.fn() + } + } + }, repos: { getContents: jest.fn(() => Promise.resolve({ data: { content: '' } })) - } + }, + paginate: jest.fn(() => Promise.resolve([])) } app.auth = () => Promise.resolve(github) - app.log = { debug: jest.fn(), error: console.error } + app.log = { debug: jest.fn(), info: jest.fn(), error: console.error } event = { name: 'push', payload: JSON.parse(JSON.stringify(require('../fixtures/events/push.settings.json'))) } sync = jest.fn() + syncAll = jest.fn() - plugin(app, {}, { sync, FILE_NAME: '.github/settings.yml' }) + plugin(app, {}, { sync, syncAll, FILE_NAME: '.github/settings.yml' }) }) - describe('with settings modified on master', () => { - it('syncs settings', async () => { - await app.receive(event) - expect(sync).toHaveBeenCalled() + describe('regular repo', () => { + describe('with settings modified on master', () => { + it('syncs settings', async () => { + await app.receive(event) + expect(sync).toHaveBeenCalled() + }) }) - }) - describe('on another branch', () => { - beforeEach(() => { - event.payload.ref = 'refs/heads/other-branch' - }) + describe('on another branch', () => { + beforeEach(() => { + event.payload.ref = 'refs/heads/other-branch' + }) - it('does not sync settings', async () => { - await app.receive(event) - expect(sync).not.toHaveBeenCalled() + it('does not sync settings', async () => { + await app.receive(event) + expect(sync).not.toHaveBeenCalled() + }) }) - }) - describe('with other files modified', () => { - beforeEach(() => { - event.payload = require('../fixtures/events/push.readme.json') - }) + describe('with other files modified', () => { + beforeEach(() => { + event.payload = require('../fixtures/events/push.readme.json') + }) - it('does not sync settings', async () => { - await app.receive(event) - expect(sync).not.toHaveBeenCalled() + it('does not sync settings', async () => { + await app.receive(event) + expect(sync).not.toHaveBeenCalled() + }) }) - }) - describe('default branch changed', () => { - beforeEach(() => { - event = { - name: 'repository.edited', - payload: require('../fixtures/events/repository.edited.json') - } - }) + describe.skip('default branch changed', () => { + beforeEach(() => { + event = { + name: 'repository.edited', + payload: require('../fixtures/events/repository.edited.json') + } + }) - it('does sync settings', async () => { - await app.receive(event) - expect(sync).toHaveBeenCalled() + it('does sync settings', async () => { + await app.receive(event) + expect(sync).toHaveBeenCalled() + }) }) - }) - describe('member event', () => { - beforeEach(() => { - event = { - name: 'member', - payload: require('../fixtures/events/member.json') - } - }) + describe('team added to repository', () => { + beforeEach(() => { + event = { + name: 'team.added_to_repository', + payload: require('../fixtures/events/team.added_to_repository.json') + } + }) - it('does sync settings', async () => { - await app.receive(event) - expect(sync).toHaveBeenCalled() + it('does sync settings', async () => { + await app.receive(event) + expect(sync).toHaveBeenCalled() + }) }) - }) - describe('team added to repository', () => { - beforeEach(() => { - event = { - name: 'team.added_to_repository', - payload: require('../fixtures/events/team.added_to_repository.json') - } - }) + describe('team removed from repository', () => { + beforeEach(() => { + event = { + name: 'team.removed_from_repository', + payload: require('../fixtures/events/team.removed_from_repository.json') + } + }) - it('does sync settings', async () => { - await app.receive(event) - expect(sync).toHaveBeenCalled() + it('does sync settings', async () => { + await app.receive(event) + expect(sync).toHaveBeenCalled() + }) }) - }) - describe('team removed from repository', () => { - beforeEach(() => { - event = { - name: 'team.removed_from_repository', - payload: require('../fixtures/events/team.removed_from_repository.json') - } - }) + describe('team access changed', () => { + beforeEach(() => { + event = { + name: 'team.edited', + payload: require('../fixtures/events/team.edited.json') + } + }) - it('does sync settings', async () => { - await app.receive(event) - expect(sync).toHaveBeenCalled() + it('does sync settings', async () => { + await app.receive(event) + expect(sync).toHaveBeenCalled() + }) }) - }) - describe('team access changed', () => { - beforeEach(() => { + describe('repository created', () => { event = { - name: 'team.edited', - payload: require('../fixtures/events/team.edited.json') + name: 'repository.created', + payload: {} } + + it('does sync settings', async () => { + await app.receive(event) + expect(sync).toHaveBeenCalled() + }) }) - it('does sync settings', async () => { - await app.receive(event) - expect(sync).toHaveBeenCalled() + describe('member event', () => { + beforeEach(() => { + event = { + name: 'member', + payload: require('../fixtures/events/member.json') + } + }) + + it('does sync settings', async () => { + await app.receive(event) + expect(sync).toHaveBeenCalled() + }) }) }) - describe('repository created', () => { - event = { - name: 'repository.created', - payload: {} - } + describe('admin repo', () => { + beforeEach(() => { + event.payload.repository.name = 'admin' + }) - it('does sync settings', async () => { - await app.receive(event) - expect(sync).toHaveBeenCalled() + describe('with settings modified on master', () => { + it('syncs settings', async () => { + await app.receive(event) + expect(syncAll).toHaveBeenCalled() + }) }) }) }) diff --git a/test/unit/lib/settings.test.js b/test/unit/lib/settings.test.js index 72dc13a75..b174644db 100644 --- a/test/unit/lib/settings.test.js +++ b/test/unit/lib/settings.test.js @@ -2,12 +2,23 @@ const { Octokit } = require('@octokit/core') const Settings = require('../../../lib/settings') const yaml = require('js-yaml') +const ConfigManager = require('../../../lib/configManager') // jest.mock('../../../lib/settings', () => { // const OriginalSettings = jest.requireActual('../../../lib/settings') // //const orginalSettingsInstance = new OriginalSettings(false, stubContext, mockRepo, config, mockRef, mockSubOrg) // return OriginalSettings // }) +jest.mock('../../../lib/plugins/repository.js', () => jest.fn(() => ({ + sync: jest.fn(async () => ({})) +}))) +const mockRepoPlugin = require('../../../lib/plugins/repository.js') + +jest.mock('../../../lib/plugins/custom_properties.js', () => jest.fn(() => ({ + sync: jest.fn(async () => ({})) +}))) +const mockCustomPropsPlugin = require('../../../lib/plugins/custom_properties.js') + describe('Settings Tests', () => { let stubContext let mockRepo @@ -15,6 +26,7 @@ describe('Settings Tests', () => { let mockRef let mockSubOrg let subOrgConfig + let mockOctokit function createSettings(config) { const settings = new Settings(false, stubContext, mockRepo, config, mockRef, mockSubOrg) @@ -22,7 +34,7 @@ describe('Settings Tests', () => { } beforeEach(() => { - const mockOctokit = jest.mocked(Octokit) + mockOctokit = jest.mocked(Octokit) const content = Buffer.from(` suborgrepos: - new-repo @@ -82,11 +94,11 @@ repository: } } - - mockRepo = { owner: 'test', repo: 'test-repo' } mockRef = 'main' mockSubOrg = 'frontend' + + jest.clearAllMocks() }) describe('restrictedRepos', () => { @@ -264,7 +276,6 @@ repository: - frontend `) - }) it("Should load configMap for suborgs'", async () => { @@ -303,4 +314,62 @@ repository: }) }) // loadConfigs + describe('updateRepos', () => { + beforeEach(() => { + repoConfig = { + repository: { + description: 'This is a description' + }, + custom_properties: [{ name: 'test', value: 'test' }] + } + }) + + describe('without unsafeFields', () => { + beforeEach(() => { + stubConfig = { + restrictedRepos: {}, + unsafeFields: [] + } + }) + + it('should not set the description or custom properties', async () => { + settings = new Settings(false, stubContext, mockRepo, stubConfig, mockRef, null) + + jest.spyOn(settings, 'loadConfigMap').mockImplementation(() => []) + jest.spyOn(settings, 'getRepoConfigMap').mockImplementation(() => []) + jest.spyOn(ConfigManager, 'loadYaml').mockImplementation(() => repoConfig) + mockOctokit.repos.get = jest.fn().mockResolvedValue({ data: {} }) + + await settings.loadConfigs({ repo: 'test-repo' }) + await settings.updateRepos({ repo: 'test-repo' }) + + expect(mockRepoPlugin).not.toHaveBeenCalled() + expect(mockCustomPropsPlugin).not.toHaveBeenCalled() + }) + }) + + describe('with unsafeFields', () => { + beforeEach(() => { + stubConfig = { + restrictedRepos: {}, + unsafeFields: ['/repository/description', '/custom_properties'] + } + }) + + it('should call the plugins to set the values', async () => { + settings = new Settings(false, stubContext, mockRepo, stubConfig, mockRef, null) + + jest.spyOn(settings, 'loadConfigMap').mockImplementation(() => []) + jest.spyOn(settings, 'getRepoConfigMap').mockImplementation(() => []) + jest.spyOn(ConfigManager, 'loadYaml').mockImplementation(() => repoConfig) + mockOctokit.repos.get = jest.fn().mockResolvedValue({ data: {} }) + + await settings.loadConfigs({ repo: 'test-repo' }) + await settings.updateRepos({ repo: 'test-repo' }) + + expect(mockRepoPlugin).toHaveBeenCalledWith(false, expect.anything(), {"repo": "test-repo"}, { "description": "This is a description" }, 123, expect.anything(), []) + expect(mockCustomPropsPlugin).toHaveBeenCalledWith(false, expect.anything(), {"repo": "test-repo"}, [{"name": "test", "value": "test"}], expect.anything(), []) + }) + }) + }) // updateRepos }) // Settings Tests From cc3ab91b3f2df52becab6a00c3adb788015c9bc5 Mon Sep 17 00:00:00 2001 From: Jonathan Morley Date: Wed, 5 Mar 2025 17:03:04 -0500 Subject: [PATCH 2/2] docs: update README for unsafeFields --- README.md | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 5cd5eeb9a..292375501 100644 --- a/README.md +++ b/README.md @@ -2,11 +2,16 @@ [![Create a release](https://github.com/github/safe-settings/actions/workflows/create-release.yml/badge.svg)](https://github.com/github/safe-settings/actions/workflows/create-release.yml) -`Safe-settings` – an app to manage policy-as-code and apply repository settings across an organization. +`Safe-settings` - an app to manage policy-as-code and apply repository settings across an organization. -1. In `safe-settings`, all the settings are stored centrally in an `admin` repo within the organization. Unlike the [GitHub Repository Settings App](https://github.com/repository-settings/app), the settings files cannot be in individual repositories. +## Settings Locations - > It is possible specify a custom repo instead of the `admin` repo with `ADMIN_REPO`. See [Environment variables](#environment-variables) for more details. +Settings can be stored in: +- An `admin` repository within the organization. + > It is possible to specify a custom repo instead of the `admin` repo with `ADMIN_REPO`. See [Environment variables](#environment-variables) for more details. +- A `.github/settings.yml` file in each repository (like the [GitHub Repository Settings App](https://github.com/repository-settings/app)). See [Unsafe Settings](#unsafe-settings) for more details. + +### `admin` Repository 1. The **settings** in the **default** branch are applied. If the settings are changed on a non-default branch and a PR is created to merge the changes, the app runs in a `dry-run` mode to evaluate and validate the changes. Checks pass or fail based on the `dry-run` results. @@ -31,6 +36,13 @@ > The `suborg` and `repo` level settings directory structure cannot be customized. > +### Unsafe Settings + +In the [runtime settings](#runtime-settings) file, it is possible to specify a list of `unsafeFields` that are allowed to be configured by repositories in the organization. Each `unsafeField` is expressed as a [JSON Pointer](https://datatracker.ietf.org/doc/html/rfc6901). All values under an `unsafeField` are also 'unsafe', so it is advised to provide the unsafeFields as precisely as possible. + +See the [sample deployment settings](./docs/sample-settings/sample-deployment-settings.yml) for an example of configuring `unsafeFields`. + +With this set, repositories can create `.github/settings.yml` files (not currently configurable) that will be merged with values in the `admin` repository when safe-settings is run. Entries in this file that do not match `unsafeFields` in the runtime settings will be ignored. ## How it works @@ -409,8 +421,17 @@ You can pass environment variables; the easiest way to do it is via a `.env` fil 1. Besides the above settings files, the application can be bootstrapped with `runtime` settings. 2. The `runtime` settings are configured in `deployment-settings.yml` that is in the directory from where the GitHub app is running. -3. Currently the only setting that is possible are `restrictedRepos: [... ]` which allows you to configure a list of repos within your `org` that are excluded from the settings. If the `deployment-settings.yml` is not present, the following repos are added by default to the `restricted`repos list: `'admin', '.github', 'safe-settings'` +#### Restricted Repos + +Use `restrictedRepos: [...]` to configure a list of repos within your `org` that are excluded from the settings. If the `deployment-settings.yml` is not present, the following repos are added by default to the `restricted`repos list: + - `admin` + - `.github` + - `safe-settings` + +#### Unsafe Fields + +Use `unsafeFields: [...]` to mark fields as configurable from individual repos. See [Unsafe Settings](#unsafe-settings) for more details. ### Notes