Skip to content

Commit

Permalink
Merge pull request #342 from github/branch-protection-checks
Browse files Browse the repository at this point in the history
Branch Ruleset Checks
  • Loading branch information
GrantBirki authored Dec 12, 2024
2 parents a4e13bd + 6a65a3b commit 5d1d9a4
Show file tree
Hide file tree
Showing 22 changed files with 550 additions and 7 deletions.
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,7 @@ As seen above, we have two steps. One for a noop deploy, and one for a regular d
| `skip_successful_noop_labels_if_approved` | `false` | `"false"` | Whether or not the post run logic should skip adding successful noop labels if the pull request is approved. This can be useful if you add a label such as "ready-for-review" after a `.noop` completes but want to skip adding that label in situations where the pull request is already approved. |
| `skip_successful_deploy_labels_if_approved` | `false` | `"false"` | Whether or not the post run logic should skip adding successful deploy labels if the pull request is approved. This can be useful if you add a label such as "ready-for-review" after a `.deploy` completes but want to skip adding that label in situations where the pull request is already approved. |
| `enforced_deployment_order` | `false` | `""` | A comma separated list of environments that must be deployed in a specific order. Example: `"development,staging,production"`. If this is set then you cannot deploy to latter environments unless the former ones have a successful and active deployment on the latest commit first - See the [enforced deployment order docs](./docs/enforced-deployment-order.md) for more details |
| `use_security_warnings` | `false` | `"true"` | Whether or not to leave security related warnings in log messages during deployments. Default is `"true"` |

## Outputs 📤

Expand Down
221 changes: 221 additions & 0 deletions __tests__/functions/branch-protection-checks.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,221 @@
import {branchRulesetChecks} from '../../src/functions/branch-ruleset-checks'
import * as core from '@actions/core'
import {COLORS} from '../../src/functions/colors'
import {SUGGESTED_RULESETS} from '../../src/functions/suggested-rulesets'

var context
var octokit
var data
var rulesets

// const debugMock = jest.spyOn(core, 'debug').mockImplementation(() => {})
const warningMock = jest.spyOn(core, 'warning').mockImplementation(() => {})
const infoMock = jest.spyOn(core, 'info').mockImplementation(() => {})

beforeEach(() => {
jest.spyOn(core, 'info').mockImplementation(() => {})
jest.spyOn(core, 'debug').mockImplementation(() => {})
jest.spyOn(core, 'warning').mockImplementation(() => {})
jest.clearAllMocks()

data = {
branch: 'main'
}

rulesets = [
{
type: 'deletion'
},
{
type: 'non_fast_forward'
},
{
type: 'pull_request',
parameters: {
required_approving_review_count: 1,
dismiss_stale_reviews_on_push: true,
required_reviewers: [],
require_code_owner_review: true,
require_last_push_approval: false,
required_review_thread_resolution: false,
automatic_copilot_code_review_enabled: false,
allowed_merge_methods: ['merge', 'squash', 'rebase']
}
},
{
type: 'required_status_checks',
parameters: {
strict_required_status_checks_policy: true,
do_not_enforce_on_create: false,
required_status_checks: []
}
},
{
type: 'required_deployments',
parameters: {
required_deployment_environments: []
}
}
]

context = {
repo: {
owner: 'corp',
repo: 'test'
},
issue: {
number: 1
}
}

octokit = {
rest: {
repos: {
getBranchRules: jest.fn().mockReturnValueOnce({data: rulesets})
}
}
}
})

test('finds that no branch protections or rulesets are defined', async () => {
octokit = {
rest: {
repos: {
getBranchRules: jest.fn().mockReturnValueOnce({data: []})
}
}
}
expect(await branchRulesetChecks(context, octokit, data)).toStrictEqual({
success: false,
failed_checks: ['missing_branch_rulesets']
})
expect(warningMock).toHaveBeenCalledWith(
`🔐 branch ${COLORS.highlight}rulesets${COLORS.reset} are not defined for branch ${COLORS.highlight}${data.branch}${COLORS.reset}`
)
})

