Skip to content

Commit

Permalink
Resolve simultaneous deployments with rebase (#1054)
Browse files Browse the repository at this point in the history
* Return early from dry run

Determining whether to create a merge commit would elicit a nested
conditional, which could be hard to parse for a human reader. This is
avoided by returning early as soon as possible for a dry run.

This also resolves the erroneous 'changes committed' message when no
changes were actually committed because of the dry run. A message
specific to dry-run is logged instead.

* Add force parameter to action interface

Existing behaviour is equivalent to force=true, so the default value is
true.

* Implement pull+rebase procedure

* Declare force parameter in action

* Detect both rejection syntaxes

* Return both stdout and stderr from execute

* Ignore non-zero exit status on push

* Remove unnecessary error catch

* Fetch and rebase in separate steps

* Explicitly bind incoming branch

I think the fetch will update the origin/gh-pages branch but not the
gh-pages branch, despite requesting gh-pages. This means that when I
later attempt to rebase the temp branch on top of the gh-pages branch,
there will be nothing to do, because that's already where it is.

* Implement attempt limit

I don't expect this to ever require more than one attempt in production,
but in theory it's possible that this procedure could loop forever.

We would need to keep fetching and rebasing if changes keep being added
to the remote. In practice, I believe this would only happen if there
are lots of workflows simultaneously deploying to the same branch, all
using this action. In this case only one would be able to secure a lock
at a time, leading to the total number of attempts being equal to the
number of simultaneous deployments, assuming each deployment makes each
attempt at the exact same time.

The limit may need to be increased or even be configurable, but 3 should
cover most uses.

* Update tests for execute output split

* Document 'force' parameter

* Create integration test for rebase procedure

This test is composed of 3 jobs.

The first two jobs run simultaneously, and as such both depend on the
previous integration test only. The final job cleans up afterwards, and
depends on both of the prior jobs.

The two jobs are identical except that they both create a temporary file
in a different location. This is to ensure that they conflict. Correctly
resolving this conflict by rebasing one deployment over the other,
resulting in a deployment containing both files, indicates a successful
test.
  • Loading branch information
rossjrw committed Apr 4, 2022
1 parent cd846de commit 95f8a2c
Show file tree
Hide file tree
Showing 12 changed files with 183 additions and 28 deletions.
56 changes: 56 additions & 0 deletions .github/workflows/integration.yml
Expand Up @@ -270,3 +270,59 @@ jobs:
with:
github_token: ${{ secrets.GITHUB_TOKEN }}
branches: integration-test-delete-prod

# Creates two competing deployments, one of which should rebase over the other.
# First conflicting deployment
integration-rebase-conflicts-1:
needs: integration-branch-creation
runs-on: ubuntu-latest
steps:
- name: Checkout
uses: actions/checkout@v3

- name: Create random file
runs: echo $RANDOM > integration/1

- name: Build and Deploy
uses: JamesIves/github-pages-deploy-action@v4.2.5
with:
git-config-name: Montezuma
git-config-email: montezuma@jamesiv.es
repository-name: MontezumaIves/lab
token: ${{ secrets.ACCESS_TOKEN }}
branch: gh-pages-rebase-conflict
folder: integration
force: false
# Second conflicting deployment
integration-rebase-conflicts-2:
needs: integration-branch-creation
runs-on: ubuntu-latest
steps:
- name: Checkout
uses: actions/checkout@v3

- name: Create random file
runs: echo $RANDOM > integration/2

- name: Build and Deploy
uses: JamesIves/github-pages-deploy-action@v4.2.5
with:
git-config-name: Montezuma
git-config-email: montezuma@jamesiv.es
repository-name: MontezumaIves/lab
token: ${{ secrets.ACCESS_TOKEN }}
branch: gh-pages-rebase-conflict
folder: integration
force: false
# Clean up conflicting deployments
integration-rebase-conflicts-cleanup:
needs: [integration-rebase-conflicts-1, integration-rebase-conflicts-2]
runs-on: ubuntu-latest
steps:
- name: Cleanup Generated Branch
uses: dawidd6/action-delete-branch@v3.1.0
with:
github_token: ${{ secrets.ACCESS_TOKEN }}
owner: MontezumaIves
repository: lab
branches: gh-pages
1 change: 1 addition & 0 deletions README.md
Expand Up @@ -158,6 +158,7 @@ By default, the action does not need any token configuration and uses the provid
| `clean-exclude` | If you need to use `clean` but you'd like to preserve certain files or folders you can use this option. This should contain each pattern as a single line in a multiline string. | `with` | **No** |
| `dry-run` | Do not actually push back, but use `--dry-run` on `git push` invocations instead. | `with` | **No** |
| `single-commit` | This option can be toggled to `true` if you'd prefer to have a single commit on the deployment branch instead of maintaining the full history. **Using this option will also cause any existing history to be wiped from the deployment branch**. | `with` | **No** |
| `force` | Force-push new deployments to overwrite the previous version; otherwise, attempt to rebase new deployments onto any existing ones. This option is turned on by default and can be toggled off by setting it to `false`, which may be useful if there are multiple deployments in a single branch. | `with` | **No** |
| `silent` | Silences the action output preventing it from displaying git messages. | `with` | **No** |
| `workspace` | This should point to where your project lives on the virtual machine. The GitHub Actions environment will set this for you. It is only necessary to set this variable if you're using the node module. | `with` | **No** |

Expand Down
8 changes: 6 additions & 2 deletions __tests__/execute.test.ts
Expand Up @@ -13,8 +13,10 @@ describe('execute', () => {
expect(exec).toBeCalledWith('echo Montezuma', [], {
cwd: './',
silent: true,
ignoreReturnCode: false,
listeners: {
stdout: expect.any(Function)
stdout: expect.any(Function),
stderr: expect.any(Function)
}
})
})
Expand All @@ -28,8 +30,10 @@ describe('execute', () => {
expect(exec).toBeCalledWith('echo Montezuma', [], {
cwd: './',
silent: false,
ignoreReturnCode: false,
listeners: {
stdout: expect.any(Function)
stdout: expect.any(Function),
stderr: expect.any(Function)
}
})
})
Expand Down
2 changes: 1 addition & 1 deletion __tests__/git.test.ts
Expand Up @@ -30,7 +30,7 @@ jest.mock('@actions/io', () => ({
jest.mock('../src/execute', () => ({
// eslint-disable-next-line @typescript-eslint/naming-convention
__esModule: true,
execute: jest.fn()
execute: jest.fn(() => ({stdout: '', stderr: ''}))
}))

describe('git', () => {
Expand Down
2 changes: 1 addition & 1 deletion __tests__/main.test.ts
Expand Up @@ -16,7 +16,7 @@ import {setFailed, exportVariable} from '@actions/core'
const originalAction = JSON.stringify(action)

jest.mock('../src/execute', () => ({
execute: jest.fn()
execute: jest.fn(() => ({stdout: '', stderr: ''}))
}))

jest.mock('@actions/io', () => ({
Expand Down
2 changes: 1 addition & 1 deletion __tests__/ssh.test.ts
Expand Up @@ -33,7 +33,7 @@ jest.mock('@actions/core', () => ({
}))

jest.mock('../src/execute', () => ({
execute: jest.fn()
execute: jest.fn(() => ({stdout: '', stderr: ''}))
}))

describe('configureSSH', () => {
Expand Down
2 changes: 1 addition & 1 deletion __tests__/worktree.error.test.ts
Expand Up @@ -5,7 +5,7 @@ import {generateWorktree} from '../src/worktree'
jest.mock('../src/execute', () => ({
// eslint-disable-next-line @typescript-eslint/naming-convention
__esModule: true,
execute: jest.fn()
execute: jest.fn(() => ({stdout: '', stderr: ''}))
}))

describe('generateWorktree', () => {
Expand Down
4 changes: 2 additions & 2 deletions __tests__/worktree.test.ts
Expand Up @@ -104,7 +104,7 @@ describe('generateWorktree', () => {
path.join(workspace, 'worktree'),
true
)
expect(commitMessages).toBe('gh1')
expect(commitMessages.stdout).toBe('gh1')
})
})
describe('with missing branch and new commits', () => {
Expand Down Expand Up @@ -132,7 +132,7 @@ describe('generateWorktree', () => {
path.join(workspace, 'worktree'),
true
)
expect(commitMessages).toBe('Initial no-pages commit')
expect(commitMessages.stdout).toBe('Initial no-pages commit')
})
})
describe('with existing branch and singleCommit', () => {
Expand Down
5 changes: 5 additions & 0 deletions action.yml
Expand Up @@ -60,6 +60,11 @@ inputs:
description: 'Do not actually push back, but use `--dry-run` on `git push` invocations insead.'
required: false

force:
description: 'Whether to force-push and overwrite any existing deployment. Setting this to false will attempt to rebase simultaneous deployments. This option is on by default and can be toggled off by setting it to false.'
required: false
default: true

git-config-name:
description: 'Allows you to customize the name that is attached to the GitHub config which is used when pushing the deployment commits. If this is not included it will use the name in the GitHub context, followed by the name of the action.'
required: false
Expand Down
5 changes: 5 additions & 0 deletions src/constants.ts
Expand Up @@ -33,6 +33,8 @@ export interface ActionInterface {
folder: string
/** The auto generated folder path. */
folderPath?: string
/** Whether to force-push or attempt to merge existing changes. */
force?: boolean
/** Determines test scenarios the action is running in. */
isTest: TestFlag
/** The git config name. */
Expand Down Expand Up @@ -85,6 +87,9 @@ export const action: ActionInterface = {
dryRun: !isNullOrUndefined(getInput('dry-run'))
? getInput('dry-run').toLowerCase() === 'true'
: false,
force: !isNullOrUndefined(getInput('force'))
? getInput('force').toLowerCase() === 'true'
: true,
clean: !isNullOrUndefined(getInput('clean'))
? getInput('clean').toLowerCase() === 'true'
: false,
Expand Down
39 changes: 30 additions & 9 deletions src/execute.ts
@@ -1,37 +1,58 @@
import {exec} from '@actions/exec'
import buffer from 'buffer'

let output = ''
type ExecuteOutput = {
stdout: string
stderr: string
}

const output: ExecuteOutput = {stdout: '', stderr: ''}

/** Wrapper around the GitHub toolkit exec command which returns the output.
* Also allows you to easily toggle the current working directory.
*
* @param {string} cmd - The command to execute.
* @param {string} cwd - The current working directory.
* @param {boolean} silent - Determines if the in/out should be silenced or not.
* @param {boolean} ignoreReturnCode - Determines whether to throw an error
* on a non-zero exit status or to leave implementation up to the caller.
*/
export async function execute(
cmd: string,
cwd: string,
silent: boolean
): Promise<string> {
output = ''
silent: boolean,
ignoreReturnCode = false
): Promise<ExecuteOutput> {
output.stdout = ''
output.stderr = ''

await exec(cmd, [], {
// Silences the input unless the INPUT_DEBUG flag is set.
silent,
cwd,
listeners: {
stdout
}
listeners: {stdout, stderr},
ignoreReturnCode
})

return Promise.resolve(output)
}

export function stdout(data: Buffer | string): void {
const dataString = data.toString().trim()
if (output.length + dataString.length < buffer.constants.MAX_STRING_LENGTH) {
output += dataString
if (
output.stdout.length + dataString.length <
buffer.constants.MAX_STRING_LENGTH
) {
output.stdout += dataString
}
}

export function stderr(data: Buffer | string): void {
const dataString = data.toString().trim()
if (
output.stderr.length + dataString.length <
buffer.constants.MAX_STRING_LENGTH
) {
output.stderr += dataString
}
}
85 changes: 74 additions & 11 deletions src/git.ts
Expand Up @@ -108,11 +108,15 @@ export async function deploy(action: ActionInterface): Promise<Status> {
// Checks to see if the remote exists prior to deploying.
const branchExists =
action.isTest & TestFlag.HAS_REMOTE_BRANCH ||
(await execute(
`git ls-remote --heads ${action.repositoryPath} refs/heads/${action.branch}`,
action.workspace,
action.silent
))
Boolean(
(
await execute(
`git ls-remote --heads ${action.repositoryPath} refs/heads/${action.branch}`,
action.workspace,
action.silent
)
).stdout
)

await generateWorktree(action, temporaryDeploymentDirectory, branchExists)

Expand Down Expand Up @@ -186,11 +190,15 @@ export async function deploy(action: ActionInterface): Promise<Status> {

const hasFilesToCommit =
action.isTest & TestFlag.HAS_CHANGED_FILES ||
(await execute(
checkGitStatus,
`${action.workspace}/${temporaryDeploymentDirectory}`,
true // This output is always silenced due to the large output it creates.
))
Boolean(
(
await execute(
checkGitStatus,
`${action.workspace}/${temporaryDeploymentDirectory}`,
true // This output is always silenced due to the large output it creates.
)
).stdout
)

if (
(!action.singleCommit && !hasFilesToCommit) ||
Expand All @@ -216,12 +224,67 @@ export async function deploy(action: ActionInterface): Promise<Status> {
`${action.workspace}/${temporaryDeploymentDirectory}`,
action.silent
)
if (!action.dryRun) {

if (action.dryRun) {
info(`Dry run complete`)
return Status.SUCCESS
}

if (action.force) {
// Force-push our changes, overwriting any changes that were added in
// the meantime
info(`Force-pushing changes...`)
await execute(
`git push --force ${action.repositoryPath} ${temporaryDeploymentBranch}:${action.branch}`,
`${action.workspace}/${temporaryDeploymentDirectory}`,
action.silent
)
} else {
// Attempt to push our changes, but fetch + rebase if there were
// other changes added in the meantime
const ATTEMPT_LIMIT = 3
let attempt = 0
// Keep track of whether the most recent attempt was rejected
let rejected = false
do {
attempt++
if (attempt > ATTEMPT_LIMIT) throw new Error(`Attempt limit exceeded`)

// Handle rejection for the previous attempt first such that, on
// the final attempt, time is not wasted rebasing it when it will
// not be pushed
if (rejected) {
info(`Fetching upstream ${action.branch}...`)
await execute(
`git fetch ${action.repositoryPath} ${action.branch}:${action.branch}`,
`${action.workspace}/${temporaryDeploymentDirectory}`,
action.silent
)
info(`Rebasing this deployment onto ${action.branch}...`)
await execute(
`git rebase ${action.branch} ${temporaryDeploymentBranch}`,
`${action.workspace}/${temporaryDeploymentDirectory}`,
action.silent
)
}

info(`Pushing changes... (attempt ${attempt} of ${ATTEMPT_LIMIT})`)
const pushResult = await execute(
`git push --porcelain ${action.repositoryPath} ${temporaryDeploymentBranch}:${action.branch}`,
`${action.workspace}/${temporaryDeploymentDirectory}`,
action.silent,
true // Ignore non-zero exit status
)

rejected =
pushResult.stdout.includes(`[rejected]`) ||
pushResult.stdout.includes(`[remote rejected]`)
if (rejected) info('Updates were rejected')

// If the push failed for any reason other than being rejected,
// there is a problem
if (!rejected && pushResult.stderr) throw new Error(pushResult.stderr)
} while (rejected)
}

info(`Changes committed to the ${action.branch} branch… 📦`)
Expand Down

0 comments on commit 95f8a2c

Please sign in to comment.