diff options
Diffstat (limited to 'doc')
| -rw-r--r-- | doc/development/README.md | 1 | ||||
| -rw-r--r-- | doc/development/code_review.md | 2 | ||||
| -rw-r--r-- | doc/development/dangerbot.md | 116 |
3 files changed, 118 insertions, 1 deletions
diff --git a/doc/development/README.md b/doc/development/README.md index 3912a828dec..bbe73570f49 100644 --- a/doc/development/README.md +++ b/doc/development/README.md @@ -22,6 +22,7 @@ description: 'Learn how to contribute to GitLab.' - [Guidelines for implementing Enterprise Edition features](ee_features.md) - [Security process for developers](https://gitlab.com/gitlab-org/release/docs/blob/master/general/security/developer.md#security-releases-critical-non-critical-as-a-developer) - [Requesting access to Chatops on GitLab.com](chatops_on_gitlabcom.md#requesting-access) (for GitLabbers) +- [Danger bot](dangerbot.md) ## UX and Frontend guides diff --git a/doc/development/code_review.md b/doc/development/code_review.md index b7d74b17eb3..80890d96159 100644 --- a/doc/development/code_review.md +++ b/doc/development/code_review.md @@ -34,7 +34,7 @@ more than one approval, the last maintainer to review and approve it will also m ### Reviewer roulette -The `danger-review` CI job will randomly pick a reviewer and a maintainer for +The [Danger bot](dangerbot.md) randomly picks a reviewer and a maintainer for each area of the codebase that your merge request seems to touch. It only makes recommendations - feel free to override it if you think someone else is a better fit! diff --git a/doc/development/dangerbot.md b/doc/development/dangerbot.md new file mode 100644 index 00000000000..5fc5886e3a2 --- /dev/null +++ b/doc/development/dangerbot.md @@ -0,0 +1,116 @@ +# Danger bot + +The GitLab CI pipeline includes a `danger-review` job that uses [Danger](https://github.com/danger/danger) +to perform a variety of automated checks on the code under test. + +Danger is a gem that runs in the CI environment, like any other analysis tool. +What sets it apart from, e.g., Rubocop, is that it's designed to allow you to +easily write arbitrary code to test properties of your code or changes. To this +end, it provides a set of common helpers and access to information about what +has actually changed in your environment, then simply runs your code! + +If Danger is asking you to change something about your merge request, it's best +just to make the change. If you want to learn how Danger works, or make changes +to the existing rules, then this is the document for you. + +## Operation + +On startup, Danger reads a [`Dangerfile`](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/Dangerfile) +from the project root. GitLab's Danger code is decomposed into a set of helpers +and plugins, all within the [`danger/`](https://gitlab.com/gitlab-org/gitlab-ce/tree/master/danger/) +subdirectory, so ours just tells Danger to load it all. Danger will then run +each plugin against the merge request, collecting the output from each. A plugin +may output notifications, warnings, or errors, all of which are copied to the +CI job's log. If an error happens, the CI job (and so the entire pipeline) will +be failed. + +On merge requests, Danger will also copy the output to a comment on the MR +itself, increasing visibility. + +## Development guidelines + +Danger code is Ruby code, so all our [usual backend guidelines](README.md#backend-guides) +continue to apply. However, there are a few things that deserve special emphasis. + +### When to use Danger + +Danger is a powerful tool and flexible tool, but not always the most appropriate +way to solve a given problem or workflow. + +First, be aware of GitLab's [commitment to dogfooding](https://about.gitlab.com/handbook/engineering/#dogfooding). +The code we write for Danger is GitLab-specific, and it **may not** be most +appropriate place to implement functionality that addresses a need we encounter. +Our users, customers, and even our own satellite projects, such as [Gitaly](https://gitlab.com/gitlab-org/gitaly), +often face similar challenges, after all. Think about how you could fulfil the +same need while ensuring everyone can benefit from the work, and do that instead +if you can. + +If a standard tool (e.g. `rubocop`) exists for a task, it is better to use it +directly, rather than calling it via Danger. Running and debugging the results +of those tools locally is easier if Danger isn't involved, and unless you're +using some Danger-specific functionality, there's no benefit to including it in +the Danger run. + +Danger is well-suited to prototyping and rapidly iterating on solutions, so if +what we want to build is unclear, a solution in Danger can be thought of as a +trial run to gather information about a product area. If you're doing this, make +sure the problem you're trying to solve, and the outcomes of that prototyping, +are captured in an issue or epic as you go along. This will help us to address +the need as part of the product in a future version of GitLab! + +### Implementation details + +Implement each task as an isolated piece of functionality and place it in its +own directory under `danger` as `danger/<task-name>/Dangerfile`. + +Add a line to the top-level `Dangerfile` to ensure it is loaded like: + +```ruby +danger.import_dangerfile('danger/<task-name>') +``` + +Each task should be isolated from the others, and able to function in isolation. +If there is code that should be shared between multiple tasks, add a plugin to +`danger/plugins/...` and require it in each task that needs it. You can also +create plugins that are specific to a single task, which is a natural place for +complex logic related to that task. + +Danger code is just Ruby code. It should adhere to our coding standards, and +needs tests, like any other piece of Ruby in our codebase. However, we aren't +able to test a `Dangerfile` directly! So, to maximise test coverage, try to +minimize the number of lines of code in `danger/`. A non-trivial `Dangerfile` +should mostly call plugin code with arguments derived from the methods provided +by Danger. The plugin code itself should have unit tests. + +At present, we do this by putting the code in a module in `lib/gitlab/danger/...`, +and including it in the matching `danger/plugins/...` file. Specs can then be +added in `spec/lib/gitlab/danger/...`. + +You'll only know if your `Dangerfile` works by pushing the branch that contains +it to GitLab. This can be quite frustrating, as it significantly increases the +cycle time when developing a new task, or trying to debug something in an +existing one. If you've followed the guidelines above, most of your code can +be exercised locally in RSpec, minimizing the number of cycles you need to go +through in CI. However, you can speed these cycles up somewhat by emptying the +`.gitlab/ci/rails.gitlab-ci.yml` file in your merge request. Just don't forget +to revert the change before merging! + +You should add the `~Danger bot` label to the merge request before sending it +for review. + +## Current uses + +Here is a (non-exhaustive) list of the kinds of things Danger has been used for +at GitLab so far: + +- Coding style +- Database review workflow +- Documentation review workflow +- Merge request metrics +- Reviewer roulette workflow +- Single codebase effort + +## Limitations + +- [`danger local` does not work on GitLab](https://github.com/danger/danger/issues/458) +- Danger output is not added to a merge request comment if working on a fork. |