Skip to content

Commit

Permalink
Support uppercase environment variable names
Browse files Browse the repository at this point in the history 
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 <[email protected]>
  • Loading branch information
@klutchell
klutchell committed Sep 5, 2024
1 parent d384cf6 commit 8241e72
Show file tree
Hide file tree
Showing 2 changed files with 79 additions and 25 deletions.
20 changes: 4 additions & 16 deletions lib/plugins/environments.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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 }
Expand Down Expand Up @@ -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,
Expand Down
84 changes: 75 additions & 9 deletions test/unit/lib/plugins/environments.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -365,7 +365,7 @@ describe('Environments Plugin test suite', () => {
name: environment_name,
variables: [
{
name: 'test',
name: 'TEST',
value: 'test'
}
]
Expand Down Expand Up @@ -396,7 +396,7 @@ describe('Environments Plugin test suite', () => {
org,
repo,
environment_name: environment_name,
name: 'test',
name: 'TEST',
value: 'test'
}));
})
Expand All @@ -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'
}
]
Expand Down Expand Up @@ -462,7 +528,7 @@ describe('Environments Plugin test suite', () => {
org,
repo,
environment_name: environment_name,
variable_name: 'test',
variable_name: 'TEST',
value: 'test'
}));
})
Expand Down Expand Up @@ -702,7 +768,7 @@ describe('Environments Plugin test suite', () => {
name: 'variables_environment',
variables: [
{
name: 'test',
name: 'TEST',
value: 'test'
}
]
Expand Down Expand Up @@ -844,7 +910,7 @@ describe('Environments Plugin test suite', () => {
org,
repo,
environment_name: 'variables_environment',
name: 'test',
name: 'TEST',
value: 'test'
}));

Expand Down Expand Up @@ -906,7 +972,7 @@ describe('Environments Plugin test suite', () => {
name: 'variables_environment',
variables: [
{
name: 'test',
name: 'TEST',
value: 'test'
}
]
Expand Down Expand Up @@ -961,7 +1027,7 @@ describe('Environments Plugin test suite', () => {
name: 'new-variables',
variables: [
{
name: 'test',
name: 'TEST',
value: 'test'
}
]
Expand Down Expand Up @@ -1103,7 +1169,7 @@ describe('Environments Plugin test suite', () => {
org,
repo,
environment_name: 'variables_environment',
name: 'test',
name: 'TEST',
value: 'test'
}));

Expand Down

0 comments on commit 8241e72

Please sign in to comment.