test('exits early if the user has disabled security warnings', async () => {
data.use_security_warnings = false
expect(await branchRulesetChecks(context, octokit, data)).toStrictEqual({
success: true
})
expect(warningMock).not.toHaveBeenCalled()
expect(infoMock).not.toHaveBeenCalledWith(
`🔐 branch ruleset checks ${COLORS.success}passed${COLORS.reset}`
)
})

test('finds that the branch ruleset is missing the deletion rule', async () => {
rulesets = rulesets.filter(rule => rule.type !== 'deletion')

octokit = {
rest: {
repos: {
getBranchRules: jest.fn().mockReturnValueOnce({data: rulesets})
}
}
}

expect(await branchRulesetChecks(context, octokit, data)).toStrictEqual({
success: false,
failed_checks: ['missing_deletion']
})
expect(warningMock).toHaveBeenCalledWith(
`🔐 branch ${COLORS.highlight}rulesets${COLORS.reset} for branch ${COLORS.highlight}${data.branch}${COLORS.reset} is missing a rule of type ${COLORS.highlight}deletion${COLORS.reset}`
)
})

test('finds that the branch ruleset is missing the dismiss_stale_reviews_on_push parameter on the pull_request rule', async () => {
rulesets = rulesets.map(rule => {
if (rule.type === 'pull_request') {
return {
type: 'pull_request',
parameters: {
...rule.parameters,
dismiss_stale_reviews_on_push: false
}
}
}
return rule
})

octokit = {
rest: {
repos: {
getBranchRules: jest.fn().mockReturnValueOnce({data: rulesets})
}
}
}

expect(await branchRulesetChecks(context, octokit, data)).toStrictEqual({
success: false,
failed_checks: ['mismatch_pull_request_dismiss_stale_reviews_on_push']
})
expect(warningMock).toHaveBeenCalledWith(
`🔐 branch ${COLORS.highlight}rulesets${COLORS.reset} for branch ${COLORS.highlight}${data.branch}${COLORS.reset} contains a rule of type ${COLORS.highlight}pull_request${COLORS.reset} with a parameter ${COLORS.highlight}dismiss_stale_reviews_on_push${COLORS.reset} which does not match the suggested parameter`
)
})

test('finds that all suggested branch rulesets are defined', async () => {
rulesets = SUGGESTED_RULESETS.map(suggested_rule => {
return {
type: suggested_rule.type,
parameters: suggested_rule.parameters
}
})

octokit = {
rest: {
repos: {
getBranchRules: jest.fn().mockReturnValueOnce({data: rulesets})
}
}
}

expect(await branchRulesetChecks(context, octokit, data)).toStrictEqual({
success: true,
failed_checks: []
})
expect(warningMock).not.toHaveBeenCalled()
expect(infoMock).toHaveBeenCalledWith(
`🔐 branch ruleset checks ${COLORS.success}passed${COLORS.reset}`
)
})

test('finds that all suggested branch rulesets are defined but required reviews is set to 0', async () => {
rulesets = SUGGESTED_RULESETS.map(suggested_rule => {
return {
type: suggested_rule.type,
parameters: suggested_rule.parameters
}
})

rulesets = rulesets.map(rule => {
if (rule.type === 'pull_request') {
return {
type: 'pull_request',
parameters: {
...rule.parameters,
required_approving_review_count: 0
}
}
}
return rule
})

octokit = {
rest: {
repos: {
getBranchRules: jest.fn().mockReturnValueOnce({data: rulesets})
}
}
}

expect(await branchRulesetChecks(context, octokit, data)).toStrictEqual({
success: false,
failed_checks: ['mismatch_pull_request_required_approving_review_count']
})
expect(warningMock).toHaveBeenCalledWith(
`🔐 branch ${COLORS.highlight}rulesets${COLORS.reset} for branch ${COLORS.highlight}${data.branch}${COLORS.reset} contains the required_approving_review_count parameter but it is set to 0`
)
})
15 changes: 11 additions & 4 deletions __tests__/functions/help.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,8 @@ const defaultInputs = {
checks: 'all',
commit_verification: true,
ignored_checks: [],
enforced_deployment_order: []
enforced_deployment_order: [],
use_security_warnings: true
}

