From 377bc7e9335f290680bb3f76ca32c407164dbb13 Mon Sep 17 00:00:00 2001 From: Kyle Harding Date: Thu, 5 Sep 2024 14:56:27 -0400 Subject: [PATCH 1/3] Add test to update existing environment variables Signed-off-by: Kyle Harding --- test/unit/lib/plugins/environments.test.js | 66 ++++++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/test/unit/lib/plugins/environments.test.js b/test/unit/lib/plugins/environments.test.js index 276a82cd..efa97e03 100644 --- a/test/unit/lib/plugins/environments.test.js +++ b/test/unit/lib/plugins/environments.test.js @@ -403,6 +403,72 @@ describe('Environments Plugin test suite', () => { }) }) + // patch variable + describe('When there are existing variables and one requires an update', () => { + it('detect divergence and add the variable', async () => { + //arrange + environment_name = 'variables_environment' + // represent config with a reviewers being a user and a team + const plugin = new Environments(undefined, github, { owner: org, repo }, [ + { + name: environment_name, + variables: [ + { + name: 'test', + value: 'test' + } + ] + } + ], log, errors); + + //model an existing environment with no reviewers + when(github.request) + .calledWith('GET /repos/:org/:repo/environments', { org, repo }) + .mockResolvedValue({ + data: { + environments: [ + fillEnvironment({ + name: environment_name, + variables: [] + }) + ] + } + }); + + when(github.request) + .calledWith('GET /repos/:org/:repo/environments/:environment_name/variables', { org, repo, environment_name }) + .mockResolvedValue({ + data: { + variables: [ + { + name: 'test', + value: 'old' + }, + { + name: 'test2', + value: 'test2' + } + ] + } + }) + + //act - run sync() in environments.js + await plugin.sync().then(() => { + //assert - update the variables + expect(github.request).toHaveBeenCalledWith('GET /repos/:org/:repo/environments', { org, repo }); + expect(github.request).toHaveBeenCalledWith('GET /repos/:org/:repo/environments/:environment_name/variables', { org, repo, environment_name }); + expect(github.request).toHaveBeenCalledWith('GET /repos/:org/:repo/environments/:environment_name/deployment_protection_rules', { org, repo, environment_name }); + expect(github.request).toHaveBeenCalledWith('PATCH /repos/:org/:repo/environments/:environment_name/variables/:variable_name', expect.objectContaining({ + org, + repo, + environment_name: environment_name, + variable_name: 'test', + value: 'test' + })); + }) + }) + }) + // add deployment protection rules describe('When there are no existing deployment protection rules, but config calls for one', () => { it('detect divergence and add the deployment protection rule', async () => { From 893fa8ab35044c7629df8ed6716d1b4d6a4d426a Mon Sep 17 00:00:00 2001 From: Kyle Harding Date: Thu, 5 Sep 2024 14:57:04 -0400 Subject: [PATCH 2/3] Apply eslint fixes to environments.js There are still a bunch of errors like variable names that need to be resolved manually. Signed-off-by: Kyle Harding --- lib/plugins/environments.js | 625 ++++++++++++++++++------------------ 1 file changed, 310 insertions(+), 315 deletions(-) diff --git a/lib/plugins/environments.js b/lib/plugins/environments.js index 6d52b409..e146bf66 100644 --- a/lib/plugins/environments.js +++ b/lib/plugins/environments.js @@ -3,365 +3,360 @@ const MergeDeep = require('../mergeDeep') const NopCommand = require('../nopcommand') module.exports = class Environments extends Diffable { - constructor(...args) { - super(...args) - - if (this.entries) { - // Force all names to lowercase to avoid comparison issues. - this.entries.forEach(environment => { - environment.name = environment.name.toLowerCase(); - if(environment.variables) { - environment.variables.forEach(variable => { - variable.name = variable.name.toLowerCase(); - }); - } - }) + constructor (...args) { + super(...args) + + if (this.entries) { + // Force all names to lowercase to avoid comparison issues. + this.entries.forEach(environment => { + environment.name = environment.name.toLowerCase() + if (environment.variables) { + environment.variables.forEach(variable => { + variable.name = variable.name.toLowerCase() + }) } + }) + } - // Remove 'name' from filtering list so Environments with only a name defined are processed. - MergeDeep.NAME_FIELDS.splice(MergeDeep.NAME_FIELDS.indexOf('name'), 1) - + // Remove 'name' from filtering list so Environments with only a name defined are processed. + MergeDeep.NAME_FIELDS.splice(MergeDeep.NAME_FIELDS.indexOf('name'), 1) + } + + async find () { + const { data: { environments } } = await this.github.request('GET /repos/:org/:repo/environments', { + org: this.repo.owner, + repo: this.repo.repo + }) + + const environments_mapped = [] + + for (const environment of environments) { + const mapped = { + name: environment.name.toLowerCase(), + repo: this.repo.repo, + wait_timer: (environment.protection_rules.find(rule => rule.type === 'wait_timer') || { wait_timer: 0 }).wait_timer, + prevent_self_review: (environment.protection_rules.find(rule => rule.type === 'required_reviewers') || { prevent_self_review: false }).prevent_self_review, + reviewers: (environment.protection_rules.find(rule => rule.type === 'required_reviewers') || { reviewers: [] }).reviewers.map(reviewer => ({ id: reviewer.reviewer.id, type: reviewer.type })), + deployment_branch_policy: environment.deployment_branch_policy === null + ? null + : { + protected_branches: (environment.deployment_branch_policy || { protected_branches: false }).protected_branches, + custom_branch_policies: (environment.deployment_branch_policy || { custom_branch_policies: false }).custom_branch_policies && (await this.github.request('GET /repos/:org/:repo/environments/:environment_name/deployment-branch-policies', { + org: this.repo.owner, + repo: this.repo.repo, + environment_name: environment.name + })).data.branch_policies.map(policy => ({ + name: policy.name + })) + }, + variables: (await this.github.request('GET /repos/:org/:repo/environments/:environment_name/variables', { + org: this.repo.owner, + repo: this.repo.repo, + environment_name: environment.name + })).data.variables.map(variable => ({ name: variable.name.toLowerCase(), value: variable.value })), + deployment_protection_rules: (await this.github.request('GET /repos/:org/:repo/environments/:environment_name/deployment_protection_rules', { + org: this.repo.owner, + repo: this.repo.repo, + environment_name: environment.name + })).data.custom_deployment_protection_rules.map(rule => ({ + app_id: rule.app.id, + id: rule.id + })) + } + environments_mapped.push(mapped) + // console.log(mapped); } - async find() { - const { data: { environments } } = await this.github.request('GET /repos/:org/:repo/environments', { - org: this.repo.owner, - repo: this.repo.repo - }); + return environments_mapped + } - let environments_mapped = []; + comparator (existing, attrs) { + return existing.name === attrs.name + } - for(let environment of environments) { - const mapped = { - name: environment.name.toLowerCase(), - repo: this.repo.repo, - wait_timer: (environment.protection_rules.find(rule => rule.type === 'wait_timer') || { wait_timer: 0 }).wait_timer, - prevent_self_review: (environment.protection_rules.find(rule => rule.type === 'required_reviewers') || { prevent_self_review: false }).prevent_self_review, - reviewers: (environment.protection_rules.find(rule => rule.type === 'required_reviewers') || { reviewers: [] }).reviewers.map(reviewer => ({id: reviewer.reviewer.id, type: reviewer.type})), - deployment_branch_policy: environment.deployment_branch_policy === null ? null : { - protected_branches: (environment.deployment_branch_policy || { protected_branches: false }).protected_branches, - custom_branch_policies: (environment.deployment_branch_policy || { custom_branch_policies: false }).custom_branch_policies && (await this.github.request('GET /repos/:org/:repo/environments/:environment_name/deployment-branch-policies', { - org: this.repo.owner, - repo: this.repo.repo, - environment_name: environment.name - })).data.branch_policies.map(policy => ({ - name: policy.name - })) - }, - variables: (await this.github.request('GET /repos/:org/:repo/environments/:environment_name/variables', { - org: this.repo.owner, - repo: this.repo.repo, - environment_name: environment.name - })).data.variables.map(variable => ({name: variable.name.toLowerCase(), value: variable.value})), - deployment_protection_rules: (await this.github.request('GET /repos/:org/:repo/environments/:environment_name/deployment_protection_rules', { - org: this.repo.owner, - repo: this.repo.repo, - environment_name: environment.name - })).data.custom_deployment_protection_rules.map(rule => ({ - app_id: rule.app.id, - id: rule.id - })) - } - environments_mapped.push(mapped); - //console.log(mapped); - } + getChanged (existing, attrs) { + if (!attrs.wait_timer) attrs.wait_timer = 0 + if (!attrs.prevent_self_review) attrs.prevent_self_review = false + if (!attrs.reviewers) attrs.reviewers = [] + if (!attrs.deployment_branch_policy) attrs.deployment_branch_policy = null + if (!attrs.variables) attrs.variables = [] + if (!attrs.deployment_protection_rules) attrs.deployment_protection_rules = [] - return environments_mapped; - } + const wait_timer = existing.wait_timer !== attrs.wait_timer + const prevent_self_review = existing.prevent_self_review !== attrs.prevent_self_review + const reviewers = JSON.stringify(existing.reviewers.sort((x1, x2) => x1.id - x2.id)) !== JSON.stringify(attrs.reviewers.sort((x1, x2) => x1.id - x2.id)) - comparator(existing, attrs) { - return existing.name === attrs.name + let existing_custom_branch_policies = existing.deployment_branch_policy === null ? null : existing.deployment_branch_policy.custom_branch_policies + if (typeof (existing_custom_branch_policies) === 'object' && existing_custom_branch_policies !== null) { + existing_custom_branch_policies = existing_custom_branch_policies.sort() } - - getChanged(existing, attrs) { - if (!attrs.wait_timer) attrs.wait_timer = 0; - if (!attrs.prevent_self_review) attrs.prevent_self_review = false; - if (!attrs.reviewers) attrs.reviewers = []; - if (!attrs.deployment_branch_policy) attrs.deployment_branch_policy = null; - if(!attrs.variables) attrs.variables = []; - if(!attrs.deployment_protection_rules) attrs.deployment_protection_rules = []; - - const wait_timer = existing.wait_timer !== attrs.wait_timer; - const prevent_self_review = existing.prevent_self_review !== attrs.prevent_self_review; - const reviewers = JSON.stringify(existing.reviewers.sort((x1, x2) => x1.id - x2.id)) !== JSON.stringify(attrs.reviewers.sort((x1, x2) => x1.id - x2.id)); - - let existing_custom_branch_policies = existing.deployment_branch_policy === null ? null : existing.deployment_branch_policy.custom_branch_policies; - if(typeof(existing_custom_branch_policies) === 'object' && existing_custom_branch_policies !== null) { - existing_custom_branch_policies = existing_custom_branch_policies.sort(); - } - let attrs_custom_branch_policies = attrs.deployment_branch_policy === null ? null : attrs.deployment_branch_policy.custom_branch_policies; - if(typeof(attrs_custom_branch_policies) === 'object' && attrs_custom_branch_policies !== null) { - attrs_custom_branch_policies = attrs_custom_branch_policies.sort(); - } - let deployment_branch_policy; - if(existing.deployment_branch_policy === attrs.deployment_branch_policy) { - deployment_branch_policy = false; - } - else { - deployment_branch_policy = ( - (existing.deployment_branch_policy === null && attrs.deployment_branch_policy !== null) || + let attrs_custom_branch_policies = attrs.deployment_branch_policy === null ? null : attrs.deployment_branch_policy.custom_branch_policies + if (typeof (attrs_custom_branch_policies) === 'object' && attrs_custom_branch_policies !== null) { + attrs_custom_branch_policies = attrs_custom_branch_policies.sort() + } + let deployment_branch_policy + if (existing.deployment_branch_policy === attrs.deployment_branch_policy) { + deployment_branch_policy = false + } else { + deployment_branch_policy = ( + (existing.deployment_branch_policy === null && attrs.deployment_branch_policy !== null) || (existing.deployment_branch_policy !== null && attrs.deployment_branch_policy === null) || (existing.deployment_branch_policy.protected_branches !== attrs.deployment_branch_policy.protected_branches) || (JSON.stringify(existing_custom_branch_policies) !== JSON.stringify(attrs_custom_branch_policies)) - ); - } - - const variables = JSON.stringify(existing.variables.sort((x1, x2) => x1.name - x2.name)) !== JSON.stringify(attrs.variables.sort((x1, x2) => x1.name - x2.name)); - const deployment_protection_rules = JSON.stringify(existing.deployment_protection_rules.map(x => ({app_id: x.app_id})).sort((x1, x2) => x1.app_id - x2.app_id)) !== JSON.stringify(attrs.deployment_protection_rules.map(x => ({app_id: x.app_id})).sort((x1, x2) => x1.app_id - x2.app_id)); + ) + } - return {wait_timer, prevent_self_review, reviewers, deployment_branch_policy, variables, deployment_protection_rules}; + const variables = JSON.stringify(existing.variables.sort((x1, x2) => x1.name - x2.name)) !== JSON.stringify(attrs.variables.sort((x1, x2) => x1.name - x2.name)) + const deployment_protection_rules = JSON.stringify(existing.deployment_protection_rules.map(x => ({ app_id: x.app_id })).sort((x1, x2) => x1.app_id - x2.app_id)) !== JSON.stringify(attrs.deployment_protection_rules.map(x => ({ app_id: x.app_id })).sort((x1, x2) => x1.app_id - x2.app_id)) + + return { wait_timer, prevent_self_review, reviewers, deployment_branch_policy, variables, deployment_protection_rules } + } + + changed (existing, attrs) { + const { wait_timer, prevent_self_review, reviewers, deployment_branch_policy, variables, deployment_protection_rules } = this.getChanged(existing, attrs) + + return wait_timer || prevent_self_review || reviewers || deployment_branch_policy || variables || deployment_protection_rules + } + + async update (existing, attrs) { + const { wait_timer, prevent_self_review, reviewers, deployment_branch_policy, variables, deployment_protection_rules } = this.getChanged(existing, attrs) + + if (wait_timer || prevent_self_review || reviewers || deployment_branch_policy) { + await this.github.request('PUT /repos/:org/:repo/environments/:environment_name', { + org: this.repo.owner, + repo: this.repo.repo, + environment_name: attrs.name, + wait_timer: attrs.wait_timer, + prevent_self_review: attrs.prevent_self_review, + reviewers: attrs.reviewers, + deployment_branch_policy: attrs.deployment_branch_policy === null + ? null + : { + protected_branches: attrs.deployment_branch_policy.protected_branches, + custom_branch_policies: !!attrs.deployment_branch_policy.custom_branch_policies + } + }) } - changed(existing, attrs) { - const {wait_timer, prevent_self_review, reviewers, deployment_branch_policy, variables, deployment_protection_rules} = this.getChanged(existing, attrs); + if (deployment_branch_policy && attrs.deployment_branch_policy && attrs.deployment_branch_policy.custom_branch_policies) { + const existingPolicies = (await this.github.request('GET /repos/:org/:repo/environments/:environment_name/deployment-branch-policies', { + org: this.repo.owner, + repo: this.repo.repo, + environment_name: attrs.name + })).data.branch_policies + + for (const policy of existingPolicies) { + await this.github.request('DELETE /repos/:org/:repo/environments/:environment_name/deployment-branch-policies/:branch_policy_id', { + org: this.repo.owner, + repo: this.repo.repo, + environment_name: attrs.name, + branch_policy_id: policy.id + }) + } - return wait_timer || prevent_self_review || reviewers || deployment_branch_policy || variables || deployment_protection_rules; + for (const policy of attrs.deployment_branch_policy.custom_branch_policies) { + await this.github.request('POST /repos/:org/:repo/environments/:environment_name/deployment-branch-policies', { + org: this.repo.owner, + repo: this.repo.repo, + environment_name: attrs.name, + name: policy + }) + } } - async update(existing, attrs) { - const {wait_timer, prevent_self_review, reviewers, deployment_branch_policy, variables, deployment_protection_rules} = this.getChanged(existing, attrs); - - if(wait_timer || prevent_self_review || reviewers || deployment_branch_policy) { - await this.github.request(`PUT /repos/:org/:repo/environments/:environment_name`, { - org: this.repo.owner, - repo: this.repo.repo, - environment_name: attrs.name, - wait_timer: attrs.wait_timer, - prevent_self_review: attrs.prevent_self_review, - reviewers: attrs.reviewers, - deployment_branch_policy: attrs.deployment_branch_policy === null ? null : { - protected_branches: attrs.deployment_branch_policy.protected_branches, - custom_branch_policies: !!attrs.deployment_branch_policy.custom_branch_policies - } + if (variables) { + let existingVariables = [...existing.variables] + + for (const variable of attrs.variables) { + const existingVariable = existingVariables.find((_var) => _var.name === variable.name) + if (existingVariable) { + existingVariables = existingVariables.filter(_var => _var.name !== variable.name) + if (existingVariable.value !== variable.value) { + await this.github.request('PATCH /repos/:org/:repo/environments/:environment_name/variables/:variable_name', { + org: this.repo.owner, + repo: this.repo.repo, + environment_name: attrs.name, + variable_name: variable.name, + value: variable.value }) + } + } else { + await this.github.request('POST /repos/:org/:repo/environments/:environment_name/variables', { + org: this.repo.owner, + repo: this.repo.repo, + environment_name: attrs.name, + name: variable.name, + value: variable.value + }) } + } - if(deployment_branch_policy && attrs.deployment_branch_policy && attrs.deployment_branch_policy.custom_branch_policies) { - const existingPolicies = (await this.github.request('GET /repos/:org/:repo/environments/:environment_name/deployment-branch-policies', { - org: this.repo.owner, - repo: this.repo.repo, - environment_name: attrs.name - })).data.branch_policies; - - for(let policy of existingPolicies) { - await this.github.request('DELETE /repos/:org/:repo/environments/:environment_name/deployment-branch-policies/:branch_policy_id', { - org: this.repo.owner, - repo: this.repo.repo, - environment_name: attrs.name, - branch_policy_id: policy.id - }); - } - - for(let policy of attrs.deployment_branch_policy.custom_branch_policies) { - await this.github.request('POST /repos/:org/:repo/environments/:environment_name/deployment-branch-policies', { - org: this.repo.owner, - repo: this.repo.repo, - environment_name: attrs.name, - name: policy - }); - } - } - - if(variables) { - let existingVariables = [...existing.variables]; - - for(let variable of attrs.variables) { - const existingVariable = existingVariables.find((_var) => _var.name === variable.name); - if(existingVariable) { - existingVariables = existingVariables.filter(_var => _var.name !== variable.name); - if(existingVariable.value !== variable.value) { - await this.github.request(`PATCH /repos/:org/:repo/environments/:environment_name/variables/:variable_name`, { - org: this.repo.owner, - repo: this.repo.repo, - environment_name: attrs.name, - variable_name: variable.name, - value: variable.value - }); - } - } - else { - await this.github.request(`POST /repos/:org/:repo/environments/:environment_name/variables`, { - org: this.repo.owner, - repo: this.repo.repo, - environment_name: attrs.name, - name: variable.name, - value: variable.value - }); - } - } - - for(let variable of existingVariables) { - await this.github.request('DELETE /repos/:org/:repo/environments/:environment_name/variables/:variable_name', { - org: this.repo.owner, - repo: this.repo.repo, - environment_name: attrs.name, - variable_name: variable.name - }); - } - } - - if(deployment_protection_rules) { - let existingRules = [...existing.deployment_protection_rules]; - - for(let rule of attrs.deployment_protection_rules) { - const existingRule = existingRules.find((_rule) => _rule.id === rule.id); + for (const variable of existingVariables) { + await this.github.request('DELETE /repos/:org/:repo/environments/:environment_name/variables/:variable_name', { + org: this.repo.owner, + repo: this.repo.repo, + environment_name: attrs.name, + variable_name: variable.name + }) + } + } - if(!existingRule) { - await this.github.request(`POST /repos/:org/:repo/environments/:environment_name/deployment_protection_rules`, { - org: this.repo.owner, - repo: this.repo.repo, - environment_name: attrs.name, - integration_id: rule.app_id - }); - } - } + if (deployment_protection_rules) { + const existingRules = [...existing.deployment_protection_rules] - for(let rule of existingRules) { - await this.github.request('DELETE /repos/:org/:repo/environments/:environment_name/deployment_protection_rules/:rule_id', { - org: this.repo.owner, - repo: this.repo.repo, - environment_name: attrs.name, - rule_id: rule.id - }); - } - } - } + for (const rule of attrs.deployment_protection_rules) { + const existingRule = existingRules.find((_rule) => _rule.id === rule.id) - async add(attrs) { - await this.github.request(`PUT /repos/:org/:repo/environments/:environment_name`, { + if (!existingRule) { + await this.github.request('POST /repos/:org/:repo/environments/:environment_name/deployment_protection_rules', { org: this.repo.owner, repo: this.repo.repo, environment_name: attrs.name, - wait_timer: attrs.wait_timer, - prevent_self_review: attrs.prevent_self_review, - reviewers: attrs.reviewers, - deployment_branch_policy: attrs.deployment_branch_policy == null ? null : { - protected_branches: !!attrs.deployment_branch_policy.protected_branches, - custom_branch_policies: !!attrs.deployment_branch_policy.custom_branch_policies - } - }); - - if(attrs.deployment_branch_policy && attrs.deployment_branch_policy.custom_branch_policies) { - - for(let policy of attrs.deployment_branch_policy.custom_branch_policies) { - await this.github.request('POST /repos/:org/:repo/environments/:environment_name/deployment-branch-policies', { - org: this.repo.owner, - repo: this.repo.repo, - environment_name: attrs.name, - name: policy.name - }); - } - + integration_id: rule.app_id + }) } + } - if(attrs.variables) { - - for(let variable of attrs.variables) { - await this.github.request(`POST /repos/:org/:repo/environments/:environment_name/variables`, { - org: this.repo.owner, - repo: this.repo.repo, - environment_name: attrs.name, - name: variable.name, - value: variable.value - }); - } - + for (const rule of existingRules) { + await this.github.request('DELETE /repos/:org/:repo/environments/:environment_name/deployment_protection_rules/:rule_id', { + org: this.repo.owner, + repo: this.repo.repo, + environment_name: attrs.name, + rule_id: rule.id + }) + } + } + } + + async add (attrs) { + await this.github.request('PUT /repos/:org/:repo/environments/:environment_name', { + org: this.repo.owner, + repo: this.repo.repo, + environment_name: attrs.name, + wait_timer: attrs.wait_timer, + prevent_self_review: attrs.prevent_self_review, + reviewers: attrs.reviewers, + deployment_branch_policy: attrs.deployment_branch_policy == null + ? null + : { + protected_branches: !!attrs.deployment_branch_policy.protected_branches, + custom_branch_policies: !!attrs.deployment_branch_policy.custom_branch_policies } + }) + + if (attrs.deployment_branch_policy && attrs.deployment_branch_policy.custom_branch_policies) { + for (const policy of attrs.deployment_branch_policy.custom_branch_policies) { + await this.github.request('POST /repos/:org/:repo/environments/:environment_name/deployment-branch-policies', { + org: this.repo.owner, + repo: this.repo.repo, + environment_name: attrs.name, + name: policy.name + }) + } + } - if(attrs.deployment_protection_rules) { + if (attrs.variables) { + for (const variable of attrs.variables) { + await this.github.request('POST /repos/:org/:repo/environments/:environment_name/variables', { + org: this.repo.owner, + repo: this.repo.repo, + environment_name: attrs.name, + name: variable.name, + value: variable.value + }) + } + } - for(let rule of attrs.deployment_protection_rules) { - await this.github.request(`POST /repos/:org/:repo/environments/:environment_name/deployment_protection_rules`, { - org: this.repo.owner, - repo: this.repo.repo, - environment_name: attrs.name, - integration_id: rule.app_id - }); + if (attrs.deployment_protection_rules) { + for (const rule of attrs.deployment_protection_rules) { + await this.github.request('POST /repos/:org/:repo/environments/:environment_name/deployment_protection_rules', { + org: this.repo.owner, + repo: this.repo.repo, + environment_name: attrs.name, + integration_id: rule.app_id + }) + } + } + } + + async remove (existing) { + await this.github.request('DELETE /repos/:org/:repo/environments/:environment_name', { + org: this.repo.owner, + repo: this.repo.repo, + environment_name: existing.name + }) + } + + sync () { + const resArray = [] + if (this.entries) { + let filteredEntries = this.filterEntries() + return this.find().then(existingRecords => { + // Filter out all empty entries (usually from repo override) + for (const entry of filteredEntries) { + for (const key of Object.keys(entry)) { + if (entry[key] === null || entry[key] === undefined) { + delete entry[key] } - + } } - } + filteredEntries = filteredEntries.filter(entry => Object.keys(entry).filter(key => !MergeDeep.NAME_FIELDS.includes(key)).length !== 0) - async remove(existing) { - await this.github.request(`DELETE /repos/:org/:repo/environments/:environment_name`, { - org: this.repo.owner, - repo: this.repo.repo, - environment_name: existing.name - }); - } + const changes = [] - sync () { - const resArray = [] - if (this.entries) { - let filteredEntries = this.filterEntries() - return this.find().then(existingRecords => { - - // Filter out all empty entries (usually from repo override) - for (const entry of filteredEntries) { - for (const key of Object.keys(entry)) { - if (entry[key] === null || entry[key] === undefined) { - delete entry[key] + existingRecords.forEach(x => { + if (!filteredEntries.find(y => this.comparator(x, y))) { + const change = this.remove(x).then(res => { + if (this.nop) { + return resArray.push(res) } - } + return res + }) + changes.push(change) } - filteredEntries = filteredEntries.filter(entry => Object.keys(entry).filter(key => !MergeDeep.NAME_FIELDS.includes(key)).length !== 0) - - const changes = [] - - existingRecords.forEach(x => { - if (!filteredEntries.find(y => this.comparator(x, y))) { - const change = this.remove(x).then(res => { - if (this.nop) { - return resArray.push(res) - } - return res - }) - changes.push(change) - } + }) + + filteredEntries.forEach(attrs => { + const existing = existingRecords.find(record => { + return this.comparator(record, attrs) }) - filteredEntries.forEach(attrs => { - const existing = existingRecords.find(record => { - return this.comparator(record, attrs) + if (!existing) { + const change = this.add(attrs).then(res => { + if (this.nop) { + return resArray.push(res) + } + return res }) + changes.push(change) + } else if (this.changed(existing, attrs)) { + const change = this.update(existing, attrs).then(res => { + if (this.nop) { + return resArray.push(res) + } + return res + }) + changes.push(change) + } + }) - if (!existing) { - const change = this.add(attrs).then(res => { - if (this.nop) { - return resArray.push(res) - } - return res - }) - changes.push(change) - } else if (this.changed(existing, attrs)) { - const change = this.update(existing, attrs).then(res => { - if (this.nop) { - return resArray.push(res) - } - return res - }) - changes.push(change) - } - }) - - if (this.nop) { + if (this.nop) { + return Promise.resolve(resArray) + } + return Promise.all(changes) + }).catch(e => { + if (this.nop) { + if (e.status === 404) { + // Ignore 404s which can happen in dry-run as the repo may not exist. return Promise.resolve(resArray) - } - return Promise.all(changes) - }).catch(e => { - if (this.nop) { - if (e.status === 404) { - // Ignore 404s which can happen in dry-run as the repo may not exist. - return Promise.resolve(resArray) - } else { - resArray.push(new NopCommand(this.constructor.name, this.repo, null, `error ${e} in ${this.constructor.name} for repo: ${JSON.stringify(this.repo)} entries ${JSON.stringify(this.entries)}`, 'ERROR')) - return Promise.resolve(resArray) - } } else { - this.logError(`Error ${e} in ${this.constructor.name} for repo: ${JSON.stringify(this.repo)} entries ${JSON.stringify(this.entries)}`) + resArray.push(new NopCommand(this.constructor.name, this.repo, null, `error ${e} in ${this.constructor.name} for repo: ${JSON.stringify(this.repo)} entries ${JSON.stringify(this.entries)}`, 'ERROR')) + return Promise.resolve(resArray) } - }) - } + } else { + this.logError(`Error ${e} in ${this.constructor.name} for repo: ${JSON.stringify(this.repo)} entries ${JSON.stringify(this.entries)}`) + } + }) } - + } } From 0aaadf3cdd7b706123ada0a0ebeb3bc563b12a0c Mon Sep 17 00:00:00 2001 From: Kyle Harding Date: Thu, 5 Sep 2024 14:59:49 -0400 Subject: [PATCH 3/3] Support uppercase environment variable names Github REST API supports case-sensitive names on this endpoint so we should always create (POST) variables with the desired case. On subsequent GET, PATCH, and DELETE requests the Github REST API will use loose matching to find any existing objects, ignoring case. So in general we need to force to lowercase to perform diffs, and respect the provided case for create, but for all other calls it doesn't matter which case is used. Signed-off-by: Kyle Harding --- lib/plugins/environments.js | 20 ++---- test/unit/lib/plugins/environments.test.js | 84 +++++++++++++++++++--- 2 files changed, 79 insertions(+), 25 deletions(-) diff --git a/lib/plugins/environments.js b/lib/plugins/environments.js index e146bf66..526fef96 100644 --- a/lib/plugins/environments.js +++ b/lib/plugins/environments.js @@ -6,18 +6,6 @@ module.exports = class Environments extends Diffable { constructor (...args) { super(...args) - if (this.entries) { - // Force all names to lowercase to avoid comparison issues. - this.entries.forEach(environment => { - environment.name = environment.name.toLowerCase() - if (environment.variables) { - environment.variables.forEach(variable => { - variable.name = variable.name.toLowerCase() - }) - } - }) - } - // Remove 'name' from filtering list so Environments with only a name defined are processed. MergeDeep.NAME_FIELDS.splice(MergeDeep.NAME_FIELDS.indexOf('name'), 1) } @@ -53,7 +41,7 @@ module.exports = class Environments extends Diffable { org: this.repo.owner, repo: this.repo.repo, environment_name: environment.name - })).data.variables.map(variable => ({ name: variable.name.toLowerCase(), value: variable.value })), + })).data.variables.map(variable => ({ name: variable.name, value: variable.value })), deployment_protection_rules: (await this.github.request('GET /repos/:org/:repo/environments/:environment_name/deployment_protection_rules', { org: this.repo.owner, repo: this.repo.repo, @@ -106,7 +94,7 @@ module.exports = class Environments extends Diffable { ) } - const variables = JSON.stringify(existing.variables.sort((x1, x2) => x1.name - x2.name)) !== JSON.stringify(attrs.variables.sort((x1, x2) => x1.name - x2.name)) + const variables = JSON.stringify(existing.variables.sort((x1, x2) => x1.name.toLowerCase() - x2.name.toLowerCase())) !== JSON.stringify(attrs.variables.sort((x1, x2) => x1.name.toLowerCase() - x2.name.toLowerCase())) const deployment_protection_rules = JSON.stringify(existing.deployment_protection_rules.map(x => ({ app_id: x.app_id })).sort((x1, x2) => x1.app_id - x2.app_id)) !== JSON.stringify(attrs.deployment_protection_rules.map(x => ({ app_id: x.app_id })).sort((x1, x2) => x1.app_id - x2.app_id)) return { wait_timer, prevent_self_review, reviewers, deployment_branch_policy, variables, deployment_protection_rules } @@ -168,9 +156,9 @@ module.exports = class Environments extends Diffable { let existingVariables = [...existing.variables] for (const variable of attrs.variables) { - const existingVariable = existingVariables.find((_var) => _var.name === variable.name) + const existingVariable = existingVariables.find((_var) => _var.name.toLowerCase() === variable.name.toLowerCase()) if (existingVariable) { - existingVariables = existingVariables.filter(_var => _var.name !== variable.name) + existingVariables = existingVariables.filter(_var => _var.name.toLowerCase() !== variable.name.toLowerCase()) if (existingVariable.value !== variable.value) { await this.github.request('PATCH /repos/:org/:repo/environments/:environment_name/variables/:variable_name', { org: this.repo.owner, diff --git a/test/unit/lib/plugins/environments.test.js b/test/unit/lib/plugins/environments.test.js index efa97e03..3fba8bf9 100644 --- a/test/unit/lib/plugins/environments.test.js +++ b/test/unit/lib/plugins/environments.test.js @@ -365,7 +365,7 @@ describe('Environments Plugin test suite', () => { name: environment_name, variables: [ { - name: 'test', + name: 'TEST', value: 'test' } ] @@ -396,7 +396,7 @@ describe('Environments Plugin test suite', () => { org, repo, environment_name: environment_name, - name: 'test', + name: 'TEST', value: 'test' })); }) @@ -414,7 +414,73 @@ describe('Environments Plugin test suite', () => { name: environment_name, variables: [ { - name: 'test', + name: 'TEST', + value: 'test' + } + ] + } + ], log, errors); + + //model an existing environment with no reviewers + when(github.request) + .calledWith('GET /repos/:org/:repo/environments', { org, repo }) + .mockResolvedValue({ + data: { + environments: [ + fillEnvironment({ + name: environment_name, + variables: [] + }) + ] + } + }); + + when(github.request) + .calledWith('GET /repos/:org/:repo/environments/:environment_name/variables', { org, repo, environment_name }) + .mockResolvedValue({ + data: { + variables: [ + { + name: 'TEST', + value: 'old' + }, + { + name: 'TEST2', + value: 'test2' + } + ] + } + }) + + //act - run sync() in environments.js + await plugin.sync().then(() => { + //assert - update the variables + expect(github.request).toHaveBeenCalledWith('GET /repos/:org/:repo/environments', { org, repo }); + expect(github.request).toHaveBeenCalledWith('GET /repos/:org/:repo/environments/:environment_name/variables', { org, repo, environment_name }); + expect(github.request).toHaveBeenCalledWith('GET /repos/:org/:repo/environments/:environment_name/deployment_protection_rules', { org, repo, environment_name }); + expect(github.request).toHaveBeenCalledWith('PATCH /repos/:org/:repo/environments/:environment_name/variables/:variable_name', expect.objectContaining({ + org, + repo, + environment_name: environment_name, + variable_name: 'TEST', + value: 'test' + })); + }) + }) + }) + + // patch variable + describe('When there are existing lowercase variables and one requires an update', () => { + it('detect divergence and add the variable', async () => { + //arrange + environment_name = 'variables_environment' + // represent config with a reviewers being a user and a team + const plugin = new Environments(undefined, github, { owner: org, repo }, [ + { + name: environment_name, + variables: [ + { + name: 'TEST', value: 'test' } ] @@ -462,7 +528,7 @@ describe('Environments Plugin test suite', () => { org, repo, environment_name: environment_name, - variable_name: 'test', + variable_name: 'TEST', value: 'test' })); }) @@ -702,7 +768,7 @@ describe('Environments Plugin test suite', () => { name: 'variables_environment', variables: [ { - name: 'test', + name: 'TEST', value: 'test' } ] @@ -844,7 +910,7 @@ describe('Environments Plugin test suite', () => { org, repo, environment_name: 'variables_environment', - name: 'test', + name: 'TEST', value: 'test' })); @@ -906,7 +972,7 @@ describe('Environments Plugin test suite', () => { name: 'variables_environment', variables: [ { - name: 'test', + name: 'TEST', value: 'test' } ] @@ -961,7 +1027,7 @@ describe('Environments Plugin test suite', () => { name: 'new-variables', variables: [ { - name: 'test', + name: 'TEST', value: 'test' } ] @@ -1103,7 +1169,7 @@ describe('Environments Plugin test suite', () => { org, repo, environment_name: 'variables_environment', - name: 'test', + name: 'TEST', value: 'test' }));