diff options
Diffstat (limited to 'doc/development')
31 files changed, 2542 insertions, 533 deletions
diff --git a/doc/development/README.md b/doc/development/README.md index 3c797505aa9..934c6849ff9 100644 --- a/doc/development/README.md +++ b/doc/development/README.md @@ -33,7 +33,6 @@ ## Backend howtos - [Architecture](architecture.md) of GitLab -- [CI setup](ci_setup.md) for testing GitLab - [Gotchas](gotchas.md) to avoid - [How to dump production data to staging](db_dump.md) - [Instrumentation](instrumentation.md) @@ -42,12 +41,18 @@ - [Shell commands](shell_commands.md) in the GitLab codebase - [Sidekiq debugging](sidekiq_debugging.md) - [Object state models](object_state_models.md) +- [Building a package for testing purposes](build_test_package.md) ## Databases - [What requires downtime?](what_requires_downtime.md) - [Adding database indexes](adding_database_indexes.md) - [Post Deployment Migrations](post_deployment_migrations.md) +- [Foreign Keys & Associations](foreign_keys.md) + +## i18n + +- [Internationalization for GitLab](i18n_guide.md) ## Compliance diff --git a/doc/development/architecture.md b/doc/development/architecture.md index 4eb7a8eee48..b36fd52603b 100644 --- a/doc/development/architecture.md +++ b/doc/development/architecture.md @@ -4,7 +4,7 @@ There are two editions of GitLab: [Enterprise Edition](https://about.gitlab.com/gitlab-ee/) (EE) and [Community Edition](https://about.gitlab.com/gitlab-ce/) (CE). GitLab CE is delivered via git from the [gitlabhq repository](https://gitlab.com/gitlab-org/gitlab-ce/tree/master). New versions of GitLab are released in stable branches and the master branch is for bleeding edge development. -EE releases are available not long after CE releases. To obtain the GitLab EE there is a [repository at gitlab.com](https://gitlab.com/subscribers/gitlab-ee). For more information about the release process see the section 'New versions and upgrading' in the readme. +EE releases are available not long after CE releases. To obtain the GitLab EE there is a [repository at gitlab.com](https://gitlab.com/gitlab-org/gitlab-ee). For more information about the release process see the section 'New versions and upgrading' in the readme. Both EE and CE require some add-on components called gitlab-shell and Gitaly. These components are available from the [gitlab-shell](https://gitlab.com/gitlab-org/gitlab-shell/tree/master) and [gitaly](https://gitlab.com/gitlab-org/gitaly/tree/master) repositories respectively. New versions are usually tags but staying on the master branch will give you the latest stable version. New releases are generally around the same time as GitLab CE releases with exception for informal security updates deemed critical. diff --git a/doc/development/build_test_package.md b/doc/development/build_test_package.md new file mode 100644 index 00000000000..439d228baef --- /dev/null +++ b/doc/development/build_test_package.md @@ -0,0 +1,39 @@ +# Building a package for testing + +While developing a new feature or modifying an existing one, it is helpful if an +installable package (or a docker image) containing those changes is available +for testing. For this very purpose, a manual job is provided in the GitLab CI/CD +pipeline that can be used to trigger a pipeline in the omnibus-gitlab repository +that will create +1. A deb package for Ubuntu 16.04, available as a build artifact, and +2. A docker image, which is pushed to [Omnibus GitLab's container +registry](https://gitlab.com/gitlab-org/omnibus-gitlab/container_registry) +(images titled `gitlab-ce` and `gitlab-ee` respectively and image tag is the +commit which triggered the pipeline). + +When you push a commit to either the gitlab-ce or gitlab-ee project, the +pipeline for that commit will have a `build-package` manual action you can +trigger. + + + + + +## Specifying versions of components + +If you want to create a package from a specific branch, commit or tag of any of +the GitLab components (like GitLab Workhorse, Gitaly, GitLab Pages, etc.), you +can specify the branch name, commit sha or tag in the component's respective +`*_VERSION` file. For example, if you want to build a package that uses the +branch `0-1-stable`, modify the content of `GITALY_SERVER_VERSION` to +`0-1-stable` and push the commit. This will create a manual job that can be +used to trigger the build. + +## Specifying the branch in omnibus-gitlab repository + +In scenarios where a configuration change is to be introduced and omnibus-gitlab +repository already has the necessary changes in a specific branch, you can build +a package against that branch through an environment variable named +`OMNIBUS_BRANCH`. To do this, specify that environment variable with the name of +the branch as value in `.gitlab-ci.yml` and push a commit. This will create a +manual job that can be used to trigger the build. diff --git a/doc/development/ci_setup.md b/doc/development/ci_setup.md deleted file mode 100644 index 0810b32efd7..00000000000 --- a/doc/development/ci_setup.md +++ /dev/null @@ -1,47 +0,0 @@ -# CI setup - -This document describes what services we use for testing GitLab and GitLab CI. - -We currently use four CI services to test GitLab: - -1. GitLab CI on [GitHost.io](https://gitlab-ce.githost.io/projects/4/) for the [GitLab.com repo](https://gitlab.com/gitlab-org/gitlab-ce) -2. GitLab CI at ci.gitlab.org to test the private GitLab B.V. repo at dev.gitlab.org -3. [Semephore](https://semaphoreapp.com/gitlabhq/gitlabhq/) for [GitHub.com repo](https://github.com/gitlabhq/gitlabhq) -4. [Mock CI Service](../user/project/integrations/mock_ci.md) for local development - -| Software @ configuration being tested | GitLab CI (ci.gitlab.org) | GitLab CI (GitHost.io) | Semaphore | -|---------------------------------------|---------------------------|---------------------------------------------------------------------------|-----------| -| GitLab CE @ MySQL | ✓ | ✓ [Core team can trigger builds](https://gitlab-ce.githost.io/projects/4) | | -| GitLab CE @ PostgreSQL | | | ✓ [Core team can trigger builds](https://semaphoreapp.com/gitlabhq/gitlabhq/branches/master) | -| GitLab EE @ MySQL | ✓ | | | -| GitLab CI @ MySQL | ✓ | | | -| GitLab CI @ PostgreSQL | | | ✓ | -| GitLab CI Runner | ✓ | | ✓ | -| GitLab Shell | ✓ | | ✓ | -| GitLab Shell | ✓ | | ✓ | - -Core team has access to trigger builds if needed for GitLab CE. - -We use [these build scripts](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/.gitlab-ci.yml) for testing with GitLab CI. - -# Build configuration on [Semaphore](https://semaphoreapp.com/gitlabhq/gitlabhq/) for testing the [GitHub.com repo](https://github.com/gitlabhq/gitlabhq) - -- Language: Ruby -- Ruby version: 2.1.8 -- database.yml: pg - -Build commands - -```bash -sudo apt-get install cmake libicu-dev -y (Setup) -bundle install --deployment --path vendor/bundle (Setup) -cp config/gitlab.yml.example config/gitlab.yml (Setup) -bundle exec rake db:create (Setup) -bundle exec rake spinach (Thread #1) -bundle exec rake spec (thread #2) -bundle exec rake rubocop (thread #3) -bundle exec rake brakeman (thread #4) -bundle exec rake jasmine:ci (thread #5) -``` - -Use rubygems mirror. diff --git a/doc/development/code_review.md b/doc/development/code_review.md index 819578404b6..4ed89146072 100644 --- a/doc/development/code_review.md +++ b/doc/development/code_review.md @@ -1,5 +1,27 @@ # Code Review Guidelines +## Getting your merge request reviewed, approved, and merged + +There are a few rules to get your merge request accepted: + +1. Your merge request should only be **merged by a [maintainer][team]**. + 1. If your merge request includes only backend changes [^1], it must be + **approved by a [backend maintainer][projects]**. + 1. If your merge request includes only frontend changes [^1], it must be + **approved by a [frontend maintainer][projects]**. + 1. If your merge request includes frontend and backend changes [^1], it must + be **approved by a [frontend and a backend maintainer][projects]**. +1. To lower the amount of merge requests maintainers need to review, you can + ask or assign any [reviewers][projects] for a first review. + 1. If you need some guidance (e.g. it's your first merge request), feel free + to ask one of the [Merge request coaches][team]. + 1. The reviewer will assign the merge request to a maintainer once the + reviewer is satisfied with the state of the merge request. + +For more guidance, see [CONTRIBUTING.md](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md). + +## Best practices + This guide contains advice and best practices for performing code review, and having your code reviewed. @@ -10,9 +32,9 @@ code is effective, understandable, and maintainable. Any developer can, and is encouraged to, perform code review on merge requests of colleagues and contributors. However, the final decision to accept a merge request is up to one the project's maintainers, denoted on the -[team page](https://about.gitlab.com/team). +[engineering projects][projects]. -## Everyone +### Everyone - Accept that many programming decisions are opinions. Discuss tradeoffs, which you prefer, and reach a resolution quickly. @@ -31,8 +53,11 @@ request is up to one the project's maintainers, denoted on the - Consider one-on-one chats or video calls if there are too many "I didn't understand" or "Alternative solution:" comments. Post a follow-up comment summarizing one-on-one discussion. +- If you ask a question to a specific person, always start the comment by + mentioning them; this will ensure they see it if their notification level is + set to "mentioned" and other people will understand they don't have to respond. -## Having your code reviewed +### Having your code reviewed Please keep in mind that code review is a process that can take multiple iterations, and reviewers may spot things later that they may not have seen the @@ -50,11 +75,12 @@ first time. - Extract unrelated changes and refactorings into future merge requests/issues. - Seek to understand the reviewer's perspective. - Try to respond to every comment. +- Let the reviewer select the "Resolve discussion" buttons. - Push commits based on earlier rounds of feedback as isolated commits to the branch. Do not squash until the branch is ready to merge. Reviewers should be able to read individual updates based on their earlier feedback. -## Reviewing code +### Reviewing code Understand why the change is necessary (fixes a bug, improves the user experience, refactors the existing code). Then: @@ -69,12 +95,19 @@ experience, refactors the existing code). Then: someone else would be confused by it as well. - After a round of line notes, it can be helpful to post a summary note such as "LGTM :thumbsup:", or "Just a couple things to address." +- Assign the merge request to the author if changes are required following your + review. +- Set the milestone before merging a merge request. - Avoid accepting a merge request before the job succeeds. Of course, "Merge When Pipeline Succeeds" (MWPS) is fine. - If you set the MR to "Merge When Pipeline Succeeds", you should take over subsequent revisions for anything that would be spotted after that. +- Consider using the [Squash and + merge][squash-and-merge] feature when the merge request has a lot of commits. + +[squash-and-merge]: https://docs.gitlab.com/ee/user/project/merge_requests/squash_and_merge.html#squash-and-merge -## The right balance +### The right balance One of the most difficult things during code review is finding the right balance in how deep the reviewer can interfere with the code created by a @@ -100,7 +133,7 @@ reviewee. tomorrow. When you are not able to find the right balance, ask other people about their opinion. -## Credits +### Credits Largely based on the [thoughtbot code review guide]. @@ -109,3 +142,6 @@ Largely based on the [thoughtbot code review guide]. --- [Return to Development documentation](README.md) + +[projects]: https://about.gitlab.com/handbook/engineering/projects/ +[team]: https://about.gitlab.com/team/ diff --git a/doc/development/doc_styleguide.md b/doc/development/doc_styleguide.md index bb78a0de0c5..5b09f79f143 100644 --- a/doc/development/doc_styleguide.md +++ b/doc/development/doc_styleguide.md @@ -29,7 +29,8 @@ The table below shows what kind of documentation goes where. | `doc/legal/` | Legal documents about contributing to GitLab. | | `doc/install/`| Probably the most visited directory, since `installation.md` is there. Ideally this should go under `doc/administration/`, but it's best to leave it as-is in order to avoid confusion (still debated though). | | `doc/update/` | Same with `doc/install/`. Should be under `administration/`, but this is a well known location, better leave as-is, at least for now. | -| `doc/topics/` | Indexes per Topic (`doc/topics/topic-name/index.md`); Technical Articles: user guides, admin guides, technical overviews, tutorials (`doc/topics/topic-name/`). | +| `doc/topics/` | Indexes per Topic (`doc/topics/topic-name/index.md`): all resources for that topic (user and admin documentation, articles, and third-party docs) | +| `doc/articles/` | [Technical Articles](writing_documentation.md#technical-articles): user guides, admin guides, technical overviews, tutorials (`doc/articles/article-title/index.md`). | --- @@ -61,8 +62,8 @@ The table below shows what kind of documentation goes where. located at `doc/user/admin_area/settings/visibility_and_access_controls.md`. 1. The `doc/topics/` directory holds topic-related technical content. Create `doc/topics/topic-name/subtopic-name/index.md` when subtopics become necessary. - Note that `topics` holds the index page per topic, and technical articles. General - user- and admin- related documentation, should be placed accordingly. + General user- and admin- related documentation, should be placed accordingly. +1. For technical articles, place their images under `doc/articles/article-title/img/`. --- @@ -197,10 +198,17 @@ You can combine one or more of the following: the `.md` document that you're working on is located. Always prepend their names with the name of the document that they will be included in. For example, if there is a document called `twitter.md`, then a valid image name - could be `twitter_login_screen.png`. + could be `twitter_login_screen.png`. [**Exception**: images for + [articles](writing_documentation.md#technical-articles) should be + put in a directory called `img` underneath `/articles/article_title/img/`, therefore, + there's no need to prepend the document name to their filenames.] - Images should have a specific, non-generic name that will differentiate them. - Keep all file names in lower case. - Consider using PNG images instead of JPEG. +- Compress all images with <https://tinypng.com/> or similar tool. +- Compress gifs with <https://ezgif.com/optimize> or similar toll. +- Images should be used (only when necessary) to _illustrate_ the description +of a process, not to _replace_ it. Inside the document: diff --git a/doc/development/fe_guide/droplab/droplab.md b/doc/development/fe_guide/droplab/droplab.md new file mode 100644 index 00000000000..112ff3419d9 --- /dev/null +++ b/doc/development/fe_guide/droplab/droplab.md @@ -0,0 +1,258 @@ +# DropLab + +A generic dropdown for all of your custom dropdown needs. + +## Usage + +DropLab can be used by simply adding a `data-dropdown-trigger` HTML attribute. +This attribute allows us to find the "trigger" _(toggle)_ for the dropdown, +whether that is a button, link or input. + +The value of the `data-dropdown-trigger` should be a CSS selector that +DropLab can use to find the trigger's dropdown list. + +You should also add the `data-dropdown` attribute to declare the dropdown list. +The value is irrelevant. + +The DropLab class has no side effects, so you must always call `.init` when +the DOM is ready. `DropLab.prototype.init` takes the same arguments as `DropLab.prototype.addHook`. +If you do not provide any arguments, it will globally query and instantiate all droplab compatible dropdowns. + +```html +<a href="#" data-dropdown-trigger="#list">Toggle</a> + +<ul id="list" data-dropdown> + <!-- ... --> +<ul> +``` +```js +const droplab = new DropLab(); +droplab.init(); +``` + +As you can see, we have a "Toggle" link, that is declared as a trigger. +It provides a selector to find the dropdown list it should control. + +### Static data + +You can add static list items. + +```html +<a href="#" data-dropdown-trigger="#list">Toggle</a> + +<ul id="list" data-dropdown> + <li>Static value 1</li> + <li>Static value 2</li> +<ul> +``` +```js +const droplab = new DropLab(); +droplab.init(); +``` + +### Explicit instantiation + +You can pass the trigger and list elements as constructor arguments to return +a non-global instance of DropLab using the `DropLab.prototype.init` method. + +```html +<a href="#" id="trigger" data-dropdown-trigger="#list">Toggle</a> + +<ul id="list" data-dropdown> + <!-- ... --> +<ul> +``` +```js +const trigger = document.getElementById('trigger'); +const list = document.getElementById('list'); + +const droplab = new DropLab(); +droplab.init(trigger, list); +``` + +You can also add hooks to an existing DropLab instance using `DropLab.prototype.addHook`. + +```html +<a href="#" data-dropdown-trigger="#auto-dropdown">Toggle</a> +<ul id="auto-dropdown" data-dropdown><!-- ... --><ul> + +<a href="#" id="trigger" data-dropdown-trigger="#list">Toggle</a> +<ul id="list" data-dropdown><!-- ... --><ul> +``` +```js +const droplab = new DropLab(); + +droplab.init(); + +const trigger = document.getElementById('trigger'); +const list = document.getElementById('list'); + +droplab.addHook(trigger, list); +``` + + +### Dynamic data + +Adding `data-dynamic` to your dropdown element will enable dynamic list rendering. + +You can template a list item using the keys of the data object provided. +Use the handlebars syntax `{{ value }}` to HTML escape the value. +Use the `<%= value %>` syntax to simply interpolate the value. +Use the `<%= value %>` syntax to evaluate the value. + +Passing an array of objects to `DropLab.prototype.addData` will render that data +for all `data-dynamic` dropdown lists tracked by that DropLab instance. + +```html +<a href="#" data-dropdown-trigger="#list">Toggle</a> + +<ul id="list" data-dropdown data-dynamic> + <li><a href="#" data-id="{{id}}">{{text}}</a></li> +</ul> +``` +```js +const droplab = new DropLab(); + +droplab.init().addData([{ + id: 0, + text: 'Jacob', +}, { + id: 1, + text: 'Jeff', +}]); +``` + +Alternatively, you can specify a specific dropdown to add this data to but passing +the data as the second argument and and the `id` of the trigger element as the first argument. + +```html +<a href="#" data-dropdown-trigger="#list" id="trigger">Toggle</a> + +<ul id="list" data-dropdown data-dynamic> + <li><a href="#" data-id="{{id}}">{{text}}</a></li> +</ul> +``` +```js +const droplab = new DropLab(); + +droplab.init().addData('trigger', [{ + id: 0, + text: 'Jacob', +}, { + id: 1, + text: 'Jeff', +}]); +``` + +This allows you to mix static and dynamic content with ease, even with one trigger. + +Note the use of scoping regarding the `data-dropdown` attribute to capture both +dropdown lists, one of which is dynamic. + +```html +<input id="trigger" data-dropdown-trigger="#list"> +<div id="list" data-dropdown> + <ul> + <li><a href="#">Static item 1</a></li> + <li><a href="#">Static item 2</a></li> + </ul> + <ul data-dynamic> + <li><a href="#" data-id="{{id}}">{{text}}</a></li> + </ul> +</div> +``` +```js +const droplab = new DropLab(); + +droplab.init().addData('trigger', [{ + id: 0, + text: 'Jacob', +}, { + id: 1, + text: 'Jeff', +}]); +``` + +## Internal selectors + +DropLab adds some CSS classes to help lower the barrier to integration. + +For example, + +* The `droplab-item-selected` css class is added to items that have been selected +either by a mouse click or by enter key selection. +* The `droplab-item-active` css class is added to items that have been selected +using arrow key navigation. +* You can add the `droplab-item-ignore` css class to any item that you do not want to be selectable. For example, +an `<li class="divider"></li>` list divider element that should not be interactive. + +## Internal events + +DropLab uses some custom events to help lower the barrier to integration. + +For example, + +* The `click.dl` event is fired when an `li` list item has been clicked. It is also +fired when a list item has been selected with the keyboard. It is also fired when a +`HookButton` button is clicked (a registered `button` tag or `a` tag trigger). +* The `input.dl` event is fired when a `HookInput` (a registered `input` tag trigger) triggers an `input` event. +* The `mousedown.dl` event is fired when a `HookInput` triggers a `mousedown` event. +* The `keyup.dl` event is fired when a `HookInput` triggers a `keyup` event. +* The `keydown.dl` event is fired when a `HookInput` triggers a `keydown` event. + +These custom events add a `detail` object to the vanilla `Event` object that provides some potentially useful data. + +## Plugins + +Plugins are objects that are registered to be executed when a hook is added (when a droplab trigger and dropdown are instantiated). + +If no modules API is detected, the library will fall back as it does with `window.DropLab` and will add `window.DropLab.plugins.PluginName`. + +### Usage + +To use plugins, you can pass them in an array as the third argument of `DropLab.prototype.init` or `DropLab.prototype.addHook`. +Some plugins require configuration values, the config object can be passed as the fourth argument. + +```html +<a href="#" id="trigger" data-dropdown-trigger="#list">Toggle</a> +<ul id="list" data-dropdown><!-- ... --><ul> +``` +```js +const droplab = new DropLab(); + +const trigger = document.getElementById('trigger'); +const list = document.getElementById('list'); + +droplab.init(trigger, list, [droplabAjax], { + droplabAjax: { + endpoint: '/some-endpoint', + method: 'setData', + }, +}); +``` + +### Documentation + +* [Ajax plugin](plugins/ajax.md) +* [Filter plugin](plugins/filter.md) +* [InputSetter plugin](plugins/input_setter.md) + +### Development + +When plugins are initialised for a droplab trigger+dropdown, DropLab will +call the plugins `init` function, so this must be implemented in the plugin. + +```js +class MyPlugin { + static init() { + this.someProp = 'someProp'; + this.someMethod(); + } + + static someMethod() { + this.otherProp = 'otherProp'; + } +} + +export default MyPlugin; +``` diff --git a/doc/development/fe_guide/droplab/plugins/ajax.md b/doc/development/fe_guide/droplab/plugins/ajax.md new file mode 100644 index 00000000000..9c7e56fa448 --- /dev/null +++ b/doc/development/fe_guide/droplab/plugins/ajax.md @@ -0,0 +1,37 @@ +# Ajax + +`Ajax` is a droplab plugin that allows for retrieving and rendering list data from a server. + +## Usage + +Add the `Ajax` object to the plugins array of a `DropLab.prototype.init` or `DropLab.prototype.addHook` call. + +`Ajax` requires 2 config values, the `endpoint` and `method`. + +* `endpoint` should be a URL to the request endpoint. +* `method` should be `setData` or `addData`. +* `setData` completely replaces the dropdown with the response data. +* `addData` appends the response data to the current dropdown list. + +```html +<a href="#" id="trigger" data-dropdown-trigger="#list">Toggle</a> +<ul id="list" data-dropdown><!-- ... --><ul> +``` +```js + const droplab = new DropLab(); + + const trigger = document.getElementById('trigger'); + const list = document.getElementById('list'); + + droplab.addHook(trigger, list, [Ajax], { + Ajax: { + endpoint: '/some-endpoint', + method: 'setData', + }, + }); +``` + +Optionally you can set `loadingTemplate` to a HTML string. This HTML string will +replace the dropdown list whilst the request is pending. + +Additionally, you can set `onError` to a function to catch any XHR errors. diff --git a/doc/development/fe_guide/droplab/plugins/filter.md b/doc/development/fe_guide/droplab/plugins/filter.md new file mode 100644 index 00000000000..0853ea4d320 --- /dev/null +++ b/doc/development/fe_guide/droplab/plugins/filter.md @@ -0,0 +1,45 @@ +# Filter + +`Filter` is a plugin that allows for filtering data that has been added +to the dropdown using a simple fuzzy string search of an input value. + +## Usage + +Add the `Filter` object to the plugins array of a `DropLab.prototype.init` or `DropLab.prototype.addHook` call. + +* `Filter` requires a config value for `template`. +* `template` should be the key of the objects within your data array that you want to compare +to the user input string, for filtering. + +```html +<input href="#" id="trigger" data-dropdown-trigger="#list"> +<ul id="list" data-dropdown data-dynamic> + <li><a href="#" data-id="{{id}}">{{text}}</a></li> +<ul> +``` +```js + const droplab = new DropLab(); + + const trigger = document.getElementById('trigger'); + const list = document.getElementById('list'); + + droplab.init(trigger, list, [Filter], { + Filter: { + template: 'text', + }, + }); + + droplab.addData('trigger', [{ + id: 0, + text: 'Jacob', + }, { + id: 1, + text: 'Jeff', + }]); +``` + +Above, the input string will be compared against the `test` key of the passed data objects. + +Optionally you can set `filterFunction` to a function. This function will be used instead +of `Filter`'s built in string search. `filterFunction` is passed 2 arguments, the first +is one of the data objects, the second is the current input value. diff --git a/doc/development/fe_guide/droplab/plugins/input_setter.md b/doc/development/fe_guide/droplab/plugins/input_setter.md new file mode 100644 index 00000000000..a549603c20d --- /dev/null +++ b/doc/development/fe_guide/droplab/plugins/input_setter.md @@ -0,0 +1,60 @@ +# InputSetter + +`InputSetter` is a plugin that allows for udating DOM out of the scope of droplab when a list item is clicked. + +## Usage + +Add the `InputSetter` object to the plugins array of a `DropLab.prototype.init` or `DropLab.prototype.addHook` call. + +* `InputSetter` requires a config value for `input` and `valueAttribute`. +* `input` should be the DOM element that you want to manipulate. +* `valueAttribute` should be a string that is the name of an attribute on your list items that is used to get the value +to update the `input` element with. + +You can also set the `InputSetter` config to an array of objects, which will allow you to update multiple elements. + + +```html +<input id="input" value=""> +<div id="div" data-selected-id=""></div> + +<input href="#" id="trigger" data-dropdown-trigger="#list"> +<ul id="list" data-dropdown data-dynamic> + <li><a href="#" data-id="{{id}}">{{text}}</a></li> +<ul> +``` +```js + const droplab = new DropLab(); + + const trigger = document.getElementById('trigger'); + const list = document.getElementById('list'); + + const input = document.getElementById('input'); + const div = document.getElementById('div'); + + droplab.init(trigger, list, [InputSetter], { + InputSetter: [{ + input: input, + valueAttribute: 'data-id', + } { + input: div, + valueAttribute: 'data-id', + inputAttribute: 'data-selected-id', + }], + }); + + droplab.addData('trigger', [{ + id: 0, + text: 'Jacob', + }, { + id: 1, + text: 'Jeff', + }]); +``` + +Above, if the second list item was clicked, it would update the `#input` element +to have a `value` of `1`, it would also update the `#div` element's `data-selected-id` to `1`. + +Optionally you can set `inputAttribute` to a string that is the name of an attribute on your `input` element that you want to update. +If you do not provide an `inputAttribute`, `InputSetter` will update the `value` of the `input` element if it is an `INPUT` element, +or the `textContent` of the `input` element if it is not an `INPUT` element. diff --git a/doc/development/fe_guide/img/boards_diagram.png b/doc/development/fe_guide/img/boards_diagram.png Binary files differnew file mode 100644 index 00000000000..7a2cf972fd0 --- /dev/null +++ b/doc/development/fe_guide/img/boards_diagram.png diff --git a/doc/development/fe_guide/img/testing_triangle.png b/doc/development/fe_guide/img/testing_triangle.png Binary files differnew file mode 100644 index 00000000000..7a9a848c2ee --- /dev/null +++ b/doc/development/fe_guide/img/testing_triangle.png diff --git a/doc/development/fe_guide/img/vue_arch.png b/doc/development/fe_guide/img/vue_arch.png Binary files differnew file mode 100644 index 00000000000..a67706c7c1e --- /dev/null +++ b/doc/development/fe_guide/img/vue_arch.png diff --git a/doc/development/fe_guide/index.md b/doc/development/fe_guide/index.md index f963bffde37..64bcb4a0257 100644 --- a/doc/development/fe_guide/index.md +++ b/doc/development/fe_guide/index.md @@ -27,6 +27,55 @@ For our currently-supported browsers, see our [requirements][requirements]. --- +## Development Process + +When you are assigned an issue please follow the next steps: + +### Divide a big feature into small Merge Requests +1. Big Merge Request are painful to review. In order to make this process easier we +must break a big feature into smaller ones and create a Merge Request for each step. +1. First step is to create a branch from `master`, let's call it `new-feature`. This branch +will be the recipient of all the smaller Merge Requests. Only this one will be merged to master. +1. Don't do any work on this one, let's keep it synced with master. +1. Create a new branch from `new-feature`, let's call it `new-feature-step-1`. We advise you +to clearly identify which step the branch represents. +1. Do the first part of the modifications in this branch. The target branch of this Merge Request +should be `new-feature`. +1. Once `new-feature-step-1` gets merged into `new-feature` we can continue our work. Create a new +branch from `new-feature`, let's call it `new-feature-step-2` and repeat the process done before. + +```shell +master +└─ new-feature + ├─ new-feature-step-1 + ├─ new-feature-step-2 + └─ new-feature-step-3 +``` + +**Tips** +- Make sure `new-feature` branch is always synced with `master`: merge master frequently. +- Do the same for the feature branch you have opened. This can be accomplished by merging `master` into `new-feature` and `new-feature` into `new-feature-step-*` +- Avoid rewriting history. + +### Share your work early +1. Before writing code guarantee your vision of the architecture is aligned with +GitLab's architecture. +1. Add a diagram to the issue and ask a Frontend Architecture about it. + +  + +1. Don't take more than one week between starting work on a feature and +sharing a Merge Request with a reviewer or a maintainer. + +### Vue features +1. Follow the steps in [Vue.js Best Practices](vue.md) +1. Follow the style guide. +1. Only a handful of people are allowed to merge Vue related features. +Reach out to one of Vue experts early in this process. + + +--- + ## [Architecture](architecture.md) How we go about making fundamental design decisions in GitLab's frontend team or make changes to our frontend development guidelines. @@ -90,3 +139,13 @@ Our accessibility standards and resources. [scss-lint]: https://github.com/brigade/scss-lint [install]: ../../install/installation.md#4-node [requirements]: ../../install/requirements.md#supported-web-browsers + +--- + +## [DropLab](droplab/droplab.md) +Our internal `DropLab` dropdown library. + +* [DropLab](droplab/droplab.md) +* [Ajax plugin](droplab/plugins/ajax.md) +* [Filter plugin](droplab/plugins/filter.md) +* [InputSetter plugin](droplab/plugins/input_setter.md) diff --git a/doc/development/fe_guide/performance.md b/doc/development/fe_guide/performance.md index 9437a5f7a6e..2ddcbe13afa 100644 --- a/doc/development/fe_guide/performance.md +++ b/doc/development/fe_guide/performance.md @@ -12,8 +12,8 @@ Thus, we must strike a balance between sending requests and the feeling of realt Use the following rules when creating realtime solutions. 1. The server will tell you how much to poll by sending `Poll-Interval` in the header. -Use that as your polling interval. This way it is easy for system administrators to change the -polling rate. +Use that as your polling interval. This way it is [easy for system administrators to change the +polling rate](../../administration/polling.md). A `Poll-Interval: -1` means you should disable polling, and this must be implemented. 1. A response with HTTP status `4XX` or `5XX` should disable polling as well. 1. Use a common library for polling. @@ -48,8 +48,8 @@ Steps to split page-specific JavaScript from the main `main.js`: ```haml - content_for :page_specific_javascripts do - = page_specific_javascript_bundle_tag('lib_chart') - = page_specific_javascript_bundle_tag('graphs') + = webpack_bundle_tag 'lib_chart' + = webpack_bundle_tag 'graphs' ``` The above loads `chart.js` and `graphs_bundle.js` for this page only. `chart.js` diff --git a/doc/development/fe_guide/style_guide_js.md b/doc/development/fe_guide/style_guide_js.md index abd241c0bc8..d2d89517241 100644 --- a/doc/development/fe_guide/style_guide_js.md +++ b/doc/development/fe_guide/style_guide_js.md @@ -11,401 +11,483 @@ See [our current .eslintrc][eslintrc] for specific rules and patterns. #### ESlint -- **Never** disable eslint rules unless you have a good reason. You may see a lot of legacy files with `/* eslint-disable some-rule, some-other-rule */` at the top, but legacy files are a special case. Any time you develop a new feature or refactor an existing one, you should abide by the eslint rules. - -- **Never Ever EVER** disable eslint globally for a file +1. **Never** disable eslint rules unless you have a good reason. +You may see a lot of legacy files with `/* eslint-disable some-rule, some-other-rule */` +at the top, but legacy files are a special case. Any time you develop a new feature or +refactor an existing one, you should abide by the eslint rules. +1. **Never Ever EVER** disable eslint globally for a file ```javascript - // bad - /* eslint-disable */ + // bad + /* eslint-disable */ - // better - /* eslint-disable some-rule, some-other-rule */ + // better + /* eslint-disable some-rule, some-other-rule */ - // best - // nothing :) + // best + // nothing :) ``` -- If you do need to disable a rule for a single violation, try to do it as locally as possible - +1. If you do need to disable a rule for a single violation, try to do it as locally as possible ```javascript - // bad - /* eslint-disable no-new */ + // bad + /* eslint-disable no-new */ - import Foo from 'foo'; + import Foo from 'foo'; - new Foo(); + new Foo(); - // better - import Foo from 'foo'; + // better + import Foo from 'foo'; - // eslint-disable-next-line no-new - new Foo(); + // eslint-disable-next-line no-new + new Foo(); ``` +1. There are few rules that we need to disable due to technical debt. Which are: + 1. [no-new][eslint-new] + 1. [class-methods-use-this][eslint-this] -- When they are needed _always_ place ESlint directive comment blocks on the first line of a script, followed by any global declarations, then a blank newline prior to any imports or code. - +1. When they are needed _always_ place ESlint directive comment blocks on the first line of a script, +followed by any global declarations, then a blank newline prior to any imports or code. ```javascript - // bad - /* global Foo */ - /* eslint-disable no-new */ - import Bar from './bar'; + // bad + /* global Foo */ + /* eslint-disable no-new */ + import Bar from './bar'; - // good - /* eslint-disable no-new */ - /* global Foo */ + // good + /* eslint-disable no-new */ + /* global Foo */ - import Bar from './bar'; + import Bar from './bar'; ``` -- **Never** disable the `no-undef` rule. Declare globals with `/* global Foo */` instead. +1. **Never** disable the `no-undef` rule. Declare globals with `/* global Foo */` instead. + +1. When declaring multiple globals, always use one `/* global [name] */` line per variable. + ```javascript + // bad + /* globals Flash, Cookies, jQuery */ -- When declaring multiple globals, always use one `/* global [name] */` line per variable. + // good + /* global Flash */ + /* global Cookies */ + /* global jQuery */ + ``` +1. Use up to 3 parameters for a function or class. If you need more accept an Object instead. ```javascript - // bad - /* globals Flash, Cookies, jQuery */ + // bad + fn(p1, p2, p3, p4) {} - // good - /* global Flash */ - /* global Cookies */ - /* global jQuery */ + // good + fn(options) {} ``` #### Modules, Imports, and Exports -- Use ES module syntax to import modules - +1. Use ES module syntax to import modules ```javascript - // bad - require('foo'); + // bad + require('foo'); - // good - import Foo from 'foo'; + // good + import Foo from 'foo'; - // bad - module.exports = Foo; + // bad + module.exports = Foo; - // good - export default Foo; + // good + export default Foo; ``` -- Relative paths - - Unless you are writing a test, always reference other scripts using relative paths instead of `~` +1. Relative paths: Unless you are writing a test, always reference other scripts using +relative paths instead of `~` + * In **app/assets/javascripts**: - In **app/assets/javascripts**: - ```javascript - // bad - import Foo from '~/foo' - - // good - import Foo from '../foo'; - ``` + ```javascript + // bad + import Foo from '~/foo' - In **spec/javascripts**: - ```javascript - // bad - import Foo from '../../app/assets/javascripts/foo' + // good + import Foo from '../foo'; + ``` + * In **spec/javascripts**: - // good - import Foo from '~/foo'; - ``` + ```javascript + // bad + import Foo from '../../app/assets/javascripts/foo' -- Avoid using IIFE. Although we have a lot of examples of files which wrap their contents in IIFEs (immediately-invoked function expressions), this is no longer necessary after the transition from Sprockets to webpack. Do not use them anymore and feel free to remove them when refactoring legacy code. + // good + import Foo from '~/foo'; + ``` -- Avoid adding to the global namespace. +1. Avoid using IIFE. Although we have a lot of examples of files which wrap their +contents in IIFEs (immediately-invoked function expressions), +this is no longer necessary after the transition from Sprockets to webpack. +Do not use them anymore and feel free to remove them when refactoring legacy code. +1. Avoid adding to the global namespace. ```javascript - // bad - window.MyClass = class { /* ... */ }; + // bad + window.MyClass = class { /* ... */ }; - // good - export default class MyClass { /* ... */ } + // good + export default class MyClass { /* ... */ } ``` -- Side effects are forbidden in any script which contains exports - +1. Side effects are forbidden in any script which contains exports ```javascript - // bad - export default class MyClass { /* ... */ } + // bad + export default class MyClass { /* ... */ } - document.addEventListener("DOMContentLoaded", function(event) { - new MyClass(); - } + document.addEventListener("DOMContentLoaded", function(event) { + new MyClass(); + } ``` #### Data Mutation and Pure functions -- Strive to write many small pure functions, and minimize where mutations occur. - +1. Strive to write many small pure functions, and minimize where mutations occur. ```javascript - // bad - const values = {foo: 1}; + // bad + const values = {foo: 1}; - function impureFunction(items) { - const bar = 1; + function impureFunction(items) { + const bar = 1; - items.foo = items.a * bar + 2; + items.foo = items.a * bar + 2; - return items.a; - } + return items.a; + } - const c = impureFunction(values); + const c = impureFunction(values); - // good - var values = {foo: 1}; + // good + var values = {foo: 1}; - function pureFunction (foo) { - var bar = 1; + function pureFunction (foo) { + var bar = 1; - foo = foo * bar + 2; + foo = foo * bar + 2; - return foo; - } + return foo; + } - var c = pureFunction(values.foo); + var c = pureFunction(values.foo); ``` -- Avoid constructors with side-effects +1. Avoid constructors with side-effects +1. Prefer `.map`, `.reduce` or `.filter` over `.forEach` +A forEach will cause side effects, it will be mutating the array being iterated. Prefer using `.map`, +`.reduce` or `.filter` + ```javascript + const users = [ { name: 'Foo' }, { name: 'Bar' } ]; -#### Parse Strings into Numbers -- `parseInt()` is preferable over `Number()` or `+` + // bad + users.forEach((user, index) => { + user.id = index; + }); + // good + const usersWithId = users.map((user, index) => { + return Object.assign({}, user, { id: index }); + }); + ``` + +#### Parse Strings into Numbers +1. `parseInt()` is preferable over `Number()` or `+` ```javascript - // bad - +'10' // 10 + // bad + +'10' // 10 - // good - Number('10') // 10 + // good + Number('10') // 10 - // better - parseInt('10', 10); + // better + parseInt('10', 10); ``` +#### CSS classes used for JavaScript +1. If the class is being used in Javascript it needs to be prepend with `js-` + ```html + // bad + <button class="add-user"> + Add User + </button> + + // good + <button class="js-add-user"> + Add User + </button> + ``` ### Vue.js - #### Basic Rules -- Only include one Vue.js component per file. -- Export components as plain objects: - +1. The service has it's own file +1. The store has it's own file +1. Use a function in the bundle file to instantiate the Vue component: ```javascript - export default { - template: `<h1>I'm a component</h1> - } + // bad + class { + init() { + new Component({}) + } + } + + // good + document.addEventListener('DOMContentLoaded', () => new Vue({ + el: '#element', + components: { + componentName + }, + render: createElement => createElement('component-name'), + })); ``` -#### Naming -- **Extensions**: Use `.vue` extension for Vue components. -- **Reference Naming**: Use PascalCase for Vue components and camelCase for their instances: +1. Don not use a singleton for the service or the store ```javascript - // bad - import cardBoard from 'cardBoard'; + // bad + class Store { + constructor() { + if (!this.prototype.singleton) { + // do something + } + } + } - // good - import CardBoard from 'cardBoard' + // good + class Store { + constructor() { + // do something + } + } + ``` - // bad - components: { - CardBoard: CardBoard - }; +#### Naming +1. **Extensions**: Use `.vue` extension for Vue components. +1. **Reference Naming**: Use camelCase for their instances: + ```javascript + // good + import cardBoard from 'cardBoard' - // good - components: { - cardBoard: CardBoard - }; + components: { + cardBoard: + }; ``` -- **Props Naming:** -- Avoid using DOM component prop names. -- Use kebab-case instead of camelCase to provide props in templates. +1. **Props Naming:** Avoid using DOM component prop names. +1. **Props Naming:** Use kebab-case instead of camelCase to provide props in templates. ```javascript - // bad - <component class="btn"> + // bad + <component class="btn"> - // good - <component css-class="btn"> + // good + <component css-class="btn"> - // bad - <component myProp="prop" /> + // bad + <component myProp="prop" /> - // good - <component my-prop="prop" /> -``` + // good + <component my-prop="prop" /> + ``` #### Alignment -- Follow these alignment styles for the template method: - +1. Follow these alignment styles for the template method: ```javascript - // bad - <component v-if="bar" - param="baz" /> - - // good - <component - v-if="bar" - param="baz" - /> - - // if props fit in one line then keep it on the same line - <component bar="bar" /> + // bad + <component v-if="bar" + param="baz" /> + + <button class="btn">Click me</button> + + // good + <component + v-if="bar" + param="baz" + /> + + <button class="btn"> + Click me + </button> + + // if props fit in one line then keep it on the same line + <component bar="bar" /> ``` #### Quotes -- Always use double quotes `"` inside templates and single quotes `'` for all other JS. - +1. Always use double quotes `"` inside templates and single quotes `'` for all other JS. ```javascript - // bad - template: ` - <button :class='style'>Button</button> - ` - - // good - template: ` - <button :class="style">Button</button> - ` + // bad + template: ` + <button :class='style'>Button</button> + ` + + // good + template: ` + <button :class="style">Button</button> + ` ``` #### Props -- Props should be declared as an object - +1. Props should be declared as an object ```javascript - // bad - props: ['foo'] - - // good - props: { - foo: { - type: String, - required: false, - default: 'bar' + // bad + props: ['foo'] + + // good + props: { + foo: { + type: String, + required: false, + default: 'bar' + } } - } ``` -- Required key should always be provided when declaring a prop - +1. Required key should always be provided when declaring a prop ```javascript - // bad - props: { - foo: { - type: String, + // bad + props: { + foo: { + type: String, + } } - } - - // good - props: { - foo: { - type: String, - required: false, - default: 'bar' + + // good + props: { + foo: { + type: String, + required: false, + default: 'bar' + } } - } ``` -- Default key should always be provided if the prop is not required: - +1. Default key should always be provided if the prop is not required: ```javascript - // bad - props: { - foo: { - type: String, - required: false, + // bad + props: { + foo: { + type: String, + required: false, + } } - } - - // good - props: { - foo: { - type: String, - required: false, - default: 'bar' + + // good + props: { + foo: { + type: String, + required: false, + default: 'bar' + } } - } - // good - props: { - foo: { - type: String, - required: true + // good + props: { + foo: { + type: String, + required: true + } } - } ``` #### Data -- `data` method should always be a function +1. `data` method should always be a function ```javascript - // bad - data: { - foo: 'foo' - } - - // good - data() { - return { + // bad + data: { foo: 'foo' - }; - } + } + + // good + data() { + return { + foo: 'foo' + }; + } ``` #### Directives -- Shorthand `@` is preferable over `v-on` - +1. Shorthand `@` is preferable over `v-on` ```javascript - // bad - <component v-on:click="eventHandler"/> + // bad + <component v-on:click="eventHandler"/> - // good - <component @click="eventHandler"/> + // good + <component @click="eventHandler"/> ``` -- Shorthand `:` is preferable over `v-bind` - +1. Shorthand `:` is preferable over `v-bind` ```javascript - // bad - <component v-bind:class="btn"/> + // bad + <component v-bind:class="btn"/> - // good - <component :class="btn"/> + // good + <component :class="btn"/> ``` #### Closing tags -- Prefer self closing component tags - +1. Prefer self closing component tags ```javascript - // bad - <component></component> + // bad + <component></component> - // good - <component /> + // good + <component /> ``` #### Ordering -- Order for a Vue Component: +1. Order for a Vue Component: 1. `name` - 2. `props` - 3. `data` - 4. `components` - 5. `computedProps` - 6. `methods` - 7. lifecycle methods - 1. `beforeCreate` - 2. `created` - 3. `beforeMount` - 4. `mounted` - 5. `beforeUpdate` - 6. `updated` - 7. `activated` - 8. `deactivated` - 9. `beforeDestroy` - 10. `destroyed` - 8. `template` + 1. `props` + 1. `mixins` + 1. `data` + 1. `components` + 1. `computedProps` + 1. `methods` + 1. `beforeCreate` + 1. `created` + 1. `beforeMount` + 1. `mounted` + 1. `beforeUpdate` + 1. `updated` + 1. `activated` + 1. `deactivated` + 1. `beforeDestroy` + 1. `destroyed` + +#### Vue and Boostrap +1. Tooltips: Do not rely on `has-tooltip` class name for vue components + ``` javascript + // bad + <span class="has-tooltip"> + Text + </span> + + // good + <span data-toggle="tooltip"> + Text + </span> + ``` + +1. Tooltips: When using a tooltip, include the tooltip mixin + +1. Don't change `data-original-title`. + ```javascript + // bad + <span data-original-title="tooltip text">Foo</span> + + // good + <span title="tooltip text">Foo</span> + + $('span').tooltip('fixTitle'); + ``` ## SCSS @@ -413,3 +495,5 @@ See [our current .eslintrc][eslintrc] for specific rules and patterns. [airbnb-js-style-guide]: https://github.com/airbnb/javascript [eslintrc]: https://gitlab.com/gitlab-org/gitlab-ce/blob/master/.eslintrc +[eslint-this]: http://eslint.org/docs/rules/class-methods-use-this +[eslint-new]: http://eslint.org/docs/rules/no-new diff --git a/doc/development/fe_guide/testing.md b/doc/development/fe_guide/testing.md index 8d3513d3566..0ef9fc61a61 100644 --- a/doc/development/fe_guide/testing.md +++ b/doc/development/fe_guide/testing.md @@ -1,11 +1,20 @@ # Frontend Testing -There are two types of tests you'll encounter while developing frontend code -at GitLab. We use Karma and Jasmine for JavaScript unit testing, and RSpec -feature tests with Capybara for integration testing. +There are two types of test suites you'll encounter while developing frontend code +at GitLab. We use Karma and Jasmine for JavaScript unit and integration testing, and RSpec +feature tests with Capybara for e2e (end-to-end) integration testing. -Feature tests need to be written for all new features. Regression tests ought -to be written for all bug fixes to prevent them from recurring in the future. +Unit and feature tests need to be written for all new features. +Most of the time, you should use rspec for your feature tests. +There are cases where the behaviour you are testing is not worth the time spent running the full application, +for example, if you are testing styling, animation or small actions that don't involve the backend, +you should write an integration test using Jasmine. + + + +_This diagram demonstrates the relative priority of each test type we use_ + +Regression tests should be written for bug fixes to prevent them from recurring in the future. See [the Testing Standards and Style Guidelines](../testing.md) for more information on general testing practices at GitLab. @@ -13,9 +22,126 @@ for more information on general testing practices at GitLab. ## Karma test suite GitLab uses the [Karma][karma] test runner with [Jasmine][jasmine] as its test -framework for our JavaScript unit tests. For tests that rely on DOM -manipulation we use fixtures which are pre-compiled from HAML source files and -served during testing by the [jasmine-jquery][jasmine-jquery] plugin. +framework for our JavaScript unit and integration tests. For integration tests, +we generate HTML files using RSpec (see `spec/javascripts/fixtures/*.rb` for examples). +Some fixtures are still HAML templates that are translated to HTML files using the same mechanism (see `static_fixtures.rb`). +Adding these static fixtures should be avoided as they are harder to keep up to date with real views. +The existing static fixtures will be migrated over time. +Please see [gitlab-org/gitlab-ce#24753](https://gitlab.com/gitlab-org/gitlab-ce/issues/24753) to track our progress. +Fixtures are served during testing by the [jasmine-jquery][jasmine-jquery] plugin. + +JavaScript tests live in `spec/javascripts/`, matching the folder structure +of `app/assets/javascripts/`: `app/assets/javascripts/behaviors/autosize.js` +has a corresponding `spec/javascripts/behaviors/autosize_spec.js` file. + +Keep in mind that in a CI environment, these tests are run in a headless +browser and you will not have access to certain APIs, such as +[`Notification`](https://developer.mozilla.org/en-US/docs/Web/API/notification), +which will have to be stubbed. + +### Best practice + +#### Naming unit tests + +When writing describe test blocks to test specific functions/methods, +please use the method name as the describe block name. + +```javascript +// Good +describe('methodName', () => { + it('passes', () => { + expect(true).toEqual(true); + }); +}); + +// Bad +describe('#methodName', () => { + it('passes', () => { + expect(true).toEqual(true); + }); +}); + +// Bad +describe('.methodName', () => { + it('passes', () => { + expect(true).toEqual(true); + }); +}); +``` +#### Testing Promises + +When testing Promises you should always make sure that the test is asynchronous and rejections are handled. +Your Promise chain should therefore end with a call of the `done` callback and `done.fail` in case an error occurred. + +```javascript +// Good +it('tests a promise', (done) => { + promise + .then((data) => { + expect(data).toBe(asExpected); + }) + .then(done) + .catch(done.fail); +}); + +// Good +it('tests a promise rejection', (done) => { + promise + .then(done.fail) + .catch((error) => { + expect(error).toBe(expectedError); + }) + .then(done) + .catch(done.fail); +}); + +// Bad (missing done callback) +it('tests a promise', () => { + promise + .then((data) => { + expect(data).toBe(asExpected); + }) +}); + +// Bad (missing catch) +it('tests a promise', (done) => { + promise + .then((data) => { + expect(data).toBe(asExpected); + }) + .then(done) +}); + +// Bad (use done.fail in asynchronous tests) +it('tests a promise', (done) => { + promise + .then((data) => { + expect(data).toBe(asExpected); + }) + .then(done) + .catch(fail) +}); + +// Bad (missing catch) +it('tests a promise rejection', (done) => { + promise + .catch((error) => { + expect(error).toBe(expectedError); + }) + .then(done) +}); +``` + +#### Stubbing + +For unit tests, you should stub methods that are unrelated to the current unit you are testing. +If you need to use a prototype method, instantiate an instance of the class and call it there instead of mocking the instance completely. + +For integration tests, you should stub methods that will effect the stability of the test if they +execute their original behaviour. i.e. Network requests. + +### Vue.js unit tests +See this [section][vue-test]. ### Running frontend tests @@ -80,24 +206,23 @@ If an integration test depends on JavaScript to run correctly, you need to make sure the spec is configured to enable JavaScript when the tests are run. If you don't do this you'll see vague error messages from the spec runner. -To enable a JavaScript driver in an `rspec` test, add `js: true` to the +To enable a JavaScript driver in an `rspec` test, add `:js` to the individual spec or the context block containing multiple specs that need JavaScript enabled: ```ruby - # For one spec -it 'presents information about abuse report', js: true do - # assertions... +it 'presents information about abuse report', :js do + # assertions... end -describe "Admin::AbuseReports", js: true do - it 'presents information about abuse report' do - # assertions... - end - it 'shows buttons for adding to abuse report' do - # assertions... - end +describe "Admin::AbuseReports", :js do + it 'presents information about abuse report' do + # assertions... + end + it 'shows buttons for adding to abuse report' do + # assertions... + end end ``` @@ -113,13 +238,12 @@ file for the failing spec, add the `@javascript` flag above the Scenario: ``` @javascript Scenario: Developer can approve merge request - Given I am a "Shop" developer - And I visit project "Shop" merge requests page - And merge request 'Bug NS-04' must be approved - And I click link "Bug NS-04" - When I click link "Approve" - Then I should see approved merge request "Bug NS-04" - + Given I am a "Shop" developer + And I visit project "Shop" merge requests page + And merge request 'Bug NS-04' must be approved + And I click link "Bug NS-04" + When I click link "Approve" + Then I should see approved merge request "Bug NS-04" ``` [capybara]: http://teamcapybara.github.io/capybara/ @@ -127,3 +251,4 @@ Scenario: Developer can approve merge request [jasmine-focus]: https://jasmine.github.io/2.5/focused_specs.html [jasmine-jquery]: https://github.com/velesin/jasmine-jquery [karma]: http://karma-runner.github.io/ +[vue-test]:https://docs.gitlab.com/ce/development/fe_guide/vue.html#testing-vue-components diff --git a/doc/development/fe_guide/vue.md b/doc/development/fe_guide/vue.md index 3e3406e7d6a..a984bb6c94c 100644 --- a/doc/development/fe_guide/vue.md +++ b/doc/development/fe_guide/vue.md @@ -19,13 +19,31 @@ We don't want to refactor all GitLab frontend code into Vue.js, here are some gu when not to use Vue.js: - Adding or changing static information; -- Features that highly depend on jQuery will be hard to work with Vue.js +- Features that highly depend on jQuery will be hard to work with Vue.js; +- Features without reactive data; As always, the Frontend Architectural Experts are available to help with any Vue or JavaScript questions. -## How to build a new feature with Vue.js +## Vue architecture -**Components, Stores and Services** +All new features built with Vue.js must follow a [Flux architecture][flux]. +The main goal we are trying to achieve is to have only one data flow and only one data entry. +In order to achieve this goal, each Vue bundle needs a Store - where we keep all the data -, +a Service - that we use to communicate with the server - and a main Vue component. + +Think of the Main Vue Component as the entry point of your application. This is the only smart +component that should exist in each Vue feature. +This component is responsible for: +1. Calling the Service to get data from the server +1. Calling the Store to store the data received +1. Mounting all the other components + +  + +You can also read about this architecture in vue docs about [state management][state-management] +and about [one way data flow][one-way-data-flow]. + +### Components, Stores and Services In some features implemented with Vue.js, like the [issue board][issue-boards] or [environments table][environments-table] @@ -46,16 +64,17 @@ _For consistency purposes, we recommend you to follow the same structure._ Let's look into each of them: -**A `*_bundle.js` file** +### A `*_bundle.js` file This is the index file of your new feature. This is where the root Vue instance of the new feature should be. -The Store and the Service should be imported and initialized in this file and provided as a prop to the main component. +The Store and the Service should be imported and initialized in this file and +provided as a prop to the main component. Don't forget to follow [these steps.][page_specific_javascript] -**A folder for Components** +### A folder for Components This folder holds all components that are specific of this new feature. If you need to use or create a component that will probably be used somewhere @@ -70,20 +89,219 @@ in one table would not be a good use of this pattern. You can read more about components in Vue.js site, [Component System][component-system] -**A folder for the Store** +### A folder for the Store The Store is a class that allows us to manage the state in a single -source of truth. +source of truth. It is not aware of the service or the components. The concept we are trying to follow is better explained by Vue documentation itself, please read this guide: [State Management][state-management] -**A folder for the Service** +### A folder for the Service + +The Service is a class used only to communicate with the server. +It does not store or manipulate any data. It is not aware of the store or the components. +We use [vue-resource][vue-resource-repo] to communicate with the server. + +Vue Resource should only be imported in the service file. + + ```javascript + import Vue from 'vue'; + import VueResource from 'vue-resource'; + + Vue.use(VueResource); + ``` + +### CSRF token +We use a Vue Resource interceptor to manage the CSRF token. +`app/assets/javascripts/vue_shared/vue_resource_interceptor.js` holds all our common interceptors. +Note: You don't need to load `app/assets/javascripts/vue_shared/vue_resource_interceptor.js` +since it's already being loaded by `common_vue.js`. + +### End Result + +The following example shows an application: + +```javascript +// store.js +export default class Store { + + /** + * This is where we will iniatialize the state of our data. + * Usually in a small SPA you don't need any options when starting the store. In the case you do + * need guarantee it's an Object and it's documented. + * + * @param {Object} options + */ + constructor(options) { + this.options = options; + + // Create a state object to handle all our data in the same place + this.todos = []: + } + + setTodos(todos = []) { + this.todos = todos; + } + + addTodo(todo) { + this.todos.push(todo); + } + + removeTodo(todoID) { + const state = this.todos; + + const newState = state.filter((element) => {element.id !== todoID}); + + this.todos = newState; + } +} + +// service.js +import Vue from 'vue'; +import VueResource from 'vue-resource'; +import 'vue_shared/vue_resource_interceptor'; + +Vue.use(VueResource); + +export default class Service { + constructor(options) { + this.todos = Vue.resource(endpoint.todosEndpoint); + } + + getTodos() { + return this.todos.get(); + } + + addTodo(todo) { + return this.todos.put(todo); + } +} +// todo_component.vue +<script> +export default { + props: { + data: { + type: Object, + required: true, + }, + } +} +</script> +<template> + <div> + <h1> + Title: {{data.title}} + </h1> + <p> + {{data.text}} + </p> + </div> +</template> + +// todos_main_component.vue +<script> +import Store from 'store'; +import Service from 'service'; +import TodoComponent from 'todoComponent'; +export default { + /** + * Although most data belongs in the store, each component it's own state. + * We want to show a loading spinner while we are fetching the todos, this state belong + * in the component. + * + * We need to access the store methods through all methods of our component. + * We need to access the state of our store. + */ + data() { + const store = new Store(); + + return { + isLoading: false, + store: store, + todos: store.todos, + }; + }, + + components: { + todo: TodoComponent, + }, + + created() { + this.service = new Service('todos'); + + this.getTodos(); + }, + + methods: { + getTodos() { + this.isLoading = true; + + this.service.getTodos() + .then(response => response.json()) + .then((response) => { + this.store.setTodos(response); + this.isLoading = false; + }) + .catch(() => { + this.isLoading = false; + // Show an error + }); + }, + + addTodo(todo) { + this.service.addTodo(todo) + then(response => response.json()) + .then((response) => { + this.store.addTodo(response); + }) + .catch(() => { + // Show an error + }); + } + } +} +</script> +<template> + <div class="container"> + <div v-if="isLoading"> + <i + class="fa fa-spin fa-spinner" + aria-hidden="true" /> + </div> + + <div + v-if="!isLoading" + class="js-todo-list"> + <template v-for='todo in todos'> + <todo :data="todo" /> + </template> + + <button + @click="addTodo" + class="js-add-todo"> + Add Todo + </button> + </div> + <div> +</template> + +// bundle.js +import todoComponent from 'todos_main_component.vue'; + +new Vue({ + el: '.js-todo-app', + components: { + todoComponent, + }, + render: createElement => createElement('todo-component' { + props: { + someProp: [], + } + }), +}); -The Service is used only to communicate with the server. -It does not store or manipulate any data. -We use [vue-resource][vue-resource-repo] to -communicate with the server. +``` The [issue boards service][issue-boards-service] is a good example of this pattern. @@ -93,6 +311,128 @@ is a good example of this pattern. Please refer to the Vue section of our [style guide](style_guide_js.md#vuejs) for best practices while writing your Vue components and templates. +## Testing Vue Components + +Each Vue component has a unique output. This output is always present in the render function. + +Although we can test each method of a Vue component individually, our goal must be to test the output +of the render/template function, which represents the state at all times. + +Make use of Vue Resource Interceptors to mock data returned by the service. + +Here's how we would test the Todo App above: + +```javascript +import component from 'todos_main_component'; + +describe('Todos App', () => { + it('should render the loading state while the request is being made', () => { + const Component = Vue.extend(component); + + const vm = new Component().$mount(); + + expect(vm.$el.querySelector('i.fa-spin')).toBeDefined(); + }); + + describe('with data', () => { + // Mock the service to return data + const interceptor = (request, next) => { + next(request.respondWith(JSON.stringify([{ + title: 'This is a todo', + body: 'This is the text' + }]), { + status: 200, + })); + }; + + let vm; + + beforeEach(() => { + Vue.http.interceptors.push(interceptor); + + const Component = Vue.extend(component); + + vm = new Component().$mount(); + }); + + afterEach(() => { + Vue.http.interceptors = _.without(Vue.http.interceptors, interceptor); + }); + + + it('should render todos', (done) => { + setTimeout(() => { + expect(vm.$el.querySelectorAll('.js-todo-list div').length).toBe(1); + done(); + }, 0); + }); + }); + + describe('add todo', () => { + let vm; + beforeEach(() => { + const Component = Vue.extend(component); + vm = new Component().$mount(); + }); + it('should add a todos', (done) => { + setTimeout(() => { + vm.$el.querySelector('.js-add-todo').click(); + + // Add a new interceptor to mock the add Todo request + Vue.nextTick(() => { + expect(vm.$el.querySelectorAll('.js-todo-list div').length).toBe(2); + }); + }, 0); + }); + }); +}); +``` +#### Test the component's output +The main return value of a Vue component is the rendered output. In order to test the component we +need to test the rendered output. [Vue][vue-test] guide's to unit test show us exactly that: + + +### Stubbing API responses +[Vue Resource Interceptors][vue-resource-interceptor] allow us to add a interceptor with +the response we need: + +```javascript + // Mock the service to return data + const interceptor = (request, next) => { + next(request.respondWith(JSON.stringify([{ + title: 'This is a todo', + body: 'This is the text' + }]), { + status: 200, + })); + }; + + beforeEach(() => { + Vue.http.interceptors.push(interceptor); + }); + + afterEach(() => { + Vue.http.interceptors = _.without(Vue.http.interceptors, interceptor); + }); + + it('should do something', (done) => { + setTimeout(() => { + // Test received data + done(); + }, 0); + }); +``` + +1. Use `$.mount()` to mount the component +```javascript + // bad + new Component({ + el: document.createElement('div') + }); + + // good + new Component().$mount(); +``` [vue-docs]: http://vuejs.org/guide/index.html [issue-boards]: https://gitlab.com/gitlab-org/gitlab-ce/tree/master/app/assets/javascripts/boards @@ -100,5 +440,9 @@ for best practices while writing your Vue components and templates. [page_specific_javascript]: https://docs.gitlab.com/ce/development/frontend.html#page-specific-javascript [component-system]: https://vuejs.org/v2/guide/#Composing-with-Components [state-management]: https://vuejs.org/v2/guide/state-management.html#Simple-State-Management-from-Scratch +[one-way-data-flow]: https://vuejs.org/v2/guide/components.html#One-Way-Data-Flow [vue-resource-repo]: https://github.com/pagekit/vue-resource +[vue-resource-interceptor]: https://github.com/pagekit/vue-resource/blob/develop/docs/http.md#interceptors +[vue-test]: https://vuejs.org/v2/guide/unit-testing.html [issue-boards-service]: https://gitlab.com/gitlab-org/gitlab-ce/blob/master/app/assets/javascripts/boards/services/board_service.js.es6 +[flux]: https://facebook.github.io/flux diff --git a/doc/development/foreign_keys.md b/doc/development/foreign_keys.md new file mode 100644 index 00000000000..0ab0deb156f --- /dev/null +++ b/doc/development/foreign_keys.md @@ -0,0 +1,63 @@ +# Foreign Keys & Associations + +When adding an association to a model you must also add a foreign key. For +example, say you have the following model: + +```ruby +class User < ActiveRecord::Base + has_many :posts +end +``` + +Here you will need to add a foreign key on column `posts.user_id`. This ensures +that data consistency is enforced on database level. Foreign keys also mean that +the database can very quickly remove associated data (e.g. when removing a +user), instead of Rails having to do this. + +## Adding Foreign Keys In Migrations + +Foreign keys can be added concurrently using `add_concurrent_foreign_key` as +defined in `Gitlab::Database::MigrationHelpers`. See the [Migration Style +Guide](migration_style_guide.md) for more information. + +Keep in mind that you can only safely add foreign keys to existing tables after +you have removed any orphaned rows. The method `add_concurrent_foreign_key` +does not take care of this so you'll need to do so manually. + +## Cascading Deletes + +Every foreign key must define an `ON DELETE` clause, and in 99% of the cases +this should be set to `CASCADE`. + +## Indexes + +When adding a foreign key in PostgreSQL the column is not indexed automatically, +thus you must also add a concurrent index. Not doing so will result in cascading +deletes being very slow. + +## Dependent Removals + +Don't define options such as `dependent: :destroy` or `dependent: :delete` when +defining an association. Defining these options means Rails will handle the +removal of data, instead of letting the database handle this in the most +efficient way possible. + +In other words, this is bad and should be avoided at all costs: + +```ruby +class User < ActiveRecord::Base + has_many :posts, dependent: :destroy +end +``` + +Should you truly have a need for this it should be approved by a database +specialist first. + +You should also not define any `before_destroy` or `after_destroy` callbacks on +your models _unless_ absolutely required and only when approved by database +specialists. For example, if each row in a table has a corresponding file on a +file system it may be tempting to add a `after_destroy` hook. This however +introduces non database logic to a model, and means we can no longer rely on +foreign keys to remove the data as this would result in the filesystem data +being left behind. In such a case you should use a service class instead that +takes care of removing non database data. diff --git a/doc/development/i18n_guide.md b/doc/development/i18n_guide.md new file mode 100644 index 00000000000..44eca68aaca --- /dev/null +++ b/doc/development/i18n_guide.md @@ -0,0 +1,239 @@ +# Internationalization for GitLab + +> [Introduced](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/10669) in GitLab 9.2. + +For working with internationalization (i18n) we use +[GNU gettext](https://www.gnu.org/software/gettext/) given it's the most used +tool for this task and we have a lot of applications that will help us to work +with it. + +## Tools + +We use a couple of gems: + +1. [`gettext_i18n_rails`](https://github.com/grosser/gettext_i18n_rails): this + gem allow us to translate content from models, views and controllers. Also + it gives us access to the following raketasks: + - `rake gettext:find`: Parses almost all the files from the + Rails application looking for content that has been marked for + translation. Finally, it updates the PO files with the new content that + it has found. + - `rake gettext:pack`: Processes the PO files and generates the + MO files that are binary and are finally used by the application. + +1. [`gettext_i18n_rails_js`](https://github.com/webhippie/gettext_i18n_rails_js): + this gem is useful to make the translations available in JavaScript. It + provides the following raketask: + - `rake gettext:po_to_json`: Reads the contents from the PO files and + generates JSON files containing all the available translations. + +1. PO editor: there are multiple applications that can help us to work with PO + files, a good option is [Poedit](https://poedit.net/download) which is + available for macOS, GNU/Linux and Windows. + +## Preparing a page for translation + +We basically have 4 types of files: + +1. Ruby files: basically Models and Controllers. +1. HAML files: these are the view files. +1. ERB files: used for email templates. +1. JavaScript files: we mostly need to work with VUE JS templates. + +### Ruby files + +If there is a method or variable that works with a raw string, for instance: + +```ruby +def hello + "Hello world!" +end +``` + +Or: + +```ruby +hello = "Hello world!" +``` + +You can easily mark that content for translation with: + +```ruby +def hello + _("Hello world!") +end +``` + +Or: + +```ruby +hello = _("Hello world!") +``` + +### HAML files + +Given the following content in HAML: + +```haml +%h1 Hello world! +``` + +You can mark that content for translation with: + +```haml +%h1= _("Hello world!") +``` + +### ERB files + +Given the following content in ERB: + +```erb +<h1>Hello world!</h1> +``` + +You can mark that content for translation with: + +```erb +<h1><%= _("Hello world!") %></h1> +``` + +### JavaScript files + +In JavaScript we added the `__()` (double underscore parenthesis) function +for translations. + +### Updating the PO files with the new content + +Now that the new content is marked for translation, we need to update the PO +files with the following command: + +```sh +bundle exec rake gettext:find +``` + +This command will update the `locale/**/gitlab.edit.po` file with the +new content that the parser has found. + +New translations will be added with their default content and will be marked +fuzzy. To use the translation, look for the `#, fuzzy` mention in `gitlab.edit.po` +and remove it. + +Translations that aren't used in the source code anymore will be marked with +`~#`; these can be removed to keep our translation files clutter-free. + +## Working with special content + +### Interpolation + +- In Ruby/HAML: + + ```ruby + _("Hello %{name}") % { name: 'Joe' } + ``` + +- In JavaScript: Not supported at this moment. + +### Plurals + +- In Ruby/HAML: + + ```ruby + n_('Apple', 'Apples', 3) => 'Apples' + ``` + + Using interpolation: + ```ruby + n_("There is a mouse.", "There are %d mice.", size) % size + ``` + +- In JavaScript: + + ```js + n__('Apple', 'Apples', 3) => 'Apples' + ``` + + Using interpolation: + + ```js + n__('Last day', 'Last %d days', 30) => 'Last 30 days' + ``` + +### Namespaces + +Sometimes you need to add some context to the text that you want to translate +(if the word occurs in a sentence and/or the word is ambiguous). + +- In Ruby/HAML: + + ```ruby + s_('OpenedNDaysAgo|Opened') + ``` + + In case the translation is not found it will return `Opened`. + +- In JavaScript: + + ```js + s__('OpenedNDaysAgo|Opened') + ``` + +### Just marking content for parsing + +Sometimes there are some dynamic translations that can't be found by the +parser when running `bundle exec rake gettext:find`. For these scenarios you can +use the [`_N` method](https://github.com/grosser/gettext_i18n_rails/blob/c09e38d481e0899ca7d3fc01786834fa8e7aab97/Readme.md#unfound-translations-with-rake-gettextfind). + +There is also and alternative method to [translate messages from validation errors](https://github.com/grosser/gettext_i18n_rails/blob/c09e38d481e0899ca7d3fc01786834fa8e7aab97/Readme.md#option-a). + +## Adding a new language + +Let's suppose you want to add translations for a new language, let's say French. + +1. The first step is to register the new language in `lib/gitlab/i18n.rb`: + + ```ruby + ... + AVAILABLE_LANGUAGES = { + ..., + 'fr' => 'Français' + }.freeze + ... + ``` + +1. Next, you need to add the language: + + ```sh + bundle exec rake gettext:add_language[fr] + ``` + + If you want to add a new language for a specific region, the command is similar, + you just need to separate the region with an underscore (`_`). For example: + + ```sh + bundle exec rake gettext:add_language[en_gb] + ``` + +1. Now that the language is added, a new directory has been created under the + path: `locale/fr/`. You can now start using your PO editor to edit the PO file + located in: `locale/fr/gitlab.edit.po`. + +1. After you're done updating the translations, you need to process the PO files + in order to generate the binary MO files and finally update the JSON files + containing the translations: + + ```sh + bundle exec rake gettext:pack + bundle exec rake gettext:po_to_json + ``` + +1. In order to see the translated content we need to change our preferred language + which can be found under the user's **Settings** (`/profile`). + +1. After checking that the changes are ok, you can proceed to commit the new files. + For example: + + ```sh + git add locale/fr/ app/assets/javascripts/locale/fr/ + git commit -m "Add French translations for Cycle Analytics page" + ``` diff --git a/doc/development/img/cache-hit.svg b/doc/development/img/cache-hit.svg new file mode 100644 index 00000000000..1c37693df2d --- /dev/null +++ b/doc/development/img/cache-hit.svg @@ -0,0 +1,21 @@ +<svg version="1.1" id="mscgen_js-svg-__svg" class="mscgen_js-svg-__svg" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="976" height="310" viewBox="0 0 976 310"><desc> + +# Generated by mscgen_js - https://sverweij.github.io/mscgen_js +msc { + # options + hscale="1.5"; + + # entities + c [label="Client", textbgcolor="lime"], + rails [label="Rails", textbgcolor="cyan"], + etag [label="EtagCaching", textbgcolor="orange"], + redis [label="Redis", textbgcolor="white"]; + + # arcs + c => rails [label="GET /projects/5/pipelines"]; + rails => etag [label="GET /projects/5/pipelines"]; + etag => redis [label="read(key = 'GET <Etag>')"]; + redis => etag [label="cache hit", linecolor="green", textcolor="green"]; + |||; + etag => c [label="304 Not Modified", linecolor="blue", textcolor="blue"]; +}</desc><defs><style type="text/css">svg.mscgen_js-svg-__svg{font-family:Helvetica,sans-serif;font-size:12px;font-weight:normal;font-style:normal;text-decoration:none;background-color:white;stroke:black;stroke-width:2;color:black}.mscgen_js-svg-__svg path, .mscgen_js-svg-__svg rect{fill:none;color:black;stroke:black}.mscgen_js-svg-__svg .label-text-background{fill:white;stroke:white;stroke-width:0}.mscgen_js-svg-__svg .bglayer{fill:white;stroke:white;stroke-width:0}.mscgen_js-svg-__svg line{stroke:black}.mscgen_js-svg-__svg .return, .mscgen_js-svg-__svg .comment{stroke-dasharray:5,3}.mscgen_js-svg-__svg .inline_expression_divider{stroke-dasharray:10,5}.mscgen_js-svg-__svg text{color:inherit;stroke:none;text-anchor:middle}.mscgen_js-svg-__svg text.entity-text{text-decoration:underline}.mscgen_js-svg-__svg text.anchor-start{text-anchor:start}.mscgen_js-svg-__svg .arrow-marker{overflow:visible}.mscgen_js-svg-__svg .arrow-style{stroke-width:1}.mscgen_js-svg-__svg .arcrow, .mscgen_js-svg-__svg .arcrowomit, .mscgen_js-svg-__svg .emphasised{stroke-linecap:butt}.mscgen_js-svg-__svg .arcrowomit{stroke-dasharray:2,2;}.mscgen_js-svg-__svg .box, .mscgen_js-svg-__svg .entity{fill:white;stroke-linejoin:round}.mscgen_js-svg-__svg .inherit{stroke:inherit;color:inherit}.mscgen_js-svg-__svg .inherit-fill{fill:inherit}.mscgen_js-svg-__svg .watermark{stroke:black;color:black;fill:black;font-size:48pt;font-weight:bold;opacity:0.14}</style><marker orient="auto" id="mscgen_js-svg-__svgmethod-black" class="arrow-marker" viewBox="0 0 10 10" refX="9" refY="3" markerUnits="strokeWidth" markerWidth="10" markerHeight="10"><polygon points="1,1 9,3 1,5" class="arrow-style" stroke="black" fill="black"></polygon></marker><marker orient="auto" id="mscgen_js-svg-__svgmethod-l-black" class="arrow-marker" viewBox="0 0 10 10" refX="9" refY="3" markerUnits="strokeWidth" markerWidth="10" markerHeight="10"><polygon points="17,1 9,3 17,5" class="arrow-style" stroke="black" fill="black"></polygon></marker><marker orient="auto" id="mscgen_js-svg-__svgmethod-blue" class="arrow-marker" viewBox="0 0 10 10" refX="9" refY="3" markerUnits="strokeWidth" markerWidth="10" markerHeight="10"><polygon points="1,1 9,3 1,5" class="arrow-style" stroke="blue" fill="blue"></polygon></marker><marker orient="auto" id="mscgen_js-svg-__svgmethod-l-blue" class="arrow-marker" viewBox="0 0 10 10" refX="9" refY="3" markerUnits="strokeWidth" markerWidth="10" markerHeight="10"><polygon points="17,1 9,3 17,5" class="arrow-style" stroke="blue" fill="blue"></polygon></marker><marker orient="auto" id="mscgen_js-svg-__svgmethod-green" class="arrow-marker" viewBox="0 0 10 10" refX="9" refY="3" markerUnits="strokeWidth" markerWidth="10" markerHeight="10"><polygon points="1,1 9,3 1,5" class="arrow-style" stroke="green" fill="green"></polygon></marker><marker orient="auto" id="mscgen_js-svg-__svgmethod-l-green" class="arrow-marker" viewBox="0 0 10 10" refX="9" refY="3" markerUnits="strokeWidth" markerWidth="10" markerHeight="10"><polygon points="17,1 9,3 17,5" class="arrow-style" stroke="green" fill="green"></polygon></marker></defs><g id="mscgen_js-svg-__svg__body" transform="translate(53,3) scale(1,1)"><g id="mscgen_js-svg-__svg__background"><rect width="976" height="310" x="-53" y="-3" class="bglayer"></rect></g><g id="mscgen_js-svg-__svg__arcspanlayer"></g><g id="mscgen_js-svg-__svg__lifelinelayer"><line x1="75" y1="38" x2="75" y2="76" class="arcrow"></line><line x1="315" y1="38" x2="315" y2="76" class="arcrow"></line><line x1="555" y1="38" x2="555" y2="76" class="arcrow"></line><line x1="795" y1="38" x2="795" y2="76" class="arcrow"></line><line x1="75" y1="76" x2="75" y2="114" class="arcrow"></line><line x1="315" y1="76" x2="315" y2="114" class="arcrow"></line><line x1="555" y1="76" x2="555" y2="114" class="arcrow"></line><line x1="795" y1="76" x2="795" y2="114" class="arcrow"></line><line x1="75" y1="114" x2="75" y2="152" class="arcrow"></line><line x1="315" y1="114" x2="315" y2="152" class="arcrow"></line><line x1="555" y1="114" x2="555" y2="152" class="arcrow"></line><line x1="795" y1="114" x2="795" y2="152" class="arcrow"></line><line x1="75" y1="152" x2="75" y2="190" class="arcrow"></line><line x1="315" y1="152" x2="315" y2="190" class="arcrow"></line><line x1="555" y1="152" x2="555" y2="190" class="arcrow"></line><line x1="795" y1="152" x2="795" y2="190" class="arcrow"></line><line x1="75" y1="190" x2="75" y2="228" class="arcrow"></line><line x1="315" y1="190" x2="315" y2="228" class="arcrow"></line><line x1="555" y1="190" x2="555" y2="228" class="arcrow"></line><line x1="795" y1="190" x2="795" y2="228" class="arcrow"></line><line x1="75" y1="228" x2="75" y2="266" class="arcrow"></line><line x1="315" y1="228" x2="315" y2="266" class="arcrow"></line><line x1="555" y1="228" x2="555" y2="266" class="arcrow"></line><line x1="795" y1="228" x2="795" y2="266" class="arcrow"></line><line x1="75" y1="266" x2="75" y2="304" class="arcrow"></line><line x1="315" y1="266" x2="315" y2="304" class="arcrow"></line><line x1="555" y1="266" x2="555" y2="304" class="arcrow"></line><line x1="795" y1="266" x2="795" y2="304" class="arcrow"></line></g><g id="mscgen_js-svg-__svg__sequencelayer"><g id="mscgen_js-svg-__svgentities"><g><rect width="150" height="38" class="entity" style="fill:lime;"></rect><g><text x="75" y="22.5" class="entity-text "><tspan>Client</tspan></text></g></g><g><rect width="150" height="38" x="240" class="entity" style="fill:cyan;"></rect><g><text x="315" y="22.5" class="entity-text "><tspan>Rails</tspan></text></g></g><g><rect width="150" height="38" x="480" class="entity" style="fill:orange;"></rect><g><text x="555" y="22.5" class="entity-text "><tspan>EtagCaching</tspan></text></g></g><g><rect width="150" height="38" x="720" class="entity" style="fill:white;"></rect><g><text x="795" y="22.5" class="entity-text "><tspan>Redis</tspan></text></g></g></g><g><line x1="75" y1="95" x2="315" y2="95" class="arc directional method" style="stroke:black" marker-end="url(#mscgen_js-svg-__svgmethod-black)"></line><g><rect width="134.06" height="14" x="127.97" y="79.5" class="label-text-background"></rect><text x="195" y="90.5" class="directional-text method-text "><tspan>GET /projects/5/pipelines</tspan></text></g></g><g><line x1="315" y1="133" x2="555" y2="133" class="arc directional method" style="stroke:black" marker-end="url(#mscgen_js-svg-__svgmethod-black)"></line><g><rect width="134.06" height="14" x="367.97" y="117.5" class="label-text-background"></rect><text x="435" y="128.5" class="directional-text method-text "><tspan>GET /projects/5/pipelines</tspan></text></g></g><g><line x1="555" y1="171" x2="795" y2="171" class="arc directional method" style="stroke:black" marker-end="url(#mscgen_js-svg-__svgmethod-black)"></line><g><rect width="135.64" height="14" x="607.17" y="155.5" class="label-text-background"></rect><text x="675" y="166.5" class="directional-text method-text "><tspan>read(key = 'GET <Etag>')</tspan></text></g></g><g><line x1="795" y1="209" x2="555" y2="209" class="arc directional method" style="stroke:green" marker-end="url(#mscgen_js-svg-__svgmethod-green)"></line><g><rect width="48.02" height="14" x="650.98" y="193.5" class="label-text-background"></rect><text x="675" y="204.5" class="directional-text method-text " style="fill:green;"><tspan>cache hit</tspan></text></g></g><g></g><g><line x1="555" y1="285" x2="75" y2="285" class="arc directional method" style="stroke:blue" marker-end="url(#mscgen_js-svg-__svgmethod-blue)"></line><g><rect width="90.72" height="14" x="269.63" y="269.5" class="label-text-background"></rect><text x="315" y="280.5" class="directional-text method-text " style="fill:blue;"><tspan>304 Not Modified</tspan></text></g></g></g><g id="mscgen_js-svg-__svg__notelayer"></g><g id="mscgen_js-svg-__svg__watermark"></g></g></svg>
\ No newline at end of file diff --git a/doc/development/img/cache-miss.svg b/doc/development/img/cache-miss.svg new file mode 100644 index 00000000000..8429e6a1918 --- /dev/null +++ b/doc/development/img/cache-miss.svg @@ -0,0 +1,24 @@ +<svg version="1.1" id="mscgen_js-svg-__svg" class="mscgen_js-svg-__svg" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="976" height="386" viewBox="0 0 976 386"><desc> + +# Generated by mscgen_js - https://sverweij.github.io/mscgen_js +msc { + # options + hscale="1.5"; + + # entities + c [label="Client", textbgcolor="lime"], + rails [label="Rails", textbgcolor="cyan"], + etag [label="EtagCaching", textbgcolor="orange"], + redis [label="Redis", textbgcolor="white"]; + + # arcs + c => rails [label="GET /projects/5/pipelines"]; + rails => etag [label="GET /projects/5/pipelines"]; + etag => redis [label="read(key = 'GET <Etag>')"]; + redis => etag [label="cache miss", linecolor="red", textcolor="red"]; + |||; + etag => redis [label="write('<New Etag>')"]; + etag => rails [label="GET /projects/5/pipelines"]; + rails => c [label="JSON response w/ ETag", linecolor="blue", textcolor="blue"]; +} +</desc><defs><style type="text/css">svg.mscgen_js-svg-__svg{font-family:Helvetica,sans-serif;font-size:12px;font-weight:normal;font-style:normal;text-decoration:none;background-color:white;stroke:black;stroke-width:2}.mscgen_js-svg-__svg path, .mscgen_js-svg-__svg rect{fill:none}.mscgen_js-svg-__svg .label-text-background{fill:white;stroke:white;stroke-width:0}.mscgen_js-svg-__svg .bglayer{fill:white;stroke:white;stroke-width:0}.mscgen_js-svg-__svg line{}.mscgen_js-svg-__svg .return, .mscgen_js-svg-__svg .comment{stroke-dasharray:5,3}.mscgen_js-svg-__svg .inline_expression_divider{stroke-dasharray:10,5}.mscgen_js-svg-__svg text{color:inherit;stroke:none;text-anchor:middle}.mscgen_js-svg-__svg text.entity-text{text-decoration:underline}.mscgen_js-svg-__svg text.anchor-start{text-anchor:start}.mscgen_js-svg-__svg .arrow-marker{overflow:visible}.mscgen_js-svg-__svg .arrow-style{stroke-width:1}.mscgen_js-svg-__svg .arcrow, .mscgen_js-svg-__svg .arcrowomit, .mscgen_js-svg-__svg .emphasised{stroke-linecap:butt}.mscgen_js-svg-__svg .arcrowomit{stroke-dasharray:2,2}.mscgen_js-svg-__svg .box, .mscgen_js-svg-__svg .entity{fill:white;stroke-linejoin:round}.mscgen_js-svg-__svg .inherit{stroke:inherit;color:inherit}.mscgen_js-svg-__svg .inherit-fill{fill:inherit}.mscgen_js-svg-__svg .watermark{font-size:48pt;font-weight:bold;opacity:0.14}</style><marker orient="auto" id="mscgen_js-svg-__svgmethod-black" class="arrow-marker" viewBox="0 0 10 10" refX="9" refY="3" markerUnits="strokeWidth" markerWidth="10" markerHeight="10"><polygon points="1,1 9,3 1,5" class="arrow-style" stroke="black" fill="black"></polygon></marker><marker orient="auto" id="mscgen_js-svg-__svgmethod-l-black" class="arrow-marker" viewBox="0 0 10 10" refX="9" refY="3" markerUnits="strokeWidth" markerWidth="10" markerHeight="10"><polygon points="17,1 9,3 17,5" class="arrow-style" stroke="black" fill="black"></polygon></marker><marker orient="auto" id="mscgen_js-svg-__svgmethod-blue" class="arrow-marker" viewBox="0 0 10 10" refX="9" refY="3" markerUnits="strokeWidth" markerWidth="10" markerHeight="10"><polygon points="1,1 9,3 1,5" class="arrow-style" stroke="blue" fill="blue"></polygon></marker><marker orient="auto" id="mscgen_js-svg-__svgmethod-l-blue" class="arrow-marker" viewBox="0 0 10 10" refX="9" refY="3" markerUnits="strokeWidth" markerWidth="10" markerHeight="10"><polygon points="17,1 9,3 17,5" class="arrow-style" stroke="blue" fill="blue"></polygon></marker><marker orient="auto" id="mscgen_js-svg-__svgmethod-red" class="arrow-marker" viewBox="0 0 10 10" refX="9" refY="3" markerUnits="strokeWidth" markerWidth="10" markerHeight="10"><polygon points="1,1 9,3 1,5" class="arrow-style" stroke="red" fill="red"></polygon></marker><marker orient="auto" id="mscgen_js-svg-__svgmethod-l-red" class="arrow-marker" viewBox="0 0 10 10" refX="9" refY="3" markerUnits="strokeWidth" markerWidth="10" markerHeight="10"><polygon points="17,1 9,3 17,5" class="arrow-style" stroke="red" fill="red"></polygon></marker></defs><g id="mscgen_js-svg-__svg__body" transform="translate(53,3) scale(1,1)"><g id="mscgen_js-svg-__svg__background"><rect width="976" height="386" x="-53" y="-3" class="bglayer"></rect></g><g id="mscgen_js-svg-__svg__arcspanlayer"></g><g id="mscgen_js-svg-__svg__lifelinelayer"><line x1="75" y1="38" x2="75" y2="76" class="arcrow"></line><line x1="315" y1="38" x2="315" y2="76" class="arcrow"></line><line x1="555" y1="38" x2="555" y2="76" class="arcrow"></line><line x1="795" y1="38" x2="795" y2="76" class="arcrow"></line><line x1="75" y1="76" x2="75" y2="114" class="arcrow"></line><line x1="315" y1="76" x2="315" y2="114" class="arcrow"></line><line x1="555" y1="76" x2="555" y2="114" class="arcrow"></line><line x1="795" y1="76" x2="795" y2="114" class="arcrow"></line><line x1="75" y1="114" x2="75" y2="152" class="arcrow"></line><line x1="315" y1="114" x2="315" y2="152" class="arcrow"></line><line x1="555" y1="114" x2="555" y2="152" class="arcrow"></line><line x1="795" y1="114" x2="795" y2="152" class="arcrow"></line><line x1="75" y1="152" x2="75" y2="190" class="arcrow"></line><line x1="315" y1="152" x2="315" y2="190" class="arcrow"></line><line x1="555" y1="152" x2="555" y2="190" class="arcrow"></line><line x1="795" y1="152" x2="795" y2="190" class="arcrow"></line><line x1="75" y1="190" x2="75" y2="228" class="arcrow"></line><line x1="315" y1="190" x2="315" y2="228" class="arcrow"></line><line x1="555" y1="190" x2="555" y2="228" class="arcrow"></line><line x1="795" y1="190" x2="795" y2="228" class="arcrow"></line><line x1="75" y1="228" x2="75" y2="266" class="arcrow"></line><line x1="315" y1="228" x2="315" y2="266" class="arcrow"></line><line x1="555" y1="228" x2="555" y2="266" class="arcrow"></line><line x1="795" y1="228" x2="795" y2="266" class="arcrow"></line><line x1="75" y1="266" x2="75" y2="304" class="arcrow"></line><line x1="315" y1="266" x2="315" y2="304" class="arcrow"></line><line x1="555" y1="266" x2="555" y2="304" class="arcrow"></line><line x1="795" y1="266" x2="795" y2="304" class="arcrow"></line><line x1="75" y1="304" x2="75" y2="342" class="arcrow"></line><line x1="315" y1="304" x2="315" y2="342" class="arcrow"></line><line x1="555" y1="304" x2="555" y2="342" class="arcrow"></line><line x1="795" y1="304" x2="795" y2="342" class="arcrow"></line><line x1="75" y1="342" x2="75" y2="380" class="arcrow"></line><line x1="315" y1="342" x2="315" y2="380" class="arcrow"></line><line x1="555" y1="342" x2="555" y2="380" class="arcrow"></line><line x1="795" y1="342" x2="795" y2="380" class="arcrow"></line></g><g id="mscgen_js-svg-__svg__sequencelayer"><g id="mscgen_js-svg-__svgentities"><g><rect width="150" height="38" class="entity" style="fill:lime;"></rect><g><text x="75" y="22.5" class="entity-text "><tspan>Client</tspan></text></g></g><g><rect width="150" height="38" x="240" class="entity" style="fill:cyan;"></rect><g><text x="315" y="22.5" class="entity-text "><tspan>Rails</tspan></text></g></g><g><rect width="150" height="38" x="480" class="entity" style="fill:orange;"></rect><g><text x="555" y="22.5" class="entity-text "><tspan>EtagCaching</tspan></text></g></g><g><rect width="150" height="38" x="720" class="entity" style="fill:white;"></rect><g><text x="795" y="22.5" class="entity-text "><tspan>Redis</tspan></text></g></g></g><g><line x1="75" y1="95" x2="315" y2="95" class="arc directional method" style="stroke:black" marker-end="url(#mscgen_js-svg-__svgmethod-black)"></line><g><rect width="134.06" height="14" x="127.97" y="79.5" class="label-text-background"></rect><text x="195" y="90.5" class="directional-text method-text "><tspan>GET /projects/5/pipelines</tspan></text></g></g><g><line x1="315" y1="133" x2="555" y2="133" class="arc directional method" style="stroke:black" marker-end="url(#mscgen_js-svg-__svgmethod-black)"></line><g><rect width="134.06" height="14" x="367.97" y="117.5" class="label-text-background"></rect><text x="435" y="128.5" class="directional-text method-text "><tspan>GET /projects/5/pipelines</tspan></text></g></g><g><line x1="555" y1="171" x2="795" y2="171" class="arc directional method" style="stroke:black" marker-end="url(#mscgen_js-svg-__svgmethod-black)"></line><g><rect width="135.64" height="14" x="607.17" y="155.5" class="label-text-background"></rect><text x="675" y="166.5" class="directional-text method-text "><tspan>read(key = 'GET <Etag>')</tspan></text></g></g><g><line x1="795" y1="209" x2="555" y2="209" class="arc directional method" style="stroke:red" marker-end="url(#mscgen_js-svg-__svgmethod-red)"></line><g><rect width="60.02" height="14" x="644.98" y="193.5" class="label-text-background"></rect><text x="675" y="204.5" class="directional-text method-text " style="fill:red;"><tspan>cache miss</tspan></text></g></g><g></g><g><line x1="555" y1="285" x2="795" y2="285" class="arc directional method" style="stroke:black" marker-end="url(#mscgen_js-svg-__svgmethod-black)"></line><g><rect width="103.94" height="14" x="623.02" y="269.5" class="label-text-background"></rect><text x="675" y="280.5" class="directional-text method-text "><tspan>write('<New Etag>')</tspan></text></g></g><g><line x1="555" y1="323" x2="315" y2="323" class="arc directional method" style="stroke:black" marker-end="url(#mscgen_js-svg-__svgmethod-black)"></line><g><rect width="134.06" height="14" x="367.97" y="307.5" class="label-text-background"></rect><text x="435" y="318.5" class="directional-text method-text "><tspan>GET /projects/5/pipelines</tspan></text></g></g><g><line x1="315" y1="361" x2="75" y2="361" class="arc directional method" style="stroke:blue" marker-end="url(#mscgen_js-svg-__svgmethod-blue)"></line><g><rect width="130.72" height="14" x="129.63" y="345.5" class="label-text-background"></rect><text x="195" y="356.5" class="directional-text method-text " style="fill:blue;"><tspan>JSON response w/ ETag</tspan></text></g></g></g><g id="mscgen_js-svg-__svg__notelayer"></g><g id="mscgen_js-svg-__svg__watermark"></g></g></svg>
\ No newline at end of file diff --git a/doc/development/img/trigger_ss1.png b/doc/development/img/trigger_ss1.png Binary files differnew file mode 100644 index 00000000000..ccff1009a25 --- /dev/null +++ b/doc/development/img/trigger_ss1.png diff --git a/doc/development/img/trigger_ss2.png b/doc/development/img/trigger_ss2.png Binary files differnew file mode 100644 index 00000000000..94dfd048793 --- /dev/null +++ b/doc/development/img/trigger_ss2.png diff --git a/doc/development/migration_style_guide.md b/doc/development/migration_style_guide.md index fd8335d251e..77ba2a5fd87 100644 --- a/doc/development/migration_style_guide.md +++ b/doc/development/migration_style_guide.md @@ -4,28 +4,53 @@ When writing migrations for GitLab, you have to take into account that these will be ran by hundreds of thousands of organizations of all sizes, some with many years of data in their database. -In addition, having to take a server offline for a an upgrade small or big is -a big burden for most organizations. For this reason it is important that your -migrations are written carefully, can be applied online and adhere to the style guide below. +In addition, having to take a server offline for a a upgrade small or big is a +big burden for most organizations. For this reason it is important that your +migrations are written carefully, can be applied online and adhere to the style +guide below. -Migrations should not require GitLab installations to be taken offline unless -_absolutely_ necessary - see the ["What Requires Downtime?"](what_requires_downtime.md) -page. If a migration requires downtime, this should be clearly mentioned during -the review process, as well as being documented in the monthly release post. For -more information, see the "Downtime Tagging" section below. +Migrations are **not** allowed to require GitLab installations to be taken +offline unless _absolutely necessary_. Downtime assumptions should be based on +the behaviour of a migration when performed using PostgreSQL, as various +operations in MySQL may require downtime without there being alternatives. + +When downtime is necessary the migration has to be approved by: + +1. The VP of Engineering +1. A Backend Lead +1. A Database Specialist + +An up-to-date list of people holding these titles can be found at +<https://about.gitlab.com/team/>. + +The document ["What Requires Downtime?"](what_requires_downtime.md) specifies +various database operations, whether they require downtime and how to +work around that whenever possible. When writing your migrations, also consider that databases might have stale data -or inconsistencies and guard for that. Try to make as little assumptions as possible -about the state of the database. +or inconsistencies and guard for that. Try to make as few assumptions as +possible about the state of the database. -Please don't depend on GitLab specific code since it can change in future versions. -If needed copy-paste GitLab code into the migration to make it forward compatible. +Please don't depend on GitLab-specific code since it can change in future +versions. If needed copy-paste GitLab code into the migration to make it forward +compatible. + +## Commit Guidelines + +Each migration **must** be added in its own commit with a descriptive commit +message. If a commit adds a migration it _should only_ include the migration and +any corresponding changes to `db/schema.rb`. This makes it easy to revert a +database migration without accidentally reverting other changes. ## Downtime Tagging Every migration must specify if it requires downtime or not, and if it should -require downtime it must also specify a reason for this. To do so, add the -following two constants to the migration class' body: +require downtime it must also specify a reason for this. This is required even +if 99% of the migrations won't require downtime as this makes it easier to find +the migrations that _do_ require downtime. + +To tag a migration, add the following two constants to the migration class' +body: * `DOWNTIME`: a boolean that when set to `true` indicates the migration requires downtime. @@ -50,23 +75,79 @@ from a migration class. ## Reversibility -Your migration should be reversible. This is very important, as it should +Your migration **must be** reversible. This is very important, as it should be possible to downgrade in case of a vulnerability or bugs. In your migration, add a comment describing how the reversibility of the migration was tested. +## Multi Threading + +Sometimes a migration might need to use multiple Ruby threads to speed up a +migration. For this to work your migration needs to include the module +`Gitlab::Database::MultiThreadedMigration`: + +```ruby +class MyMigration < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + include Gitlab::Database::MultiThreadedMigration +end +``` + +You can then use the method `with_multiple_threads` to perform work in separate +threads. For example: + +```ruby +class MyMigration < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + include Gitlab::Database::MultiThreadedMigration + + def up + with_multiple_threads(4) do + disable_statement_timeout + + # ... + end + end +end +``` + +Here the call to `disable_statement_timeout` will use the connection local to +the `with_multiple_threads` block, instead of re-using the global connection +pool. This ensures each thread has its own connection object, and won't time +out when trying to obtain one. + +**NOTE:** PostgreSQL has a maximum amount of connections that it allows. This +limit can vary from installation to installation. As a result it's recommended +you do not use more than 32 threads in a single migration. Usually 4-8 threads +should be more than enough. + ## Removing indices -If you need to remove index, please add a condition like in following example: +When removing an index make sure to use the method `remove_concurrent_index` instead +of the regular `remove_index` method. The `remove_concurrent_index` method +automatically drops concurrent indexes when using PostgreSQL, removing the +need for downtime. To use this method you must disable transactions by calling +the method `disable_ddl_transaction!` in the body of your migration class like +so: ```ruby -remove_index :namespaces, column: :name if index_exists?(:namespaces, :name) +class MyMigration < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + disable_ddl_transaction! + + def up + remove_concurrent_index :table_name, :column_name if index_exists?(:table_name, :column_name) + end +end ``` ## Adding indices -If you need to add an unique index please keep in mind there is possibility of existing duplicates. If it is possible write a separate migration for handling this situation. It can be just removing or removing with overwriting all references to these duplicates depend on situation. +If you need to add a unique index please keep in mind there is the possibility +of existing duplicates being present in the database. This means that should +always _first_ add a migration that removes any duplicates, before adding the +unique index. When adding an index make sure to use the method `add_concurrent_index` instead of the regular `add_index` method. The `add_concurrent_index` method @@ -78,17 +159,22 @@ so: ```ruby class MyMigration < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers + disable_ddl_transaction! - def change + def up + add_concurrent_index :table, :column + end + def down + remove_index :table, :column if index_exists?(:table, :column) end end ``` ## Adding Columns With Default Values -When adding columns with default values you should use the method +When adding columns with default values you must use the method `add_column_with_default`. This method ensures the table is updated without requiring downtime. This method is not reversible so you must manually define the `up` and `down` methods in your migration class. @@ -111,6 +197,9 @@ class MyMigration < ActiveRecord::Migration end ``` +Keep in mind that this operation can easily take 10-15 minutes to complete on +larger installations (e.g. GitLab.com). As a result you should only add default +values if absolutely necessary. ## Integer column type @@ -135,13 +224,15 @@ add_column(:projects, :foo, :integer, default: 10, limit: 8) ## Testing -Make sure that your migration works with MySQL and PostgreSQL with data. An empty database does not guarantee that your migration is correct. +Make sure that your migration works with MySQL and PostgreSQL with data. An +empty database does not guarantee that your migration is correct. Make sure your migration can be reversed. ## Data migration -Please prefer Arel and plain SQL over usual ActiveRecord syntax. In case of using plain SQL you need to quote all input manually with `quote_string` helper. +Please prefer Arel and plain SQL over usual ActiveRecord syntax. In case of +using plain SQL you need to quote all input manually with `quote_string` helper. Example with Arel: @@ -165,3 +256,42 @@ select_all("SELECT name, COUNT(id) as cnt FROM tags GROUP BY name HAVING COUNT(i execute("DELETE FROM tags WHERE id IN(#{duplicate_ids.join(",")})") end ``` + +If you need more complex logic you can define and use models local to a +migration. For example: + +```ruby +class MyMigration < ActiveRecord::Migration + class Project < ActiveRecord::Base + self.table_name = 'projects' + end +end +``` + +When doing so be sure to explicitly set the model's table name so it's not +derived from the class name or namespace. + +### Renaming reserved paths + +When a new route for projects is introduced that could conflict with any +existing records. The path for this records should be renamed, and the +related data should be moved on disk. + +Since we had to do this a few times already, there are now some helpers to help +with this. + +To use this you can include `Gitlab::Database::RenameReservedPathsMigration::V1` +in your migration. This will provide 3 methods which you can pass one or more +paths that need to be rejected. + +**`rename_root_paths`**: This will rename the path of all _namespaces_ with the +given name that don't have a `parent_id`. + +**`rename_child_paths`**: This will rename the path of all _namespaces_ with the +given name that have a `parent_id`. + +**`rename_wildcard_paths`**: This will rename the path of all _projects_, and all +_namespaces_ that have a `project_id`. + +The `path` column for these rows will be renamed to their previous value followed +by an integer. For example: `users` would turn into `users0` diff --git a/doc/development/polling.md b/doc/development/polling.md index a7f2962acf0..3b34f985cd4 100644 --- a/doc/development/polling.md +++ b/doc/development/polling.md @@ -22,6 +22,14 @@ Instead you should use polling mechanism with ETag caching in Redis. ## How it works +Cache Miss: + + + +Cache Hit: + + + 1. Whenever a resource changes we generate a random value and store it in Redis. 1. When a client makes a request we set the `ETag` response header to the value @@ -36,6 +44,13 @@ Instead you should use polling mechanism with ETag caching in Redis. 1. If the `If-None-Match` header does not match the current value in Redis we have to generate a new response, because the resource changed. +Do not use query parameters (for example `?scope=all`) for endpoints where you +want to enable ETag caching. The middleware takes into account only the request +path and ignores query parameters. All parameters should be included in the +request path. By doing this we avoid query parameter ordering problems and make +route matching easier. + For more information see: +- [`Poll-Interval` header](fe_guide/performance.md#realtime-components) - [RFC 7232](https://tools.ietf.org/html/rfc7232) - [ETag proposal](https://gitlab.com/gitlab-org/gitlab-ce/issues/26926) diff --git a/doc/development/rake_tasks.md b/doc/development/rake_tasks.md index ec9e4dcc59d..fdaaa65fa28 100644 --- a/doc/development/rake_tasks.md +++ b/doc/development/rake_tasks.md @@ -31,16 +31,26 @@ files it can find, also the ones in `/tmp` To run a single test file you can use: -- `bundle exec rspec spec/controllers/commit_controller_spec.rb` for a rspec test -- `bundle exec spinach features/project/issues/milestones.feature` for a spinach test +- `bin/rspec spec/controllers/commit_controller_spec.rb` for a rspec test +- `bin/spinach features/project/issues/milestones.feature` for a spinach test To run several tests inside one directory: -- `bundle exec rspec spec/requests/api/` for the rspec tests if you want to test API only -- `bundle exec spinach features/profile/` for the spinach tests if you want to test only profile pages +- `bin/rspec spec/requests/api/` for the rspec tests if you want to test API only +- `bin/spinach features/profile/` for the spinach tests if you want to test only profile pages -If you want to use [Spring](https://github.com/rails/spring) set -`ENABLE_SPRING=1` in your environment. +### Speed-up tests, rake tasks, and migrations + +[Spring](https://github.com/rails/spring) is a Rails application preloader. It +speeds up development by keeping your application running in the background so +you don't need to boot it every time you run a test, rake task or migration. + +If you want to use it, you'll need to export the `ENABLE_SPRING` environment +variable to `1`: + +``` +export ENABLE_SPRING=1 +``` ## Compile Frontend Assets diff --git a/doc/development/testing.md b/doc/development/testing.md index 5bc958f5a96..6d8b846d27f 100644 --- a/doc/development/testing.md +++ b/doc/development/testing.md @@ -9,59 +9,187 @@ this guide defines a rule that contradicts the thoughtbot guide, this guide takes precedence. Some guidelines may be repeated verbatim to stress their importance. -## Factories +## Definitions + +### Unit tests + +Formal definition: https://en.wikipedia.org/wiki/Unit_testing + +These kind of tests ensure that a single unit of code (a method) works as +expected (given an input, it has a predictable output). These tests should be +isolated as much as possible. For example, model methods that don't do anything +with the database shouldn't need a DB record. Classes that don't need database +records should use stubs/doubles as much as possible. + +| Code path | Tests path | Testing engine | Notes | +| --------- | ---------- | -------------- | ----- | +| `app/finders/` | `spec/finders/` | RSpec | | +| `app/helpers/` | `spec/helpers/` | RSpec | | +| `app/db/{post_,}migrate/` | `spec/migrations/` | RSpec | | +| `app/policies/` | `spec/policies/` | RSpec | | +| `app/presenters/` | `spec/presenters/` | RSpec | | +| `app/routing/` | `spec/routing/` | RSpec | | +| `app/serializers/` | `spec/serializers/` | RSpec | | +| `app/services/` | `spec/services/` | RSpec | | +| `app/tasks/` | `spec/tasks/` | RSpec | | +| `app/uploaders/` | `spec/uploaders/` | RSpec | | +| `app/views/` | `spec/views/` | RSpec | | +| `app/workers/` | `spec/workers/` | RSpec | | +| `app/assets/javascripts/` | `spec/javascripts/` | Karma | More details in the [JavaScript](#javascript) section. | + +### Integration tests + +Formal definition: https://en.wikipedia.org/wiki/Integration_testing + +These kind of tests ensure that individual parts of the application work well together, without the overhead of the actual app environment (i.e. the browser). These tests should assert at the request/response level: status code, headers, body. They're useful to test permissions, redirections, what view is rendered etc. + +| Code path | Tests path | Testing engine | Notes | +| --------- | ---------- | -------------- | ----- | +| `app/controllers/` | `spec/controllers/` | RSpec | | +| `app/mailers/` | `spec/mailers/` | RSpec | | +| `lib/api/` | `spec/requests/api/` | RSpec | | +| `lib/ci/api/` | `spec/requests/ci/api/` | RSpec | | +| `app/assets/javascripts/` | `spec/javascripts/` | Karma | More details in the [JavaScript](#javascript) section. | + +#### About controller tests + +In an ideal world, controllers should be thin. However, when this is not the +case, it's acceptable to write a system/feature test without JavaScript instead +of a controller test. The reason is that testing a fat controller usually +involves a lot of stubbing, things like: -GitLab uses [factory_girl] as a test fixture replacement. - -- Factory definitions live in `spec/factories/`, named using the pluralization - of their corresponding model (`User` factories are defined in `users.rb`). -- There should be only one top-level factory definition per file. -- FactoryGirl methods are mixed in to all RSpec groups. This means you can (and - should) call `create(...)` instead of `FactoryGirl.create(...)`. -- Make use of [traits] to clean up definitions and usages. -- When defining a factory, don't define attributes that are not required for the - resulting record to pass validation. -- When instantiating from a factory, don't supply attributes that aren't - required by the test. -- Factories don't have to be limited to `ActiveRecord` objects. - [See example](https://gitlab.com/gitlab-org/gitlab-ce/commit/0b8cefd3b2385a21cfed779bd659978c0402766d). - -[factory_girl]: https://github.com/thoughtbot/factory_girl -[traits]: http://www.rubydoc.info/gems/factory_girl/file/GETTING_STARTED.md#Traits - -## JavaScript - -GitLab uses [Karma] to run its [Jasmine] JavaScript specs. They can be run on -the command line via `bundle exec karma`. - -- JavaScript tests live in `spec/javascripts/`, matching the folder structure - of `app/assets/javascripts/`: `app/assets/javascripts/behaviors/autosize.js` - has a corresponding `spec/javascripts/behaviors/autosize_spec.js` file. -- Haml fixtures required for JavaScript tests live in - `spec/javascripts/fixtures`. They should contain the bare minimum amount of - markup necessary for the test. - - > **Warning:** Keep in mind that a Rails view may change and - invalidate your test, but everything will still pass because your fixture - doesn't reflect the latest view. Because of this we encourage you to - generate fixtures from actual rails views whenever possible. - -- Keep in mind that in a CI environment, these tests are run in a headless - browser and you will not have access to certain APIs, such as - [`Notification`](https://developer.mozilla.org/en-US/docs/Web/API/notification), - which will have to be stubbed. - -[Karma]: https://github.com/karma-runner/karma -[Jasmine]: https://github.com/jasmine/jasmine +```ruby +controller.instance_variable_set(:@user, user) +``` -For more information, see the [frontend testing guide](fe_guide/testing.md). +and use methods which are deprecated in Rails 5 ([#23768]). + +[#23768]: https://gitlab.com/gitlab-org/gitlab-ce/issues/23768 + +#### About Karma + +As you may have noticed, Karma is both in the Unit tests and the Integration +tests category. That's because Karma is a tool that provides an environment to +run JavaScript tests, so you can either run unit tests (e.g. test a single +JavaScript method), or integration tests (e.g. test a component that is composed +of multiple components). + +### System tests or Feature tests + +Formal definition: https://en.wikipedia.org/wiki/System_testing. + +These kind of tests ensure the application works as expected from a user point +of view (aka black-box testing). These tests should test a happy path for a +given page or set of pages, and a test case should be added for any regression +that couldn't have been caught at lower levels with better tests (i.e. if a +regression is found, regression tests should be added at the lowest-level +possible). + +| Tests path | Testing engine | Notes | +| ---------- | -------------- | ----- | +| `spec/features/` | [Capybara] + [RSpec] | If your spec has the `:js` metadata, the browser driver will be [Poltergeist], otherwise it's using [RackTest]. | +| `features/` | Spinach | Spinach tests are deprecated, [you shouldn't add new Spinach tests](#spinach-feature-tests). | + +[Capybara]: https://github.com/teamcapybara/capybara +[RSpec]: https://github.com/rspec/rspec-rails#feature-specs +[Poltergeist]: https://github.com/teamcapybara/capybara#poltergeist +[RackTest]: https://github.com/teamcapybara/capybara#racktest + +#### Best practices + +- Create only the necessary records in the database +- Test a happy path and a less happy path but that's it +- Every other possible path should be tested with Unit or Integration tests +- Test what's displayed on the page, not the internals of ActiveRecord models. + For instance, if you want to verify that a record was created, add + expectations that its attributes are displayed on the page, not that + `Model.count` increased by one. +- It's ok to look for DOM elements but don't abuse it since it makes the tests + more brittle + +If we're confident that the low-level components work well (and we should be if +we have enough Unit & Integration tests), we shouldn't need to duplicate their +thorough testing at the System test level. + +It's very easy to add tests, but a lot harder to remove or improve tests, so one +should take care of not introducing too many (slow and duplicated) specs. + +The reasons why we should follow these best practices are as follows: + +- System tests are slow to run since they spin up the entire application stack + in a headless browser, and even slower when they integrate a JS driver +- When system tests run with a JavaScript driver, the tests are run in a + different thread than the application. This means it does not share a + database connection and your test will have to commit the transactions in + order for the running application to see the data (and vice-versa). In that + case we need to truncate the database after each spec instead of simply + rolling back a transaction (the faster strategy that's in use for other kind + of tests). This is slower than transactions, however, so we want to use + truncation only when necessary. + +### Black-box tests or End-to-end tests + +GitLab consists of [multiple pieces] such as [GitLab Shell], [GitLab Workhorse], +[Gitaly], [GitLab Pages], [GitLab Runner], and GitLab Rails. All theses pieces +are configured and packaged by [GitLab Omnibus]. + +[GitLab QA] is a tool that allows to test that all these pieces integrate well +together by building a Docker image for a given version of GitLab Rails and +running feature tests (i.e. using Capybara) against it. + +The actual test scenarios and steps are [part of GitLab Rails] so that they're +always in-sync with the codebase. + +[multiple pieces]: ./architecture.md#components +[GitLab Shell]: https://gitlab.com/gitlab-org/gitlab-shell +[GitLab Workhorse]: https://gitlab.com/gitlab-org/gitlab-workhorse +[Gitaly]: https://gitlab.com/gitlab-org/gitaly +[GitLab Pages]: https://gitlab.com/gitlab-org/gitlab-pages +[GitLab Runner]: https://gitlab.com/gitlab-org/gitlab-ci-multi-runner +[GitLab Omnibus]: https://gitlab.com/gitlab-org/omnibus-gitlab +[GitLab QA]: https://gitlab.com/gitlab-org/gitlab-qa +[part of GitLab Rails]: https://gitlab.com/gitlab-org/gitlab-ce/tree/master/qa + +## How to test at the correct level? + +As many things in life, deciding what to test at each level of testing is a +trade-off: + +- Unit tests are usually cheap, and you should consider them like the basement + of your house: you need them to be confident that your code is behaving + correctly. However if you run only unit tests without integration / system tests, you might [miss] the [big] [picture]! +- Integration tests are a bit more expensive, but don't abuse them. A feature test + is often better than an integration test that is stubbing a lot of internals. +- System tests are expensive (compared to unit tests), even more if they require + a JavaScript driver. Make sure to follow the guidelines in the [Speed](#test-speed) + section. + +Another way to see it is to think about the "cost of tests", this is well +explained [in this article][tests-cost] and the basic idea is that the cost of a +test includes: + +- The time it takes to write the test +- The time it takes to run the test every time the suite runs +- The time it takes to understand the test +- The time it takes to fix the test if it breaks and the underlying code is OK +- Maybe, the time it takes to change the code to make the code testable. + +[miss]: https://twitter.com/ThePracticalDev/status/850748070698651649 +[big]: https://twitter.com/timbray/status/822470746773409794 +[picture]: https://twitter.com/withzombies/status/829716565834752000 +[tests-cost]: https://medium.com/table-xi/high-cost-tests-and-high-value-tests-a86e27a54df#.2ulyh3a4e + +## Frontend testing + +Please consult the [dedicated "Frontend testing" guide](./fe_guide/testing.md). ## RSpec ### General Guidelines - Use a single, top-level `describe ClassName` block. -- Use `described_class` instead of repeating the class name being described. +- Use `described_class` instead of repeating the class name being described + (_this is enforced by RuboCop_). - Use `.method` to describe class methods and `#method` to describe instance methods. - Use `context` to test branching logic. @@ -70,11 +198,12 @@ For more information, see the [frontend testing guide](fe_guide/testing.md). - Don't `describe` symbols (see [Gotchas](gotchas.md#dont-describe-symbols)). - Don't assert against the absolute value of a sequence-generated attribute (see [Gotchas](gotchas.md#dont-assert-against-the-absolute-value-of-a-sequence-generated-attribute)). - Don't supply the `:each` argument to hooks since it's the default. -- Prefer `not_to` to `to_not` (_this is enforced by Rubocop_). +- Prefer `not_to` to `to_not` (_this is enforced by RuboCop_). - Try to match the ordering of tests to the ordering within the class. - Try to follow the [Four-Phase Test][four-phase-test] pattern, using newlines to separate phases. - Try to use `Gitlab.config.gitlab.host` rather than hard coding `'localhost'` +- On `before` and `after` hooks, prefer it scoped to `:context` over `:all` [four-phase-test]: https://robots.thoughtbot.com/four-phase-test @@ -98,6 +227,20 @@ so we need to set some guidelines for their use going forward: [lets-not]: https://robots.thoughtbot.com/lets-not +#### `set` variables + +In some cases there is no need to recreate the same object for tests again for +each example. For example, a project is needed to test issues on the same +project, one project will do for the entire file. This can be achieved by using +`set` in the same way you would use `let`. + +`rspec-set` only works on ActiveRecord objects, and before new examples it +reloads or recreates the model, _only_ if needed. That is, when you changed +properties or destroyed the object. + +There is one gotcha; you can't reference a model defined in a `let` block in a +`set` block. + ### Time-sensitive tests [Timecop](https://github.com/travisjeffery/timecop) is available in our @@ -117,53 +260,124 @@ it 'is overdue' do end ``` -### Test speed +### System / Feature tests -GitLab has a massive test suite that, without parallelization, can take more -than an hour to run. It's important that we make an effort to write tests that -are accurate and effective _as well as_ fast. +- Feature specs should be named `ROLE_ACTION_spec.rb`, such as + `user_changes_password_spec.rb`. +- Use only one `feature` block per feature spec file. +- Use scenario titles that describe the success and failure paths. +- Avoid scenario titles that add no information, such as "successfully". +- Avoid scenario titles that repeat the feature title. -Here are some things to keep in mind regarding test performance: +### Matchers -- `double` and `spy` are faster than `FactoryGirl.build(...)` -- `FactoryGirl.build(...)` and `.build_stubbed` are faster than `.create`. -- Don't `create` an object when `build`, `build_stubbed`, `attributes_for`, - `spy`, or `double` will do. Database persistence is slow! -- Use `create(:empty_project)` instead of `create(:project)` when you don't need - the underlying Git repository. Filesystem operations are slow! -- Don't mark a feature as requiring JavaScript (through `@javascript` in - Spinach or `js: true` in RSpec) unless it's _actually_ required for the test - to be valid. Headless browser testing is slow! +Custom matchers should be created to clarify the intent and/or hide the +complexity of RSpec expectations.They should be placed under +`spec/support/matchers/`. Matchers can be placed in subfolder if they apply to +a certain type of specs only (e.g. features, requests etc.) but shouldn't be if +they apply to multiple type of specs. -### Features / Integration +### Shared contexts -GitLab uses [rspec-rails feature specs] to test features in a browser -environment. These are [capybara] specs running on the headless [poltergeist] -driver. +All shared contexts should be be placed under `spec/support/shared_contexts/`. +Shared contexts can be placed in subfolder if they apply to a certain type of +specs only (e.g. features, requests etc.) but shouldn't be if they apply to +multiple type of specs. -- Feature specs live in `spec/features/` and should be named - `ROLE_ACTION_spec.rb`, such as `user_changes_password_spec.rb`. -- Use only one `feature` block per feature spec file. -- Use scenario titles that describe the success and failure paths. -- Avoid scenario titles that add no information, such as "successfully." -- Avoid scenario titles that repeat the feature title. +Each file should include only one context and have a descriptive name, e.g. +`spec/support/shared_contexts/controllers/githubish_import_controller_shared_context.rb`. -[rspec-rails feature specs]: https://github.com/rspec/rspec-rails#feature-specs -[capybara]: https://github.com/teamcapybara/capybara -[poltergeist]: https://github.com/teampoltergeist/poltergeist +### Shared examples -## Spinach (feature) tests +All shared examples should be be placed under `spec/support/shared_examples/`. +Shared examples can be placed in subfolder if they apply to a certain type of +specs only (e.g. features, requests etc.) but shouldn't be if they apply to +multiple type of specs. -GitLab [moved from Cucumber to Spinach](https://github.com/gitlabhq/gitlabhq/pull/1426) -for its feature/integration tests in September 2012. +Each file should include only one context and have a descriptive name, e.g. +`spec/support/shared_examples/controllers/githubish_import_controller_shared_example.rb`. -As of March 2016, we are [trying to avoid adding new Spinach -tests](https://gitlab.com/gitlab-org/gitlab-ce/issues/14121) going forward, -opting for [RSpec feature](#features-integration) specs. +### Helpers -Adding new Spinach scenarios is acceptable _only if_ the new scenario requires -no more than one new `step` definition. If more than that is required, the -test should be re-implemented using RSpec instead. +Helpers are usually modules that provide some methods to hide the complexity of +specific RSpec examples. You can define helpers in RSpec files if they're not +intended to be shared with other specs. Otherwise, they should be be placed +under `spec/support/helpers/`. Helpers can be placed in subfolder if they apply +to a certain type of specs only (e.g. features, requests etc.) but shouldn't be +if they apply to multiple type of specs. + +Helpers should follow the Rails naming / namespacing convention. For instance +`spec/support/helpers/cycle_analytics_helpers.rb` should define: + +```ruby +module Spec + module Support + module Helpers + module CycleAnalyticsHelpers + def create_commit_referencing_issue(issue, branch_name: random_git_name) + project.repository.add_branch(user, branch_name, 'master') + create_commit("Commit for ##{issue.iid}", issue.project, user, branch_name) + end + end + end + end +end +``` + +Helpers should not change the RSpec config. For instance, the helpers module +described above should not include: + +```ruby +RSpec.configure do |config| + config.include Spec::Support::Helpers::CycleAnalyticsHelpers +end +``` + +### Factories + +GitLab uses [factory_girl] as a test fixture replacement. + +- Factory definitions live in `spec/factories/`, named using the pluralization + of their corresponding model (`User` factories are defined in `users.rb`). +- There should be only one top-level factory definition per file. +- FactoryGirl methods are mixed in to all RSpec groups. This means you can (and + should) call `create(...)` instead of `FactoryGirl.create(...)`. +- Make use of [traits] to clean up definitions and usages. +- When defining a factory, don't define attributes that are not required for the + resulting record to pass validation. +- When instantiating from a factory, don't supply attributes that aren't + required by the test. +- Factories don't have to be limited to `ActiveRecord` objects. + [See example](https://gitlab.com/gitlab-org/gitlab-ce/commit/0b8cefd3b2385a21cfed779bd659978c0402766d). + +[factory_girl]: https://github.com/thoughtbot/factory_girl +[traits]: http://www.rubydoc.info/gems/factory_girl/file/GETTING_STARTED.md#Traits + +### Fixtures + +All fixtures should be be placed under `spec/fixtures/`. + +### Config + +RSpec config files are files that change the RSpec config (i.e. +`RSpec.configure do |config|` blocks). They should be placed under +`spec/support/config/`. + +Each file should be related to a specific domain, e.g. +`spec/support/config/capybara.rb`, `spec/support/config/carrierwave.rb`, etc. + +Helpers can be included in the `spec/support/config/rspec.rb` file. If a +helpers module applies only to a certain kind of specs, it should add modifiers +to the `config.include` call. For instance if +`spec/support/helpers/cycle_analytics_helpers.rb` applies to `:lib` and +`type: :model` specs only, you would write the following: + +```ruby +RSpec.configure do |config| + config.include Spec::Support::Helpers::CycleAnalyticsHelpers, :lib + config.include Spec::Support::Helpers::CycleAnalyticsHelpers, type: :model +end +``` ## Testing Rake Tasks @@ -201,6 +415,86 @@ describe 'gitlab:shell rake tasks' do end ``` +## Test speed + +GitLab has a massive test suite that, without [parallelization], can take hours +to run. It's important that we make an effort to write tests that are accurate +and effective _as well as_ fast. + +Here are some things to keep in mind regarding test performance: + +- `double` and `spy` are faster than `FactoryGirl.build(...)` +- `FactoryGirl.build(...)` and `.build_stubbed` are faster than `.create`. +- Don't `create` an object when `build`, `build_stubbed`, `attributes_for`, + `spy`, or `double` will do. Database persistence is slow! +- Use `create(:empty_project)` instead of `create(:project)` when you don't need + the underlying Git repository. Filesystem operations are slow! +- Don't mark a feature as requiring JavaScript (through `@javascript` in + Spinach or `:js` in RSpec) unless it's _actually_ required for the test + to be valid. Headless browser testing is slow! + +[parallelization]: #test-suite-parallelization-on-the-ci + +### Test suite parallelization on the CI + +Our current CI parallelization setup is as follows: + +1. The `knapsack` job in the prepare stage that is supposed to ensure we have a + `knapsack/${CI_PROJECT_NAME}/rspec_report-master.json` file: + - The `knapsack/${CI_PROJECT_NAME}/rspec_report-master.json` file is fetched + from S3, if it's not here we initialize the file with `{}`. +1. Each `rspec x y` job are run with `knapsack rspec` and should have an evenly + distributed share of tests: + - It works because the jobs have access to the + `knapsack/${CI_PROJECT_NAME}/rspec_report-master.json` since the "artifacts + from all previous stages are passed by default". [^1] + - the jobs set their own report path to + `KNAPSACK_REPORT_PATH=knapsack/${CI_PROJECT_NAME}/${JOB_NAME[0]}_node_${CI_NODE_INDEX}_${CI_NODE_TOTAL}_report.json`. + - if knapsack is doing its job, test files that are run should be listed under + `Report specs`, not under `Leftover specs`. +1. The `update-knapsack` job takes all the + `knapsack/${CI_PROJECT_NAME}/${JOB_NAME[0]}_node_${CI_NODE_INDEX}_${CI_NODE_TOTAL}_report.json` + files from the `rspec x y` jobs and merge them all together into a single + `knapsack/${CI_PROJECT_NAME}/rspec_report-master.json` file that is then + uploaded to S3. + +After that, the next pipeline will use the up-to-date +`knapsack/${CI_PROJECT_NAME}/rspec_report-master.json` file. The same strategy +is used for Spinach tests as well. + +### Monitoring + +The GitLab test suite is [monitored] for the `master` branch, and any branch +that includes `rspec-profile` in their name. + +A [public dashboard] is available for everyone to see. Feel free to look at the +slowest test files and try to improve them. + +[monitored]: ./performance.md#rspec-profiling +[public dashboard]: https://redash.gitlab.com/public/dashboards/l1WhHXaxrCWM5Ai9D7YDqHKehq6OU3bx5gssaiWe?org_slug=default + +## CI setup + +- On CE, the test suite only runs against PostgreSQL by default. We additionally + run the suite against MySQL for tags, `master`, and any branch that includes + `mysql` in the name. +- On EE, the test suite always runs both PostgreSQL and MySQL. + +## Spinach (feature) tests + +GitLab [moved from Cucumber to Spinach](https://github.com/gitlabhq/gitlabhq/pull/1426) +for its feature/integration tests in September 2012. + +As of March 2016, we are [trying to avoid adding new Spinach +tests](https://gitlab.com/gitlab-org/gitlab-ce/issues/14121) going forward, +opting for [RSpec feature](#features-integration) specs. + +Adding new Spinach scenarios is acceptable _only if_ the new scenario requires +no more than one new `step` definition. If more than that is required, the +test should be re-implemented using RSpec instead. + --- [Return to Development documentation](README.md) + +[^1]: /ci/yaml/README.html#dependencies diff --git a/doc/development/ux_guide/basics.md b/doc/development/ux_guide/basics.md index 259b214bd59..a436e9b1948 100644 --- a/doc/development/ux_guide/basics.md +++ b/doc/development/ux_guide/basics.md @@ -22,7 +22,7 @@ GitLab's main typeface used throughout the UI is **Source Sans Pro**. We support ### Monospace typeface -This is the typeface used for code blocks. GitLab uses the OS default font. +This is the typeface used for code blocks and references to commits, branches, and tags (`.commit-sha` or `.ref-name`). GitLab uses the OS default font. - **Menlo** (Mac) - **Consolas** (Windows) - **Liberation Mono** (Linux) diff --git a/doc/development/what_requires_downtime.md b/doc/development/what_requires_downtime.md index bbcd26477f3..c4830322fa8 100644 --- a/doc/development/what_requires_downtime.md +++ b/doc/development/what_requires_downtime.md @@ -2,7 +2,8 @@ When working with a database certain operations can be performed without taking GitLab offline, others do require a downtime period. This guide describes -various operations and their impact. +various operations, their impact, and how to perform them without requiring +downtime. ## Adding Columns @@ -41,50 +42,158 @@ information on how to use this method. ## Dropping Columns -On PostgreSQL you can safely remove an existing column without the need for -downtime. When you drop a column in PostgreSQL it's not immediately removed, -instead it is simply disabled. The data is removed on the next vacuum run. +Removing columns is tricky because running GitLab processes may still be using +the columns. To work around this you will need two separate merge requests and +releases: one to ignore and then remove the column, and one to remove the ignore +rule. -On MySQL this operation requires downtime. +### Step 1: Ignoring The Column -While database wise dropping a column may be fine on PostgreSQL this operation -still requires downtime because the application code may still be using the -column that was removed. For example, consider the following migration: +The first step is to ignore the column in the application code. This is +necessary because Rails caches the columns and re-uses this cache in various +places. This can be done by including the `IgnorableColumn` module into the +model, followed by defining the columns to ignore. For example, to ignore +`updated_at` in the User model you'd use the following: ```ruby -class MyMigration < ActiveRecord::Migration - def change - remove_column :projects, :dummy - end +class User < ActiveRecord::Base + include IgnorableColumn + + ignore_column :updated_at end ``` -Now imagine that the GitLab instance is running and actively uses the `dummy` -column. If we were to run the migration this would result in the GitLab instance -producing errors whenever it tries to use the `dummy` column. +Once added you should create a _post-deployment_ migration that removes the +column. Both these changes should be submitted in the same merge request. -As a result of the above downtime _is_ required when removing a column, even -when using PostgreSQL. +### Step 2: Removing The Ignore Rule + +Once the changes from step 1 have been released & deployed you can set up a +separate merge request that removes the ignore rule. This merge request can +simply remove the `ignore_column` line, and the `include IgnorableColumn` line +if no other `ignore_column` calls remain. ## Renaming Columns -Renaming columns requires downtime as running GitLab instances will continue -using the old column name until a new version is deployed. This can result -in the instance producing errors, which in turn can impact the user experience. +Renaming columns the normal way requires downtime as an application may continue +using the old column name during/after a database migration. To rename a column +without requiring downtime we need two migrations: a regular migration, and a +post-deployment migration. Both these migration can go in the same release. -## Changing Column Constraints +### Step 1: Add The Regular Migration + +First we need to create the regular migration. This migration should use +`Gitlab::Database::MigrationHelpers#rename_column_concurrently` to perform the +renaming. For example + +```ruby +# A regular migration in db/migrate +class RenameUsersUpdatedAtToUpdatedAtTimestamp < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + disable_ddl_transaction! + + def up + rename_column_concurrently :users, :updated_at, :updated_at_timestamp + end + + def down + cleanup_concurrent_column_rename :users, :updated_at_timestamp, :updated_at + end +end +``` + +This will take care of renaming the column, ensuring data stays in sync, copying +over indexes and foreign keys, etc. + +**NOTE:** if a column contains 1 or more indexes that do not contain the name of +the original column, the above procedure will fail. In this case you will first +need to rename these indexes. -Generally changing column constraints requires checking all rows in the table to -see if they meet the new constraint, unless a constraint is _removed_. For -example, changing a column that previously allowed NULL values to not allow NULL -values requires the database to verify all existing rows. +### Step 2: Add A Post-Deployment Migration -The specific behaviour varies a bit between databases but in general the safest -approach is to assume changing constraints requires downtime. +The renaming procedure requires some cleaning up in a post-deployment migration. +We can perform this cleanup using +`Gitlab::Database::MigrationHelpers#cleanup_concurrent_column_rename`: + +```ruby +# A post-deployment migration in db/post_migrate +class CleanupUsersUpdatedAtRename < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + disable_ddl_transaction! + + def up + cleanup_concurrent_column_rename :users, :updated_at, :updated_at_timestamp + end + + def down + rename_column_concurrently :users, :updated_at_timestamp, :updated_at + end +end +``` + +## Changing Column Constraints + +Adding or removing a NOT NULL clause (or another constraint) can typically be +done without requiring downtime. However, this does require that any application +changes are deployed _first_. Thus, changing the constraints of a column should +happen in a post-deployment migration. +NOTE: Avoid using `change_column` as it produces inefficient query because it re-defines +the whole column type. For example, to add a NOT NULL constraint, prefer `change_column_null ` ## Changing Column Types -This operation requires downtime. +Changing the type of a column can be done using +`Gitlab::Database::MigrationHelpers#change_column_type_concurrently`. This +method works similarly to `rename_column_concurrently`. For example, let's say +we want to change the type of `users.username` from `string` to `text`. + +### Step 1: Create A Regular Migration + +A regular migration is used to create a new column with a temporary name along +with setting up some triggers to keep data in sync. Such a migration would look +as follows: + +```ruby +# A regular migration in db/migrate +class ChangeUsersUsernameStringToText < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + disable_ddl_transaction! + + def up + change_column_type_concurrently :users, :username, :text + end + + def down + cleanup_concurrent_column_type_change :users, :username + end +end +``` + +### Step 2: Create A Post Deployment Migration + +Next we need to clean up our changes using a post-deployment migration: + +```ruby +# A post-deployment migration in db/post_migrate +class ChangeUsersUsernameStringToTextCleanup < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + disable_ddl_transaction! + + def up + cleanup_concurrent_column_type_change :users + end + + def down + change_column_type_concurrently :users, :username, :string + end +end +``` + +And that's it, we're done! ## Adding Indexes @@ -101,12 +210,19 @@ Migrations can take advantage of this by using the method ```ruby class MyMigration < ActiveRecord::Migration - def change + def up add_concurrent_index :projects, :column_name end + + def down + remove_index(:projects, :column_name) if index_exists?(:projects, :column_name) + end end ``` +Note that `add_concurrent_index` can not be reversed automatically, thus you +need to manually define `up` and `down`. + When running this on PostgreSQL the `CONCURRENTLY` option mentioned above is used. On MySQL this method produces a regular `CREATE INDEX` query. @@ -125,43 +241,54 @@ This operation is safe as there's no code using the table just yet. ## Dropping Tables -This operation requires downtime as application code may still be using the -table. +Dropping tables can be done safely using a post-deployment migration, but only +if the application no longer uses the table. ## Adding Foreign Keys -Adding foreign keys acquires an exclusive lock on both the source and target -tables in PostgreSQL. This requires downtime as otherwise the entire application -grinds to a halt for the duration of the operation. +Adding foreign keys usually works in 3 steps: + +1. Start a transaction +1. Run `ALTER TABLE` to add the constraint(s) +1. Check all existing data -On MySQL this operation also requires downtime _unless_ foreign key checks are -disabled. Because this means checks aren't enforced this is not ideal, as such -one should assume MySQL also requires downtime. +Because `ALTER TABLE` typically acquires an exclusive lock until the end of a +transaction this means this approach would require downtime. + +GitLab allows you to work around this by using +`Gitlab::Database::MigrationHelpers#add_concurrent_foreign_key`. This method +ensures that when PostgreSQL is used no downtime is needed. ## Removing Foreign Keys -This operation should not require downtime on both PostgreSQL and MySQL. +This operation does not require downtime. -## Updating Data +## Data Migrations -Updating data should generally be safe. The exception to this is data that's -being migrated from one version to another while the application still produces -data in the old version. +Data migrations can be tricky. The usual approach to migrate data is to take a 3 +step approach: -For example, imagine the application writes the string `'dog'` to a column but -it really is meant to write `'cat'` instead. One might think that the following -migration is all that is needed to solve this problem: +1. Migrate the initial batch of data +1. Deploy the application code +1. Migrate any remaining data -```ruby -class MyMigration < ActiveRecord::Migration - def up - execute("UPDATE some_table SET column = 'cat' WHERE column = 'dog';") - end -end -``` +Usually this works, but not always. For example, if a field's format is to be +changed from JSON to something else we have a bit of a problem. If we were to +change existing data before deploying application code we'll most likely run +into errors. On the other hand, if we were to migrate after deploying the +application code we could run into the same problems. + +If you merely need to correct some invalid data, then a post-deployment +migration is usually enough. If you need to change the format of data (e.g. from +JSON to something else) it's typically best to add a new column for the new data +format, and have the application use that. In such a case the procedure would +be: -Unfortunately this is not enough. Because the application is still running and -using the old value this may result in the table still containing rows where -`column` is set to `dog`, even after the migration finished. +1. Add a new column in the new format +1. Copy over existing data to this new column +1. Deploy the application code +1. In a post-deployment migration, copy over any remaining data -In these cases downtime _is_ required, even for rarely updated tables. +In general there is no one-size-fits-all solution, therefore it's best to +discuss these kind of migrations in a merge request to make sure they are +implemented in the best way possible. diff --git a/doc/development/writing_documentation.md b/doc/development/writing_documentation.md index 482ec54207b..eac9ec2a470 100644 --- a/doc/development/writing_documentation.md +++ b/doc/development/writing_documentation.md @@ -2,7 +2,7 @@ - **General Documentation**: written by the developers responsible by creating features. Should be submitted in the same merge request containing code. Feature proposals (by GitLab contributors) should also be accompanied by its respective documentation. They can be later improved by PMs and Technical Writers. - **Technical Articles**: written by any [GitLab Team](https://about.gitlab.com/team/) member, GitLab contributors, or [Community Writers](https://about.gitlab.com/handbook/product/technical-writing/community-writers/). - - **Indexes per topic**: initially prepared by the Technical Writing Team, and kept up-to-date by developers and PMs, in the same merge request containing code. + - **Indexes per topic**: initially prepared by the Technical Writing Team, and kept up-to-date by developers and PMs in the same merge request containing code. They gather all resources for that topic in a single page (user and admin documentation, articles, and third-party docs). ## Distinction between General Documentation and Technical Articles @@ -18,7 +18,7 @@ They are topic-related documentation, written with an user-friendly approach and A technical article guides users and/or admins to achieve certain objectives (within guides and tutorials), or provide an overview of that particular topic or feature (within technical overviews). It can also describe the use, implementation, or integration of third-party tools with GitLab. -They live under `doc/topics/topic-name/`, and can be searched per topic, within "Indexes per Topic" pages. The topics are listed on the main [Indexes per Topic](../topics/index.md) page. +They live under `doc/articles/article-title/index.md`, and their images should be placed under `doc/articles/article-title/img/`. Find a list of existing [technical articles](../articles/index.md) here. #### Types of Technical Articles @@ -52,11 +52,13 @@ Every **Technical Article** contains, in the very beginning, a blockquote with t - A reference to the **type of article** (user guide, admin guide, tech overview, tutorial) - A reference to the **knowledge level** expected from the reader to be able to follow through (beginner, intermediate, advanced) - A reference to the **author's name** and **GitLab.com handle** +- A reference of the **publication date** ```md -> **Type:** tutorial || +> **Article [Type](../../development/writing_documentation.html#types-of-technical-articles):** tutorial || > **Level:** intermediary || -> **Author:** [Name Surname](https://gitlab.com/username) +> **Author:** [Name Surname](https://gitlab.com/username) || +> **Publication date:** AAAA/MM/DD ``` #### Technical Articles - Writing Method @@ -70,3 +72,34 @@ All the docs follow the same [styleguide](doc_styleguide.md). ### Markdown Currently GitLab docs use Redcarpet as [markdown](../user/markdown.md) engine, but there's an [open discussion](https://gitlab.com/gitlab-com/gitlab-docs/issues/50) for implementing Kramdown in the near future. + +## Testing + +We try to treat documentation as code, thus have implemented some testing. +Currently, the following tests are in place: + +1. `docs lint`: Check that all internal (relative) links work correctly and + that all cURL examples in API docs use the full switches. + +If your contribution contains **only** documentation changes, you can speed up +the CI process by following some branch naming conventions. You have three +choices: + +| Branch name | Valid example | +| ----------- | ------------- | +| Starting with `docs/` | `docs/update-api-issues` | +| Starting with `docs-` | `docs-update-api-issues` | +| Ending in `-docs` | `123-update-api-issues-docs` | + +If your branch name matches any of the above, it will run only the docs +tests. If it doesn't, the whole test suite will run (including docs). + +--- + +When you submit a merge request to GitLab Community Edition (CE), there is an +additional job called `rake ee_compat_check` that runs against Enterprise +Edition (EE) and checks if your changes can apply cleanly to the EE codebase. +If that job fails, read the instructions in the job log for what to do next. +Contributors do not need to submit their changes to EE, GitLab Inc. employees +on the other hand need to make sure that their changes apply cleanly to both +CE and EE. |