test('successfully calls help with defaults', async () => {
Expand Down Expand Up @@ -87,7 +88,8 @@ test('successfully calls help with non-defaults', async () => {
checks: ['test,build,security'],
ignored_checks: ['lint', 'format'],
commit_verification: false,
enforced_deployment_order: []
enforced_deployment_order: [],
use_security_warnings: false
}

expect(await help(octokit, context, 123, inputs))
Expand Down Expand Up @@ -124,7 +126,8 @@ test('successfully calls help with non-defaults again', async () => {
checks: 'required',
ignored_checks: ['lint'],
commit_verification: false,
enforced_deployment_order: ['development', 'staging', 'production']
enforced_deployment_order: ['development', 'staging', 'production'],
use_security_warnings: false
}

expect(await help(octokit, context, 123, inputs))
Expand Down Expand Up @@ -172,7 +175,8 @@ test('successfully calls help with non-defaults and unknown update_branch settin
allow_sha_deployments: false,
checks: 'required',
ignored_checks: ['lint'],
enforced_deployment_order: []
enforced_deployment_order: [],
use_security_warnings: false
}

expect(await help(octokit, context, 123, inputs))
Expand All @@ -190,4 +194,7 @@ test('successfully calls help with non-defaults and unknown update_branch settin
expect(debugMock).toHaveBeenCalledWith(
expect.stringMatching(/Unknown value for update_branch/)
)
expect(debugMock).toHaveBeenCalledWith(
expect.stringMatching(/not use security warnings/)
)
})
7 changes: 7 additions & 0 deletions __tests__/main.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import {run} from '../src/main'
import * as reactEmote from '../src/functions/react-emote'
import * as contextCheck from '../src/functions/context-check'
import * as prechecks from '../src/functions/prechecks'
import * as branchRulesetChecks from '../src/functions/branch-ruleset-checks'
import * as help from '../src/functions/help'
import * as validPermissions from '../src/functions/valid-permissions'
import * as identicalCommitCheck from '../src/functions/identical-commit-check'
Expand Down Expand Up @@ -87,6 +88,7 @@ beforeEach(() => {
process.env.INPUT_ENFORCED_DEPLOYMENT_ORDER = ''
process.env.INPUT_COMMIT_VERIFICATION = 'false'
process.env.INPUT_IGNORED_CHECKS = ''
process.env.INPUT_USE_SECURITY_WARNINGS = 'true'

github.context.payload = {
issue: {
Expand Down Expand Up @@ -163,6 +165,11 @@ beforeEach(() => {
isFork: false
}
})
jest
.spyOn(branchRulesetChecks, 'branchRulesetChecks')
.mockImplementation(() => {
return undefined
})
jest
.spyOn(commitSafetyChecks, 'commitSafetyChecks')
.mockImplementation(() => {
Expand Down
10 changes: 10 additions & 0 deletions __tests__/schemas/action.schema.yml
Original file line number Diff line number Diff line change
Expand Up @@ -480,6 +480,16 @@ inputs:
default:
type: string
required: false
use_security_warnings:
description:
type: string
required: true
required:
type: boolean
required: true
default:
type: string
required: false

# outputs section
outputs:
Expand Down
4 changes: 4 additions & 0 deletions action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,10 @@ inputs:
description: 'A comma separated list of environments that must be deployed in a specific order. Example: `"development,staging,production"`. If this is set then you cannot deploy to latter environments unless the former ones have a successful and active deployment on the latest commit first.'
required: false
default: ""
use_security_warnings:
description: 'Whether or not to leave security related warnings in log messages during deployments. Default is "true"'
required: false
default: "true"
outputs:
continue:
description: 'The string "true" if the deployment should continue, otherwise empty - Use this to conditionally control if your deployment should proceed or not'
Expand Down
Loading

0 comments on commit 5d1d9a4

Please sign in to comment.