diff options
Diffstat (limited to 'doc/development/contributing')
-rw-r--r-- | doc/development/contributing/index.md | 12 | ||||
-rw-r--r-- | doc/development/contributing/merge_request_workflow.md | 10 | ||||
-rw-r--r-- | doc/development/contributing/style_guides.md | 99 |
3 files changed, 83 insertions, 38 deletions
diff --git a/doc/development/contributing/index.md b/doc/development/contributing/index.md index 329303558b0..7a1c189e087 100644 --- a/doc/development/contributing/index.md +++ b/doc/development/contributing/index.md @@ -181,11 +181,21 @@ reasons for including it. `@mention` a maintainer in merge requests that contain: - More than 500 changes. -- Any major breaking changes. +- Any major [breaking changes](#breaking-changes). - External libraries. If you are not sure who to mention, the reviewer will do this for you early in the merge request process. +#### Breaking changes + +A "breaking change" is any change that requires users to make a corresponding change to their code, settings, or workflow. "Users" might be humans, API clients, or even code classes that "use" another class. Examples of breaking changes include: + +- Removing a user-facing feature without a replacement/workaround. +- Changing the definition of an existing API (by re-naming query parameters, changing routes, etc.). +- Removing a public method from a code class. + +A breaking change can be considered "major" if it affects many users, or represents a significant change in behavior. + #### Issues workflow This [documentation](issue_workflow.md) outlines the current issue workflow: diff --git a/doc/development/contributing/merge_request_workflow.md b/doc/development/contributing/merge_request_workflow.md index 7859af4e88b..166f7b350bf 100644 --- a/doc/development/contributing/merge_request_workflow.md +++ b/doc/development/contributing/merge_request_workflow.md @@ -65,9 +65,9 @@ request is as follows: template already provided in the "Description" field. 1. If you are contributing documentation, choose `Documentation` from the "Choose a template" menu and fill in the description according to the template. - 1. Mention the issue(s) your merge request solves, using the `Solves #XXX` or - `Closes #XXX` syntax to [auto-close](../../user/project/issues/managing_issues.md#closing-issues-automatically) - the issue(s) once the merge request is merged. + 1. Use the syntax `Solves #XXX`, `Closes #XXX`, or `Refs #XXX` to mention the issue(s) your merge + request addresses. Referenced issues do not [close automatically](../../user/project/issues/managing_issues.md#closing-issues-automatically). + You must close them manually once the merge request is merged. 1. If you're allowed to, set a relevant milestone and [labels](issue_workflow.md). 1. UI changes should use available components from the GitLab Design System, [Pajamas](https://design.gitlab.com/). The MR must include *Before* and @@ -150,7 +150,7 @@ Commit messages should follow the guidelines below, for reasons explained by Chr #### Why these standards matter 1. Consistent commit messages that follow these guidelines make the history more readable. -1. Concise standard commit messages helps to identify breaking changes for a deployment or ~"master:broken" quicker when +1. Concise standard commit messages helps to identify [breaking changes](index.md#breaking-changes) for a deployment or ~"master:broken" quicker when reviewing commits between two points in time. #### Commit message template @@ -212,7 +212,7 @@ the contribution acceptance criteria below: 1. Changes do not degrade performance: - Avoid repeated polling of endpoints that require a significant amount of overhead. - Check for N+1 queries via the SQL log or [`QueryRecorder`](../merge_request_performance_guidelines.md). - - Avoid repeated access of the filesystem. + - Avoid repeated access of the file system. - Use [polling with ETag caching](../polling.md) if needed to support real-time features. 1. If the merge request adds any new libraries (gems, JavaScript libraries, etc.), they should conform to our [Licensing guidelines](../licensing.md). See those diff --git a/doc/development/contributing/style_guides.md b/doc/development/contributing/style_guides.md index c316d50c88c..2a2cfebe964 100644 --- a/doc/development/contributing/style_guides.md +++ b/doc/development/contributing/style_guides.md @@ -15,52 +15,83 @@ settings automatically by default. If your editor/IDE does not automatically sup we suggest investigating to see if a plugin exists. For instance here is the [plugin for vim](https://github.com/editorconfig/editorconfig-vim). -## Pre-push static analysis +## Pre-push static analysis with Lefthook -We strongly recommend installing [Lefthook](https://github.com/Arkweid/lefthook) to automatically check -for static analysis offenses before pushing your changes. +[Lefthook](https://github.com/Arkweid/lefthook) is a Git hooks manager that allows +custom logic to be executed prior to Git committing or pushing. GitLab comes with +Lefthook configuration (`lefthook.yml`), but it must be installed. -To install `lefthook`, run the following in your GitLab source directory: +We have a `lefthook.yml` checked in but it is ignored until Lefthook is installed. -```shell -# 1. Make sure to uninstall Overcommit first -overcommit --uninstall +### Uninstall Overcommit + +We were using Overcommit prior to Lefthook, so you may want to uninstall it first with `overcommit --uninstall`. + +### Install Lefthook + +1. Install the `lefthook` Ruby gem: + + ```shell + bundle install + ``` + +1. Install Lefthook managed Git hooks: + + ```shell + bundle exec lefthook install + ``` + +1. Test Lefthook is working by running the Lefthook `prepare-commit-msg` Git hook: + + ```shell + bundle exec lefthook run prepare-commit-msg + ``` + +This should return a fully qualified path command with no other output. + +### Lefthook configuration -# If using rbenv, at this point you may need to do: rbenv rehash +The current Lefthook configuration can be found in [`lefthook.yml`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/lefthook.yml). -# 2. Install lefthook... +Before you push your changes, Lefthook automatically runs the following checks: -## With Homebrew (macOS) -brew install Arkweid/lefthook/lefthook +- Danger: Runs a subset of checks that `danger-review` runs on your merge requests. +- ES lint: Run `yarn eslint` checks (with the [`.eslintrc.yml`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/.eslintrc.yml) configuration) on the modified `*.{js,vue}` files. Tags: `frontend`, `style`. +- HAML lint: Run `bundle exec haml-lint` checks (with the [`.haml-lint.yml`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/.haml-lint.yml) configuration) on the modified `*.html.haml` files. Tags: `view`, `haml`, `style`. +- Markdown lint: Run `yarn markdownlint` checks on the modified `*.md` files. Tags: `documentation`, `style`. +- SCSS lint: Run `bundle exec scss-lint` checks (with the [`.scss-lint.yml`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/.scss-lint.yml) configuration) on the modified `*.scss{,.css}` files. Tags: `stylesheet`, `css`, `style`. +- RuboCop: Run `bundle exec rubocop` checks (with the [`.rubocop.yml`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/.rubocop.yml) configuration) on the modified `*.rb` files. Tags: `backend`, `style`. +- Vale: Run `vale` checks (with the [`.vale.ini`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/.vale.ini) configuration) on the modified `*.md` files. Tags: `documentation`, `style`. -## Or with Go -go get github.com/Arkweid/lefthook +In addition to the default configuration, you can define a [local configuration](https://github.com/Arkweid/lefthook/blob/master/docs/full_guide.md#local-config). -## Or with Rubygems -gem install lefthook +### Disable Lefthook temporarily -### You may need to run the following if you're using rbenv -rbenv rehash +To disable Lefthook temporarily, you can set the `LEFTHOOK` environment variable to `0`. For instance: -# 3. Install the Git hooks -lefthook install -f +```shell +LEFTHOOK=0 git push ... ``` -Before you push your changes, Lefthook then automatically run Danger checks, and other checks -for changed files. This saves you time as you don't have to wait for the same errors to be detected -by CI/CD. +### Run Lefthook hooks manually + +To run the `pre-push` Git hook, run: + +```shell +bundle exec lefthook run pre-push +``` -Lefthook relies on a pre-push hook to prevent commits that violate its ruleset. -To override this behavior, pass the environment variable `LEFTHOOK=0`. That is, -`LEFTHOOK=0 git push`. +For more information, check out [Lefthook documentation](https://github.com/Arkweid/lefthook/blob/master/docs/full_guide.md#run-githook-group-directly). -You can also: +### Skip Lefthook checks per tag -- Define [local configuration](https://github.com/Arkweid/lefthook/blob/master/docs/full_guide.md#local-config). -- Skip [checks per tag on the fly](https://github.com/Arkweid/lefthook/blob/master/docs/full_guide.md#skip-some-tags-on-the-fly). - For example, `LEFTHOOK_EXCLUDE=frontend git push origin`. -- Run [hooks manually](https://github.com/Arkweid/lefthook/blob/master/docs/full_guide.md#run-githook-group-directly). - For example, `lefthook run pre-push`. +To skip some checks based on tags when pushing, you can set the `LEFTHOOK_EXCLUDE` environment variable. For instance: + +```shell +LEFTHOOK_EXCLUDE=frontend,documentation git push ... +``` + +For more information, check out [Lefthook documentation](https://github.com/Arkweid/lefthook/blob/master/docs/full_guide.md#skip-some-tags-on-the-fly). ## Ruby, Rails, RSpec @@ -111,7 +142,7 @@ This ensures that our list isn't mistakenly removed by another auto generation o the `.rubocop_todo.yml`. This also allows us greater visibility into the exceptions which are currently being resolved. -One way to generate the initial list is to run the todo auto generation, +One way to generate the initial list is to run the `todo` auto generation, with `exclude limit` set to a high number. ```shell @@ -149,8 +180,12 @@ See the dedicated [Shell scripting standards and style guidelines](../shell_scri ## Markdown +<!-- vale gitlab.Spelling = NO --> + We're following [Ciro Santilli's Markdown Style Guide](https://cirosantilli.com/markdown-style-guide/). +<!-- vale gitlab.Spelling = YES --> + ## Documentation See the dedicated [Documentation Style Guide](../documentation/styleguide/index.md). |