From d1533fddf60fb578bbadc8693fa2371c5592e33e Mon Sep 17 00:00:00 2001 From: "rick.stokkingreef" Date: Thu, 27 Feb 2025 17:22:28 +0100 Subject: [PATCH 1/2] {commit_message} From 2b82e20be2cf5235d5106e2e91b78336c5bb6a8e Mon Sep 17 00:00:00 2001 From: "rick.stokkingreef" Date: Fri, 28 Feb 2025 14:56:26 +0100 Subject: [PATCH 2/2] feat: Add support for checking all changes of PR This Adds support for checking entire PR context instead of only the last commit. Also fixes a bunch of errors making sure it uses the correct Repo when looping over the list. When setting `RepoConfig` it Assigned the object to the last one in the loop of Repositories. Resulting in wrong comparision. --- Dockerfile | 4 +-- index.js | 8 ++--- lib/env.js | 1 + lib/plugins/rulesets.js | 4 +-- lib/settings.js | 71 +++++++++++++++++++++-------------------- 5 files changed, 46 insertions(+), 42 deletions(-) diff --git a/Dockerfile b/Dockerfile index 5c2fcdf5..6c8edb02 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,6 +1,6 @@ FROM node:20-alpine WORKDIR /opt/safe-settings -ENV NODE_ENV production +ENV NODE_ENV=production ## Set the Labels LABEL version="1.0" \ description="Probot app which is a modified version of Settings Probot GitHub App" \ @@ -22,4 +22,4 @@ USER node ## This does not start properly when using the ['npm','start'] format ## so stick with just calling it outright -CMD npm start +CMD ["npm", "start"] diff --git a/index.js b/index.js index 40e05739..03be803a 100644 --- a/index.js +++ b/index.js @@ -587,10 +587,10 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) => robot.log.debug(`Updating check run ${JSON.stringify(params)}`) await context.octokit.checks.update(params) - // guarding against null value from upstream libary that is - // causing a 404 and the check to stall - // from issue: https://github.com/github/safe-settings/issues/185#issuecomment-1075240374 - if (check_suite.before === '0000000000000000000000000000000000000000') { + if (env.PR_USE_BASE_SHA === 'true') { + check_suite.before = check_suite.pull_requests[0].base.sha + robot.log.debug(`Using PR's base sha: ${check_suite.before}...${check_suite.after}`) + } else if (check_suite.before === '0000000000000000000000000000000000000000') { check_suite.before = check_suite.pull_requests[0].base.sha } params = Object.assign(context.repo(), { basehead: `${check_suite.before}...${check_suite.after}` }) diff --git a/lib/env.js b/lib/env.js index fbc2afed..37d2b3f1 100644 --- a/lib/env.js +++ b/lib/env.js @@ -4,6 +4,7 @@ module.exports = { SETTINGS_FILE_PATH: process.env.SETTINGS_FILE_PATH || 'settings.yml', DEPLOYMENT_CONFIG_FILE: process.env.DEPLOYMENT_CONFIG_FILE || 'deployment-settings.yml', CREATE_PR_COMMENT: process.env.CREATE_PR_COMMENT || 'true', + PR_USE_BASE_SHA: process.env.PR_USE_BASE_SHA || 'false', CREATE_ERROR_ISSUE: process.env.CREATE_ERROR_ISSUE || 'true', BLOCK_REPO_RENAME_BY_HUMAN: process.env.BLOCK_REPO_RENAME_BY_HUMAN || 'false' } diff --git a/lib/plugins/rulesets.js b/lib/plugins/rulesets.js index c4208de8..2499eea4 100644 --- a/lib/plugins/rulesets.js +++ b/lib/plugins/rulesets.js @@ -15,7 +15,7 @@ const version = { 'X-GitHub-Api-Version': '2022-11-28' } module.exports = class Rulesets extends Diffable { - constructor (nop, github, repo, entries, log, errors, scope) { + constructor(nop, github, repo, entries, log, errors, scope) { super(nop, github, repo, entries, log, errors) this.github = github this.repo = repo @@ -28,7 +28,7 @@ module.exports = class Rulesets extends Diffable { // Find all Rulesets for this org find () { if (this.scope === 'org') { - this.log.debug(`Getting all rulesets for the org ${this.org}`) + this.log.debug(`Getting all rulesets for the org ${this.repo.owner}`) const listOptions = this.github.request.endpoint.merge('GET /orgs/{org}/rulesets', { org: this.repo.owner, diff --git a/lib/settings.js b/lib/settings.js index 9e00c140..8183b7e4 100644 --- a/lib/settings.js +++ b/lib/settings.js @@ -14,6 +14,7 @@ class Settings { static async syncAll (nop, context, repo, config, ref) { const settings = new Settings(nop, context, repo, config, ref) try { + settings.log.debug('Starting syncAll') await settings.loadConfigs() // settings.repoConfigs = await settings.getRepoConfigs() await settings.updateOrg() @@ -26,9 +27,10 @@ class Settings { return settings } - static async syncSubOrgs(nop, context, suborg, repo, config, ref) { + static async syncSubOrgs (nop, context, suborg, repo, config, ref) { const settings = new Settings(nop, context, repo, config, ref, suborg) try { + settings.log.debug('Starting syncSubOrgs') await settings.loadConfigs() await settings.updateAll() await settings.handleResults() @@ -38,9 +40,10 @@ class Settings { } } - static async sync(nop, context, repo, config, ref) { + static async sync (nop, context, repo, config, ref) { const settings = new Settings(nop, context, repo, config, ref) try { + settings.log.debug('Starting sync') await settings.loadConfigs(repo) if (settings.isRestricted(repo.repo)) { return @@ -53,7 +56,7 @@ class Settings { } } - static async handleError(nop, context, repo, config, ref, nopcommand) { + static async handleError (nop, context, repo, config, ref, nopcommand) { const settings = new Settings(nop, context, repo, config, ref) settings.appendToResults([nopcommand]) await settings.handleResults() @@ -98,7 +101,7 @@ class Settings { } // Create a check in the Admin repo for safe-settings. - async createCheckRun() { + async createCheckRun () { const startTime = new Date() let conclusion = 'success' let details = `Run on: \`${new Date().toISOString()}\`` @@ -144,7 +147,7 @@ class Settings { }) } - logError(msg) { + logError (msg) { this.log.error(msg) this.errors.push({ owner: this.repo.owner, @@ -154,7 +157,7 @@ class Settings { }) } - async handleResults() { + async handleResults () { const { payload } = this.context // Create a checkrun if not in nop mode @@ -166,10 +169,10 @@ class Settings { //remove duplicate rows in this.results this.results = this.results.filter((thing, index, self) => { - return index === self.findIndex((t) => { - return t.type === thing.type && t.repo === thing.repo && t.plugin === thing.plugin - }) + return index === self.findIndex((t) => { + return t.type === thing.type && t.repo === thing.repo && t.plugin === thing.plugin }) + }) let error = false // Different logic @@ -281,12 +284,12 @@ ${this.results.reduce((x, y) => { await this.github.checks.update(params) } - async loadConfigs(repo) { + async loadConfigs (repo) { this.subOrgConfigs = await this.getSubOrgConfigs() this.repoConfigs = await this.getRepoConfigs(repo) } - async updateOrg() { + async updateOrg () { const rulesetsConfig = this.config.rulesets if (rulesetsConfig) { const RulesetsPlugin = Settings.PLUGINS.rulesets @@ -296,11 +299,11 @@ ${this.results.reduce((x, y) => { } } - async updateRepos(repo) { + 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 }) + repoConfig = Object.assign({}, repoConfig, { name: repo.repo, org: repo.owner }) } const subOrgConfig = this.getSubOrgConfig(repo.repo) @@ -327,7 +330,7 @@ ${this.results.reduce((x, y) => { if (overrideRepoConfig) { repoConfig = this.mergeDeep.mergeDeep({}, repoConfig, overrideRepoConfig) } - const {shouldContinue, nopCommands} = await new Archive(this.nop, this.github, repo, repoConfig, this.log).sync() + const { shouldContinue, nopCommands } = await new Archive(this.nop, this.github, repo, repoConfig, this.log).sync() if (nopCommands) this.appendToResults(nopCommands) if (shouldContinue) { if (repoConfig) { @@ -366,7 +369,7 @@ ${this.results.reduce((x, y) => { } } - async updateAll() { + async updateAll () { // this.subOrgConfigs = this.subOrgConfigs || await this.getSubOrgConfigs(this.github, this.repo, this.log) // this.repoConfigs = this.repoConfigs || await this.getRepoConfigs(this.github, this.repo, this.log) return this.eachRepositoryRepos(this.github, this.log).then(res => { @@ -374,7 +377,7 @@ ${this.results.reduce((x, y) => { }) } - getSubOrgConfig(repoName) { + getSubOrgConfig (repoName) { if (this.subOrgConfigs) { for (const k of Object.keys(this.subOrgConfigs)) { const repoPattern = new Glob(k) @@ -387,13 +390,13 @@ ${this.results.reduce((x, y) => { } // Remove Org specific configs from the repo config - returnRepoSpecificConfigs(config) { + returnRepoSpecificConfigs (config) { const newConfig = Object.assign({}, config) // clone delete newConfig.rulesets return newConfig } - childPluginsList(repo) { + childPluginsList (repo) { const repoName = repo.repo const subOrgOverrideConfig = this.getSubOrgConfig(repoName) this.log.debug(`suborg config for ${repoName} is ${JSON.stringify(subOrgOverrideConfig)}`) @@ -425,7 +428,7 @@ ${this.results.reduce((x, y) => { return childPlugins } - validate(section, baseConfig, overrideConfig) { + validate (section, baseConfig, overrideConfig) { const configValidator = this.configvalidators[section] if (configValidator) { this.log.debug(`Calling configvalidator for key ${section} `) @@ -444,7 +447,7 @@ ${this.results.reduce((x, y) => { } } - isRestricted(repoName) { + isRestricted (repoName) { const restrictedRepos = this.config.restrictedRepos // Skip configuring any restricted repos if (Array.isArray(restrictedRepos)) { @@ -476,7 +479,7 @@ ${this.results.reduce((x, y) => { return false } - includesRepo(repoName, restrictedRepos) { + includesRepo (repoName, restrictedRepos) { return restrictedRepos.filter((restrictedRepo) => { return RegExp(restrictedRepo).test(repoName) }).length > 0 } @@ -531,7 +534,7 @@ ${this.results.reduce((x, y) => { * @param params Params to fetch the file with * @return The parsed YAML file */ - async loadConfigMap(params) { + async loadConfigMap (params) { try { this.log.debug(` In loadConfigMap ${JSON.stringify(params)}`) const response = await this.github.repos.getContent(params).catch(e => { @@ -578,7 +581,7 @@ ${this.results.reduce((x, y) => { * @param params Params to fetch the file with * @return The parsed YAML file */ - async getRepoConfigMap() { + async getRepoConfigMap () { try { this.log.debug(` In getRepoConfigMap ${JSON.stringify(this.repo)}`) // GitHub getContent api has a hard limit of returning 1000 entries without @@ -645,7 +648,7 @@ ${this.results.reduce((x, y) => { * @param params Params to fetch the file with * @return The parsed YAML file */ - async getSubOrgConfigMap() { + async getSubOrgConfigMap () { try { this.log.debug(` In getSubOrgConfigMap ${JSON.stringify(this.repo)}`) const repo = { owner: this.repo.owner, repo: env.ADMIN_REPO } @@ -672,7 +675,7 @@ ${this.results.reduce((x, y) => { * @param {*} repo repo param * @returns repoConfigs object */ - async getRepoConfigs(repo) { + async getRepoConfigs (repo) { try { const overridePaths = await this.getRepoConfigMap() const repoConfigs = {} @@ -724,7 +727,7 @@ ${this.results.reduce((x, y) => { * @param params Params to fetch the file with * @return The parsed YAML file */ - async getSubOrgConfigs() { + async getSubOrgConfigs () { try { // Get all suborg configs even though we might be here becuase of a suborg config change // we will filter them out if request is due to a suborg config change @@ -791,7 +794,7 @@ ${this.results.reduce((x, y) => { } )) { delete subOrgConfigs[key] - } + } } } return subOrgConfigs @@ -807,7 +810,7 @@ ${this.results.reduce((x, y) => { } } - storeSubOrgConfigIfNoConflicts(subOrgConfigs, overridePath, repoName, data) { + storeSubOrgConfigIfNoConflicts (subOrgConfigs, overridePath, repoName, data) { const existingConfigForRepo = subOrgConfigs[repoName] if (existingConfigForRepo && existingConfigForRepo.source !== overridePath) { throw new Error(`Multiple suborg configs for ${repoName} in ${overridePath} and ${existingConfigForRepo?.source}`) @@ -821,7 +824,7 @@ ${this.results.reduce((x, y) => { * @param params Params to fetch the file with * @return The parsed YAML file */ - async loadYaml(filePath) { + async loadYaml (filePath) { try { const repo = { owner: this.repo.owner, repo: env.ADMIN_REPO } const params = Object.assign(repo, { path: filePath, ref: this.ref }) @@ -858,16 +861,16 @@ ${this.results.reduce((x, y) => { } } - appendToResults(res) { + appendToResults (res) { if (this.nop) { //Remove nulls and undefined from the results - const results = res.flat(3).filter(r => r) - + const results = Array.isArray(res) ? res.flat(3).filter(r => r) : []; + this.results = this.results.concat(results) } } - async getReposForTeam(teamslug) { + async getReposForTeam (teamslug) { const options = this.github.rest.teams.listReposInOrg.endpoint.merge({ org: this.repo.owner, team_slug: teamslug, @@ -888,7 +891,7 @@ ${this.results.reduce((x, y) => { return (item && typeof item === 'object' && !Array.isArray(item)) } - isIterable(obj) { + isIterable (obj) { // checks for null and undefined if (obj == null) { return false