diff options
Diffstat (limited to 'doc/development')
| -rw-r--r-- | doc/development/README.md | 4 | ||||
| -rw-r--r-- | doc/development/changelog.md | 54 | ||||
| -rw-r--r-- | doc/development/database_merge_request_checklist.md | 15 | ||||
| -rw-r--r-- | doc/development/doc_styleguide.md | 13 | ||||
| -rw-r--r-- | doc/development/fe_guide/style_guide_js.md | 120 | ||||
| -rw-r--r-- | doc/development/fe_guide/vue.md | 291 | ||||
| -rw-r--r-- | doc/development/hash_indexes.md | 20 | ||||
| -rw-r--r-- | doc/development/i18n_guide.md | 41 | ||||
| -rw-r--r-- | doc/development/img/manual_build_docs.png | bin | 0 -> 14869 bytes | |||
| -rw-r--r-- | doc/development/licensing.md | 7 | ||||
| -rw-r--r-- | doc/development/ordering_table_columns.md | 127 | ||||
| -rw-r--r-- | doc/development/serializing_data.md | 3 | ||||
| -rw-r--r-- | doc/development/sql.md | 26 | ||||
| -rw-r--r-- | doc/development/testing.md | 80 | ||||
| -rw-r--r-- | doc/development/verifying_database_capabilities.md | 26 | ||||
| -rw-r--r-- | doc/development/writing_documentation.md | 21 |
16 files changed, 761 insertions, 87 deletions
diff --git a/doc/development/README.md b/doc/development/README.md index 58993c52dcd..dd150421b65 100644 --- a/doc/development/README.md +++ b/doc/development/README.md @@ -46,6 +46,7 @@ ## Databases +- [Merge Request Checklist](database_merge_request_checklist.md) - [What requires downtime?](what_requires_downtime.md) - [Adding database indexes](adding_database_indexes.md) - [Post Deployment Migrations](post_deployment_migrations.md) @@ -56,6 +57,9 @@ - [Background Migrations](background_migrations.md) - [Storing SHA1 Hashes As Binary](sha1_as_binary.md) - [Iterating Tables In Batches](iterating_tables_in_batches.md) +- [Ordering Table Columns](ordering_table_columns.md) +- [Verifying Database Capabilities](verifying_database_capabilities.md) +- [Hash Indexes](hash_indexes.md) ## i18n diff --git a/doc/development/changelog.md b/doc/development/changelog.md index ce39a379a0e..f869938fe11 100644 --- a/doc/development/changelog.md +++ b/doc/development/changelog.md @@ -15,11 +15,14 @@ following format: title: "Going through change[log]s" merge_request: 1972 author: Ozzy Osbourne +type: added ``` The `merge_request` value is a reference to a merge request that adds this entry, and the `author` key is used to give attribution to community contributors. **Both are optional**. +The `type` field maps the category of the change, +valid options are: added, fixed, changed, deprecated, removed, security, other. **Type field is mandatory**. Community contributors and core team members are encouraged to add their name to the `author` field. GitLab team members **should not**. @@ -94,6 +97,19 @@ Its simplest usage is to provide the value for `title`: $ bin/changelog 'Hey DZ, I added a feature to GitLab!' ``` +At this point the script would ask you to select the category of the change (mapped to the `type` field in the entry): + +```text +>> Please specify the category of your change: +1. New feature +2. Bug fix +3. Feature change +4. New deprecation +5. Feature removal +6. Security fix +7. Other +``` + The entry filename is based on the name of the current Git branch. If you run the command above on a branch called `feature/hey-dz`, it will generate a `changelogs/unreleased/feature-hey-dz.yml` file. @@ -106,26 +122,29 @@ create changelogs/unreleased/my-feature.yml title: Hey DZ, I added a feature to GitLab! merge_request: author: +type: ``` If you're working on the GitLab EE repository, the entry will be added to `changelogs/unreleased-ee/` instead. #### Arguments -| Argument | Shorthand | Purpose | -| ----------------- | --------- | --------------------------------------------- | -| [`--amend`] | | Amend the previous commit | -| [`--force`] | `-f` | Overwrite an existing entry | -| [`--merge-request`] | `-m` | Set merge request ID | -| [`--dry-run`] | `-n` | Don't actually write anything, just print | -| [`--git-username`] | `-u` | Use Git user.name configuration as the author | -| [`--help`] | `-h` | Print help message | +| Argument | Shorthand | Purpose | +| ----------------- | --------- | ---------------------------------------------------------------------------------------------------------- | +| [`--amend`] | | Amend the previous commit | +| [`--force`] | `-f` | Overwrite an existing entry | +| [`--merge-request`] | `-m` | Set merge request ID | +| [`--dry-run`] | `-n` | Don't actually write anything, just print | +| [`--git-username`] | `-u` | Use Git user.name configuration as the author | +| [`--type`] | `-t` | The category of the change, valid options are: added, fixed, changed, deprecated, removed, security, other | +| [`--help`] | `-h` | Print help message | [`--amend`]: #-amend [`--force`]: #-force-or-f [`--merge-request`]: #-merge-request-or-m [`--dry-run`]: #-dry-run-or-n [`--git-username`]: #-git-username-or-u +[`--type`]: #-type-or-t [`--help`]: #-help ##### `--amend` @@ -147,6 +166,7 @@ create changelogs/unreleased/feature-hey-dz.yml title: Added an awesome new feature to GitLab merge_request: author: +type: ``` ##### `--force` or `-f` @@ -164,6 +184,7 @@ create changelogs/unreleased/feature-hey-dz.yml title: Hey DZ, I added a feature to GitLab! merge_request: 1983 author: +type: ``` ##### `--merge-request` or `-m` @@ -178,6 +199,7 @@ create changelogs/unreleased/feature-hey-dz.yml title: Hey DZ, I added a feature to GitLab! merge_request: 1983 author: +type: ``` ##### `--dry-run` or `-n` @@ -192,6 +214,7 @@ create changelogs/unreleased/feature-hey-dz.yml title: Added an awesome new feature to GitLab merge_request: author: +type: $ ls changelogs/unreleased/ ``` @@ -211,6 +234,21 @@ create changelogs/unreleased/feature-hey-dz.yml title: Hey DZ, I added a feature to GitLab! merge_request: author: Jane Doe +type: +``` + +##### `--type` or `-t` + +Use the **`--type`** or **`-t`** argument to provide the `type` value: + +```text +$ bin/changelog 'Hey DZ, I added a feature to GitLab!' -t added +create changelogs/unreleased/feature-hey-dz.yml +--- +title: Hey DZ, I added a feature to GitLab! +merge_request: +author: +type: added ``` ### History and Reasoning diff --git a/doc/development/database_merge_request_checklist.md b/doc/development/database_merge_request_checklist.md new file mode 100644 index 00000000000..75c395b61ef --- /dev/null +++ b/doc/development/database_merge_request_checklist.md @@ -0,0 +1,15 @@ +# Merge Request Checklist + +When creating a merge request that performs database related changes (schema +changes, adjusting queries to optimise performance, etc) you should use the +merge request template called "Database Changes". This template contains a +checklist of steps to follow to make sure the changes are up to snuff. + +To use the checklist, create a new merge request and click on the "Choose a +template" dropdown, then click "Database Changes". + +An example of this checklist can be found at +https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/12463. + +The source code of the checklist can be found in at +https://gitlab.com/gitlab-org/gitlab-ce/blob/master/.gitlab/merge_request_templates/Database%20Changes.md diff --git a/doc/development/doc_styleguide.md b/doc/development/doc_styleguide.md index 90d1d9657b9..798f40eef3d 100644 --- a/doc/development/doc_styleguide.md +++ b/doc/development/doc_styleguide.md @@ -113,13 +113,12 @@ merge request. ## Links -- If a link makes the paragraph to span across multiple lines, do not use - the regular Markdown approach: `[Text](https://example.com)`. Instead use - `[Text][identifier]` and at the very bottom of the document add: - `[identifier]: https://example.com`. This is another way to create Markdown - links which keeps the document clear and concise. Bonus points if you also - add an alternative text: `[identifier]: https://example.com "Alternative text"` - that appears when hovering your mouse on a link +- Use the regular inline link markdown markup `[Text](https://example.com)`. + It's easier to read, review, and maintain. +- If there's a link that repeats several times through the same document, + you can use `[Text][identifier]` and at the bottom of the section or the + document add: `[identifier]: https://example.com`, in which case, we do + encourage you to also add an alternative text: `[identifier]: https://example.com "Alternative text"` that appears when hovering your mouse on a link. ### Linking to inline docs diff --git a/doc/development/fe_guide/style_guide_js.md b/doc/development/fe_guide/style_guide_js.md index 6ade3231fac..4f20aa070de 100644 --- a/doc/development/fe_guide/style_guide_js.md +++ b/doc/development/fe_guide/style_guide_js.md @@ -102,42 +102,41 @@ followed by any global declarations, then a blank newline prior to any imports o 1. Relative paths: when importing a module in the same directory, a child directory, or an immediate parent directory prefer relative paths. When -importing a module which is two or more levels up, prefer either `~/` or `ee/` -. +importing a module which is two or more levels up, prefer either `~/` or `ee/`. -In **app/assets/javascripts/my-feature/subdir**: + In **app/assets/javascripts/my-feature/subdir**: -``` javascript -// bad -import Foo from '~/my-feature/foo'; -import Bar from '~/my-feature/subdir/bar'; -import Bin from '~/my-feature/subdir/lib/bin'; + ```javascript + // bad + import Foo from '~/my-feature/foo'; + import Bar from '~/my-feature/subdir/bar'; + import Bin from '~/my-feature/subdir/lib/bin'; -// good -import Foo from '../foo'; -import Bar from './bar'; -import Bin from './lib/bin'; -``` + // good + import Foo from '../foo'; + import Bar from './bar'; + import Bin from './lib/bin'; + ``` -In **spec/javascripts**: + In **spec/javascripts**: -``` javascript -// bad -import Foo from '../../app/assets/javascripts/my-feature/foo'; + ```javascript + // bad + import Foo from '../../app/assets/javascripts/my-feature/foo'; -// good -import Foo from '~/my-feature/foo'; -``` + // good + import Foo from '~/my-feature/foo'; + ``` -When referencing an **EE component**: + When referencing an **EE component**: -``` javascript -// bad -import Foo from '../../../../../ee/app/assets/javascripts/my-feature/ee-foo'; + ```javascript + // bad + import Foo from '../../../../../ee/app/assets/javascripts/my-feature/ee-foo'; -// good -import Foo from 'ee/my-feature/foo'; -``` + // good + import Foo from 'ee/my-feature/foo'; + ``` 1. Avoid using IIFE. Although we have a lot of examples of files which wrap their contents in IIFEs (immediately-invoked function expressions), @@ -145,24 +144,23 @@ 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 { /* ... */ }; + ```javascript + // bad + window.MyClass = class { /* ... */ }; - // good - export default class MyClass { /* ... */ } - ``` + // good + export default class MyClass { /* ... */ } + ``` 1. Side effects are forbidden in any script which contains exports - ```javascript - // bad - export default class MyClass { /* ... */ } - - document.addEventListener("DOMContentLoaded", function(event) { - new MyClass(); - } - ``` + ```javascript + // bad + export default class MyClass { /* ... */ } + document.addEventListener("DOMContentLoaded", function(event) { + new MyClass(); + } + ``` #### Data Mutation and Pure functions 1. Strive to write many small pure functions, and minimize where mutations occur. @@ -414,19 +412,19 @@ A forEach will cause side effects, it will be mutating the array being iterated. #### Data 1. `data` method should always be a function - ```javascript - // bad - data: { - foo: 'foo' - } - - // good - data() { - return { + ```javascript + // bad + data: { foo: 'foo' - }; - } - ``` + } + + // good + data() { + return { + foo: 'foo' + }; + } + ``` #### Directives @@ -481,7 +479,8 @@ A forEach will cause side effects, it will be mutating the array being iterated. 1. `beforeDestroy` 1. `destroyed` -#### Vue and Boostrap +#### Vue and Bootstrap + 1. Tooltips: Do not rely on `has-tooltip` class name for Vue components ```javascript // bad @@ -512,6 +511,19 @@ A forEach will cause side effects, it will be mutating the array being iterated. $('span').tooltip('fixTitle'); ``` +### The Javascript/Vue Accord +The goal of this accord is to make sure we are all on the same page. + +1. When writing Vue, you may not use jQuery in your application. + 1. If you need to grab data from the DOM, you may query the DOM 1 time while bootstrapping your application to grab data attributes using `dataset`. You can do this without jQuery. + 1. You may use a jQuery dependency in Vue.js following [this example from the docs](https://vuejs.org/v2/examples/select2.html). + 1. If an outside jQuery Event needs to be listen to inside the Vue application, you may use jQuery event listeners. + 1. We will avoid adding new jQuery events when they are not required. Instead of adding new jQuery events take a look at [different methods to do the same task](https://vuejs.org/v2/api/#vm-emit). +1. You may query the `window` object 1 time, while bootstrapping your application for application specific data (e.g. `scrollTo` is ok to access anytime). Do this access during the bootstrapping of your application. +1. You may have a temporary but immediate need to create technical debt by writing code that does not follow our standards, to be refactored later. Maintainers need to be ok with the tech debt in the first place. An issue should be created for that tech debt to evaluate it further and discuss. In the coming months you should fix that tech debt, with it's priority to be determined by maintainers. +1. When creating tech debt you must write the tests for that code before hand and those tests may not be rewritten. e.g. jQuery tests rewritten to Vue tests. +1. You may choose to use VueX as a centralized state management. If you choose not to use VueX, you must use the *store pattern* which can be found in the [Vue.js documentation](https://vuejs.org/v2/guide/state-management.html#Simple-State-Management-from-Scratch). +1. Once you have chosen a centralized state management solution you must use it for your entire application. i.e. Don't mix and match your state management solutions. ## SCSS - [SCSS](style_guide_scss.md) diff --git a/doc/development/fe_guide/vue.md b/doc/development/fe_guide/vue.md index 0742b202807..2607353782a 100644 --- a/doc/development/fe_guide/vue.md +++ b/doc/development/fe_guide/vue.md @@ -28,8 +28,9 @@ As always, the Frontend Architectural Experts are available to help with any Vue 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. +In order to achieve this goal, you can either use [vuex](#vuex) or use the [store pattern][state-management], explained below: + +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. @@ -74,6 +75,59 @@ provided as a prop to the main component. Don't forget to follow [these steps.][page_specific_javascript] +### Bootstrapping Gotchas +#### Providing data from Haml to JavaScript +While mounting a Vue application may be a need to provide data from Rails to JavaScript. +To do that, provide the data through `data` attributes in the HTML element and query them while mounting the application. + +_Note:_ You should only do this while initing the application, because the mounted element will be replaced with Vue-generated DOM. + +The advantage of providing data from the DOM to the Vue instance through `props` in the `render` function +instead of querying the DOM inside the main vue component is that makes tests easier by avoiding the need to +create a fixture or an HTML element in the unit test. See the following example: + +```javascript +// haml +.js-vue-app{ data: { endpoint: 'foo' }} + +document.addEventListener('DOMContentLoaded', () => new Vue({ + el: '.js-vue-app', + data() { + const dataset = this.$options.el.dataset; + return { + endpoint: dataset.endpoint, + }; + }, + render(createElement) { + return createElement('my-component', { + props: { + endpoint: this.isLoading, + }, + }); + }, +})); +``` + +#### Accessing the `gl` object +When we need to query the `gl` object for data that won't change during the application's lyfecyle, we should do it in the same place where we query the DOM. +By following this practice, we can avoid the need to mock the `gl` object, which will make tests easier. +It should be done while initializing our Vue instance, and the data should be provided as `props` to the main component: + +##### example: +```javascript + +document.addEventListener('DOMContentLoaded', () => new Vue({ + el: '.js-vue-app', + render(createElement) { + return createElement('my-component', { + props: { + username: gon.current_username, + }, + }); + }, +})); +``` + ### A folder for Components This folder holds all components that are specific of this new feature. @@ -89,6 +143,29 @@ 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] +#### Components Gotchas +1. Using SVGs in components: To use an SVG in a template we need to make it a property we can access through the component. +A `prop` and a property returned by the `data` functions require `vue` to set a `getter` and a `setter` for each of them. +The SVG should be a computed property in order to improve performance, note that computed properties are cached based on their dependencies. + +```javascript +// bad +import svg from 'svg.svg'; +data() { + return { + myIcon: svg, + }; +}; + +// good +import svg from 'svg.svg'; +computed: { + myIcon() { + return svg; + } +} +``` + ### A folder for the Store The Store is a class that allows us to manage the state in a single @@ -430,11 +507,23 @@ describe('Todos App', () => { }); }); ``` +#### `mountComponent` helper +There is an helper in `spec/javascripts/helpers/vue_mount_component_helper.js` that allows you to mount a component with the given props: + +```javascript +import Vue from 'vue'; +import mountComponent from 'helpers/vue_mount_component_helper.js' +import component from 'component.vue' + +const Component = Vue.extend(component); +const data = {prop: 'foo'}; +const vm = mountComponent(Component, data); +``` + #### 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: @@ -481,6 +570,198 @@ new Component({ new Component().$mount(); ``` +## Vuex +To manage the state of an application you may use [Vuex][vuex-docs]. + +_Note:_ All of the below is explained in more detail in the official [Vuex documentation][vuex-docs]. + +### Separation of concerns +Vuex is composed of State, Getters, Mutations, Actions and Modules. + +When a user clicks on an action, we need to `dispatch` it. This action will `commit` a mutation that will change the state. +_Note:_ The action itself will not update the state, only a mutation should update the state. + +#### File structure +When using Vuex at GitLab, separate this concerns into different files to improve readability. If you can, separate the Mutation Types as well: + +``` +└── store + ├── index.js # where we assemble modules and export the store + ├── actions.js # actions + ├── mutations.js # mutations + ├── getters.js # getters + └── mutation_types.js # mutation types +``` +The following examples show an application that lists and adds users to the state. + +##### `index.js` +This is the entry point for our store. You can use the following as a guide: + +```javascript +import Vue from 'vue'; +import Vuex from 'vuex'; +import * as actions from './actions'; +import * as mutations from './mutations'; + +Vue.use(Vuex); + +export default new Vuex.Store({ + actions, + getters, + state: { + users: [], + }, +}); +``` +_Note:_ If the state of the application is too complex, an individual file for the state may be better. + +#### `actions.js` +An action commits a mutatation. In this file, we will write the actions that will call the respective mutation: + +```javascript + import * as types from './mutation-types' + + export const addUser = ({ commit }, user) => { + commit(types.ADD_USER, user); + }; +``` + +To dispatch an action from a component, use the `mapActions` helper: +```javascript +import { mapActions } from 'vuex'; + +{ + methods: { + ...mapActions([ + 'addUser', + ]), + onClickUser(user) { + this.addUser(user); + }, + }, +}; +``` + +#### `getters.js` +Sometimes we may need to get derived state based on store state, like filtering for a specific prop. This can be done through the `getters`: + +```javascript +// get all the users with pets +export getUsersWithPets = (state, getters) => { + return state.users.filter(user => user.pet !== undefined); +}; +``` + +To access a getter from a component, use the `mapGetters` helper: +```javascript +import { mapGetters } from 'vuex'; + +{ + computed: { + ...mapGetters([ + 'getUsersWithPets', + ]), + }, +}; +``` + +#### `mutations.js` +The only way to actually change state in a Vuex store is by committing a mutation. + +```javascript + import * as types from './mutation-types' + export default { + [types.ADD_USER](state, user) { + state.users.push(user); + }, + }; +``` + +#### `mutations_types.js` +From [vuex mutations docs][vuex-mutations]: +> It is a commonly seen pattern to use constants for mutation types in various Flux implementations. This allows the code to take advantage of tooling like linters, and putting all constants in a single file allows your collaborators to get an at-a-glance view of what mutations are possible in the entire application. + +```javascript +export const ADD_USER = 'ADD_USER'; +``` + +### How to include the store in your application +The store should be included in the main component of your application: +```javascript + // app.vue + import store from 'store'; // it will include the index.js file + + export default { + name: 'application', + store, + ... + }; +``` + +### Vuex Gotchas +1. Avoid calling a mutation directly. Always use an action to commit a mutation. Doing so will keep consistency through out the application. From Vuex docs: + + > why don't we just call store.commit('action') directly? Well, remember that mutations must be synchronous? Actions aren't. We can perform asynchronous operations inside an action. + + ```javascript + // component.vue + + // bad + created() { + this.$store.commit('mutation'); + } + + // good + created() { + this.$store.dispatch('action'); + } + ``` +1. When possible, use mutation types instead of hardcoding strings. It will be less error prone. +1. The State will be accessible in all components descending from the use where the store is instantiated. + +### Testing Vuex +#### Testing Vuex concerns +Refer to [vuex docs][vuex-testing] regarding testing Actions, Getters and Mutations. + +#### Testing components that need a store +Smaller components might use `store` properties to access the data. +In order to write unit tests for those components, we need to include the store and provide the correct state: + +```javascript +//component_spec.js +import Vue from 'vue'; +import store from './store'; +import component from './component.vue' + +describe('component', () => { + let vm; + let Component; + + beforeEach(() => { + Component = Vue.extend(issueActions); + }); + + afterEach(() => { + vm.$destroy(); + }); + + it('should show a user', () => { + const user = { + name: 'Foo', + age: '30', + }; + + // populate the store + store.dipatch('addUser', user); + + vm = new Component({ + store, + propsData: props, + }).$mount(); + }); +}); +``` + [vue-docs]: http://vuejs.org/guide/index.html [issue-boards]: https://gitlab.com/gitlab-org/gitlab-ce/tree/master/app/assets/javascripts/boards [environments-table]: https://gitlab.com/gitlab-org/gitlab-ce/tree/master/app/assets/javascripts/environments @@ -493,3 +774,7 @@ new Component().$mount(); [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 +[vuex-docs]: https://vuex.vuejs.org +[vuex-structure]: https://vuex.vuejs.org/en/structure.html +[vuex-mutations]: https://vuex.vuejs.org/en/mutations.html +[vuex-testing]: https://vuex.vuejs.org/en/testing.html diff --git a/doc/development/hash_indexes.md b/doc/development/hash_indexes.md new file mode 100644 index 00000000000..e6c1b3590b1 --- /dev/null +++ b/doc/development/hash_indexes.md @@ -0,0 +1,20 @@ +# Hash Indexes + +Both PostgreSQL and MySQL support hash indexes besides the regular btree +indexes. Hash indexes however are to be avoided at all costs. While they may +_sometimes_ provide better performance the cost of rehashing can be very high. +More importantly: at least until PostgreSQL 10.0 hash indexes are not +WAL-logged, meaning they are not replicated to any replicas. From the PostgreSQL +documentation: + +> Hash index operations are not presently WAL-logged, so hash indexes might need +> to be rebuilt with REINDEX after a database crash if there were unwritten +> changes. Also, changes to hash indexes are not replicated over streaming or +> file-based replication after the initial base backup, so they give wrong +> answers to queries that subsequently use them. For these reasons, hash index +> use is presently discouraged. + +RuboCop is configured to register an offence when it detects the use of a hash +index. + +Instead of using hash indexes you should use regular btree indexes. diff --git a/doc/development/i18n_guide.md b/doc/development/i18n_guide.md index 756535e28bc..bd0ef39ca62 100644 --- a/doc/development/i18n_guide.md +++ b/doc/development/i18n_guide.md @@ -138,6 +138,47 @@ translations. There's no need to generate `.po` files. 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. +### Validating PO files + +To make sure we keep our translation files up to date, there's a linter that is +running on CI as part of the `static-analysis` job. + +To lint the adjustments in PO files locally you can run `rake gettext:lint`. + +The linter will take the following into account: + +- Valid PO-file syntax +- Variable usage + - Only one unnamed (`%d`) variable, since the order of variables might change + in different languages + - All variables used in the message-id are used in the translation + - There should be no variables used in a translation that aren't in the + message-id +- Errors during translation. + +The errors are grouped per file, and per message ID: + +``` +Errors in `locale/zh_HK/gitlab.po`: + PO-syntax errors + SimplePoParser::ParserErrorSyntax error in lines + Syntax error in msgctxt + Syntax error in msgid + Syntax error in msgstr + Syntax error in message_line + There should be only whitespace until the end of line after the double quote character of a message text. + Parseing result before error: '{:msgid=>["", "You are going to remove %{project_name_with_namespace}.\\n", "Removed project CANNOT be restored!\\n", "Are you ABSOLUTELY sure?"]}' + SimplePoParser filtered backtrace: SimplePoParser::ParserError +Errors in `locale/zh_TW/gitlab.po`: + 1 pipeline + <%d 條流水線> is using unknown variables: [%d] + Failure translating to zh_TW with []: too few arguments +``` + +In this output the `locale/zh_HK/gitlab.po` has syntax errors. +The `locale/zh_TW/gitlab.po` has variables that are used in the translation that +aren't in the message with id `1 pipeline`. + ## Working with special content ### Interpolation diff --git a/doc/development/img/manual_build_docs.png b/doc/development/img/manual_build_docs.png Binary files differnew file mode 100644 index 00000000000..fef767c2a79 --- /dev/null +++ b/doc/development/img/manual_build_docs.png diff --git a/doc/development/licensing.md b/doc/development/licensing.md index 1f115059fb8..9a5811d8474 100644 --- a/doc/development/licensing.md +++ b/doc/development/licensing.md @@ -55,6 +55,7 @@ Libraries with the following licenses are acceptable for use: - [BSD 3-Clause License][BSD-3-Clause] (also known as New BSD or Modified BSD): A permissive (non-copyleft) license as defined by the Open Source Initiative - [ISC License][ISC] (also known as the OpenBSD License): A permissive (non-copyleft) license as defined by the Open Source Initiative. - [Creative Commons Zero (CC0)][CC0]: A public domain dedication, recommended as a way to disclaim copyright on your work to the maximum extent possible. +- [Unlicense][UNLICENSE]: Another public domain dedication. ## Unacceptable Licenses @@ -63,10 +64,11 @@ Libraries with the following licenses are unacceptable for use: - [GNU GPL][GPL] (version 1, [version 2][GPLv2], [version 3][GPLv3], or any future versions): GPL-licensed libraries cannot be linked to from non-GPL projects. - [GNU AGPLv3][AGPLv3]: AGPL-licensed libraries cannot be linked to from non-GPL projects. - [Open Software License (OSL)][OSL]: is a copyleft license. In addition, the FSF [recommend against its use][OSL-GNU]. +- [Facebook BSD + PATENTS][Facebook]: is a 3-clause BSD license with a patent grant that has been deemed [Category X][x-list] by the Apache foundation. ## Requesting Approval for Licenses -Libraries that are not listed in the [Acceptable Licenses][Acceptable-Licenses] or [Unacceptable Licenses][Unacceptable-Licenses] list can be submitted to the legal team for review. Please create an issue in the [Organization Repository][Org-Repo] and cc `@gl-legal`. After a decision has been made, the original requestor is responsible for updating this document. +Libraries that are not listed in the [Acceptable Licenses][Acceptable-Licenses] or [Unacceptable Licenses][Unacceptable-Licenses] list can be submitted to the legal team for review. Please email `legal@gitlab.com` with the details. After a decision has been made, the original requestor is responsible for updating this document. ## Notes @@ -101,5 +103,8 @@ Gems which are included only in the "development" or "test" groups by Bundler ar [OSL]: https://opensource.org/licenses/OSL-3.0 [OSL-GNU]: https://www.gnu.org/licenses/license-list.en.html#OSL [Org-Repo]: https://gitlab.com/gitlab-com/organization +[UNLICENSE]: https://unlicense.org +[Facebook]: https://code.facebook.com/pages/850928938376556 +[x-list]: https://www.apache.org/legal/resolved.html#category-x [Acceptable-Licenses]: #acceptable-licenses [Unacceptable-Licenses]: #unacceptable-licenses diff --git a/doc/development/ordering_table_columns.md b/doc/development/ordering_table_columns.md new file mode 100644 index 00000000000..249e70c7b0e --- /dev/null +++ b/doc/development/ordering_table_columns.md @@ -0,0 +1,127 @@ +# Ordering Table Columns + +Similar to C structures the space of a table is influenced by the order of +columns. This is because the size of columns is aligned depending on the type of +the column. Take the following column order for example: + +* id (integer, 4 bytes) +* name (text, variable) +* user_id (integer, 4 bytes) + +Integers are aligned to the word size. This means that on a 64 bit platform the +actual size of each column would be: 8 bytes, variable, 8 bytes. This means that +each row will require at least 16 bytes for the two integers, and a variable +amount for the text field. If a table has a few rows this is not an issue, but +once you start storing millions of rows you can save space by using a different +order. For the above example a more ideal column order would be the following: + +* id (integer, 4 bytes) +* user_id (integer, 4 bytes) +* name (text, variable) + +In this setup the `id` and `user_id` columns can be packed together, which means +we only need 8 bytes to store _both_ of them. This in turn each row will require +8 bytes less of space. + +For GitLab we require that columns of new tables are ordered based to use the +least amount of space. An easy way of doing this is to order them based on the +type size in descending order with variable sizes (string and text columns for +example) at the end. + +## Type Sizes + +While the PostgreSQL docuemntation +(https://www.postgresql.org/docs/current/static/datatype.html) contains plenty +of information we will list the sizes of common types here so it's easier to +look them up. Here "word" refers to the word size, which is 4 bytes for a 32 +bits platform and 8 bytes for a 64 bits platform. + +| Type | Size | Aligned To | +|:-----------------|:-------------------------------------|:-----------| +| smallint | 2 bytes | 1 word | +| integer | 4 bytes | 1 word | +| bigint | 8 bytes | 8 bytes | +| real | 4 bytes | 1 word | +| double precision | 8 bytes | 8 bytes | +| boolean | 1 byte | not needed | +| text / string | variable, 1 byte plus the data | 1 word | +| bytea | variable, 1 or 4 bytes plus the data | 1 word | +| timestamp | 8 bytes | 8 bytes | +| timestamptz | 8 bytes | 8 bytes | +| date | 4 bytes | 1 word | + +A "variable" size means the actual size depends on the value being stored. If +PostgreSQL determines this can be embedded directly into a row it may do so, but +for very large values it will store the data externally and store a pointer (of +1 word in size) in the column. Because of this variable sized columns should +always be at the end of a table. + +## Real Example + +Let's use the "events" table as an example, which currently has the following +layout: + +| Column | Type | Size | +|:------------|:----------------------------|:---------| +| id | integer | 4 bytes | +| target_type | character varying | variable | +| target_id | integer | 4 bytes | +| title | character varying | variable | +| data | text | variable | +| project_id | integer | 4 bytes | +| created_at | timestamp without time zone | 8 bytes | +| updated_at | timestamp without time zone | 8 bytes | +| action | integer | 4 bytes | +| author_id | integer | 4 bytes | + +After adding padding to align the columns this would translate to columns being +divided into fixed size chunks as follows: + +| Chunk Size | Columns | +|:-----------|:------------------| +| 8 bytes | id | +| variable | target_type | +| 8 bytes | target_id | +| variable | title | +| variable | data | +| 8 bytes | project_id | +| 8 bytes | created_at | +| 8 bytes | updated_at | +| 8 bytes | action, author_id | + +This means that excluding the variable sized data we need at least 48 bytes per +row. + +We can optimise this by using the following column order instead: + +| Column | Type | Size | +|:------------|:----------------------------|:---------| +| created_at | timestamp without time zone | 8 bytes | +| updated_at | timestamp without time zone | 8 bytes | +| id | integer | 4 bytes | +| target_id | integer | 4 bytes | +| project_id | integer | 4 bytes | +| action | integer | 4 bytes | +| author_id | integer | 4 bytes | +| target_type | character varying | variable | +| title | character varying | variable | +| data | text | variable | + +This would produce the following chunks: + +| Chunk Size | Columns | +|:-----------|:-------------------| +| 8 bytes | created_at | +| 8 bytes | updated_at | +| 8 bytes | id, target_id | +| 8 bytes | project_id, action | +| 8 bytes | author_id | +| variable | target_type | +| variable | title | +| variable | data | + +Here we only need 40 bytes per row excluding the variable sized data. 8 bytes +being saved may not sound like much, but for tables as large as the "events" +table it does begin to matter. For example, when storing 80 000 000 rows this +translates to a space saving of at least 610 MB: all by just changing the order +of a few columns. diff --git a/doc/development/serializing_data.md b/doc/development/serializing_data.md index 2b56f48bc44..37332c20147 100644 --- a/doc/development/serializing_data.md +++ b/doc/development/serializing_data.md @@ -1,7 +1,8 @@ # Serializing Data **Summary:** don't store serialized data in the database, use separate columns -and/or tables instead. +and/or tables instead. This includes storing of comma separated values as a +string. Rails makes it possible to store serialized data in JSON, YAML or other formats. Such a field can be defined as follows: diff --git a/doc/development/sql.md b/doc/development/sql.md index 23fd7604957..974b1d99dff 100644 --- a/doc/development/sql.md +++ b/doc/development/sql.md @@ -216,4 +216,30 @@ exact same results. This also means there's no need to add an index on `created_at` to ensure consistent performance as `id` is already indexed by default. +## Use WHERE EXISTS instead of WHERE IN + +While `WHERE IN` and `WHERE EXISTS` can be used to produce the same data it is +recommended to use `WHERE EXISTS` whenever possible. While in many cases +PostgreSQL can optimise `WHERE IN` quite well there are also many cases where +`WHERE EXISTS` will perform (much) better. + +In Rails you have to use this by creating SQL fragments: + +```ruby +Project.where('EXISTS (?)', User.select(1).where('projects.creator_id = users.id AND users.foo = X')) +``` + +This would then produce a query along the lines of the following: + +```sql +SELECT * +FROM projects +WHERE EXISTS ( + SELECT 1 + FROM users + WHERE projects.creator_id = users.id + AND users.foo = X +) +``` + [gin-index]: http://www.postgresql.org/docs/current/static/gin.html diff --git a/doc/development/testing.md b/doc/development/testing.md index 3d5aa3d45e9..83269303005 100644 --- a/doc/development/testing.md +++ b/doc/development/testing.md @@ -157,8 +157,9 @@ 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 + 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 system 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) @@ -188,24 +189,34 @@ Please consult the [dedicated "Frontend testing" guide](./fe_guide/testing.md). ### General Guidelines - Use a single, top-level `describe ClassName` block. -- 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. -- Use multi-line `do...end` blocks for `before` and `after`, even when it would - fit on a single line. - 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_). - 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'` +- Use `Gitlab.config.gitlab.host` rather than hard coding `'localhost'` +- 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. - On `before` and `after` hooks, prefer it scoped to `:context` over `:all` [four-phase-test]: https://robots.thoughtbot.com/four-phase-test +### Automatic retries and flaky tests detection + +On our CI, we use [rspec-retry] to automatically retry a failing example a few +times (see [`spec/spec_helper.rb`] for the precise retries count). + +We also use a home-made `RspecFlaky::Listener` listener which records flaky +examples in a JSON report file on `master` (`retrieve-tests-metadata` and `update-tests-metadata` jobs), and warns when a new flaky example +is detected in any other branch (`flaky-examples-check` job). In the future, the +`flaky-examples-check` job will not be allowed to fail. + +[rspec-retry]: https://github.com/NoRedInk/rspec-retry +[`spec/spec_helper.rb`]: https://gitlab.com/gitlab-org/gitlab-ce/blob/master/spec/spec_helper.rb + ### `let` variables GitLab's RSpec suite has made extensive use of `let` variables to reduce @@ -268,6 +279,43 @@ end - Avoid scenario titles that add no information, such as "successfully". - Avoid scenario titles that repeat the feature title. +### Table-based / Parameterized tests + +This style of testing is used to exercise one piece of code with a comprehensive +range of inputs. By specifying the test case once, alongside a table of inputs +and the expected output for each, your tests can be made easier to read and more +compact. + +We use the [rspec-parameterized](https://github.com/tomykaira/rspec-parameterized) +gem. A short example, using the table syntax and checking Ruby equality for a +range of inputs, might look like this: + +```ruby +describe "#==" do + using Rspec::Parameterized::TableSyntax + + let(:project1) { create(:project) } + let(:project2) { create(:project) } + where(:a, :b, :result) do + 1 | 1 | true + 1 | 2 | false + true | true | true + true | false | false + project1 | project1 | true + project2 | project2 | true + project 1 | project2 | false + end + + with_them do + it { expect(a == b).to eq(result) } + + it 'is isomorphic' do + expect(b == a).to eq(result) + end + end +end +``` + ### Matchers Custom matchers should be created to clarify the intent and/or hide the @@ -276,6 +324,15 @@ complexity of RSpec expectations.They should be placed under a certain type of specs only (e.g. features, requests etc.) but shouldn't be if they apply to multiple type of specs. +#### have_gitlab_http_status + +Prefer `have_gitlab_http_status` over `have_http_status` because the former +could also show the response body whenever the status mismatched. This would +be very useful whenever some tests start breaking and we would love to know +why without editing the source and rerun the tests. + +This is especially useful whenever it's showing 500 internal server error. + ### Shared contexts All shared contexts should be be placed under `spec/support/shared_contexts/`. @@ -472,10 +529,7 @@ slowest test files and try to improve them. ## 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. +- On CE and EE, the test suite runs both PostgreSQL and MySQL. - Rails logging to `log/test.log` is disabled by default in CI [for performance reasons][logging]. To override this setting, provide the `RAILS_ENABLE_TEST_LOG` environment variable. diff --git a/doc/development/verifying_database_capabilities.md b/doc/development/verifying_database_capabilities.md new file mode 100644 index 00000000000..cc6d62957e3 --- /dev/null +++ b/doc/development/verifying_database_capabilities.md @@ -0,0 +1,26 @@ +# Verifying Database Capabilities + +Sometimes certain bits of code may only work on a certain database and/or +version. While we try to avoid such code as much as possible sometimes it is +necessary to add database (version) specific behaviour. + +To facilitate this we have the following methods that you can use: + +* `Gitlab::Database.postgresql?`: returns `true` if PostgreSQL is being used +* `Gitlab::Database.mysql?`: returns `true` if MySQL is being used +* `Gitlab::Database.version`: returns the PostgreSQL version number as a string + in the format `X.Y.Z`. This method does not work for MySQL + +This allows you to write code such as: + +```ruby +if Gitlab::Database.postgresql? + if Gitlab::Database.version.to_f >= 9.6 + run_really_fast_query + else + run_fast_query + end +else + run_query +end +``` diff --git a/doc/development/writing_documentation.md b/doc/development/writing_documentation.md index eac9ec2a470..479258f743e 100644 --- a/doc/development/writing_documentation.md +++ b/doc/development/writing_documentation.md @@ -103,3 +103,24 @@ 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. + +## Previewing the changes live + +If you want to preview your changes live, you can use the manual `build-docs` +job in your merge request. + + + +This job will: + +1. Create a new branch in the [gitlab-docs](https://gitlab.com/gitlab-com/gitlab-docs) + project named after the scheme: `<CE/EE-branch-slug>-built-from-ce-ee` +1. Trigger a pipeline and build the docs site with your changes + +Look for the docs URL at the output of the `build-docs` job. + +>**Note:** +Make sure that you always delete the branch of the merge request you were +working on. If you don't, the remote docs branch won't be removed either, +and the server where the Review Apps are hosted will eventually be out of +disk space. |
