-
Notifications
You must be signed in to change notification settings - Fork 13
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Add justification for why/how the plugin helps
Additionally documents checkout vs trigger modes, closing #6
- Loading branch information
Showing
1 changed file
with
98 additions
and
11 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,38 +1,125 @@ | ||
# GitHub Pull Request Plugin | ||
|
||
A [Buildkite plugin](https://buildkite.com/docs/agent/v3/plugins) to help build the GitHub post-merge check PR ref (`refs/pull/123/merge`). | ||
A [Buildkite plugin](https://buildkite.com/docs/agent/v3/plugins) to build the merged state of GitHub pull requests. | ||
|
||
This plugin helps you run CI run against the "merged" branch, helping catch bad merges. | ||
Why would I want this? | ||
1. Parity with other CI setups. Including [TeamCity](https://blog.jetbrains.com/teamcity/2013/02/automatically-building-pull-requests-from-github-with-teamcity/), Travis, and [Jenkins](https://wiki.jenkins.io/display/JENKINS/GitHub+pull+request+builder+plugin). | ||
2. Can help avoid building stale branches, as CI is done against the target branch at the time of the PR being raised rather than when the source branch was created | ||
3. Reduce the risk of failed builds from reaching master, due to bad merges (See the example at the bottom of this document) | ||
|
||
# Modes | ||
|
||
The plugin has two modes: | ||
- `mode: trigger` to async trigger another build (of the current pipeline); and | ||
- `mode: checkout` to checkout the PR merged ref (`refs/pull/123/merge`) rather than the head (`refs/pull/123/head`) | ||
|
||
## Example | ||
# Example | ||
|
||
## Checkout Mode | ||
When using `checkout` mode, the plugin should be specified on each build step (e.g. where a checkout happens)he | ||
|
||
```yml | ||
plugins: &plugins | ||
seek-oss/github-merged-pr#v0.0.5: | ||
mode: checkout | ||
|
||
steps: | ||
- plugins: | ||
seek-oss/github-merged-pr#v0.0.5: | ||
mode: checkout | ||
- label: 'Make something' | ||
commands: | ||
- make something | ||
plugins: | ||
<<: *plugins | ||
|
||
- label: 'Make something else' | ||
commands: | ||
- make something-else | ||
plugins: | ||
<<: *plugins | ||
``` | ||
## Trigger Mode | ||
In `trigger` mode the plugin should only be specified on one step, to prevent triggering multiple builds. | ||
|
||
```yml | ||
steps: | ||
- plugins: | ||
seek-oss/github-merged-pr#v0.0.5: | ||
mode: trigger | ||
- label: 'Make something' | ||
commands: | ||
- make something | ||
seek-oss/github-merged-pr#v0.0.5: | ||
mode: trigger | ||
- label: 'Make something else' | ||
commands: | ||
- make something-else | ||
``` | ||
|
||
Ensure `Skip pull request builds for existing commits` is set to `false` in your Pipeline settings, as BuildKite will build the branch and skip the PR build. | ||
|
||
## Tests | ||
# Tests | ||
|
||
To run the tests of this plugin, run | ||
```sh | ||
docker-compose run --rm tests | ||
``` | ||
|
||
## License | ||
# License | ||
|
||
MIT (see [LICENSE](LICENSE)) | ||
|
||
# Bad Merge Example | ||
|
||
An example of why it may be desirable to build the merged commit. Let's say you're raising a PR to implement a `subtract` function to a calculator that already does `add`ition: | ||
|
||
`origin/master` (commit `1abcdef`): calculator.js | ||
``` | ||
class Calculator { | ||
constructor() { | ||
this.currentAnswer = 0; | ||
} | ||
subtract(value) { | ||
this.currentAnswer -= value; | ||
} | ||
} | ||
``` | ||
|
||
(Tests omitted for brevity). | ||
|
||
You create a branch locally to implement your feature | ||
|
||
`feature/add` (commit `feba123`) (parent commit `1abcdef`): calculator.js | ||
``` | ||
class Calculator { | ||
constructor() { | ||
this.currentAnswer = 0; | ||
} | ||
subtract(value) { | ||
this.currentAnswer -= value; | ||
} | ||
add(value) { | ||
this.currentAnswer += value; | ||
} | ||
} | ||
``` | ||
|
||
In the meantime, `origin/master` is updated to rename `currentAnswer` to `answer`. | ||
`origin/master` (commit `56789ab`) (parent commit `1abcdef`): calculator.js | ||
``` | ||
class Calculator { | ||
constructor() { | ||
this.answer = 0; | ||
} | ||
subtract(value) { | ||
this.answer -= value; | ||
} | ||
} | ||
``` | ||
|
||
You then raise your PR for `feature/add`, but: | ||
1. CI will run against the `HEAD` of the branch (commit `feba123`); and | ||
2. The PR will likely be merged (as tests pass) -- unless someone on your team notices; and | ||
3. The `git merge` will succeed (as there's no text conflicts); and | ||
4. The target branch (`master`) will be broken :( |