diff options
Diffstat (limited to 'doc/development')
22 files changed, 465 insertions, 130 deletions
diff --git a/doc/development/README.md b/doc/development/README.md index ea5d9e10e2c..99c88146be5 100644 --- a/doc/development/README.md +++ b/doc/development/README.md @@ -113,6 +113,10 @@ description: 'Learn how to contribute to GitLab.' - [Database helper modules](database_helpers.md) - [Code comments](code_comments.md) +## Case studies + +- [Database case study: Filtering by label](filtering_by_label.md) + ## Integration guides - [Jira Connect app](integrations/jira_connect.md) diff --git a/doc/development/chaos_endpoints.md b/doc/development/chaos_endpoints.md index eb6dde2d24e..961520db7d8 100644 --- a/doc/development/chaos_endpoints.md +++ b/doc/development/chaos_endpoints.md @@ -69,7 +69,7 @@ curl http://localhost:3000/-/chaos/leakmem?memory_mb=1024&duration_s=10&token=se This endpoint attempts to fully utilise a single core, at 100%, for the given period. -Depending on your rack server setup, your request may timeout after a predermined period (normally 60 seconds). +Depending on your rack server setup, your request may timeout after a predetermined period (normally 60 seconds). If you're using Unicorn, this is done by killing the worker process. ``` @@ -80,7 +80,7 @@ GET /-/chaos/cpu_spin?duration_s=50&async=true | Attribute | Type | Required | Description | | ------------ | ------- | -------- | --------------------------------------------------------------------- | -| `duration_s` | integer | no | Duration, in seconds, that the core will be utilised. Defaults to 30s | +| `duration_s` | integer | no | Duration, in seconds, that the core will be utilized. Defaults to 30s | | `async` | boolean | no | Set to true to consume CPU in a Sidekiq background worker process | ```bash @@ -93,7 +93,7 @@ curl http://localhost:3000/-/chaos/cpu_spin?duration_s=60&token=secret This endpoint attempts to fully utilise a single core, and interleave it with DB request, for the given period. This endpoint can be used to model yielding execution to another threads when running concurrently. -Depending on your rack server setup, your request may timeout after a predermined period (normally 60 seconds). +Depending on your rack server setup, your request may timeout after a predetermined period (normally 60 seconds). If you're using Unicorn, this is done by killing the worker process. ``` @@ -105,7 +105,7 @@ GET /-/chaos/db_spin?duration_s=50&async=true | Attribute | Type | Required | Description | | ------------ | ------- | -------- | --------------------------------------------------------------------------- | | `interval_s` | float | no | Interval, in seconds, for every DB request. Defaults to 1s | -| `duration_s` | integer | no | Duration, in seconds, that the core will be utilised. Defaults to 30s | +| `duration_s` | integer | no | Duration, in seconds, that the core will be utilized. Defaults to 30s | | `async` | boolean | no | Set to true to perform the operation in a Sidekiq background worker process | ```bash diff --git a/doc/development/code_review.md b/doc/development/code_review.md index 709cd0c806b..b7d74b17eb3 100644 --- a/doc/development/code_review.md +++ b/doc/development/code_review.md @@ -365,6 +365,31 @@ Enterprise Edition instance. This has some implications: 1. **Filesystem access** can be slow, so try to avoid [shared files](shared_files.md) when an alternative solution is available. +## Examples + +How code reviews are conducted can surprise new contributors. Here are some examples of code reviews that should help to orient you as to what to expect. + +**["Modify `DiffNote` to reuse it for Designs"](https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/13703):** +It contained everything from nitpicks around newlines to reasoning +about what versions for designs are, how we should compare them +if there was no previous version of a certain file (parent vs. +blank `sha` vs empty tree). + +**["Support multi-line suggestions"](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/25211)**: +The MR itself consists of a collaboration between FE and BE, +and documenting comments from the author for the reviewer. +There's some nitpicks, some questions for information, and +towards the end, a security vulnerability. + +**["Allow multiple repositories per project"](https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/10251)**: +ZJ referred to the other projects (workhorse) this might impact, +suggested some improvements for consistency. And James' comments +helped us with overall code quality (using delegation, `&.` those +types of things), and making the code more robust. + +**["Support multiple assignees for merge requests"](https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/10161)**: +A good example of collaboration on an MR touching multiple parts of the codebase. Nick pointed out interesting edge cases, James Lopes also joined in raising concerns on import/export feature. + ### Credits Largely based on the [thoughtbot code review guide]. diff --git a/doc/development/contributing/community_roles.md b/doc/development/contributing/community_roles.md index 3296cb173d7..7d2d1b77a0e 100644 --- a/doc/development/contributing/community_roles.md +++ b/doc/development/contributing/community_roles.md @@ -1,4 +1,4 @@ -### Community members & roles +# Community members & roles GitLab community members and their privileges/responsibilities. diff --git a/doc/development/contributing/issue_workflow.md b/doc/development/contributing/issue_workflow.md index abe11b8d1a8..79c701d7abf 100644 --- a/doc/development/contributing/issue_workflow.md +++ b/doc/development/contributing/issue_workflow.md @@ -102,20 +102,7 @@ any issue. Stage labels specify which [DevOps stage][devops-stages] the issue belongs to. -The current stage labels are: - -- ~"devops::manage" -- ~"devops::plan" -- ~"devops::create" -- ~"devops::verify" -- ~"devops::package" -- ~"devops::release" -- ~"devops::configure" -- ~"devops::monitor" -- ~"devops::secure" -- ~"devops::defend" -- ~"devops::growth" -- ~"devops::enablement" +The current stage labels can be found by [searching the labels list for `devops::`](https://gitlab.com/groups/gitlab-org/-/labels?search=devops%3A%3A). These labels are [scoped labels](../../user/project/labels.md#scoped-labels-premium) and thus are mutually exclusive. @@ -138,44 +125,8 @@ The Stage labels are used to generate the [direction pages][direction-pages] aut Group labels specify which [groups][structure-groups] the issue belongs to. -The current group labels are: - -- ~"group::access" -- ~"group::measure" -- ~"group::source code" -- ~"group::knowledge" -- ~"group::editor" -- ~"group::gitaly" -- ~"group::gitter" -- ~"group::team planning" -- ~"group::enterprise planning" -- ~"group::certify" -- ~"group::ci and runner" -- ~"group::testing" -- ~"group::package" -- ~"group::progressive delivery" -- ~"group::release management" -- ~"group::autodevops and kubernetes" -- ~"group::serverless and paas" -- ~"group::apm" -- ~"group::health" -- ~"group::static analysis" -- ~"group::dynamic analysis" -- ~"group::software composition analysis" -- ~"group::runtime application security" -- ~"group::threat management" -- ~"group::application infrastructure security" -- ~"group::activation" -- ~"group::adoption" -- ~"group::upsell" -- ~"group::retention" -- ~"group::fulfillment" -- ~"group::telemetry" -- ~"group::distribution" -- ~"group::geo" -- ~"group::memory" -- ~"group::ecosystem" - +The current group labels can be found by [searching the labels list for `group::`](https://gitlab.com/groups/gitlab-org/-/labels?search=group%3A%3A). + These labels are [scoped labels](../../user/project/labels.md#scoped-labels-premium) and thus are mutually exclusive. @@ -248,7 +199,7 @@ If a bug seems to fall between two severity labels, assign it to the higher-seve - Example(s) of ~S1 - Data corruption/loss. - Security breach. - - Unable to create an issue or merge request. + - Unable to create an issue or merge request. - Unable to add a comment or thread to the issue or merge request. - Example(s) of ~S2 - Cannot submit changes through the web IDE but the commandline works. @@ -259,7 +210,7 @@ If a bug seems to fall between two severity labels, assign it to the higher-seve - Example(s) of ~S4 - Label colors are incorrect. - UI elements are not fully aligned. - + ## Label for community contributors Issues that are beneficial to our users, 'nice to haves', that we currently do diff --git a/doc/development/contributing/style_guides.md b/doc/development/contributing/style_guides.md index 5c6ea1f469d..7832850a9f0 100644 --- a/doc/development/contributing/style_guides.md +++ b/doc/development/contributing/style_guides.md @@ -23,6 +23,7 @@ 1. Code should be written in [US English][us-english] 1. [Go](../go_guide/index.md) 1. [Python](../python_guide/index.md) +1. [Shell scripting](../shell_scripting_guide/index.md) This is also the style used by linting tools such as [RuboCop](https://github.com/rubocop-hq/rubocop) and [Hound CI](https://houndci.com). diff --git a/doc/development/database_review.md b/doc/development/database_review.md index 1413c2f69fb..3d10a0c84e5 100644 --- a/doc/development/database_review.md +++ b/doc/development/database_review.md @@ -68,6 +68,17 @@ make sure you have applied the ~database label and rerun the `danger-review` CI job, or pick someone from the [`@gl-database` team](https://gitlab.com/groups/gl-database/-/group_members). +### How to prepare for speedy database reviews + +In order to make reviewing easier and therefore faster, please consider preparing a comment +and details for a database reviewer: + +- Provide queries in SQL form rather than ActiveRecord. +- Format any queries with a SQL query formatter, for example with [sqlformat.darold.net](http://sqlformat.darold.net). +- Consider providing query plans via a link to [explain.depesz.com](https://explain.depesz.com) or another tool instead of textual form. +- For query changes, it is best to provide the SQL query along with a plan *before* and *after* the change. This helps to spot differences quickly. +- When providing query plans, make sure to use good parameter values, so that the query executed is a good example and also hits enough data. Usually, the `gitlab-org` namespace (`namespace_id = 9970`) and the `gitlab-org/gitlab-ce` project (`project_id = 13083`) provides enough data to serve as a good example. + ### How to review for database - Check migrations diff --git a/doc/development/documentation/index.md b/doc/development/documentation/index.md index 36a2f47a55b..d717b17136e 100644 --- a/doc/development/documentation/index.md +++ b/doc/development/documentation/index.md @@ -57,35 +57,22 @@ See the [Structure](styleguide.md#structure) section of the [Documentation Style We currently maintain two sets of docs: one in the [gitlab-ce](https://gitlab.com/gitlab-org/gitlab-ce/tree/master/doc) repo and one in [gitlab-ee](https://gitlab.com/gitlab-org/gitlab-ee/tree/master/doc). -They are similar, and most pages are identical, but they are different repositories. -With the single codebase effort, we want to make those two sets identical, so when the -time comes to have only one codebase, we'll be ready. - -Here are some links to get you up to speed with the current effort: - -- [CE/EE codebases blueprint](https://about.gitlab.com/handbook/engineering/infrastructure/blueprint/ce-ee-codebases/) -- [CE/EE codebases merge design](https://about.gitlab.com/handbook/engineering/infrastructure/design/merge-ce-ee-codebases/) -- [Single docs codebase epic](https://gitlab.com/groups/gitlab-org/-/epics/199) -- [Issue board of related issues](https://gitlab.com/groups/gitlab-org/-/boards/981090?&label_name[]=Documentation&label_name[]=single%20codebase) -- [Related merge requests](https://gitlab.com/groups/gitlab-org/-/merge_requests?scope=all&utf8=%E2%9C%93&state=all&label_name[]=Documentation&label_name[]=single%20codebase) -- [Visualize the existing diffs](https://leipert-projects.gitlab.io/is-gitlab-pretty-yet/diff/?search=%5Edoc) +They are identical, but they are different repositories. When the +time comes to have only one codebase for the GitLab project, we'll be ready. ### CE first -After a given documentation path is aligned across CE and EE, all merge requests -affecting that path must be submitted to CE, regardless of the content it has. -This means that: +All merge requests for documentation must be submitted to CE, regardless of the content +it has. This means that: -- For **EE-only docs changes**, you only have to submit a CE MR. +- For **EE-only docs changes**, you only have to submit an MR in the CE project. - For **EE-only features** that touch both the code and the docs, you have to submit - an EE MR containing all changes, and a CE MR containing only the docs changes + an EE MR containing all code changes, and a CE MR containing only the docs changes and without a changelog entry. This might seem like a duplicate effort, but it's only for the short term. -A list of the already aligned docs can be found in -[the epic description](https://gitlab.com/groups/gitlab-org/-/epics/199#ee-specific-lines-check). -Since the docs will be combined, it's crucial to add the relevant +Since the CE and EE docs are combined, it's crucial to add the relevant [product badges](styleguide.md#product-badges) for all EE documentation, so that we can discern which features belong to which tier. @@ -94,19 +81,9 @@ we can discern which features belong to which tier. There's a special test in place ([`ee_specific_check.rb`](https://gitlab.com/gitlab-org/gitlab-ee/blob/master/scripts/ee_specific_check/ee_specific_check.rb)), which, among others, checks and prevents creating/editing new files and directories -in EE under `doc/`. - -We have a long list of documentation paths that are either whitelisted or not. -Paths in the whitelist (not commented out) will not be subject to the test, -which means you are allowed to create/change docs content in EE for the time -being. The goal is to not have any doc whitelisted. - -At the time of this writing, the only items left to be aligned are the API docs: - -- `doc/api/*` ([issue](https://gitlab.com/gitlab-org/gitlab-ce/issues/60045) / [merge request](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/27491)) - -Eventually, once all docs are aligned, we'll remove any doc reference from that -script, so it catches everything. +in EE under `doc/`. This should fail when changes to anything in `/doc` are submitted +in an EE MR. To pass the test, simply remove the docs changes from the EE MR, and +[submit them in CE](#ce-first). ## Changing document location @@ -488,15 +465,17 @@ Currently, the following tests are in place: that all cURL examples in API docs use the full switches. It's recommended to [check locally](#previewing-the-changes-live) before pushing to GitLab by executing the command `bundle exec nanoc check internal_links` on your local - [`gitlab-docs`](https://gitlab.com/gitlab-org/gitlab-docs) directory. + [`gitlab-docs`](https://gitlab.com/gitlab-org/gitlab-docs) directory. In addition, + `docs-lint` also runs [markdownlint](styleguide.md#markdown-rules) to ensure the + markdown is consistent across all documentation. 1. [`ee_compat_check`](../automatic_ce_ee_merge.md#avoiding-ce-ee-merge-conflicts-beforehand) (runs on CE only): - When you submit a merge request to GitLab Community Edition (CE), - there is this additional job 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. - As CE is merged into EE once a day, it's important to avoid merge conflicts. - Submitting an EE-equivalent merge request cherry-picking all commits from CE to EE is - essential to avoid them. + When you submit a merge request to GitLab Community Edition (CE), + there is this additional job 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. + As CE is merged into EE once a day, it's important to avoid merge conflicts. + Submitting an EE-equivalent merge request cherry-picking all commits from CE to EE is + essential to avoid them. 1. [`ee-files-location-check`/`ee-specific-lines-check`](#ee-specific-lines-check) (runs on EE only): This test ensures that no new files/directories are created/changed in EE. All docs should be submitted in CE instead, regardless the tier they are on. @@ -559,15 +538,16 @@ A file with `proselint` configuration must be placed in a #### `markdownlint` `markdownlint` checks that certain rules ([example](https://github.com/DavidAnson/markdownlint/blob/master/README.md#rules--aliases)) - are followed for Markdown syntax. - Our [Documentation Style Guide](styleguide.md) and [Markdown Guide](https://about.gitlab.com/handbook/product/technical-writing/markdown-guide/) - elaborate on which choices must be made when selecting Markdown syntax for - GitLab documentation. This tool helps catch deviations from those guidelines. +are followed for Markdown syntax. Our [Documentation Style Guide](styleguide.md) and +[Markdown Guide](https://about.gitlab.com/handbook/product/technical-writing/markdown-guide/) +elaborate on which choices must be made when selecting Markdown syntax for GitLab +documentation. This tool helps catch deviations from those guidelines, and matches the +tests run on the documentation by [`docs-lint`](#testing). `markdownlint` can be used [on the command line](https://github.com/igorshubovych/markdownlint-cli#markdownlint-cli--), - either on a single Markdown file or on all Markdown files in a project. For example, to run - `markdownlint` on all documentation in the [`gitlab-ce` project](https://gitlab.com/gitlab-org/gitlab-ce), - run the following commands from within the `gitlab-ce` project: +either on a single Markdown file or on all Markdown files in a project. For example, to run +`markdownlint` on all documentation in the [`gitlab-ce` project](https://gitlab.com/gitlab-org/gitlab-ce), +run the following commands from within the `gitlab-ce` project: ```sh cd doc @@ -597,7 +577,7 @@ The following sample `markdownlint` configuration modifies the available default "line-length": false, "no-trailing-punctuation": false, "ol-prefix": { "style": "one" }, - "blanks-around-fences": false, + "blanks-around-fences": true, "no-inline-html": { "allowed_elements": [ "table", @@ -612,11 +592,15 @@ The following sample `markdownlint` configuration modifies the available default "a", "strong", "i", - "div" + "div", + "b" ] }, "hr-style": { "style": "---" }, - "fenced-code-language": false + "code-block-style": { "style": "fenced" }, + "fenced-code-language": false, + "no-duplicate-header": { "allow_different_nesting": true }, + "commands-show-output": false } ``` diff --git a/doc/development/documentation/styleguide.md b/doc/development/documentation/styleguide.md index ccf1deefd91..dd798777c12 100644 --- a/doc/development/documentation/styleguide.md +++ b/doc/development/documentation/styleguide.md @@ -107,6 +107,22 @@ Hard-coded HTML is valid, although it's discouraged to be used while we have `/h - Special styling is required. - Reviewed and approved by a technical writer. +### Markdown Rules + +GitLab ensures that the Markdown used across all documentation is consistent, as +well as easy to review and maintain, by testing documentation changes with +[Markdownlint (mdl)](https://github.com/markdownlint/markdownlint). This lint test +checks many common problems with Markdown, and fails when any document has an issue +with Markdown formatting that may cause the page to render incorrectly within GitLab. +It will also fail when a document is using non-standard Markdown (which may render +correctly, but is not the current standard in GitLab documentation). + +Each formatting issue that mdl checks has an associated [rule](https://github.com/markdownlint/markdownlint/blob/master/docs/RULES.md), +and the rules that are currently enabled for GitLab documentation are listed in the +[`.mdlrc.style`](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/.mdlrc.style) file. +Configuration options are set in the [`.mdlrc`](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/.mdlrc.style) +file. + ## Structure ### Organize by topic, not by type @@ -1076,6 +1092,6 @@ curl --request PUT --header "PRIVATE-TOKEN: <your_access_token>" --data "domain_ [cURL]: http://curl.haxx.se/ "cURL website" [single spaces]: http://www.slate.com/articles/technology/technology/2011/01/space_invaders.html -[gfm]: https://docs.gitlab.com/ce/user/markdown.html#newlines "GitLab flavored markdown documentation" +[gfm]: ../../user/markdown.md#newlines "GitLab flavored markdown documentation" [ce-1242]: https://gitlab.com/gitlab-org/gitlab-ce/issues/1242 [doc-restart]: ../../administration/restart_gitlab.md "GitLab restart documentation" diff --git a/doc/development/fe_guide/axios.md b/doc/development/fe_guide/axios.md index 0d9397c3bd5..09b4a3c3d96 100644 --- a/doc/development/fe_guide/axios.md +++ b/doc/development/fe_guide/axios.md @@ -1,15 +1,18 @@ # Axios + We use [axios][axios] to communicate with the server in Vue applications and most new code. In order to guarantee all defaults are set you *should not use `axios` directly*, you should import `axios` from `axios_utils`. ## CSRF token + All our request require a CSRF token. To guarantee this token is set, we are importing [axios][axios], setting the token, and exporting `axios` . This exported module should be used instead of directly using `axios` to ensure the token is set. ## Usage + ```javascript import axios from './lib/utils/axios_utils'; diff --git a/doc/development/fe_guide/style_guide_scss.md b/doc/development/fe_guide/style_guide_scss.md index f895cfd36ac..95c4a094c04 100644 --- a/doc/development/fe_guide/style_guide_scss.md +++ b/doc/development/fe_guide/style_guide_scss.md @@ -212,6 +212,7 @@ selectors are intended for use only with JavaScript to allow for removal or renaming without breaking styling. ### IDs + Don't use ID selectors in CSS. ```scss diff --git a/doc/development/filtering_by_label.md b/doc/development/filtering_by_label.md new file mode 100644 index 00000000000..6e6b71b1787 --- /dev/null +++ b/doc/development/filtering_by_label.md @@ -0,0 +1,166 @@ +# Filtering by label + +## Introduction + +GitLab has [labels](../user/project/labels.md) that can be assigned to issues, +merge requests, and epics. Labels on those objects are a many-to-many relation +through the polymorphic `label_links` table. + +To filter these objects by multiple labels - for instance, 'all open +issues with the label ~Plan and the label ~backend' - we generate a +query containing a `GROUP BY` clause. In a simple form, this looks like: + +```sql +SELECT + issues.* +FROM + issues + INNER JOIN label_links ON label_links.target_id = issues.id + AND label_links.target_type = 'Issue' + INNER JOIN labels ON labels.id = label_links.label_id +WHERE + issues.project_id = 13083 + AND (issues.state IN ('opened')) + AND labels.title IN ('Plan', + 'backend') +GROUP BY + issues.id +HAVING (COUNT(DISTINCT labels.title) = 2) +ORDER BY + issues.updated_at DESC, + issues.id DESC +LIMIT 20 OFFSET 0 +``` + +In particular, note that: + +1. We `GROUP BY issues.id` so that we can ... +2. Use the `HAVING (COUNT(DISTINCT labels.title) = 2)` condition to ensure that + all matched issues have both labels. + +This is more complicated than is ideal. It makes the query construction more +prone to errors (such as +[gitlab-org/gitlab-ce#15557](https://gitlab.com/gitlab-org/gitlab-ce/issues/15557)). + +## Attempt A: WHERE EXISTS + +### Attempt A1: use multiple subqueries with WHERE EXISTS + +In +[gitlab-org/gitlab-ce#37137](https://gitlab.com/gitlab-org/gitlab-ce/issues/37137) +and its associated merge request +[gitlab-org/gitlab-ce!14022](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/14022), +we tried to replace the `GROUP BY` with multiple uses of `WHERE EXISTS`. For the +example above, this would give: + +```sql +WHERE (EXISTS ( + SELECT + TRUE + FROM + label_links + INNER JOIN labels ON labels.id = label_links.label_id + WHERE + labels.title = 'Plan' + AND target_type = 'Issue' + AND target_id = issues.id)) +AND (EXISTS ( + SELECT + TRUE + FROM + label_links + INNER JOIN labels ON labels.id = label_links.label_id + WHERE + labels.title = 'backend' + AND target_type = 'Issue' + AND target_id = issues.id)) +``` + +While this worked without schema changes, and did improve readability somewhat, +it did not improve query performance. + +## Attempt B: Denormalize using an array column + +Having [removed MySQL support in GitLab +12.1](https://about.gitlab.com/2019/06/27/removing-mysql-support/), using +[Postgres's arrays](https://www.postgresql.org/docs/9.6/arrays.html) became more +tractable as we didn't have to support two databases. We discussed denormalizing +the `label_links` table for querying in +[gitlab-org/gitlab-ce#49651](https://gitlab.com/gitlab-org/gitlab-ce/issues/49651), +with two options: label IDs and titles. + +We can think of both of those as array columns on `issues`, `merge_requests`, +and `epics`: `issues.label_ids` would be an array column of label IDs, and +`issues.label_titles` would be an array of label titles. + +These array columns can be complemented with [GIN +indexes](https://www.postgresql.org/docs/9.6/gin-intro.html) to improve +matching. + +### Attempt B1: store label IDs for each object + +This has some strong advantages over titles: + +1. Unless a label is deleted, or a project is moved, we never need to + bulk-update the denormalized column. +2. It uses less storage than the titles. + +Unfortunately, our application design makes this hard. If we were able to query +just by label ID easily, we wouldn't need the `INNER JOIN labels` in the initial +query at the start of this document. GitLab allows users to filter by label +title across projects and even across groups, so a filter by the label ~Plan may +include labels with multiple distinct IDs. + +We do not want users to have to know about the different IDs, which means that +given this data set: + +| Project | ~Plan label ID | ~backend label ID | +| --- | --- | --- | +| A | 11 | 12 | +| B | 21 | 22 | +| C | 31 | 32 | + +We would need something like: + +```sql +WHERE + label_ids @> ARRAY[11, 12] + OR label_ids @> ARRAY[21, 22] + OR label_ids @> ARRAY[31, 32] +``` + +This can get even more complicated when we consider that in some cases, there +might be two ~backend labels - with different IDs - that could apply to the same +object, so the number of combinations would balloon further. + +### Attempt B2: store label titles for each object + +From the perspective of updating the labelable object, this is the worst +option. We have to bulk update the objects when: + +1. The objects are moved from one project to another. +1. The project is moved from one group to another. +1. The label is renamed. +1. The label is deleted. + +It also uses much more storage. Querying is simple, though: + +```sql +WHERE + label_titles @> ARRAY['Plan', 'backend'] +``` + +And our [tests in +gitlab-org/gitlab-ce#49651](https://gitlab.com/gitlab-org/gitlab-ce/issues/49651#note_188777346) +showed that this could be fast. + +However, at present, the disadvantages outweigh the advantages. + +## Conclusion + +We have yet to find a method that is demonstratably better than the current +method, when considering: + +1. Query performance. +1. Readability. +1. Ease of maintaining schema consistency. diff --git a/doc/development/go_guide/index.md b/doc/development/go_guide/index.md index f827d240bf6..9f0ac8cc753 100644 --- a/doc/development/go_guide/index.md +++ b/doc/development/go_guide/index.md @@ -173,6 +173,45 @@ func Test(t *testing.T) { } ``` +### Table-Driven Tests + +Using [Table-Driven Tests](https://github.com/golang/go/wiki/TableDrivenTests) +is generally good practice when you have multiple entries of +inputs/outputs for the same function. Below are some guidelines one can +follow when writing table-driven test. These guidelines are mostly +extracted from Go standard library source code. Keep in mind it's OK not +to follow these guidelines when it makes sense. + +#### Defining test cases + +Each table entry is a complete test case with inputs and expected +results, and sometimes with additional information such as a test name +to make the test output easily readable. + +- [Define a slice of anonymous struct](https://github.com/golang/go/blob/50bd1c4d4eb4fac8ddeb5f063c099daccfb71b26/src/encoding/csv/reader_test.go#L16) + inside of the test. +- [Define a slice of anonymous struct](https://github.com/golang/go/blob/55d31e16c12c38d36811bdee65ac1f7772148250/src/cmd/go/internal/module/module_test.go#L9-L66) + outside of the test. +- [Named structs](https://github.com/golang/go/blob/2e0cd2aef5924e48e1ceb74e3d52e76c56dd34cc/src/cmd/go/internal/modfetch/coderepo_test.go#L54-L69) + for code reuse. +- [Using `map[string]struct{}`](https://github.com/golang/go/blob/6d5caf38e37bf9aeba3291f1f0b0081f934b1187/src/cmd/trace/annotations_test.go#L180-L235). + +#### Contents of the test case + +- Ideally, each test case should have a field with a unique identifier + to use for naming subtests. In the Go standard library, this is commonly the + `name string` field. +- Use `want`/`expect`/`actual` when you are specifcing something in the + test case that will be used for assertion. + +#### Variable names + +- Each table-driven test map/slice of struct can be named `tests`. +- When looping through `tests` the anonymous struct can be referred + to as `tt` or `tc`. +- The description of the test can be referred to as + `name`/`testName`/`tn`. + ### Benchmarks Programs handling a lot of IO or complex operations should always include diff --git a/doc/development/import_export.md b/doc/development/import_export.md index 4c78c62f5f5..0c343bc22e4 100644 --- a/doc/development/import_export.md +++ b/doc/development/import_export.md @@ -19,6 +19,7 @@ Project.find_by_full_path('group/project').import_state.slice(:jid, :status, :la grep JID /var/log/gitlab/sidekiq/current grep "Import/Export error" /var/log/gitlab/sidekiq/current grep "Import/Export backtrace" /var/log/gitlab/sidekiq/current +tail /var/log/gitlab/gitlab-rails/importer.log ``` ## Troubleshooting performance issues diff --git a/doc/development/module_with_instance_variables.md b/doc/development/module_with_instance_variables.md index 7bdfa04fc57..443eee0b62c 100644 --- a/doc/development/module_with_instance_variables.md +++ b/doc/development/module_with_instance_variables.md @@ -1,6 +1,6 @@ -## Modules with instance variables could be considered harmful +# Modules with instance variables could be considered harmful -### Background +## Background Rails somehow encourages people using modules and instance variables everywhere. For example, using instance variables in the controllers, @@ -9,7 +9,7 @@ helpers, and views. They're also encouraging the use of saving everything in a giant, single object, and people could access everything in that one giant object. -### The problems +## The problems Of course this is convenient to develop, because we just have everything within reach. However this has a number of downsides when that chosen object @@ -24,7 +24,7 @@ manipulated from 3 different modules. It's hard to track when those variables start giving us troubles. We don't know which module would suddenly change one of the variables. Everything could touch anything. -### Similar concerns +## Similar concerns People are saying multiple inheritance is bad. Mixing multiple modules with multiple instance variables scattering everywhere suffer from the same issue. @@ -40,7 +40,7 @@ Note that `included` doesn't solve the whole issue. They define the dependencies, but they still allow each modules to talk implicitly via the instance variables in the final giant object, and that's where the problem is. -### Solutions +## Solutions We should split the giant object into multiple objects, and they communicate with each other with the API, i.e. public methods. In short, composition over @@ -53,7 +53,7 @@ With clearly defined API, this would make things less coupled and much easier to debug and track, and much more extensible for other objects to use, because they communicate in a clear way, rather than implicit dependencies. -### Acceptable use +## Acceptable use However, it's not always bad to use instance variables in a module, as long as it's contained in the same module; that is, no other modules or @@ -74,7 +74,7 @@ Unfortunately it's not easy to code more complex rules into the cop, so we rely on people's best judgement. If we could find another good pattern we could easily add to the cop, we should do it. -### How to rewrite and avoid disabling this cop +## How to rewrite and avoid disabling this cop Even if we could just disable the cop, we should avoid doing so. Some code could be easily rewritten in simple form. Consider this acceptable method: @@ -181,7 +181,7 @@ rather than whatever includes the module, and those modules which were also included, making it much easier to track down any issues, and reducing the chance of having name conflicts. -### How to disable this cop +## How to disable this cop Put the disabling comment right after your code in the same line: @@ -210,14 +210,14 @@ end Note that you need to enable it at some point, otherwise everything below won't be checked. -### Things we might need to ignore right now +## Things we might need to ignore right now Because of the way Rails helpers and mailers work, we might not be able to avoid the use of instance variables there. For those cases, we could ignore them at the moment. At least we're not going to share those modules with other random objects, so they're still somewhat isolated. -### Instance variables in views +## Instance variables in views They're bad because we can't easily tell who's using the instance variables (from controller's point of view) and where we set them up (from partials' diff --git a/doc/development/new_fe_guide/dependencies.md b/doc/development/new_fe_guide/dependencies.md index 12a4f089d41..8a6930acd37 100644 --- a/doc/development/new_fe_guide/dependencies.md +++ b/doc/development/new_fe_guide/dependencies.md @@ -15,6 +15,18 @@ Exceptions are made for some tools that we require in the `gitlab:assets:compile` CI job such as `webpack-bundle-analyzer` to analyze our production assets post-compile. +To add or upgrade a dependency, run: + +```sh +yarn add <your dependency here> +``` + +This may introduce duplicate dependencies. To de-duplicate `yarn.lock`, run: + +```sh +node_modules/.bin/yarn-deduplicate --list --strategy fewer yarn.lock && yarn install +``` + --- > TODO: Add Dependencies diff --git a/doc/development/new_fe_guide/development/accessibility.md b/doc/development/new_fe_guide/development/accessibility.md index 81a29170129..ae5c4c6a6cc 100644 --- a/doc/development/new_fe_guide/development/accessibility.md +++ b/doc/development/new_fe_guide/development/accessibility.md @@ -1,17 +1,21 @@ # Accessiblity + Using semantic HTML plays a key role when it comes to accessibility. ## Accessible Rich Internet Applications - ARIA + WAI-ARIA, the Accessible Rich Internet Applications specification, defines a way to make Web content and Web applications more accessible to people with disabilities. > Note: It is [recommended][using-aria] to use semantic elements as the primary method to achieve accessibility rather than adding aria attributes. Adding aria attributes should be seen as a secondary method for creating accessible elements. ### Role + The `role` attribute describes the role the element plays in the context of the document. Check the list of WAI-ARIA roles [here][roles] ## Icons + When using icons or images that aren't absolutely needed to understand the context, we should use `aria-hidden="true"`. On the other hand, if an icon is crucial to understand the context we should do one of the following: @@ -20,6 +24,7 @@ On the other hand, if an icon is crucial to understand the context we should do 1. Use `aria-labelledby` to point to an element that contains the explanation for that icon ## Form inputs + In forms we should use the `for` attribute in the label statement: ``` diff --git a/doc/development/new_fe_guide/style/index.md b/doc/development/new_fe_guide/style/index.md index 335d9e66240..f073dc56f1f 100644 --- a/doc/development/new_fe_guide/style/index.md +++ b/doc/development/new_fe_guide/style/index.md @@ -8,7 +8,7 @@ ## [Vue style guide](vue.md) -# Tooling +## Tooling ## [Prettier](prettier.md) diff --git a/doc/development/reusing_abstractions.md b/doc/development/reusing_abstractions.md index 59da02ed6fd..fce144f8dc2 100644 --- a/doc/development/reusing_abstractions.md +++ b/doc/development/reusing_abstractions.md @@ -14,13 +14,13 @@ on maintainability, the ability to easily debug problems, or even performance. An example would be to use `ProjectsFinder` in `IssuesFinder` to limit issues to those belonging to a set of projects. While initially this may seem like a good idea, both classes provide a very high level interface with very little control. -This means that `IssuesFinder` may not be able to produce a better optimised +This means that `IssuesFinder` may not be able to produce a better optimized database query, as a large portion of the query is controlled by the internals of `ProjectsFinder`. To work around this problem, you would use the same code used by `ProjectsFinder`, instead of using `ProjectsFinder` itself directly. This allows -you to compose your behaviour better, giving you more control over the behaviour +you to compose your behavior better, giving you more control over the behavior of the code. To illustrate, consider the following code from `IssuableFinder#projects`: @@ -52,7 +52,7 @@ functionality is added to this (high level) interface. Instead of _only_ affecting the cases where this is necessary, it may also start affecting `IssuableFinder` in a negative way. For example, the query produced by `GroupProjectsFinder` may include unnecessary conditions. Since we're using a -finder here, we can't easily opt-out of that behaviour. We could add options to +finder here, we can't easily opt-out of that behavior. We could add options to do so, but then we'd need as many options as we have features. Every option adds two code paths, which means that for four features we have to cover 8 different code paths. @@ -213,6 +213,5 @@ The API provided by Active Record itself, such as the `where` method, `save`, Everything in `app/workers`. -The scheduling of Sidekiq jobs using `SomeWorker.perform_async`, `perform_in`, -etc. Directly invoking a worker using `SomeWorker.new.perform` should be avoided -at all times in application code, though this is fine to use in tests. +Use `SomeWorker.perform_async` or `SomeWorker.perform_in` to schedule Sidekiq +jobs. Never directly invoke a worker using `SomeWorker.new.perform`. diff --git a/doc/development/shell_scripting_guide/index.md b/doc/development/shell_scripting_guide/index.md new file mode 100644 index 00000000000..ae7f2154682 --- /dev/null +++ b/doc/development/shell_scripting_guide/index.md @@ -0,0 +1,117 @@ +# Shell scripting standards and style guidelines + +## Overview + +GitLab consists of many various services and sub-projects. The majority of +their backend code is written in [Ruby](https://www.ruby-lang.org) and +[Go](https://golang.org). However, some of them use shell scripts for +automation of routine system administration tasks like deployment, +installation, etc. It's being done either for historical reasons or as an effort +to minimize the dependencies, for instance, for Docker images. + +This page aims to define and organize our shell scripting guidelines, +based on our various experiences. All shell scripts across GitLab project +should be eventually harmonized with this guide. If there are any per-project +deviations from this guide, they should be described in the +`README.md` or `PROCESS.md` file for such a project. + +### Avoid using shell scripts + +CAUTION: **Caution:** +This is a must-read section. + +Having said all of the above, we recommend staying away from shell scripts +as much as possible. A language like Ruby or Python (if required for +consistency with codebases that we leverage) is almost always a better choice. +The high-level interpreted languages have more readable syntax, offer much more +mature capabilities for unit-testing, linting, and error reporting. +Use shell scripts only if there's a strong restriction on project's +dependencies size or any other requirements that are more important +in a particular case. + +## Scope of this guide + +According to the [GitLab installation requirements](../../install/requirements.md), +this guide covers only those shells that are used by +[supported Linux distributions](../../install/requirements.md#supported-linux-distributions), +that is: + +- [POSIX Shell](https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html) +- [Bash](https://www.gnu.org/software/bash/) + +## Shell language choice + +- When you need to reduce the dependencies list, use what's provided by the environment. For example, for Docker images it's `sh` from `alpine` which is the base image for most of our tool images. +- Everywhere else, use `bash` if possible. It's more powerful than `sh` but still a widespread shell. + +## Code style and format + +This section describes the tools that should be made a mandatory part of +a project's CI pipeline if it contains shell scripts. These tools +automate shell code formatting, checking for errors or vulnerabilities, etc. + +### Linting + +We're using the [ShellCheck](https://www.shellcheck.net/) utility in its default configuration to lint our +shell scripts. + +All projects with shell scripts should use this GitLab CI/CD job: + +```yaml +shell check: + image: koalaman/shellcheck-alpine + stage: test + before_script: + - shellcheck --version + script: + - shellcheck scripts/**/*.sh # path to your shell scripts +``` + +TIP: **Tip:** +By default, ShellCheck will use the [shell detection](https://github.com/koalaman/shellcheck/wiki/SC2148#rationale) +to determine the shell dialect in use. If the shell file is out of your control and ShellCheck cannot +detect the dialect, use `-s` flag to specify it: `-s sh` or `-s bash`. + +### Formatting + +It's recommended to use the [shfmt](https://github.com/mvdan/sh#shfmt) tool to maintain consistent formatting. +We format shell scripts according to the [Google Shell Style Guide](https://google.github.io/styleguide/shell.xml), +so the following `shfmt` invocation should be applied to the project's script files: + +```bash +shfmt -i 2 -ci scripts/**/*.sh +``` + +TIP: **Tip:** +By default, shfmt will use the [shell detection](https://github.com/mvdan/sh#shfmt) similar to one of ShellCheck +and ignore files starting with a period. To override this, use `-ln` flag to specify the shell dialect: +`-ln posix` or `-ln bash`. + +NOTE: **Note:** +Currently, the `shfmt` tool [is not shipped](https://github.com/mvdan/sh/issues/68) as a Docker image containing +a Linux shell. This makes it impossible to use the [official Docker image](https://hub.docker.com/r/mvdan/shfmt) +in GitLab Runner. This [may change](https://github.com/mvdan/sh/issues/68#issuecomment-507721371) in future. + +## Testing + +NOTE: **Note:** +This is a work in progress. + +It is an [ongoing effort](https://gitlab.com/gitlab-org/gitlab-ce/issues/64016) to evaluate different tools for the +automated testing of shell scripts (like [BATS](https://github.com/sstephenson/bats)). + +## Code Review + +The code review should be performed according to: + +- [ShellCheck Checks list](https://github.com/koalaman/shellcheck/wiki/Checks) +- [Google Shell Style Guide](https://google.github.io/styleguide/shell.xml) +- [Shfmt formatting caveats](https://github.com/mvdan/sh#caveats) + +However, the recommended course of action is to use the aforementioned +tools and address reported offenses. This should eliminate the need +for code review. + +--- + +[Return to Development documentation](../README.md). diff --git a/doc/development/testing_guide/frontend_testing.md b/doc/development/testing_guide/frontend_testing.md index ff28c2ea5e2..2985278cc92 100644 --- a/doc/development/testing_guide/frontend_testing.md +++ b/doc/development/testing_guide/frontend_testing.md @@ -588,7 +588,7 @@ end [jasmine-focus]: https://jasmine.github.io/2.5/focused_specs.html [karma]: http://karma-runner.github.io/ -[vue-test]: https://docs.gitlab.com/ce/development/fe_guide/vue.html#testing-vue-components +[vue-test]: ../fe_guide/vue.md#testing-vue-components [rspec]: https://github.com/rspec/rspec-rails#feature-specs [capybara]: https://github.com/teamcapybara/capybara [jasmine]: https://jasmine.github.io/ diff --git a/doc/development/verifying_database_capabilities.md b/doc/development/verifying_database_capabilities.md index 661ab9cef1a..ccec6f7d719 100644 --- a/doc/development/verifying_database_capabilities.md +++ b/doc/development/verifying_database_capabilities.md @@ -25,7 +25,7 @@ else end ``` -# Read-only database +## Read-only database The database can be used in read-only mode. In this case we have to make sure all GET requests don't attempt any write operations to the |