diff options
Diffstat (limited to 'doc/development')
126 files changed, 2626 insertions, 1312 deletions
diff --git a/doc/development/adding_service_component.md b/doc/development/adding_service_component.md index 0048b10c9da..ee14d8e6414 100644 --- a/doc/development/adding_service_component.md +++ b/doc/development/adding_service_component.md @@ -87,6 +87,10 @@ In addition, any system dependencies used in Omnibus packages or the Cloud Nativ ## Release management -If the service component needs to be updated or released with the monthly GitLab release, then the component should be added to the [release tools automation](https://gitlab.com/gitlab-org/release-tools). This project is maintained by the [Delivery team](https://about.gitlab.com/handbook/engineering/infrastructure/team/delivery/). A list of the projects managed this way can be found in the [release tools project directory](https://about.gitlab.com/handbook/engineering/infrastructure/team/delivery/). +If the service component needs to be updated or released with the monthly GitLab release, then the component should be added to the [release tools automation](https://gitlab.com/gitlab-org/release-tools). This project is maintained by the [Delivery group](https://about.gitlab.com/handbook/engineering/infrastructure/team/delivery/). + +There are different levels of automation available to include a component in GitLab releases. The requirements and process for including a component in a release at these different levels are detailed in the [release documentation](https://gitlab.com/gitlab-org/release/docs/-/tree/master/components). + +A list of the projects with releases managed by release tools can be found in the [release tools project directory](https://gitlab.com/gitlab-org/release-tools/-/tree/master/lib/release_tools/project). For example, during the monthly GitLab release, the desired version of Gitaly, GitLab Workhorse and GitLab Shell need to be synchronized through the various release pipelines. diff --git a/doc/development/api_graphql_styleguide.md b/doc/development/api_graphql_styleguide.md index e0db0d7e34d..396bba2623e 100644 --- a/doc/development/api_graphql_styleguide.md +++ b/doc/development/api_graphql_styleguide.md @@ -785,7 +785,7 @@ The documentation will mention that the old Global ID style is now deprecated. ## Mark schema items as Alpha You can mark GraphQL schema items (fields, arguments, enum values, and mutations) as -[Alpha](https://about.gitlab.com/handbook/product/gitlab-the-product/#alpha-beta-ga). +[Alpha](../policy/alpha-beta-support.md#alpha-features). An item marked as Alpha is [exempt from the deprecation process](#breaking-change-exemptions) and can be removed at any time without notice. Mark an item as Alpha when it is diff --git a/doc/development/application_slis/index.md b/doc/development/application_slis/index.md index 75dd066680e..bd4587333e0 100644 --- a/doc/development/application_slis/index.md +++ b/doc/development/application_slis/index.md @@ -66,7 +66,7 @@ Gitlab::Metrics::Sli::Apdex.initialize_sli(:received_email, [ email_type: :service_desk }, { - feature_category: :code_review, + feature_category: :code_review_workflow, email_type: :create_merge_request } ]) diff --git a/doc/development/approval_rules.md b/doc/development/approval_rules.md index 312bf2b1bb7..f75cf35b32a 100644 --- a/doc/development/approval_rules.md +++ b/doc/development/approval_rules.md @@ -4,7 +4,7 @@ group: Code Review info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments --- -# Approval Rules development guide **(FREE)** +# Approval Rules development guide This document explains the backend design and flow of all related functionality about [merge request approval rules](../user/project/merge_requests/approvals/index.md). diff --git a/doc/development/architecture.md b/doc/development/architecture.md index 5eb1dcc3208..cba868d3fed 100644 --- a/doc/development/architecture.md +++ b/doc/development/architecture.md @@ -543,7 +543,8 @@ GitLab CI/CD is the open-source continuous integration service included with Git #### GitLab Shell -- [Project page](https://gitlab.com/gitlab-org/gitlab-shell/-/blob/main/README.md) +- [Project page](https://gitlab.com/gitlab-org/gitlab-shell/) +- [Documentation](gitlab_shell/index.md) - Configuration: - [Omnibus](https://gitlab.com/gitlab-org/omnibus-gitlab/blob/master/files/gitlab-config-template/gitlab.rb.template) - [Charts](https://docs.gitlab.com/charts/charts/gitlab/gitlab-shell/) @@ -552,7 +553,7 @@ GitLab CI/CD is the open-source continuous integration service included with Git - Layer: Core Service (Processor) - GitLab.com: [Service Architecture](https://about.gitlab.com/handbook/engineering/infrastructure/production/architecture/#service-architecture) -[GitLab Shell](https://gitlab.com/gitlab-org/gitlab-shell) is a program designed at GitLab to handle SSH-based `git` sessions, and modifies the list of authorized keys. GitLab Shell is not a Unix shell nor a replacement for Bash or Zsh. +[GitLab Shell](gitlab_shell/index.md) is a program designed at GitLab to handle SSH-based `git` sessions, and modifies the list of authorized keys. GitLab Shell is not a Unix shell nor a replacement for Bash or Zsh. #### GitLab Workhorse @@ -761,7 +762,7 @@ See our [Redis guidelines](redis.md) for more information about how GitLab uses - [Source](../administration/packages/container_registry.md#enable-the-container-registry) - [GDK](https://gitlab.com/gitlab-org/gitlab-development-kit/blob/main/doc/howto/registry.md) - Layer: Core Service (Processor) -- GitLab.com: [GitLab Container Registry](../user/packages/container_registry/index.md#build-and-push-by-using-gitlab-cicd) +- GitLab.com: [GitLab Container Registry](../user/packages/container_registry/build_and_push_images.md#use-gitlab-cicd) The registry is what users use to store their own Docker images. The bundled registry uses NGINX as a load balancer and GitLab as an authentication manager. diff --git a/doc/development/audit_event_guide/index.md b/doc/development/audit_event_guide/index.md index dfa6d56b3b5..8df5121a2f7 100644 --- a/doc/development/audit_event_guide/index.md +++ b/doc/development/audit_event_guide/index.md @@ -243,5 +243,5 @@ development. We intentionally do not translate audit event messages because translated messages would be saved in the database and served to users, regardless of their locale settings. -This could mean, for example, that we use the locale for the currently logged in user to record an audit event message and stream the message to an external streaming +This could mean, for example, that we use the locale for the currently authenticated user to record an audit event message and stream the message to an external streaming destination in the wrong language for that destination. Users could find that confusing. diff --git a/doc/development/auto_devops.md b/doc/development/auto_devops.md index 7a684f64d64..53033cd19ff 100644 --- a/doc/development/auto_devops.md +++ b/doc/development/auto_devops.md @@ -4,7 +4,7 @@ group: Configure info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments --- -# Auto DevOps development guide **(FREE)** +# Auto DevOps development guide This document provides a development guide for contributors to [Auto DevOps](../topics/autodevops/index.md). diff --git a/doc/development/backend/create_source_code_be/index.md b/doc/development/backend/create_source_code_be/index.md index 8a1a541fac9..77c98982210 100644 --- a/doc/development/backend/create_source_code_be/index.md +++ b/doc/development/backend/create_source_code_be/index.md @@ -30,8 +30,7 @@ that would not work efficiently without Workhorse. ## GitLab Shell GitLab Shell handles Git SSH sessions for GitLab and modifies the list of authorized keys. -For more information, [refer to the README](https://gitlab.com/gitlab-org/gitlab-shell/-/blob/main/README.md). -for GitLab Shell. +For more information, refer to the [GitLab Shell documentation](../../gitlab_shell/index.md). To learn about the reasoning behind our creation of `gitlab-sshd`, read the blog post [Why we implemented our own SSHD solution](https://about.gitlab.com/blog/2022/08/17/why-we-have-implemented-our-own-sshd-solution-on-gitlab-sass/). @@ -48,3 +47,5 @@ For more information, read [Gitaly touch points](gitaly_touch_points.md). Create: Source Code has over 100 REST endpoints, being a mixture of Grape API endpoints and Rails controller endpoints. For a detailed list, refer to [Source Code REST Endpoints](rest_endpoints.md). + +An alternative list of the [Source Code endpoints and other owned objects](https://gitlab-com.gitlab.io/gl-infra/platform/stage-groups-index/source-code.html) is available. diff --git a/doc/development/cached_queries.md b/doc/development/cached_queries.md index e4625a50d79..1b590d68d18 100644 --- a/doc/development/cached_queries.md +++ b/doc/development/cached_queries.md @@ -37,9 +37,9 @@ in-memory objects whenever possible. When you introduce a new feature, you should: - Avoid N+1 queries. -- Minimize the [query count](merge_request_performance_guidelines.md#query-counts). +- Minimize the [query count](merge_request_concepts/performance.md#query-counts). - Pay special attention to ensure - [cached queries](merge_request_performance_guidelines.md#cached-queries) are not + [cached queries](merge_request_concepts/performance.md#cached-queries) are not masking N+1 problems. ## How to detect cached queries @@ -163,5 +163,5 @@ factors help improve the overall execution time: ## For more information - [Metrics that would help us detect the potential N+1 Cached SQL calls](https://gitlab.com/gitlab-org/gitlab/-/issues/259007) -- [Merge request performance guidelines for cached queries](merge_request_performance_guidelines.md#cached-queries) +- [Merge request performance guidelines for cached queries](merge_request_concepts/performance.md#cached-queries) - [Improvements for biggest offenders](https://gitlab.com/groups/gitlab-org/-/epics/4508) diff --git a/doc/development/chatops_on_gitlabcom.md b/doc/development/chatops_on_gitlabcom.md index c9cb2591e4e..378639c4a43 100644 --- a/doc/development/chatops_on_gitlabcom.md +++ b/doc/development/chatops_on_gitlabcom.md @@ -22,12 +22,12 @@ To request access to ChatOps on GitLab.com: 1. Sign in to [Internal GitLab for Operations](https://ops.gitlab.net/users/sign_in) with one of the following methods (Okta is not supported): - - The same username you use on GitLab.com. You may have to choose a different username later. + - The same username you use on GitLab.com. - Selecting the **Sign in with Google** button to sign in with your GitLab.com email address. 1. Confirm that your username in [Internal GitLab for Operations](https://ops.gitlab.net/) is the same as your username in [GitLab.com](https://gitlab.com/). If the usernames - don't match, update the username in [User Settings/Account for the Ops instance](https://ops.gitlab.net/-/profile/account). + don't match, update the username in [User Settings/Account for the Ops instance](https://ops.gitlab.net/-/profile/account). Matching usernames are required to reduce the administrative effort of running multiple platforms. Matching usernames also help with tasks like managing access requests and offboarding. 1. Comment in your onboarding issue, and tag your onboarding buddy and your manager. Request they add you to the `ops` ChatOps project by running this command diff --git a/doc/development/cicd/cicd_reference_documentation_guide.md b/doc/development/cicd/cicd_reference_documentation_guide.md index 8c75e66c33a..530bc62b603 100644 --- a/doc/development/cicd/cicd_reference_documentation_guide.md +++ b/doc/development/cicd/cicd_reference_documentation_guide.md @@ -4,7 +4,7 @@ group: Pipeline Authoring info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments --- -# Documenting the `.gitlab-ci.yml` keywords **(FREE)** +# Documenting the `.gitlab-ci.yml` keywords The [CI/CD YAML reference](../../ci/yaml/index.md) uses a standard style to make it easier to use and update. diff --git a/doc/development/cicd/index.md b/doc/development/cicd/index.md index 73ece709b8d..2a60ca18169 100644 --- a/doc/development/cicd/index.md +++ b/doc/development/cicd/index.md @@ -5,7 +5,7 @@ info: To determine the technical writer assigned to the Stage/Group associated w type: index, concepts, howto --- -# CI/CD development documentation **(FREE)** +# CI/CD development documentation Development guides that are specific to CI/CD are listed here: @@ -198,5 +198,5 @@ Watch a walkthrough of this feature in details in the video below. See the video: <a href="https://www.youtube.com/watch?v=NmdWRGT8kZg">CI/CD minutes - architectural overview</a>. </div> <figure class="video-container"> - <iframe src="https://www.youtube.com/embed/NmdWRGT8kZg" frameborder="0" allowfullscreen="true"> </iframe> + <iframe src="https://www.youtube-nocookie.com/embed/NmdWRGT8kZg" frameborder="0" allowfullscreen> </iframe> </figure> diff --git a/doc/development/cicd/schema.md b/doc/development/cicd/schema.md index 22896594443..26e63fb53d8 100644 --- a/doc/development/cicd/schema.md +++ b/doc/development/cicd/schema.md @@ -5,7 +5,7 @@ info: To determine the technical writer assigned to the Stage/Group associated w type: index, howto --- -# Contribute to the CI/CD Schema **(FREE)** +# Contribute to the CI/CD Schema The [pipeline editor](../../ci/pipeline_editor/index.md) uses a CI/CD schema to enhance the authoring experience of our CI/CD configuration files. With the CI/CD schema, the editor can: @@ -17,9 +17,6 @@ the authoring experience of our CI/CD configuration files. With the CI/CD schema As the rules and keywords for configuring our CI/CD configuration files change, so too should our CI/CD schema. -This feature is behind the [`schema_linting`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/config/feature_flags/development/schema_linting.yml) -feature flag for self-managed instances, and is enabled for GitLab.com. - ## JSON Schemas The CI/CD schema follows the [JSON Schema Draft-07](https://json-schema.org/draft-07/json-schema-release-notes.html) @@ -140,7 +137,6 @@ under the topmost **properties** key. ### Verify changes -1. Enable the `schema_linting` feature flag. 1. Go to **CI/CD** > **Editor**. 1. Write your CI/CD configuration in the editor and verify that the schema validates it correctly. diff --git a/doc/development/cicd/templates.md b/doc/development/cicd/templates.md index 6bc6c57e809..4f6799d12d8 100644 --- a/doc/development/cicd/templates.md +++ b/doc/development/cicd/templates.md @@ -5,7 +5,7 @@ info: To determine the technical writer assigned to the Stage/Group associated w type: index, concepts, howto --- -# Development guide for GitLab CI/CD templates **(FREE)** +# Development guide for GitLab CI/CD templates This document explains how to develop [GitLab CI/CD templates](../../ci/examples/index.md#cicd-templates). diff --git a/doc/development/code_intelligence/index.md b/doc/development/code_intelligence/index.md index f8fb794053d..107a1588e18 100644 --- a/doc/development/code_intelligence/index.md +++ b/doc/development/code_intelligence/index.md @@ -4,7 +4,7 @@ group: Code Review info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments --- -# Code Intelligence **(FREE)** +# Code Intelligence > [Introduced](https://gitlab.com/groups/gitlab-org/-/epics/1576) in GitLab 13.1. diff --git a/doc/development/code_review.md b/doc/development/code_review.md index 30d9d671038..e194453565a 100644 --- a/doc/development/code_review.md +++ b/doc/development/code_review.md @@ -197,10 +197,11 @@ See the [test engineering process](https://about.gitlab.com/handbook/engineering 1. You have properly separated EE content from FOSS, or this MR is FOSS only. - [Where should EE code go?](ee_features.md) 1. You have considered that existing data may be surprisingly varied. For example, a new model validation can break existing records. Consider making validation on existing data optional rather than required if you haven't confirmed that existing data will pass validation. +1. If a test passes with warnings and the failed job includes the text `Flaky test '<path/to/test>' was found in the list of files changed by this MR.`, you have fixed this test, or provided evidence explaining why this flaky test can be ignored. ##### Performance, reliability, and availability -1. You are confident that this MR does not harm performance, or you have asked a reviewer to help assess the performance impact. ([Merge request performance guidelines](merge_request_performance_guidelines.md)) +1. You are confident that this MR does not harm performance, or you have asked a reviewer to help assess the performance impact. ([Merge request performance guidelines](merge_request_concepts/performance.md)) 1. You have added [information for database reviewers in the MR description](database_review.md#required), or you have decided that it is unnecessary. - [Does this MR have database-related changes?](database_review.md) 1. You have considered the availability and reliability risks of this change. @@ -234,6 +235,10 @@ See the [test engineering process](https://about.gitlab.com/handbook/engineering - [When to use a feature flag](https://about.gitlab.com/handbook/product-development-flow/feature-flag-lifecycle/#when-to-use-feature-flags) 1. You have informed the Infrastructure department of a default setting or new setting change per [definition of done](contributing/merge_request_workflow.md#definition-of-done), or decided that this is unnecessary. +##### Compliance + +1. You have confirmed that the correct [MR type label](contributing/issue_workflow.md#type-labels) has been applied. + ### The responsibility of the merge request author The responsibility to find the best solution and implement it lies with the @@ -510,6 +515,7 @@ WARNING: Before taking the decision to merge: - Set the milestone. +- Confirm that the correct [MR type label](contributing/issue_workflow.md#type-labels) is applied. - Consider warnings and errors from danger bot, code quality, and other reports. Unless a strong case can be made for the violation, these should be resolved before merging. A comment must be posted if the MR is merged with any failed job. @@ -557,7 +563,7 @@ WARNING: [very specific cases](https://about.gitlab.com/handbook/engineering/workflow/#criteria-for-merging-during-broken-master). For other cases, follow these [handbook instructions](https://about.gitlab.com/handbook/engineering/workflow/#merging-during-broken-master). - If the latest pipeline was created before the merge request was approved, start a new pipeline to ensure that full RSpec suite has been run. You may skip this step only if the merge request does not contain any backend change. - - If the **latest [merged results pipeline](../ci/pipelines/merged_results_pipelines.md)** finished less than 2 hours ago, you + - If the **latest [merged results pipeline](../ci/pipelines/merged_results_pipelines.md)** was **created less than 6 hours ago**, and **finished less than 2 hours ago**, you may merge without starting a new pipeline as the merge request is close enough to `main`. - When you set the MR to "Merge When Pipeline Succeeds", you should take over diff --git a/doc/development/contributing/design.md b/doc/development/contributing/design.md index b8d7a8eef39..aec487ed184 100644 --- a/doc/development/contributing/design.md +++ b/doc/development/contributing/design.md @@ -47,7 +47,7 @@ Check these aspects both when _designing_ and _reviewing_ UI changes. as the secondary. - Use clear and consistent [terminology](https://design.gitlab.com/content/terminology/). - Check grammar and spelling. -- Consider help content and follow its [guidelines](https://design.gitlab.com/usability/helping-users/). +- Consider help content and follow its [guidelines](https://design.gitlab.com/usability/contextual-help). - Request review from the [appropriate Technical Writer](https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments), indicating any specific files or lines they should review, and how to preview or understand the location/context of the text from the user's perspective. @@ -64,9 +64,9 @@ Check these aspects both when _designing_ and _reviewing_ UI changes. Check visual design properties using your browser's _elements inspector_ ([Chrome](https://developer.chrome.com/docs/devtools/css/), [Firefox](https://firefox-source-docs.mozilla.org/devtools-user/page_inspector/how_to/open_the_inspector/index.html)). -- Use recommended [colors](https://design.gitlab.com/product-foundations/colors/) +- Use recommended [colors](https://design.gitlab.com/product-foundations/color) and [typography](https://design.gitlab.com/product-foundations/type-fundamentals/). -- Follow [layout guidelines](https://design.gitlab.com/layout/grid/). +- Follow [layout guidelines](https://design.gitlab.com/product-foundations/layout#grid). - Use existing [icons](https://gitlab-org.gitlab.io/gitlab-svgs/) and [illustrations](https://gitlab-org.gitlab.io/gitlab-svgs/illustrations/) or propose new ones according to [iconography](https://design.gitlab.com/product-foundations/iconography/) and [illustration](https://design.gitlab.com/product-foundations/illustration/) @@ -81,9 +81,8 @@ Check states using your browser's _styles inspector_ to toggle CSS pseudo-classe like `:hover` and others ([Chrome](https://developer.chrome.com/docs/devtools/css/reference/#pseudo-class), [Firefox](https://firefox-source-docs.mozilla.org/devtools-user/page_inspector/how_to/examine_and_edit_css/index.html#viewing-common-pseudo-classes)). -- Account for all applicable states ([error](https://design.gitlab.com/content/error-messages/), - rest, loading, focus, hover, selected, disabled). -- Account for states dependent on data size ([empty](https://design.gitlab.com/regions/empty-states/), +- Account for all applicable states (error, rest, loading, focus, hover, selected, disabled). +- Account for states dependent on data size ([empty](https://design.gitlab.com/patterns/empty-states), some data, and lots of data). - Account for states dependent on user role, user preferences, and subscription. - Consider animations and transitions, and follow their [guidelines](https://design.gitlab.com/product-foundations/motion/). @@ -126,7 +125,7 @@ When the design is ready, _before_ starting its implementation: At any moment, but usually _during_ or _after_ the design's implementation: -- Contribute [issues to Pajamas](https://design.gitlab.com/get-started/contribute/#contribute-an-issue) +- Contribute [issues to Pajamas](https://design.gitlab.com/get-started/contributing#contribute-an-issue) for additions or enhancements to the design system. - Create issues with the [`~UX debt`](issue_workflow.md#technical-and-ux-debt) label for intentional deviations from the agreed-upon UX requirements due to diff --git a/doc/development/contributing/issue_workflow.md b/doc/development/contributing/issue_workflow.md index df337bb2809..9058eded2c7 100644 --- a/doc/development/contributing/issue_workflow.md +++ b/doc/development/contributing/issue_workflow.md @@ -51,7 +51,7 @@ Most issues will have labels for at least one of the following: - Specialization: `~frontend`, `~backend`, `~documentation` - Release Scoping: `~Deliverable`, `~Stretch`, `~"Next Patch Release"` - Priority: `~"priority::1"`, `~"priority::2"`, `~"priority::3"`, `~"priority::4"` -- Severity: ~`"severity::1"`, `~"severity::2"`, `~"severity::3"`, `~"severity::4"` +- Severity: `~"severity::1"`, `~"severity::2"`, `~"severity::3"`, `~"severity::4"` Please add `~"breaking change"` label if the issue can be considered as a [breaking change](../deprecation_guidelines/index.md). diff --git a/doc/development/contributing/merge_request_workflow.md b/doc/development/contributing/merge_request_workflow.md index f06e8825660..ac3afa14b81 100644 --- a/doc/development/contributing/merge_request_workflow.md +++ b/doc/development/contributing/merge_request_workflow.md @@ -195,7 +195,7 @@ the contribution acceptance criteria below: and testing future changes. 1. Changes do not degrade performance: - Avoid repeated polling of endpoints that require a significant amount of overhead. - - Check for N+1 queries via the SQL log or [`QueryRecorder`](../merge_request_performance_guidelines.md). + - Check for N+1 queries via the SQL log or [`QueryRecorder`](../merge_request_concepts/performance.md). - Avoid repeated access of the file system. - Use [polling with ETag caching](../polling.md) if needed to support real-time features. 1. If the merge request adds any new libraries (like gems or JavaScript libraries), @@ -223,7 +223,7 @@ requirements. 1. Working and clean code that is commented where needed. 1. The change is evaluated to [limit the impact of far-reaching work](https://about.gitlab.com/handbook/engineering/development/#reducing-the-impact-of-far-reaching-work). -1. [Performance guidelines](../merge_request_performance_guidelines.md) have been followed. +1. [Performance guidelines](../merge_request_concepts/performance.md) have been followed. 1. [Secure coding guidelines](https://gitlab.com/gitlab-com/gl-security/security-guidelines) have been followed. 1. [Application and rate limit guidelines](../merge_request_application_and_rate_limit_guidelines.md) have been followed. 1. [Documented](../documentation/index.md) in the `/doc` directory. diff --git a/doc/development/dangerbot.md b/doc/development/dangerbot.md index b568809ea4e..b08eaed2afa 100644 --- a/doc/development/dangerbot.md +++ b/doc/development/dangerbot.md @@ -190,9 +190,9 @@ Contributors can configure Danger for their forks with the following steps: 1. Create a [personal API token](https://gitlab.com/-/profile/personal_access_tokens?name=GitLab+Dangerbot&scopes=api) that has the `api` scope set (don't forget to copy it to the clipboard). -1. In your fork, add a [project CI/CD variable](../ci/variables/index.md#add-a-cicd-variable-to-a-project) +1. In your fork, add a [project CI/CD variable](../ci/variables/index.md#for-a-project) called `DANGER_GITLAB_API_TOKEN` with the token copied in the previous step. 1. Make the variable [masked](../ci/variables/index.md#mask-a-cicd-variable) so it doesn't show up in the job logs. The variable cannot be - [protected](../ci/variables/index.md#protected-cicd-variables), because it needs + [protected](../ci/variables/index.md#protect-a-cicd-variable), because it needs to be present for all branches. diff --git a/doc/development/database/adding_database_indexes.md b/doc/development/database/adding_database_indexes.md index 6a401c804f5..e1d5a7af6d9 100644 --- a/doc/development/database/adding_database_indexes.md +++ b/doc/development/database/adding_database_indexes.md @@ -294,16 +294,14 @@ end ### Verify the MR was deployed and the index exists in production -You can verify if the post-deploy migration was executed on GitLab.com by: - -- Executing `/chatops run auto_deploy status <merge_sha>`. If the output returns `db/gprd`, - the post-deploy migration has been executed in the production database. More details in this - [guide](https://gitlab.com/gitlab-org/release/docs/-/blob/master/general/post_deploy_migration/readme.md#how-to-determine-if-a-post-deploy-migration-has-been-executed-on-gitlabcom). -- Use a meta-command in #database-lab, such as: `\d <index_name>`. - - Ensure that the index is not [`invalid`](https://www.postgresql.org/docs/12/sql-createindex.html#:~:text=The%20psql%20%5Cd%20command%20will%20report%20such%20an%20index%20as%20INVALID). -- Ask someone in #database to check if the index exists. -- With proper access, you can also verify directly on production or in a - production clone. +1. Verify that the post-deploy migration was executed on GitLab.com using ChatOps with + `/chatops run auto_deploy status <merge_sha>`. If the output returns `db/gprd`, + the post-deploy migration has been executed in the production database. For more information, see + [How to determine if a post-deploy migration has been executed on GitLab.com](https://gitlab.com/gitlab-org/release/docs/-/blob/master/general/post_deploy_migration/readme.md#how-to-determine-if-a-post-deploy-migration-has-been-executed-on-gitlabcom). +1. In the case of an [index created asynchronously](#schedule-the-index-to-be-created), wait + until the next week so that the index can be created over a weekend. +1. Use [Database Lab](database_lab.md) to check [if creation was successful](database_lab.md#checking-indexes). + Ensure the output does not indicate the index is `invalid`. ### Add a migration to create the index synchronously @@ -394,15 +392,15 @@ You must test the database index changes locally before creating a merge request ### Verify the MR was deployed and the index no longer exists in production -You can verify if the MR was deployed to GitLab.com with -`/chatops run auto_deploy status <merge_sha>`. To verify the existence of -the index, you can: - -- Use a meta-command in `#database-lab`, for example: `\d <index_name>`. -- Make sure the index no longer exists -- Ask someone in `#database` to check if the index exists. -- If you have access, you can verify directly on production or in a - production clone. +1. Verify that the post-deploy migration was executed on GitLab.com using ChatOps with + `/chatops run auto_deploy status <merge_sha>`. If the output returns `db/gprd`, + the post-deploy migration has been executed in the production database. For more information, see + [How to determine if a post-deploy migration has been executed on GitLab.com](https://gitlab.com/gitlab-org/release/docs/-/blob/master/general/post_deploy_migration/readme.md#how-to-determine-if-a-post-deploy-migration-has-been-executed-on-gitlabcom). +1. In the case of an [index removed asynchronously](#schedule-the-index-to-be-removed), wait + until the next week so that the index can be created over a weekend. +1. Use Database Lab [to check if removal was successful](database_lab.md#checking-indexes). + [Database Lab](database_lab.md) + should report an error when trying to find the removed index. If not, the index may still exist. ### Add a migration to destroy the index synchronously diff --git a/doc/development/database/avoiding_downtime_in_migrations.md b/doc/development/database/avoiding_downtime_in_migrations.md index b34c0bbf728..bb6e13eff53 100644 --- a/doc/development/database/avoiding_downtime_in_migrations.md +++ b/doc/development/database/avoiding_downtime_in_migrations.md @@ -282,6 +282,62 @@ Example migration: end ``` +## Changing column defaults + +Changing column defaults is difficult because of how Rails handles values +that are equal to the default. + +If running code ever explicitly writes the old default value of a column, you must follow a multi-step +process to prevent Rails replacing the old default with the new default in INSERT queries that explicitly +specify the old default. + +Doing this requires steps in two minor releases: + +1. Add the `SafelyChangeColumnDefault` concern to the model and change the default in a post-migration. +1. Clean up the `SafelyChangeColumnDefault` concern in the next minor release. + +We must wait a minor release before cleaning up the `SafelyChangeColumnDefault` because self-managed +releases bundle an entire minor release into a single zero-downtime deployment. + +### Step 1: Add the `SafelyChangeColumnDefault` concern to the model and change the default in a post-migration + +The first step is to mark the column as safe to change in application code. + +```ruby +class Ci::Build < ApplicationRecord + include SafelyChangeColumnDefault + + columns_changing_default :partition_id +end +``` + +Then create a **post-deployment migration** to change the default: + +```shell +bundle exec rails g post_deployment_migration change_ci_builds_default +``` + +```ruby +class ChangeCiBuildsDefault < Gitlab::Database::Migration[2.1] + def up + change_column_default('ci_builds', 'partition_id', from: 100, to: 101) + end + + def down + change_column_default('ci_builds', 'partition_id', from: 101, to: 100) + end +end +``` + +You can consider [enabling lock retries](../migration_style_guide.md#usage-with-transactional-migrations) +when you run a migration on big tables, because it might take some time to +acquire a lock on this table. + +### Step 2: Clean up the `SafelyChangeColumnDefault` concern in the next minor release + +In the next minor release, create a new merge request to remove the `columns_changing_default` call. Also remove the `SafelyChangeColumnDefault` include +if it is not needed for a different column. + ## Changing The Schema For Large Tables While `change_column_type_concurrently` and `rename_column_concurrently` can be @@ -319,10 +375,8 @@ This operation is safe as there's no code using the table just yet. Dropping tables can be done safely using a post-deployment migration, but only if the application no longer uses the table. -Add the table to `DELETED_TABLES` in -[gitlab_schema.rb](https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/gitlab/database/gitlab_schema.rb), -along with its `gitlab_schema`. Even though the table is deleted, it is still -referenced in database migrations. +Add the table to [`db/docs/deleted_tables`](https://gitlab.com/gitlab-org/gitlab/-/tree/master/db/docs/deleted_tables) using the process described in [database dictionary](database_dictionary.md#dropping-tables). +Even though the table is deleted, it is still referenced in database migrations. ## Renaming Tables diff --git a/doc/development/database/batched_background_migrations.md b/doc/development/database/batched_background_migrations.md index 71df4da59c3..88fdfab9828 100644 --- a/doc/development/database/batched_background_migrations.md +++ b/doc/development/database/batched_background_migrations.md @@ -269,6 +269,7 @@ In the second (filtered) example, we know exactly 100 will be updated with each class BackfillNamespaceType < BatchedMigrationJob scope_to ->(relation) { relation.where(type: nil) } operation_name :update_all + feature_category :source_code_management def perform each_sub_batch do |sub_batch| @@ -330,6 +331,7 @@ background migration. # end operation_name :update_all + feature_category :source_code_management def perform each_sub_batch( diff --git a/doc/development/database/constraint_naming_convention.md b/doc/development/database/constraint_naming_convention.md index e9e130495e6..4ac1cd2a71d 100644 --- a/doc/development/database/constraint_naming_convention.md +++ b/doc/development/database/constraint_naming_convention.md @@ -18,7 +18,7 @@ The intent is not to retroactively change names in existing databases but rather |--------------------------|---------------------------------------------------------------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------|-------------------------------------------------------------------------------------------------------------------| | **Primary Key** | `pk_<table name>` | | `pk_projects` | | **Foreign Key** | `fk_<table name>_<column name>[_and_<column name>]*_<foreign table name>` | | `fk_projects_group_id_groups` | -| **Index** | `index_<table name>_on_<column name>[_and_<column name>]*[_and_<column name in partial clause>]*` | | `index_repositories_on_group_id` | +| **Index** | `index_<table name>_on_<column name>[_and_<column name>]*[_and_<column name in partial clause>]*` | Index names must be all lowercase. | `index_repositories_on_group_id` | | **Unique Constraint** | `unique_<table name>_<column name>[_and_<column name>]*` | | `unique_projects_group_id_and_name` | | **Check Constraint** | `check_<table name>_<column name>[_and_<column name>]*[_<suffix>]?` | The optional suffix should denote the type of validation, such as `length` and `enum`. It can also be used to disambiguate multiple `CHECK` constraints on the same column. | `check_projects_name_length`<br />`check_projects_type_enum`<br />`check_projects_admin1_id_and_admin2_id_differ` | | **Exclusion Constraint** | `excl_<table name>_<column name>[_and_<column name>]*_[_<suffix>]?` | The optional suffix should denote the type of exclusion being performed. | `excl_reservations_start_at_end_at_no_overlap` | diff --git a/doc/development/database/database_dictionary.md b/doc/development/database/database_dictionary.md index d74d7e77edb..b7e6fa4b5b3 100644 --- a/doc/development/database/database_dictionary.md +++ b/doc/development/database/database_dictionary.md @@ -17,7 +17,7 @@ For the `geo` database, the dictionary files are stored under `ee/db/docs/`. ## Example dictionary file ```yaml ---- +---- table_name: terraform_states classes: - Terraform::State @@ -28,45 +28,110 @@ introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/26619 milestone: '13.0' ``` -## Schema +## Adding tables -| Attribute | Type | Required | Description | -|----------------------------|---------------|----------|-----------------------------------------------------------------------------------| -| `table_name` / `view_name` | String | yes | Database table name or view name | -| `classes` | Array(String) | no | List of classes that are associated to this table or view. | -| `feature_categories` | Array(String) | yes | List of feature categories using this table or view. | -| `description` | String | no | Text description of the information stored in the table or view, and its purpose. | -| `introduced_by_url` | URL | no | URL to the merge request or commit which introduced this table or view. | -| `milestone` | String | no | The milestone that introduced this table or view. | -| `gitlab_schema` | String | yes | GitLab schema name. | +### Schema -## Adding tables +| Attribute | Type | Required | Description | +|----------------------------|---------------|----------|-------------| +| `table_name` | String | yes | Database table name. | +| `classes` | Array(String) | no | List of classes that are associated to this table. | +| `feature_categories` | Array(String) | yes | List of feature categories using this table. | +| `description` | String | no | Text description of the information stored in the table, and its purpose. | +| `introduced_by_url` | URL | no | URL to the merge request or commit which introduced this table. | +| `milestone` | String | no | The milestone that introduced this table. | +| `gitlab_schema` | String | yes | GitLab schema name. | -When adding a new table, create a new file under `db/docs/` for the `main` and `ci` databases. -For the `geo` database use `ee/db/docs/`. -Name the file as `<table_name>.yml`, containing as much information as you know about the table. +### Process -Include this file in the commit with the migration that creates the table. +When adding a table, you should: + +1. Create a new file for this table in the appropriate directory: + - `gitlab_main` table: `db/docs/` + - `gitlab_ci` table: `db/docs/` + - `gitlab_shared` table: `db/docs/` + - `gitlab_geo` table: `ee/db/docs/` +1. Name the file `<table_name>.yml`, and include as much information as you know about the table. +1. Include this file in the commit with the migration that creates the table. ## Dropping tables -When dropping a table, you must remove the metadata file from `db/docs/` for `main` and `ci` databases. -For the `geo` database, you must remove the file from `ee/db/docs/`. -Use the same commit with the migration that drops the table. +### Schema + +| Attribute | Type | Required | Description | +|----------------------------|---------------|----------|-------------| +| `table_name` | String | yes | Database table name. | +| `classes` | Array(String) | no | List of classes that are associated to this table. | +| `feature_categories` | Array(String) | yes | List of feature categories using this table. | +| `description` | String | no | Text description of the information stored in the table, and its purpose. | +| `introduced_by_url` | URL | no | URL to the merge request or commit which introduced this table. | +| `milestone` | String | no | The milestone that introduced this table. | +| `gitlab_schema` | String | yes | GitLab schema name. | +| `removed_by_url` | String | yes | URL to the merge request or commit which removed this table. | +| `removed_in_milestone` | String | yes | The milestone that removes this table. | + +### Process + +When dropping a table, you should: + +1. Move the dictionary file for this table to the `deleted_tables` directory: + - `gitlab_main` table: `db/docs/deleted_tables/` + - `gitlab_ci` table: `db/docs/deleted_tables/` + - `gitlab_shared` table: `db/docs/deleted_tables/` + - `gitlab_geo` table: `ee/db/docs/deleted_tables/` +1. Add the fields `removed_by_url` and `removed_in_milestone` to the dictionary file. +1. Include this change in the commit with the migration that drops the table. ## Adding views +### Schema + +| Attribute | Type | Required | Description | +|----------------------------|---------------|----------|-------------| +| `table_name` | String | yes | Database view name. | +| `classes` | Array(String) | no | List of classes that are associated to this view. | +| `feature_categories` | Array(String) | yes | List of feature categories using this view. | +| `description` | String | no | Text description of the information stored in the view, and its purpose. | +| `introduced_by_url` | URL | no | URL to the merge request or commit which introduced this view. | +| `milestone` | String | no | The milestone that introduced this view. | +| `gitlab_schema` | String | yes | GitLab schema name. | + +### Process + When adding a new view, you should: 1. Create a new file for this view in the appropriate directory: - - `main` database: `db/docs/views/` - - `ci` database: `db/docs/views/` - - `geo` database: `ee/db/docs/views/` + - `gitlab_main` view: `db/docs/views/` + - `gitlab_ci` view: `db/docs/views/` + - `gitlab_shared` view: `db/docs/views/` + - `gitlab_geo` view: `ee/db/docs/views/` 1. Name the file `<view_name>.yml`, and include as much information as you know about the view. 1. Include this file in the commit with the migration that creates the view. ## Dropping views -When dropping a view, you must remove the metadata file from `db/docs/views/`. -For the `geo` database, you must remove the file from `ee/db/docs/views/`. -Use the same commit with the migration that drops the view. +## Schema + +| Attribute | Type | Required | Description | +|----------------------------|---------------|----------|-------------| +| `view_name` | String | yes | Database view name. | +| `classes` | Array(String) | no | List of classes that are associated to this view. | +| `feature_categories` | Array(String) | yes | List of feature categories using this view. | +| `description` | String | no | Text description of the information stored in the view, and its purpose. | +| `introduced_by_url` | URL | no | URL to the merge request or commit which introduced this view. | +| `milestone` | String | no | The milestone that introduced this view. | +| `gitlab_schema` | String | yes | GitLab schema name. | +| `removed_by_url` | String | yes | URL to the merge request or commit which removed this view. | +| `removed_in_milestone` | String | yes | The milestone that removes this view. | + +### Process + +When dropping a view, you should: + +1. Move the dictionary file for this table to the `deleted_views` directory: + - `gitlab_main` view: `db/docs/deleted_views/` + - `gitlab_ci` view: `db/docs/deleted_views/` + - `gitlab_shared` view: `db/docs/deleted_views/` + - `gitlab_geo` view: `ee/db/docs/deleted_views/` +1. Add the fields `removed_by_url` and `removed_in_milestone` to the dictionary file. +1. Include this change in the commit with the migration that drops the view. diff --git a/doc/development/database/database_lab.md b/doc/development/database/database_lab.md index b60091fa37c..162fc597cc4 100644 --- a/doc/development/database/database_lab.md +++ b/doc/development/database/database_lab.md @@ -75,6 +75,45 @@ the new index. `exec` does not return any results, only the time required to exe After many changes, such as after a destructive query or an ineffective index, you must start over. To reset your designated clone, run `reset`. +#### Checking indexes + +Use Database Lab to check the status of an index with the meta-command `\d <index_name>`. + +Caveats: + +- Indexes are created in both the `main` and `ci` databases, so be sure to use the instance + that matches the table's `gitlab_schema`. For example, if the index is added to + [`ci_builds`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/db/docs/ci_builds.yml#L14), + use `gitlab-production-ci`. +- Database Lab typically has a small delay of a few hours. If more up-to-date information + is required, you can instead request access to a replica [via Teleport](https://gitlab.com/gitlab-com/runbooks/-/blob/master/docs/Teleport/Connect_to_Database_Console_via_Teleport.md) + +For example: `\d index_design_management_designs_on_project_id` produces: + +```plaintext +Index "public.index_design_management_designs_on_project_id" + Column | Type | Key? | Definition +------------+---------+------+------------ + project_id | integer | yes | project_id +btree, for table "public.design_management_designs" +``` + +In the case of an invalid index, the output ends with `invalid`, like: + +```plaintext +Index "public.index_design_management_designs_on_project_id" + Column | Type | Key? | Definition +------------+---------+------+------------ + project_id | integer | yes | project_id +btree, for table "public.design_management_designs", invalid +``` + +If the index doesn't exist, JoeBot throws an error like: + +```plaintext +ERROR: psql error: psql:/tmp/psql-query-932227396:1: error: Did not find any relation named "no_index". +``` + ### Migration testing For information on testing migrations, review our diff --git a/doc/development/database/index.md b/doc/development/database/index.md index c244d784422..5abc7cd3ffa 100644 --- a/doc/development/database/index.md +++ b/doc/development/database/index.md @@ -23,59 +23,59 @@ info: To determine the technical writer assigned to the Stage/Group associated w ## Migrations -- [Different types of migrations](../migration_style_guide.md#choose-an-appropriate-migration-type) -- [Create a regular migration](../migration_style_guide.md#create-a-regular-schema-migration), including creating new models -- [Post-deployment migrations guidelines](post_deployment_migrations.md) and [how to create one](post_deployment_migrations.md#creating-migrations) -- [Legacy Background migrations guidelines](background_migrations.md) +- [Adding required stops](required_stops.md) +- [Avoiding downtime in migrations](avoiding_downtime_in_migrations.md) - [Batched background migrations guidelines](batched_background_migrations.md) +- [Create a regular migration](../migration_style_guide.md#create-a-regular-schema-migration), including creating new models - [Deleting migrations](deleting_migrations.md) -- [Running database migrations](database_debugging.md#migration-wrangling) +- [Different types of migrations](../migration_style_guide.md#choose-an-appropriate-migration-type) +- [Legacy background migrations guidelines](background_migrations.md) - [Migrations for multiple databases](migrations_for_multiple_databases.md) -- [Avoiding downtime in migrations](avoiding_downtime_in_migrations.md) -- [When and how to write Rails migrations tests](../testing_guide/testing_migrations_guide.md) - [Migrations style guide](../migration_style_guide.md) for creating safe SQL migrations -- [Testing Rails migrations](../testing_guide/testing_migrations_guide.md) guide -- [Post deployment migrations](post_deployment_migrations.md) -- [Swapping tables](swapping_tables.md) -- [Deleting migrations](deleting_migrations.md) -- [SQL guidelines](../sql.md) for working with SQL queries - [Partitioning tables](table_partitioning.md) +- [Post-deployment migrations guidelines](post_deployment_migrations.md) and [how to create one](post_deployment_migrations.md#creating-migrations) +- [Running database migrations](database_debugging.md#migration-wrangling) +- [SQL guidelines](../sql.md) for working with SQL queries +- [Swapping tables](swapping_tables.md) +- [Testing Rails migrations](../testing_guide/testing_migrations_guide.md) guide +- [When and how to write Rails migrations tests](../testing_guide/testing_migrations_guide.md) ## Debugging -- [Resetting the database](database_debugging.md#delete-everything-and-start-over) - [Accessing the database](database_debugging.md#manually-access-the-database) +- [Resetting the database](database_debugging.md#delete-everything-and-start-over) - [Troubleshooting and debugging the database](database_debugging.md) -- Tracing the source of an SQL query using query comments with [Marginalia](database_query_comments.md) -- Tracing the source of an SQL query in Rails console using [Verbose Query Logs](https://guides.rubyonrails.org/debugging_rails_applications.html#verbose-query-logs) +- Tracing the source of an SQL query: + - In Rails console using [Verbose Query Logs](https://guides.rubyonrails.org/debugging_rails_applications.html#verbose-query-logs) + - Using query comments with [Marginalia](database_query_comments.md) ## Best practices - [Adding database indexes](adding_database_indexes.md) -- [Foreign keys & associations](foreign_keys.md) - [Adding a foreign key constraint to an existing column](add_foreign_key_to_existing_column.md) -- [`NOT NULL` constraints](not_null_constraints.md) -- [Strings and the Text data type](strings_and_the_text_data_type.md) -- [Single table inheritance](single_table_inheritance.md) -- [Polymorphic associations](polymorphic_associations.md) -- [Serializing data](serializing_data.md) +- [Check for background migrations before upgrading](../../update/background_migrations.md) +- [Client-side connection-pool](client_side_connection_pool.md) +- [Constraints naming conventions](constraint_naming_convention.md) +- [Creating enums](creating_enums.md) +- [Data layout and access patterns](layout_and_access_patterns.md) +- [Efficient `IN` operator queries](efficient_in_operator_queries.md) +- [Foreign keys & associations](foreign_keys.md) - [Hash indexes](hash_indexes.md) -- [Storing SHA1 hashes as binary](sha1_as_binary.md) -- [Iterating tables in batches](iterating_tables_in_batches.md) - [Insert into tables in batches](insert_into_tables_in_batches.md) +- [Iterating tables in batches](iterating_tables_in_batches.md) +- [`NOT NULL` constraints](not_null_constraints.md) - [Ordering table columns](ordering_table_columns.md) -- [Verifying database capabilities](verifying_database_capabilities.md) -- [Query Count Limits](query_count_limits.md) -- [Creating enums](creating_enums.md) -- [Client-side connection-pool](client_side_connection_pool.md) -- [Updating multiple values](setting_multiple_values.md) -- [Constraints naming conventions](constraint_naming_convention.md) -- [Query performance guidelines](query_performance.md) - [Pagination guidelines](pagination_guidelines.md) - [Pagination performance guidelines](pagination_performance_guidelines.md) -- [Efficient `IN` operator queries](efficient_in_operator_queries.md) -- [Data layout and access patterns](layout_and_access_patterns.md) -- [Check for background migrations before upgrading](../../update/background_migrations.md) +- [Polymorphic associations](polymorphic_associations.md) +- [Query count limits](query_count_limits.md) +- [Query performance guidelines](query_performance.md) +- [Serializing data](serializing_data.md) +- [Single table inheritance](single_table_inheritance.md) +- [Storing SHA1 hashes as binary](sha1_as_binary.md) +- [Strings and the Text data type](strings_and_the_text_data_type.md) +- [Updating multiple values](setting_multiple_values.md) +- [Verifying database capabilities](verifying_database_capabilities.md) ## Case studies diff --git a/doc/development/database/pagination_guidelines.md b/doc/development/database/pagination_guidelines.md index 54b315b9dd9..aeab45e2158 100644 --- a/doc/development/database/pagination_guidelines.md +++ b/doc/development/database/pagination_guidelines.md @@ -62,7 +62,7 @@ Offset-based pagination is the easiest way to paginate over records, however, it - Avoid presenting total counts, prefer limit counts. - Example: count maximum 1001 records, and then on the UI show 1000+ if the count is 1001, show the actual number otherwise. - - See the [badge counters approach](../merge_request_performance_guidelines.md#badge-counters) for more information. + - See the [badge counters approach](../merge_request_concepts/performance.md#badge-counters) for more information. - Avoid using page numbers, use next and previous page buttons. - Keyset pagination doesn't support page numbers. - For APIs, advise against building URLs for the next page by "hand". diff --git a/doc/development/database/query_recorder.md b/doc/development/database/query_recorder.md index 84bd0fc938f..dfaaf8afcde 100644 --- a/doc/development/database/query_recorder.md +++ b/doc/development/database/query_recorder.md @@ -10,7 +10,7 @@ QueryRecorder is a tool for detecting the [N+1 queries problem](https://guides.r > Implemented in [spec/support/query_recorder.rb](https://gitlab.com/gitlab-org/gitlab/-/blob/master/spec/support/helpers/query_recorder.rb) via [9c623e3e](https://gitlab.com/gitlab-org/gitlab-foss/commit/9c623e3e5d7434f2e30f7c389d13e5af4ede770a) -As a rule, merge requests [should not increase query counts](../merge_request_performance_guidelines.md#query-counts). If you find yourself adding something like `.includes(:author, :assignee)` to avoid having `N+1` queries, consider using QueryRecorder to enforce this with a test. Without this, a new feature which causes an additional model to be accessed can silently reintroduce the problem. +As a rule, merge requests [should not increase query counts](../merge_request_concepts/performance.md#query-counts). If you find yourself adding something like `.includes(:author, :assignee)` to avoid having `N+1` queries, consider using QueryRecorder to enforce this with a test. Without this, a new feature which causes an additional model to be accessed can silently reintroduce the problem. ## How it works @@ -52,7 +52,7 @@ there are no N+1 queries. Rather than make an extra request to warm the cache, p ## Cached queries -By default, QueryRecorder ignores [cached queries](../merge_request_performance_guidelines.md#cached-queries) in the count. However, it may be better to count +By default, QueryRecorder ignores [cached queries](../merge_request_concepts/performance.md#cached-queries) in the count. However, it may be better to count all queries to avoid introducing an N+1 query that may be masked by the statement cache. To do this, this requires the `:use_sql_query_cache` flag to be set. You should pass the `skip_cached` variable to `QueryRecorder` and use the `exceed_all_query_limit` matcher: @@ -146,5 +146,5 @@ There are multiple ways to find the source of queries. - [Bullet](../profiling.md#bullet) For finding `N+1` query problems - [Performance guidelines](../performance.md) -- [Merge request performance guidelines - Query counts](../merge_request_performance_guidelines.md#query-counts) -- [Merge request performance guidelines - Cached queries](../merge_request_performance_guidelines.md#cached-queries) +- [Merge request performance guidelines - Query counts](../merge_request_concepts/performance.md#query-counts) +- [Merge request performance guidelines - Cached queries](../merge_request_concepts/performance.md#cached-queries) diff --git a/doc/development/database/required_stops.md b/doc/development/database/required_stops.md new file mode 100644 index 00000000000..46fabb5c1b4 --- /dev/null +++ b/doc/development/database/required_stops.md @@ -0,0 +1,41 @@ +--- +stage: Data Stores +group: Database +info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments +--- + +# Adding required stops + +Required stops should only be added when it is deemed absolutely necessary, due to their +disruptive effect on customers. Before adding a required stop, consider if any +alternative approaches exist to avoid a required stop. Sometimes a required +stop is unavoidable. In those cases, follow the instructions below. + +## Before the required stop is released + +Before releasing a known required stop, complete these steps. If the required stop +is identified after release, the following steps must still be completed: + +1. Update [upgrade paths](../../update/index.md#upgrade-paths) to include the new + required stop. +1. Communicate the changes with the customer Support and Release management teams. +1. File an issue with the Database group to squash migrations to that version in the + next release. Use this template for your issue: + + ```markdown + Title: `Squash migrations to <Required stop version>` + As a result of the required stop added for <required stop version> we should squash + migrations up to that version, and update the minimum schema version. + + Deliverables: + - [ ] Migrations are squashed up to <required stop version> + - [ ] `Gitlab::Database::MIN_SCHEMA_VERSION` matches init_schema version + + /label ~"group::database" ~"section::enablement" ~"devops::data_stores" ~"Category:Database" ~"type::maintenance" + /cc @gitlab-org/database-team/triage + ``` + +## In the release following the required stop + +1. Update `Gitlab::Database::MIN_SCHEMA_GITLAB_VERSION` in `lib/gitlab/database.rb` to the + new required stop versions. Do not change `Gitlab::Database::MIN_SCHEMA_VERSION`. diff --git a/doc/development/database/setting_multiple_values.md b/doc/development/database/setting_multiple_values.md index fb85386785d..48be7ff9f10 100644 --- a/doc/development/database/setting_multiple_values.md +++ b/doc/development/database/setting_multiple_values.md @@ -32,7 +32,7 @@ update issues where id = obj_id ``` -You can't express this in ActiveRecord, or by dropping down to [Arel](https://api.rubyonrails.org/v6.1.0/classes/Arel.html), +You can't express this in ActiveRecord, or by dropping down to [Arel](https://api.rubyonrails.org/classes/Arel.html), because the `UpdateManager` does not support `update from`. However, we supply an abstraction to help you generate these kinds of updates: `Gitlab::Database::BulkUpdate`. This abstraction constructs queries like the previous example, and uses diff --git a/doc/development/database/table_partitioning.md b/doc/development/database/table_partitioning.md index 5f1deb77b6c..30131fc0347 100644 --- a/doc/development/database/table_partitioning.md +++ b/doc/development/database/table_partitioning.md @@ -389,7 +389,8 @@ class PrepareForeignKeyForPartitioning < Gitlab::Database::Migration[2.1] TARGET_TABLE_NAME, column: [PARTITION_COLUMN, COLUMN], target_column: [PARTITION_COLUMN, TARGET_COLUMN], - validate: false + validate: false, + on_update: :cascade, name: CONSTRAINT_NAME ) @@ -402,6 +403,13 @@ class PrepareForeignKeyForPartitioning < Gitlab::Database::Migration[2.1] end ``` +The `on_update: :cascade` option is mandatory if we want the partitioning column +to be updated. This will cascade the update to all dependent rows. Without +specifying it, updating the partition column on the target table we would +result in a `Key is still referenced from table ...` error and updating the +partition column on the source table would raise a +`Key is not present in table ...` error. + ### Step 6 - Create parent table and attach existing table as the initial partition You can now create the parent table attaching the existing table as the initial diff --git a/doc/development/database_review.md b/doc/development/database_review.md index e66be062986..048482960f4 100644 --- a/doc/development/database_review.md +++ b/doc/development/database_review.md @@ -275,4 +275,4 @@ Include in the MR description: - [Check query plans](database/understanding_explain_plans.md) and suggest improvements to queries (changing the query, schema or adding indexes and similar) - General guideline is for queries to come in below [100ms execution time](database/query_performance.md#timing-guidelines-for-queries) - - Avoid N+1 problems and minimize the [query count](merge_request_performance_guidelines.md#query-counts). + - Avoid N+1 problems and minimize the [query count](merge_request_concepts/performance.md#query-counts). diff --git a/doc/development/deprecation_guidelines/img/deprecation_removal_process.png b/doc/development/deprecation_guidelines/img/deprecation_removal_process.png Binary files differindex 594e15861b0..3020387d052 100644 --- a/doc/development/deprecation_guidelines/img/deprecation_removal_process.png +++ b/doc/development/deprecation_guidelines/img/deprecation_removal_process.png diff --git a/doc/development/deprecation_guidelines/index.md b/doc/development/deprecation_guidelines/index.md index be4a3369dcb..f4af005b849 100644 --- a/doc/development/deprecation_guidelines/index.md +++ b/doc/development/deprecation_guidelines/index.md @@ -33,7 +33,6 @@ https://about.gitlab.com/handbook/product/gitlab-the-product/#definitions - No longer tested internally. - Will be removed in a future major release. - Begins after an end-of-support date has passed. -- Ends after all relevant code has been removed. [Announcing an End of Support period](https://about.gitlab.com/handbook/marketing/blog/release-posts/#announcing-an-end-of-support-period) should only be used in special circumstances and is not recommended for general use. @@ -42,12 +41,11 @@ Most features should be deprecated and then removed. **Removal**: - Feature usage impossible. +- Feature no longer supported (if End of Support period hasn't already been announced). - Happens in a major release in line with our [semantic versioning policy](../../policy/maintenance.md). - Begins after removal date has passed. - - **Breaking change**: A "breaking change" is any change that requires users to make a corresponding change to their code, settings, or workflow. "Users" might be humans, API clients, or even code classes that "use" another class. Examples of breaking changes include: @@ -58,6 +56,8 @@ A "breaking change" is any change that requires users to make a corresponding ch A breaking change can be considered major if it affects many users, or represents a significant change in behavior. + + ## When can a feature be deprecated? Deprecations should be announced on the [Deprecated feature removal schedule](../../update/deprecations.md). diff --git a/doc/development/diffs.md b/doc/development/diffs.md index b38fcea4f00..c84bf57e085 100644 --- a/doc/development/diffs.md +++ b/doc/development/diffs.md @@ -1,199 +1,11 @@ --- -stage: Create -group: Code Review -info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments +redirect_to: 'merge_request_concepts/diffs/index.md' +remove_date: '2023-04-10' --- -# Working with diffs +This document was moved to [another location](merge_request_concepts/diffs/index.md). -We rely on different sources to present diffs. These include: - -- Gitaly service -- Database (through `merge_request_diff_files`) -- Redis (cached highlighted diffs) - -## Deep Dive - -<!-- vale gitlab.Spelling = NO --> - -In January 2019, Oswaldo Ferreira hosted a Deep Dive (GitLab team members only: -`https://gitlab.com/gitlab-org/create-stage/issues/1`) on GitLab Diffs and Commenting on Diffs -functionality to share domain-specific knowledge with anyone who may work in this part of the -codebase in the future: - -<!-- vale gitlab.Spelling = YES --> - -- <i class="fa fa-youtube-play youtube" aria-hidden="true"></i> - [Recording on YouTube](https://www.youtube.com/watch?v=K6G3gMcFyek) -- Slides on [Google Slides](https://docs.google.com/presentation/d/1bGutFH2AT3bxOPZuLMGl1ANWHqFnrxwQwjiwAZkF-TU/edit) -- [PDF slides](https://gitlab.com/gitlab-org/create-stage/uploads/b5ad2f336e0afcfe0f99db0af0ccc71a/) - -Everything covered in this deep dive was accurate as of GitLab 11.7, and while specific details may -have changed since then, it should still serve as a good introduction. - -## Architecture overview - -### Merge request diffs - -When refreshing a merge request (pushing to a source branch, force-pushing to target branch, or if the target branch now contains any commits from the MR) -we fetch the comparison information using `Gitlab::Git::Compare`, which fetches `base` and `head` data using Gitaly and diff between them through -`Gitlab::Git::Diff.between`. -The diffs fetching process _limits_ single file diff sizes and the overall size of the whole diff through a series of constant values. Raw diff files are -then persisted on `merge_request_diff_files` table. - -Even though diffs larger than 10% of the value of `ApplicationSettings#diff_max_patch_bytes` are collapsed, -we still keep them on PostgreSQL. However, diff files larger than defined _safety limits_ -(see the [Diff limits section](#diff-limits)) are _not_ persisted in the database. - -In order to present diffs information on the merge request diffs page, we: - -1. Fetch all diff files from database `merge_request_diff_files` -1. Fetch the _old_ and _new_ file blobs in batch to: - - Highlight old and new file content - - Know which viewer it should use for each file (text, image, deleted, etc) - - Know if the file content changed - - Know if it was stored externally - - Know if it had storage errors -1. If the diff file is cacheable (text-based), it's cached on Redis - using `Gitlab::Diff::FileCollection::MergeRequestDiff` - -### Note diffs - -When commenting on a diff (any comparison), we persist a truncated diff version -on `NoteDiffFile` (which is associated with the actual `DiffNote`). So instead -of hitting the repository every time we need the diff of the file, we: - -1. Check whether we have the `NoteDiffFile#diff` persisted and use it -1. Otherwise, if it's a current MR revision, use the persisted - `MergeRequestDiffFile#diff` -1. In the last scenario, go the repository and fetch the diff - -## Diff limits - -As explained above, we limit single diff files and the size of the whole diff. There are scenarios where we collapse the diff file, -and cases where the diff file is not presented at all, and the user is guided to the Blob view. - -### Diff collection limits - -Limits that act onto all diff files collection. Files number, lines number and files size are considered. - -```ruby -Gitlab::Git::DiffCollection.collection_limits[:safe_max_files] = Gitlab::Git::DiffCollection::DEFAULT_LIMITS[:max_files] = 100 -``` - -File diffs are collapsed (but are expandable) if 100 files have already been rendered. - -```ruby -Gitlab::Git::DiffCollection.collection_limits[:safe_max_lines] = Gitlab::Git::DiffCollection::DEFAULT_LIMITS[:max_lines] = 5000 -``` - -File diffs are collapsed (but be expandable) if 5000 lines have already been rendered. - -```ruby -Gitlab::Git::DiffCollection.collection_limits[:safe_max_bytes] = Gitlab::Git::DiffCollection.collection_limits[:safe_max_files] * 5.kilobytes = 500.kilobytes -``` - -File diffs are collapsed (but be expandable) if 500 kilobytes have already been rendered. - -```ruby -Gitlab::Git::DiffCollection.collection_limits[:max_files] = Commit::DIFF_HARD_LIMIT_FILES = 1000 -``` - -No more files are rendered at all if 1000 files have already been rendered. - -```ruby -Gitlab::Git::DiffCollection.collection_limits[:max_lines] = Commit::DIFF_HARD_LIMIT_LINES = 50000 -``` - -No more files are rendered at all if 50,000 lines have already been rendered. - -```ruby -Gitlab::Git::DiffCollection.collection_limits[:max_bytes] = Gitlab::Git::DiffCollection.collection_limits[:max_files] * 5.kilobytes = 5000.kilobytes -``` - -No more files are rendered at all if 5 megabytes have already been rendered. - -All collection limit parameters are sent and applied on Gitaly. That is, after the limit is surpassed, -Gitaly only returns the safe amount of data to be persisted on `merge_request_diff_files`. - -### Individual diff file limits - -Limits that act onto each diff file of a collection. Files number, lines number and files size are considered. - -#### Expandable patches (collapsed) - -Diff patches are collapsed when surpassing 10% of the value set in `ApplicationSettings#diff_max_patch_bytes`. -That is, it's equivalent to 10kb if the maximum allowed value is 100kb. -The diff is persisted and expandable if the patch size doesn't -surpass `ApplicationSettings#diff_max_patch_bytes`. - -Although this nomenclature (Collapsing) is also used on Gitaly, this limit is only used on GitLab (hardcoded - not sent to Gitaly). -Gitaly only returns `Diff.Collapsed` (RPC) when surpassing collection limits. - -#### Not expandable patches (too large) - -The patch not be rendered if it's larger than `ApplicationSettings#diff_max_patch_bytes`. -Users see a `Changes are too large to be shown.` message and a button to view only that file in that commit. - -```ruby -Commit::DIFF_SAFE_LINES = Gitlab::Git::DiffCollection::DEFAULT_LIMITS[:max_lines] = 5000 -``` - -File diff is suppressed (technically different from collapsed, but behaves the same, and is expandable) if it has more than 5000 lines. - -This limit is hardcoded and only applied on GitLab. - -## Viewers - -Diff Viewers, which can be found on `models/diff_viewer/*` are classes used to map metadata about each type of Diff File. It has information -whether it's a binary, which partial should be used to render it or which File extensions this class accounts for. - -`DiffViewer::Base` validates _blobs_ (old and new versions) content, extension and file type to check if it can be rendered. - -## Merge request diffs against the `HEAD` of the target branch - -Historically, merge request diffs have been calculated by `git diff target...source` which compares the -`HEAD` of the source branch with the merge base (or a common ancestor) of the target branch and the source's. -This solution works well until the target branch starts containing some of the -changes introduced by the source branch: Consider the following case, in which the source branch -is `feature_a` and the target is `main`: - -1. Checkout a new branch `feature_a` from `main` and remove `file_a` and `file_b` in it. -1. Add a commit that removes `file_a` to `main`. - -The merge request diff still contains the `file_a` removal while the actual diff compared to -`main`'s `HEAD` has only the `file_b` removal. The diff with such redundant -changes is harder to review. - -In order to display an up-to-date diff, in GitLab 12.9 we -[introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/27008) merge request -diffs compared against `HEAD` of the target branch: the -target branch is artificially merged into the source branch, then the resulting -merge ref is compared to the source branch to calculate an accurate -diff. - -Until we complete the epics ["use merge refs for diffs"](https://gitlab.com/groups/gitlab-org/-/epics/854) -and ["merge conflicts in diffs"](https://gitlab.com/groups/gitlab-org/-/epics/4893), -both options `main (base)` and `main (HEAD)` are available to be displayed in merge requests: - - - -The `main (HEAD)` option is meant to replace `main (base)` in the future. - -In order to support comments for both options, diff note positions are stored for -both `main (base)` and `main (HEAD)` versions ([introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/198457) in 12.10). -The position for `main (base)` version is stored in `Note#position` and -`Note#original_position` columns, for `main (HEAD)` version `DiffNotePosition` -has been introduced. - -One of the key challenges to deal with when working on merge ref diffs are merge -conflicts. If the target and source branch contains a merge conflict, the branches -cannot be automatically merged. The -<i class="fa fa-youtube-play youtube" aria-hidden="true"></i> [recording on YouTube](https://www.youtube.com/watch?v=GFXIFA4ZuZw&feature=youtu.be&ab_channel=GitLabUnfiltered) -is a quick introduction to the problem and the motivation behind the [epic](https://gitlab.com/groups/gitlab-org/-/epics/854). - -In 13.5 a solution for both-modified merge -conflict has been -[introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/232484). However, -there are more classes of merge conflicts that are to be -[addressed](https://gitlab.com/groups/gitlab-org/-/epics/4893) in the future. +<!-- This redirect file can be deleted after <2023-04-10>. --> +<!-- Redirects that point to other docs in the same project expire in three months. --> +<!-- Redirects that point to docs in a different project or site (for example, link is not relative and starts with `https:`) expire in one year. --> +<!-- Before deletion, see: https://docs.gitlab.com/ee/development/documentation/redirects.html --> diff --git a/doc/development/distributed_tracing.md b/doc/development/distributed_tracing.md index 9e62f019fbf..79d0ff84713 100644 --- a/doc/development/distributed_tracing.md +++ b/doc/development/distributed_tracing.md @@ -4,7 +4,7 @@ group: Respond info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments --- -# Distributed Tracing - development guidelines **(FREE)** +# Distributed Tracing - development guidelines GitLab is instrumented for distributed tracing. Distributed Tracing in GitLab is currently considered **experimental**, as it has not yet been tested at scale on GitLab.com. diff --git a/doc/development/documentation/index.md b/doc/development/documentation/index.md index a9f2726ea93..8a5a993d913 100644 --- a/doc/development/documentation/index.md +++ b/doc/development/documentation/index.md @@ -241,7 +241,7 @@ with the following conventions: - It omits the `.md` extension. - It doesn't end with a forward slash (`/`). -The help text follows the [Pajamas guidelines](https://design.gitlab.com/usability/helping-users/#formatting-help-content). +The help text follows the [Pajamas guidelines](https://design.gitlab.com/usability/contextual-help#formatting-help-content). #### Linking to `/help` in HAML diff --git a/doc/development/documentation/styleguide/index.md b/doc/development/documentation/styleguide/index.md index 3e55b334992..921bb13721b 100644 --- a/doc/development/documentation/styleguide/index.md +++ b/doc/development/documentation/styleguide/index.md @@ -17,7 +17,7 @@ In addition to this page, the following resources can help you craft and contrib - [Doc contribution guidelines](../index.md) - [Recommended word list](word_list.md) - [Doc style and consistency testing](../testing.md) -- [Guidelines for UI error messages](https://design.gitlab.com/content/error-messages/) +- [Guidelines for UI error messages](https://design.gitlab.com/content/voice-and-tone#clear-error-messages) - [Documentation global navigation](../site_architecture/global_nav.md) - [GitLab Handbook style guidelines](https://about.gitlab.com/handbook/communication/#writing-style-guidelines) - [Microsoft Style Guide](https://learn.microsoft.com/en-us/style-guide/welcome/) @@ -523,10 +523,10 @@ When using code block style: ## Lists -- Use a period after every sentence, including those that complete an introductory phrase. - Do not use semicolons or commas. -- Majority rules. Use either full sentences or all fragments. Avoid a mix. -- Always start list items with a capital letter. +- Do not use a period if the phrase is not a full sentence. +- Use a period after every sentence. Do not use semicolons or commas. +- Majority rules. All items should have the same punctuation. +- Start list items with a capital letter. - Separate the introductory phrase from explanatory text with a colon (`:`). For example: ```markdown @@ -1217,7 +1217,7 @@ To embed a video: the video title and link in the line under `<div class="video-fallback">`. 1. In YouTube, select **Share**, and then select **Embed**. 1. Copy the `<iframe>` source (`src`) **URL only** - (`https://www.youtube.com/embed/VIDEO-ID`), + (`https://www.youtube-nocookie.com/embed/VIDEO-ID`), and paste it, replacing the content of the `src` field in the `iframe` tag. @@ -1227,7 +1227,7 @@ leave a blank line here See the video: <a href="https://www.youtube.com/watch?v=MqL6BMOySIQ">Video title</a>. </div> <figure class="video-container"> - <iframe src="https://www.youtube.com/embed/MqL6BMOySIQ" frameborder="0" allowfullscreen="true"> </iframe> + <iframe src="https://www.youtube-nocookie.com/embed/MqL6BMOySIQ" frameborder="0" allowfullscreen> </iframe> </figure> leave a blank line here ``` @@ -1238,7 +1238,7 @@ This is how it renders on the GitLab documentation site: See the video: <a href="https://www.youtube.com/watch?v=enMumwvLAug">What is GitLab</a>. </div> <figure class="video-container"> - <iframe src="https://www.youtube.com/embed/MqL6BMOySIQ" frameborder="0" allowfullscreen="true"> </iframe> + <iframe src="https://www.youtube-nocookie.com/embed/MqL6BMOySIQ" frameborder="0" allowfullscreen> </iframe> </figure> > Notes: @@ -1249,6 +1249,7 @@ different mobile devices. > - The `<div class="video-fallback">` is a fallback necessary for `/help`, because the GitLab Markdown processor doesn't support iframes. It's hidden on the documentation site, but is displayed by `/help`. +> - The `www.youtube-nocookie.com` domain enables the [Privacy Enhanced Mode](https://support.google.com/youtube/answer/171780?hl=en#zippy=%2Cturn-on-privacy-enhanced-mode) of the YouTube embedded player. This mode allows users with resticted cookie preferences to view embedded videos. ## Alert boxes @@ -1446,6 +1447,8 @@ For example: [configuration edits guide](#configuration-documentation-for-different-installation-methods)) - `15.1 and earlier`, `15.2 and later` +Until we implement automated testing for broken links to tabs ([Issue 1355](https://gitlab.com/gitlab-org/gitlab-docs/-/issues/1355)), do not link directly to a single tab, even though they do have unique URL parameters. + See [Pajamas](https://design.gitlab.com/components/tabs/#guidelines) for more details on tabs. diff --git a/doc/development/documentation/styleguide/word_list.md b/doc/development/documentation/styleguide/word_list.md index 333a5521536..fcebe3b3649 100644 --- a/doc/development/documentation/styleguide/word_list.md +++ b/doc/development/documentation/styleguide/word_list.md @@ -186,6 +186,10 @@ Instead, use **assign**. For example: - Assign the issue to an epic. - Assign a user to the issue. +## authenticated user + +Use **authenticated user** instead of other variations, like **signed in user** or **logged in user**. + ## below Try to avoid **below** when referring to an example or table in a documentation page. If required, use **following** instead. For example: @@ -309,6 +313,13 @@ Users can set the default branch by using a UI setting. For examples that use the default branch, use `main` instead of [`master`](#master). +## delete + +Use **delete** when an object is completely deleted. **Delete** is the opposite of **create**. + +When the object continues to exist, use [**remove**](#remove) instead. +For example, you can remove an issue from an epic, but the issue still exists. + ## Dependency Proxy Use title case for the GitLab Dependency Proxy. @@ -487,7 +498,7 @@ Use title case for **Geo**. ## GitLab -Do not make **GitLab** possessive (GitLab's). This guidance follows [GitLab Trademark Guidelines](https://about.gitlab.com/handbook/marketing/corporate-marketing/brand-activation/trademark-guidelines/). +Do not make **GitLab** possessive (GitLab's). This guidance follows [GitLab Trademark Guidelines](https://about.gitlab.com/handbook/marketing/brand-and-product-marketing/brand/brand-activation/trademark-guidelines/). ## GitLab.com @@ -691,6 +702,10 @@ Do not use **limitations**. Use **known issues** instead. Do not use **log in** or **log on**. Use [sign in](#sign-in) instead. If the user interface has **Log in**, you can use it. +## logged-in user, logged in user + +Use **authenticated user** instead of **logged-in user** or **logged in user**. + ## lower Do not use **lower** when talking about version numbers. @@ -937,6 +952,12 @@ we would talk to a colleague, and to avoid differentiation between `we` and `the Use **register** instead of **sign up** when talking about creating an account. +## remove + +Use **remove** when an object continues to exist. For example, you can remove an issue from an epic, but the issue still exists. + +When an object is completely deleted, use [**delete**](#delete) instead. + ## Reporter When writing about the Reporter role: @@ -1075,6 +1096,10 @@ You can use **single sign-on**. Use **register** instead of **sign up** when talking about creating an account. +## signed-in user, signed in user + +Use **authenticated user** instead of **signed-in user** or **signed in user**. + ## simply, simple Do not use **simply** or **simple**. If the user doesn't find the process to be simple, we lose their trust. ([Vale](../testing.md#vale) rule: [`Simplicity.yml`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/doc/.vale/gitlab/Simplicity.yml)) @@ -1318,7 +1343,7 @@ in present tense, active voice. ## you, your, yours Use **you**, **your**, and **yours** instead of **the user** and **the user's**. -Documentation should be from the [point of view](https://design.gitlab.com/content/voice-tone/#point-of-view) of the reader. +Documentation should be from the [point of view](https://design.gitlab.com/content/voice-and-tone#point-of-view) of the reader. Use: diff --git a/doc/development/documentation/testing.md b/doc/development/documentation/testing.md index 8b8f281d7c1..58584b5168b 100644 --- a/doc/development/documentation/testing.md +++ b/doc/development/documentation/testing.md @@ -282,16 +282,16 @@ Vale returns three types of results: - **Error** - For branding guidelines, trademark guidelines, and anything that causes content on the docs site to render incorrectly. -- **Warning** - For Technical Writing team style preferences. -- **Suggestion** - For basic technical writing tenets and best practices. +- **Warning** - For general style guide rules, tenets, and best practices. +- **Suggestion** - For technical writing style preferences that may require refactoring of documentation or updates to an exceptions list. The result types have these attributes: -| Result type | Displayed in CI/CD job output | Causes CI/CD jobs to fail | Vale rule link | -|--------------|-------------------------------|---------------------------|----------------| -| `error` | **{check-circle}** Yes | **{check-circle}** Yes | [Error-level Vale rules](https://gitlab.com/search?utf8=✓&snippets=false&scope=&repository_ref=master&search=path%3Adoc%2F.vale%2Fgitlab+Error%3A&group_id=9970&project_id=278964) | -| `warning` | **{dotted-circle}** No | **{dotted-circle}** No | [Warning-level Vale rules](https://gitlab.com/search?utf8=✓&snippets=false&scope=&repository_ref=master&search=path%3Adoc%2F.vale%2Fgitlab+Warning%3A&group_id=9970&project_id=278964) | -| `suggestion` | **{dotted-circle}** No | **{dotted-circle}** No | [Suggestion-level Vale rules](https://gitlab.com/search?utf8=✓&snippets=false&scope=&repository_ref=master&search=path%3Adoc%2F.vale%2Fgitlab+Suggestion%3A&group_id=9970&project_id=278964) | +| Result type | Displays in CI/CD job output | Displays in MR diff | Causes CI/CD jobs to fail | Vale rule link | +|--------------|------------------------------|---------------------|---------------------------|----------------| +| `error` | **{check-circle}** Yes | **{check-circle}** Yes | **{check-circle}** Yes | [Error-level Vale rules](https://gitlab.com/search?utf8=✓&snippets=false&scope=&repository_ref=master&search=path%3Adoc%2F.vale%2Fgitlab+Error%3A&group_id=9970&project_id=278964) | +| `warning` | **{dotted-circle}** No | **{check-circle}** Yes | **{dotted-circle}** No | [Warning-level Vale rules](https://gitlab.com/search?utf8=✓&snippets=false&scope=&repository_ref=master&search=path%3Adoc%2F.vale%2Fgitlab+Warning%3A&group_id=9970&project_id=278964) | +| `suggestion` | **{dotted-circle}** No | **{dotted-circle}** No | **{dotted-circle}** No | [Suggestion-level Vale rules](https://gitlab.com/search?utf8=✓&snippets=false&scope=&repository_ref=master&search=path%3Adoc%2F.vale%2Fgitlab+Suggestion%3A&group_id=9970&project_id=278964) | #### Vale spelling test @@ -360,6 +360,10 @@ In general, follow these guidelines: If the rule is too subjective, it cannot be adequately enforced and creates unnecessary additional warnings. + - Whether it's appropriate to display in the merge request diff in the GitLab UI. + If the rule is difficult to implement directly in the merge request (for example, + it requires page refactoring), set it to suggestion-level so it displays in local editors only. + ### Install linters At a minimum, install [markdownlint](#markdownlint) and [Vale](#vale) to match the checks run in @@ -407,15 +411,13 @@ To configure markdownlint in your editor, install one of the following as approp - Sublime Text [`SublimeLinter-contrib-markdownlint` package](https://packagecontrol.io/packages/SublimeLinter-contrib-markdownlint). - Visual Studio Code [`DavidAnson.vscode-markdownlint` extension](https://marketplace.visualstudio.com/items?itemName=DavidAnson.vscode-markdownlint). -- Atom [`linter-node-markdownlint` package](https://atom.io/packages/linter-node-markdownlint). - Vim [ALE plugin](https://github.com/dense-analysis/ale). To configure Vale in your editor, install one of the following as appropriate: -- Sublime Text [`SublimeLinter-contrib-vale` package](https://packagecontrol.io/packages/SublimeLinter-contrib-vale). +- Sublime Text [`SublimeLinter-vale` package](https://packagecontrol.io/packages/SublimeLinter-vale). - Visual Studio Code [`errata-ai.vale-server` extension](https://marketplace.visualstudio.com/items?itemName=errata-ai.vale-server). You can configure the plugin to [display only a subset of alerts](#show-subset-of-vale-alerts). -- Atom [`atomic-vale` package](https://atom.io/packages/atomic-vale). - Vim [ALE plugin](https://github.com/dense-analysis/ale). - JetBrains IDEs - No plugin exists, but [this issue comment](https://github.com/errata-ai/vale-server/issues/39#issuecomment-751714451) diff --git a/doc/development/documentation/topic_types/concept.md b/doc/development/documentation/topic_types/concept.md index e01b06c2c07..66af8780b9b 100644 --- a/doc/development/documentation/topic_types/concept.md +++ b/doc/development/documentation/topic_types/concept.md @@ -10,8 +10,8 @@ A concept introduces a single feature or concept. A concept should answer the questions: -- What is this? -- Why would you use it? +- **What** is this? +- **Why** would you use it? Think of everything someone might want to know if they've never heard of this concept before. @@ -24,12 +24,15 @@ Concepts should be in this format: ```markdown # Title (a noun, like "Widgets") -A paragraph that explains what this thing is. +A paragraph or two that explains what this thing is and why you would use it. -Another paragraph that explains what this thing is. +If you start to describe another concept, stop yourself. +Each concept should be about **one concept only**. -Remember, if you start to describe about another concept, stop yourself. -Each concept should be about one concept only. +If you start to describe **how to use the thing**, stop yourself. +Task topics explain how to use something, not concept topics. + +Do not include links to related tasks. The navigation provides links to tasks. ``` ## Concept topic titles diff --git a/doc/development/documentation/topic_types/index.md b/doc/development/documentation/topic_types/index.md index 964b41303cb..cfc231c268a 100644 --- a/doc/development/documentation/topic_types/index.md +++ b/doc/development/documentation/topic_types/index.md @@ -28,7 +28,7 @@ If inline links are not sufficient, you can create a topic called **Related topi and include an unordered list of related topics. This topic should be above the Troubleshooting section. ```markdown -# Related topics +## Related topics - [Configure your pipeline](link-to-topic). - [Trigger a pipeline manually](link-to-topic). diff --git a/doc/development/documentation/workflow.md b/doc/development/documentation/workflow.md index 2effa21b266..3c73030aceb 100644 --- a/doc/development/documentation/workflow.md +++ b/doc/development/documentation/workflow.md @@ -21,7 +21,7 @@ If you are not a GitLab team member, or do not have the Developer role for the G 1. Select an [issue](https://about.gitlab.com/handbook/product/ux/technical-writing/#community-contribution-opportunities) you'd like to work on. - You don't need an issue to open a merge request. - - For a Hackathon, in the issue, in a comment, mention the person who opened the issue and ask for the issue to be assigned to you. + - For a Hackathon, mention `@docs-hackathon` in a comment and ask for the issue to be assigned to you. To be fair to other contributors, if you see someone has already asked to work on the issue, choose another issue. If you are looking for issues to work on and don't see any that suit you, you can always fix [Vale](testing.md#vale) issues. 1. Go to the [GitLab repository](https://gitlab.com/gitlab-org/gitlab). diff --git a/doc/development/ee_features.md b/doc/development/ee_features.md index 5e236c3e322..4eb5bedef1c 100644 --- a/doc/development/ee_features.md +++ b/doc/development/ee_features.md @@ -209,7 +209,10 @@ To test an EE class that doesn't exist in CE, create the spec file as you normal would in the `ee/spec` directory, but without the second `ee/` subdirectory. For example, a class `ee/app/models/vulnerability.rb` would have its tests in `ee/spec/models/vulnerability_spec.rb`. -By default, licensed features are disabled while specs are running. To effectively test your feature +By default, licensed features are disabled for specs in `specs/`. +Specs in the `ee/spec` directory have Starter license initialized by default. + +To effectively test your feature you must explicitly enable the feature using the `stub_licensed_features` helper, for example: ```ruby diff --git a/doc/development/elasticsearch.md b/doc/development/elasticsearch.md index 88a417b4745..961a47e005b 100644 --- a/doc/development/elasticsearch.md +++ b/doc/development/elasticsearch.md @@ -4,7 +4,7 @@ group: Global Search info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments --- -# Elasticsearch knowledge **(PREMIUM SELF)** +# Elasticsearch knowledge This area is to maintain a compendium of useful information when working with Elasticsearch. @@ -191,8 +191,7 @@ If the current version is `v12p1`, and we need to create a new version for `v12p NOTE: This only supported for indices created with GitLab 13.0 or greater. -Migrations are stored in the [`ee/elastic/migrate/`](https://gitlab.com/gitlab-org/gitlab/-/tree/master/ee/elastic/migrate) folder with `YYYYMMDDHHMMSS_migration_name.rb` -filename format, which is similar to Rails database migrations: +In the [`ee/elastic/migrate/`](https://gitlab.com/gitlab-org/gitlab/-/tree/master/ee/elastic/migrate) folder, create a new file with the filename format `YYYYMMDDHHMMSS_migration_name.rb`. This format is the same for Rails database migrations. ```ruby # frozen_string_literal: true @@ -225,6 +224,86 @@ To update Elastic index mappings, apply the configuration to the respective file Migrations can be built with a retry limit and have the ability to be [failed and marked as halted](https://gitlab.com/gitlab-org/gitlab/-/blob/66e899b6637372a4faf61cfd2f254cbdd2fb9f6d/ee/lib/elastic/migration.rb#L40). Any data or index cleanup needed to support migration retries should be handled within the migration. +### Migration helpers + +The following migration helpers are available in `ee/app/workers/concerns/elastic/`: + +#### `Elastic::MigrationBackfillHelper` + +Backfills a specific field in an index. In most cases, the mapping for the field should already be added. + +Requires the `index_name` and `field_name` methods. + +```ruby +class MigrationName < Elastic::Migration + include Elastic::MigrationBackfillHelper + + private + + def index_name + Issue.__elasticsearch__.index_name + end + + def field_name + :schema_version + end +end +``` + +#### `Elastic::MigrationUpdateMappingsHelper` + +Updates a mapping in an index by calling `put_mapping` with the mapping specified. + +Requires the `index_name` and `new_mappings` methods. + +```ruby +class MigrationName < Elastic::Migration + include Elastic::MigrationUpdateMappingsHelper + + private + + def index_name + Issue.__elasticsearch__.index_name + end + + def new_mappings + { + schema_version: { + type: 'short' + } + } + end +end +``` + +#### `Elastic::MigrationObsolete` + +Marks a migration as obsolete when it's no longer required. + +```ruby +class MigrationName < Elastic::Migration + include Elastic::MigrationObsolete +end +``` + +#### `Elastic::MigrationHelper` + +Contains methods you can use when a migration doesn't fit the previous examples. + +```ruby +class MigrationName < Elastic::Migration + include Elastic::MigrationHelper + + def migrate + ... + end + + def completed? + ... + end +end +``` + ### Migration options supported by the `Elastic::MigrationWorker` [`Elastic::MigrationWorker`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/ee/app/workers/elastic/migration_worker.rb) supports the following migration options: diff --git a/doc/development/experiment_guide/implementing_experiments.md b/doc/development/experiment_guide/implementing_experiments.md index e1407473731..5bce9f1fab5 100644 --- a/doc/development/experiment_guide/implementing_experiments.md +++ b/doc/development/experiment_guide/implementing_experiments.md @@ -138,7 +138,7 @@ communicate about experiments as something that's wider than just user behavior. NOTE: Using `actor:` uses cookies if the `current_user` is nil. If you don't need cookies though - meaning that the exposed functionality would only be visible to -signed in users - `{ user: current_user }` would be just as effective. +authenticated users - `{ user: current_user }` would be just as effective. WARNING: The caching of variant assignment is done by using this context, and so consider diff --git a/doc/development/fe_guide/content_editor.md b/doc/development/fe_guide/content_editor.md index 982033cf2ad..6d13f419430 100644 --- a/doc/development/fe_guide/content_editor.md +++ b/doc/development/fe_guide/content_editor.md @@ -4,7 +4,7 @@ group: Editor info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments --- -# Content Editor development guidelines **(FREE)** +# Content Editor development guidelines The Content Editor is a UI component that provides a WYSIWYG editing experience for [GitLab Flavored Markdown](../../user/markdown.md) in the GitLab application. @@ -64,7 +64,7 @@ Instead, you should obtain an instance of the `ContentEditor` class by listening ```html <script> -import createFlash from '~/flash'; +import { createAlert } from '~/flash'; import { __ } from '~/locale'; export default { @@ -75,7 +75,7 @@ export default { try { await this.contentEditor.setSerializedContent(this.content); } catch (e) { - createFlash(__('Could not load initial document')); + createAlert({ message: __('Could not load initial document') }); } }, submitChanges() { diff --git a/doc/development/fe_guide/customizable_dashboards.md b/doc/development/fe_guide/customizable_dashboards.md index 807f83f5bec..7e7718f8e60 100644 --- a/doc/development/fe_guide/customizable_dashboards.md +++ b/doc/development/fe_guide/customizable_dashboards.md @@ -4,13 +4,15 @@ group: Product Analytics info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments --- -# Customizable dashboards **(PREMIUM)** +# Customizable dashboards > [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/98610) in GitLab 15.5 as an [Alpha feature](../../policy/alpha-beta-support.md#alpha-features). Customizable dashboards provide a dashboard structure that allows users to create their own dashboards and commit the structure to a repository. +This feature is available for Premium and Ultimate subscriptions. + ## Usage To use customizable dashboards: diff --git a/doc/development/fe_guide/merge_request_widget_extensions.md b/doc/development/fe_guide/merge_request_widget_extensions.md index 49c6664c6d6..5ad918d466b 100644 --- a/doc/development/fe_guide/merge_request_widget_extensions.md +++ b/doc/development/fe_guide/merge_request_widget_extensions.md @@ -4,7 +4,7 @@ group: Code Review info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments --- -# Merge request widget extensions **(FREE)** +# Merge request widget extensions > [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/44616) in GitLab 13.6. @@ -40,7 +40,7 @@ export default { summary(data) {}, // Required: Level 1 summary text statusIcon(data) {}, // Required: Level 1 status icon tertiaryButtons() {}, // Optional: Level 1 action buttons - shouldCollapse() {}, // Optional: Add logic to determine if the widget can expand or not + shouldCollapse(data) {}, // Optional: Add logic to determine if the widget can expand or not }, methods: { fetchCollapsedData(props) {}, // Required: Fetches data required for collapsed state diff --git a/doc/development/fe_guide/source_editor.md b/doc/development/fe_guide/source_editor.md index 4cfc68553e0..5f2e8c1e029 100644 --- a/doc/development/fe_guide/source_editor.md +++ b/doc/development/fe_guide/source_editor.md @@ -4,7 +4,7 @@ group: Editor info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments --- -# Source Editor **(FREE)** +# Source Editor **Source Editor** provides the editing experience at GitLab. This thin wrapper around [the Monaco editor](https://microsoft.github.io/monaco-editor/) provides necessary diff --git a/doc/development/fe_guide/style/scss.md b/doc/development/fe_guide/style/scss.md index 7a5c955db93..b84f41311b6 100644 --- a/doc/development/fe_guide/style/scss.md +++ b/doc/development/fe_guide/style/scss.md @@ -61,6 +61,41 @@ Inspiration: - <https://tailwindcss.com/docs/utility-first> - <https://tailwindcss.com/docs/extracting-components> +#### Utility mixins + +In addition to utility classes GitLab UI provides utility mixins named after the utility classes. + +For example a utility class `.gl-mt-3` will have a corresponding mixin `gl-mt-3`. Here's how it can be used in an SCSS file: + +```scss +.my-class { + @include gl-mt-3; +} +``` + +These mixins should be used to replace _magic values_ in our code. +For example a `margin-top: 8px` is a good candidate for the `@include gl-mt-3` mixin replacement. + +Avoid using utility mixins for [pre-defined CSS keywords](https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Values_and_Units#pre-defined_keyword_values). +For example prefer `display: flex` over `@include gl-display-flex`. + +```scss +// Bad +.my-class { + @include gl-display-flex; +} + +// Good +.my-class { + display: flex; +} + +// Good +.my-class { + @include gl-mt-3; +} +``` + ### Naming Filenames should use `snake_case`. diff --git a/doc/development/fe_guide/view_component.md b/doc/development/fe_guide/view_component.md index 90bb75514d8..f9d148d8b82 100644 --- a/doc/development/fe_guide/view_component.md +++ b/doc/development/fe_guide/view_component.md @@ -179,9 +179,9 @@ For the full list of options, see its The `Pajamas::CheckboxComponent` follows the [Pajamas Checkbox](https://design.gitlab.com/components/checkbox/) specification. NOTE: -`Pajamas::CheckboxComponent` is used internally by the [GitLab UI form builder](haml.md#use-the-gitlab-ui-form-builder) and requires an instance of [ActionView::Helpers::FormBuilder](https://api.rubyonrails.org/v6.1.0/classes/ActionView/Helpers/FormBuilder.html) to be passed as the `form` argument. +`Pajamas::CheckboxComponent` is used internally by the [GitLab UI form builder](haml.md#use-the-gitlab-ui-form-builder) and requires an instance of [ActionView::Helpers::FormBuilder](https://api.rubyonrails.org/classes/ActionView/Helpers/FormBuilder.html) to be passed as the `form` argument. It is preferred to use the [`gitlab_ui_checkbox_component`](haml.md#gitlab_ui_checkbox_component) method to render this ViewComponent. -To use a checkbox without an instance of [ActionView::Helpers::FormBuilder](https://api.rubyonrails.org/v6.1.0/classes/ActionView/Helpers/FormBuilder.html) use [CheckboxTagComponent](#checkbox-tag). +To use a checkbox without an instance of [ActionView::Helpers::FormBuilder](https://api.rubyonrails.org/classes/ActionView/Helpers/FormBuilder.html) use [CheckboxTagComponent](#checkbox-tag). For the full list of options, see its [source](https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/components/pajamas/checkbox_component.rb). diff --git a/doc/development/fe_guide/vuex.md b/doc/development/fe_guide/vuex.md index 19bbfa314ea..01ee50fb6ca 100644 --- a/doc/development/fe_guide/vuex.md +++ b/doc/development/fe_guide/vuex.md @@ -97,7 +97,7 @@ In this file, we write the actions that call mutations for handling a list of us ```javascript import * as types from './mutation_types'; import axios from '~/lib/utils/axios_utils'; - import createFlash from '~/flash'; + import { createAlert } from '~/flash'; export const fetchUsers = ({ state, dispatch }) => { commit(types.REQUEST_USERS); @@ -106,7 +106,7 @@ In this file, we write the actions that call mutations for handling a list of us .then(({ data }) => commit(types.RECEIVE_USERS_SUCCESS, data)) .catch((error) => { commit(types.RECEIVE_USERS_ERROR, error) - createFlash({ message: 'There was an error' }) + createAlert({ message: 'There was an error' }) }); } diff --git a/doc/development/feature_categorization/index.md b/doc/development/feature_categorization/index.md index 47663915ea7..7f275f25c3d 100644 --- a/doc/development/feature_categorization/index.md +++ b/doc/development/feature_categorization/index.md @@ -8,7 +8,7 @@ info: To determine the technical writer assigned to the Stage/Group associated w > [Introduced](https://gitlab.com/groups/gitlab-com/gl-infra/-/epics/269) in GitLab 13.2. -Each Sidekiq worker, controller action, [test example](../testing_guide/best_practices.md#feature-category-metadata) or API endpoint +Each Sidekiq worker, Batched Background migrations, controller action, [test example](../testing_guide/best_practices.md#feature-category-metadata) or API endpoint must declare a `feature_category` attribute. This attribute maps each of these to a [feature category](https://about.gitlab.com/handbook/product/categories/). This is done for error budgeting, alert routing, and team attribution. @@ -76,6 +76,25 @@ category (worker or HTTP endpoint) in metrics and logs. For instance, `ReactiveCachingWorker` can have multiple feature categories in metrics and logs. +## Batched background migrations + +Long-running migrations (as per the [time limits guidelines](../migration_style_guide.md#how-long-a-migration-should-take)) +are pulled out as [batched background migrations](../database/batched_background_migrations.md). +They should define a `feature_category`, like this: + +```ruby +# File name: lib/gitlab/background_migration/my_background_migration_job.rb + +class MyBackgroundMigrationJob < BatchedMigrationJob + feature_category :gitaly + + #... +end +``` + +NOTE: +[`RuboCop::Cop::BackgroundMigration::FeatureCategory`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/rubocop/cop/background_migration/feature_category.rb) cop ensures a valid `feature_category` is defined. + ## Rails controllers Specifying feature categories on controller actions can be done using @@ -96,7 +115,7 @@ second argument: ```ruby class DashboardController < ApplicationController feature_category :team_planning, [:issues, :issues_calendar] - feature_category :code_review, [:merge_requests] + feature_category :code_review_workflow, [:merge_requests] end ``` @@ -203,3 +222,9 @@ can be done using the `not_owned` feature category. ```ruby RSpec.describe Utils, feature_category: :not_owned do ``` + +### Tooling feature category + +For Engineering Productivity internal tooling we use `feature_category: :tooling`. + +For example in [`spec/tooling/danger/specs_spec.rb`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/spec/tooling/danger/specs_spec.rb#L12). diff --git a/doc/development/feature_development.md b/doc/development/feature_development.md index b9e093b9479..874a56555fb 100644 --- a/doc/development/feature_development.md +++ b/doc/development/feature_development.md @@ -108,7 +108,7 @@ Consult these topics for information on contributing to specific GitLab features - [Performance guidelines](performance.md) for writing code, benchmarks, and certain patterns to avoid. - [Caching guidelines](caching.md) for using caching in Rails under a GitLab environment. -- [Merge request performance guidelines](merge_request_performance_guidelines.md) +- [Merge request performance guidelines](merge_request_concepts/performance.md) for ensuring merge requests do not negatively impact GitLab performance - [Profiling](profiling.md) a URL or tracking down N+1 queries using Bullet. - [Cached queries guidelines](cached_queries.md), for tracking down N+1 queries diff --git a/doc/development/feature_flags/controls.md b/doc/development/feature_flags/controls.md index 3e6491a92b5..f1eafc2a95a 100644 --- a/doc/development/feature_flags/controls.md +++ b/doc/development/feature_flags/controls.md @@ -328,6 +328,21 @@ If a project A has `:feature-set-1` enabled, there is no guarantee that project For more detail, see [This is how percentages work in Flipper](https://www.hackwithpassion.com/this-is-how-percentages-work-in-flipper/). +### Verifying metrics after enabling feature flag + +After turning on the feature flag, you need to [monitor the relevant graphs](https://about.gitlab.com/handbook/engineering/monitoring/) between each step: + +1. Go to [`dashboards.gitlab.net`](https://dashboards.gitlab.net). +1. Turn on the `feature-flag`. +1. Watch `Latency: Apdex` for services that might be impacted by your change + (like `sidekiq service`, `api service` or `web service`). Then check out more in-depth + dashboards by selecting `Service Overview Dashboards` and choosing a dashboard that might + be related to your change. + +In this illustration, you can see that the Apdex score started to decline after the feature flag was enabled at `09:46`. The feature flag was then deactivated at `10:31`, and the service returned to the original value: + + + ### Feature flag change logging #### ChatOps level diff --git a/doc/development/features_inside_dot_gitlab.md b/doc/development/features_inside_dot_gitlab.md index 7a46cd40da1..dda2c05288f 100644 --- a/doc/development/features_inside_dot_gitlab.md +++ b/doc/development/features_inside_dot_gitlab.md @@ -15,7 +15,7 @@ When implementing new features, please refer to these existing features to avoid - [GitLab agent](https://gitlab.com/gitlab-org/cluster-integration/gitlab-agent/-/blob/master/doc/configuration_repository.md#layout): `.gitlab/agents/`. - [CODEOWNERS](../user/project/code_owners.md#set-up-code-owners): `.gitlab/CODEOWNERS`. - [Route Maps](../ci/review_apps/index.md#route-maps): `.gitlab/route-map.yml`. -- [Customize Auto DevOps Helm Values](../topics/autodevops/customize.md#customize-values-for-helm-chart): `.gitlab/auto-deploy-values.yaml`. +- [Customize Auto DevOps Helm Values](../topics/autodevops/customize.md#customize-helm-chart-values): `.gitlab/auto-deploy-values.yaml`. - [Insights](../user/project/insights/index.md#configure-project-insights): `.gitlab/insights.yml`. - [Service Desk Templates](../user/project/service_desk.md#using-customized-email-templates): `.gitlab/service_desk_templates/`. - [Web IDE](../user/project/web_ide/index.md#web-ide-configuration-file): `.gitlab/.gitlab-webide.yml`. diff --git a/doc/development/fips_compliance.md b/doc/development/fips_compliance.md index 187a9b0cc93..147ff5fa6e9 100644 --- a/doc/development/fips_compliance.md +++ b/doc/development/fips_compliance.md @@ -22,7 +22,7 @@ mean FIPS 140-2. ## Current status -GitLab is actively working towards FIPS compliance. Progress on this initiative can be tracked with this [FIPS compliance Epic](https://gitlab.com/groups/gitlab-org/-/epics/6452). +GitLab has completed FIPS 140-2 Compliance for the build specified in this documentation. You can find our FIPS 140-2 Attestation in our [customer assurance package](https://about.gitlab.com/security/cap/), specifically the community package. ## FIPS compliance at GitLab diff --git a/doc/development/geo.md b/doc/development/geo.md index 76c75cb1c6a..710d0eec3b0 100644 --- a/doc/development/geo.md +++ b/doc/development/geo.md @@ -4,7 +4,7 @@ group: Geo info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments --- -# Geo (development) **(PREMIUM SELF)** +# Geo (development) Geo connects GitLab instances together. One GitLab instance is designated as a **primary** site and can be run with multiple @@ -602,6 +602,8 @@ For comparison with your own features, see [Supported Geo data types](../adminis If you add a feature that is backed by Git repositories, then you must add Geo support. See [the repository replicator strategy of the Geo self-service framework](geo/framework.md#repository-replicator-strategy). +Create an issue based on the [Geo Replicate a new blob type template](https://gitlab.com/gitlab-org/gitlab/-/issues/new?issuable_template=Geo%20Replicate%20a%20new%20blob%20type) and follow the guidelines. + ### Blobs If you add a subclass of `CarrierWave::Uploader::Base`, then you are adding what Geo calls a blob. If you specifically subclass [`AttachmentUploader` as generally recommended](uploads/working_with_uploads.md#recommendations), then the data has Geo support with no work needed. This is because `AttachmentUploader` tracks blobs with the `Upload` model using the `uploads` table, and Geo support is already implemented for that model. @@ -610,6 +612,8 @@ If your blobs are tracked in a new table, perhaps because you expect millions of [Geo detects new blobs with a spec](https://gitlab.com/gitlab-org/gitlab/-/blob/eeba0e4d231ae39012a5bbaeac43a72c2bd8affb/ee/spec/uploaders/every_gitlab_uploader_spec.rb) that fails when an `Uploader` does not have a corresponding `Replicator`. +Create an issue based on the [Geo Replicate a new Git repository type template](https://gitlab.com/gitlab-org/gitlab/-/issues/new?issuable_template=Geo%20Replicate%20a%20new%20Git%20repository%20type) and follow the guidelines. + ### Features with more than one kind of data If a new complex feature is backed by multiple kinds of data, for example, a Git repository and a blob, then you can likely consider each kind of data separately. diff --git a/doc/development/gitlab_flavored_markdown/index.md b/doc/development/gitlab_flavored_markdown/index.md index 0af31892726..f115ae9a11c 100644 --- a/doc/development/gitlab_flavored_markdown/index.md +++ b/doc/development/gitlab_flavored_markdown/index.md @@ -4,7 +4,9 @@ group: Editor info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments --- -# GitLab Flavored Markdown (GLFM) developer documentation **(FREE)** +<!-- vale gitlab.GitLabFlavoredMarkdown = NO --> + +# GitLab Flavored Markdown (GLFM) developer documentation This page contains the MVC for the developer documentation for GitLab Flavored Markdown (GLFM). For the user documentation about Markdown in GitLab, refer to diff --git a/doc/development/gitlab_flavored_markdown/specification_guide/index.md b/doc/development/gitlab_flavored_markdown/specification_guide/index.md index 85c6653180b..79a4ac9b49a 100644 --- a/doc/development/gitlab_flavored_markdown/specification_guide/index.md +++ b/doc/development/gitlab_flavored_markdown/specification_guide/index.md @@ -4,7 +4,9 @@ group: Editor info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments --- -# GitLab Flavored Markdown (GLFM) Specification Guide **(FREE)** +<!-- vale gitlab.GitLabFlavoredMarkdown = NO --> + +# GitLab Flavored Markdown (GLFM) Specification Guide ## Summary @@ -108,17 +110,18 @@ Here are the HTML-rendered versions of the specifications: - [GitHub Flavored Markdown (GFM) specification](https://github.github.com/gfm/) (rendered from the [source `spec.txt` for GFM specification](https://github.com/github/cmark-gfm/blob/master/test/spec.txt)), which extends the: - [CommonMark specification](https://spec.commonmark.org/0.30/) (rendered from the [source `spec.txt` for CommonMark specification](https://github.com/commonmark/commonmark-spec/blob/master/spec.txt)) -NOTE: -The creation of the -[HTML-rendered version of the GitLab Flavored Markdown (GLFM) specification](https://gitlab.com/gitlab-org/gitlab/-/blob/master/glfm_specification/output_spec/spec.html) -file is still pending. - However, GLFM has more complex parsing, rendering, and testing requirements than GFM or CommonMark. Therefore, it does not have a static, hardcoded, manually updated `spec.txt`. Instead, the GLFM `spec.txt` is automatically generated based on other input files. This process is explained in detail in the [Implementation](#implementation) sections below. +NOTE: +As of December 2022, the HTML version of the [GitHub Flavored Markdown (GFM) specification](https://github.github.com/gfm/) +is outdated, and does not match the [specification's `spec.txt`](https://github.com/github/cmark-gfm/blob/master/test/spec.txt). +An issue has been [filed in the `cmark-gfm` project](https://github.com/github/cmark-gfm/issues/288) +to report this. + #### Official specifications vs internal extensions Within GFM and GLFM respectively, both GitHub and GitLab have two "sets" of Markdown they support: @@ -240,6 +243,12 @@ This means that it uses configuration files to support providing GitLab-specific which is required by internal extension examples, such as [`glfm_example_metadata.yml`](#glfm_example_metadatayml). +The design of the snapshot testing helps ensure the correctness of the user-facing GLFM Markdown. +The testing thoroughly exercises the backend and frontend [parsers and renderers](#parsers-and-renderers) +by using a [black-box testing](https://en.wikipedia.org/wiki/Black-box_testing) approach. +It can be considered a type of high-level testing at the ["top of the testing pyramid"](https://martinfowler.com/articles/practical-test-pyramid.html) +because of this comprehensive style. + Regarding the terminology used for Markdown snapshot testing: <!-- vale gitlab.InclusionCultural = NO --> @@ -1357,7 +1366,9 @@ This section describes how the scripts can be used to manage the GLFM specificat 1. Run [`update-specification.rb`](#update-specificationrb-script) to update the GLFM specification [output specification files](#output-specification-files). 1. Visually inspect and confirm any resulting changes to the [output specification files](#output-specification-files). -1. Run [`run-spec-tests.sh`](#run-spec-testssh-script) to run the conformance tests against the canonicalized GLFM specification. +1. Run [`run-spec-tests.sh`](#run-spec-testssh-script). This script is not yet implemented + and only prints a placeholder message. When implemented, it should run the conformance tests + against the canonicalized GLFM specification. 1. Commit any changes to the [output specification files](#output-specification-files). ### Update the example snapshots and run snapshot tests diff --git a/doc/development/gitlab_shell/features.md b/doc/development/gitlab_shell/features.md new file mode 100644 index 00000000000..f7931c4b94d --- /dev/null +++ b/doc/development/gitlab_shell/features.md @@ -0,0 +1,89 @@ +--- +stage: Create +group: Source Code +info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments +--- + +# GitLab Shell feature list + +## Discover + +Allows users to identify themselves on an instance via SSH. The command helps to +confirm quickly whether a user has SSH access to the instance: + +```shell +ssh git@<hostname> + +PTY allocation request failed on channel 0 +Welcome to GitLab, @username! +Connection to staging.gitlab.com closed. +``` + +When permission is denied, it returns: + +```shell +ssh git@<hostname> +git@<hostname>: Permission denied (publickey). +``` + +## Git operations + +GitLab Shell provides support for Git operations over SSH by processing +`git-upload-pack`, `git-receive-pack` and `git-upload-archive` SSH commands. +It limits the set of commands to predefined Git commands: + +- `git archive` +- `git clone` +- `git pull` +- `git push` + +## Generate new 2FA recovery codes + +Enables users to +[generate new 2FA recovery codes](../../user/profile/account/two_factor_authentication.md#generate-new-recovery-codes-using-ssh): + +```shell +$ ssh git@<hostname> 2fa_recovery_codes + +Are you sure you want to generate new two-factor recovery codes? +Any existing recovery codes you saved will be invalidated. (yes/no) +yes + +Your two-factor authentication recovery codes are: +... +``` + +## Verify 2FA OTP + +Allows users to verify their +[2FA one-time password (OTP)](../../security/two_factor_authentication.md#2fa-for-git-over-ssh-operations): + +```shell +$ ssh git@<hostname> 2fa_verify + +OTP: 347419 + +OTP validation failed. +``` + +## LFS authentication + +Enables users to generate credentials for LFS authentication: + +```shell +$ ssh git@<hostname> git-lfs-authenticate <project-path> <upload/download> + +{"header":{"Authorization":"Basic ..."},"href":"https://gitlab.com/user/project.git/info/lfs","expires_in":7200} +``` + +## Personal access token + +Enables users to use personal access tokens via SSH: + +```shell +$ ssh git@<hostname> personal_access_token <name> <scope1[,scope2,...]> [ttl_days] + +Token: glpat-... +Scopes: api +Expires: 2022-02-05 +``` diff --git a/doc/development/gitlab_shell/gitlab_sshd.md b/doc/development/gitlab_shell/gitlab_sshd.md new file mode 100644 index 00000000000..4c2cd6396c1 --- /dev/null +++ b/doc/development/gitlab_shell/gitlab_sshd.md @@ -0,0 +1,36 @@ +--- +stage: Create +group: Source Code +info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/engineering/ux/technical-writing/#assignments +--- + +# `gitlab-sshd` in GitLab Shell + +`gitlab-sshd` is a binary in [`gitlab-shell`](https://gitlab.com/gitlab-org/gitlab-shell) +which runs as a persistent SSH daemon. It is intended to replace `OpenSSH` on GitLab SaaS, +and eventually other cloud-native environments. Instead of running an `sshd` process, +we run a `gitlab-sshd` process that does the same job, in a more focused manner: + +```mermaid +sequenceDiagram + participant Git on client + participant GitLab SSHD + participant Rails + participant Gitaly + participant Git on server + + Note left of Git on client: git fetch + Git on client->>+GitLab SSHD: ssh git fetch-pack request + GitLab SSHD->>+Rails: GET /internal/api/authorized_keys?key=AAAA... + Note right of Rails: Lookup key ID + Rails-->>-GitLab SSHD: 200 OK, command="gitlab-shell upload-pack key_id=1" + GitLab SSHD->>+Rails: GET /internal/api/allowed?action=upload_pack&key_id=1 + Note right of Rails: Auth check + Rails-->>-GitLab SSHD: 200 OK, { gitaly: ... } + GitLab SSHD->>+Gitaly: SSHService.SSHUploadPack request + Gitaly->>+Git on server: git upload-pack request + Note over Git on client,Git on server: Bidirectional communication between Git client and server + Git on server-->>-Gitaly: git upload-pack response + Gitaly -->>-GitLab SSHD: SSHService.SSHUploadPack response + GitLab SSHD-->>-Git on client: ssh git fetch-pack response +``` diff --git a/doc/development/gitlab_shell/index.md b/doc/development/gitlab_shell/index.md new file mode 100644 index 00000000000..7f2c113fa0c --- /dev/null +++ b/doc/development/gitlab_shell/index.md @@ -0,0 +1,222 @@ +--- +stage: Create +group: Source Code +info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/engineering/ux/technical-writing/#assignments +--- + +# GitLab Shell + +[](https://gitlab.com/gitlab-org/gitlab-shell/-/pipelines?ref=main) [](https://gitlab.com/gitlab-org/gitlab-shell/-/pipelines?ref=main) [](https://codeclimate.com/github/gitlabhq/gitlab-shell) + +GitLab Shell handles Git SSH sessions for GitLab and modifies the list of authorized keys. +GitLab Shell is not a Unix shell nor a replacement for Bash or Zsh. + +GitLab supports Git LFS authentication through SSH. + +## Requirements + +GitLab Shell is written in Go, and needs a Go compiler to build. It still requires +Ruby to build and test, but not to run. + +GitLab Shell runs on `port 22` on an Omnibus installation. To use a regular SSH +service, configure it on an alternative port. + +Download and install the current version of Go from [golang.org](https://golang.org/dl/). +We follow the [Golang Release Policy](https://golang.org/doc/devel/release.html#policy) +and support: + +- The current stable version. +- The previous two major versions. + +## How GitLab Shell works + +When you access the GitLab server over SSH, GitLab Shell then: + +1. Limits you to predefined Git commands (`git push`, `git pull`, `git fetch`). +1. Calls the GitLab Rails API to check if you are authorized, and what Gitaly server your repository is on. +1. Copies data back and forth between the SSH client and the Gitaly server. + +If you access a GitLab server over HTTP(S) you end up in [`gitlab-workhorse`](../workhorse/index.md). + +### `git pull` over SSH + +```mermaid +graph LR + A[Git pull] --> |via SSH| B[gitlab-shell] + B -->|API call| C[gitlab-rails<br>authorization] + C -->|accept or decline| D[Gitaly session] +``` + +### `git push` over SSH + +The `git push` command is not performed until after `gitlab-rails` accepts the push: + +```mermaid +graph LR +subgraph User initiates + A[Git push] -->|via SSH| B[gitlab-shell] +end +subgraph Gitaly + B -->|establish Gitaly session| C[gitlab-shell pre-receive hook] + C -->|API auth call| D[Gitlab-rails] + D --> E[accept or decline push] +end +``` + +[Full feature list](features.md) + +### Modifies `authorized_keys` + +GitLab Shell modifies the `authorized_keys` file on the client machine. + +## Contribute to GitLab Shell + +To contribute to GitLab Shell: + +1. Check if GitLab API access, and Redis via the internal API, can be reached: `make check` +1. Compile the `gitlab-shell` binaries, placing them into `bin/`: `make compile` +1. Run `make install` to build the `gitlab-shell` binaries and install. them onto the file system. + The default location is `/usr/local`. To change it, set the `PREFIX` and `DESTDIR` environment variables. +1. To install GitLab from source on a single machine, run `make setup`. + It compiles the GitLab Shell binaries, and ensures that various paths on the file system + exist with the correct permissions. Do not run this command unless your installation method + documentation instructs you to. + +For more information, see +[CONTRIBUTING.md](https://gitlab.com/gitlab-org/gitlab-shell/-/blob/main/CONTRIBUTING.md). + +### Run tests + +When contributing, run tests: + +1. Run tests with `bundle install` and `make test`. +1. Run Gofmt: `make verify` +1. Run both test and verify (the default Makefile target): + + ```shell + bundle install + make validate + ``` + +1. If needed, configure Gitaly. + +### Configure Gitaly for local testing + +Some tests need a Gitaly server. The +[`docker-compose.yml`](https://gitlab.com/gitlab-org/gitlab-shell/-/blob/main/docker-compose.yml) file runs Gitaly on port 8075. +To tell the tests where Gitaly is, set `GITALY_CONNECTION_INFO`: + +```plaintext +export GITALY_CONNECTION_INFO='{"address": "tcp://localhost:8075", "storage": "default"}' +make test +``` + +If no `GITALY_CONNECTION_INFO` is set, the test suite still runs, but any +tests requiring Gitaly are skipped. The tests always run in the CI environment. + +## Rate limiting + +GitLab Shell performs rate-limiting by user account and project for Git operations. +GitLab Shell accepts Git operation requests and then makes a call to the Rails +rate-limiter, backed by Redis. If the `user + project` exceeds the rate limit, +then GitLab Shell then drop further connection requests for that `user + project`. + +The rate-limiter is applied at the Git command (plumbing) level. Each command has +a rate limit of 600 per minute. For example, `git push` has 600 per minute, and +`git pull` has another 600 per minute. + +Because they are using the same plumbing command, `git-upload-pack`, `git pull`, +and `git clone` are in effect the same command for the purposes of rate-limiting. + +Gitaly also has a rate-limiter in place, but calls are never made to Gitaly if +the rate limit is exceeded in GitLab Shell (Rails). + +## Logs in GitLab Shell + +In general, you can determine the structure, but not content, of a GitLab Shell +or `gitlab-sshd` session by inspecting the logs. Some guidelines: + +- We use [`gitlab.com/gitlab-org/labkit/log`](https://pkg.go.dev/gitlab.com/gitlab-org/labkit/log) + for logging. +- Always include a correlation ID. +- Log messages should be invariant and unique. Include accessory information in + fields, using `log.WithField`, `log.WithFields`, or `log.WithError`. +- Log both success cases and error cases. +- Logging too much is better than not logging enough. If a message seems too + verbose, consider reducing the log level before removing the message. + +## GitLab SaaS + +A diagram of the flow of `gitlab-shell` on GitLab.com: + +```mermaid +graph LR + a2 --> b2 + a2 --> b3 + a2 --> b4 + b2 --> c1 + b3 --> c1 + b4 --> c1 + c2 --> d1 + c2 --> d2 + c2 --> d3 + d1 --> e1 + d2 --> e1 + d3 --> e1 + a1[Cloudflare] --> a2[TCP<br/> load balancer] + e1[Git] + + subgraph HAProxy Fleet + b2[HAProxy] + b3[HAProxy] + b4[HAProxy] + end + + subgraph GKE + c1[Internal TCP<br/> load balancer<br/>port 2222] --> c2[GitLab-shell<br/> pods] + end + + subgraph Gitaly + d1[Gitaly] + d2[Gitaly] + d3[Gitaly] + end +``` + +## GitLab Shell architecture + +```mermaid +sequenceDiagram + participant Git on client + participant SSH server + participant AuthorizedKeysCommand + participant GitLab Shell + participant Rails + participant Gitaly + participant Git on server + + Note left of Git on client: git fetch + Git on client->>+SSH server: ssh git fetch-pack request + SSH server->>+AuthorizedKeysCommand: gitlab-shell-authorized-keys-check git AAAA... + AuthorizedKeysCommand->>+Rails: GET /internal/api/authorized_keys?key=AAAA... + Note right of Rails: Lookup key ID + Rails-->>-AuthorizedKeysCommand: 200 OK, command="gitlab-shell upload-pack key_id=1" + AuthorizedKeysCommand-->>-SSH server: command="gitlab-shell upload-pack key_id=1" + SSH server->>+GitLab Shell: gitlab-shell upload-pack key_id=1 + GitLab Shell->>+Rails: GET /internal/api/allowed?action=upload_pack&key_id=1 + Note right of Rails: Auth check + Rails-->>-GitLab Shell: 200 OK, { gitaly: ... } + GitLab Shell->>+Gitaly: SSHService.SSHUploadPack request + Gitaly->>+Git on server: git upload-pack request + Note over Git on client,Git on server: Bidirectional communication between Git client and server + Git on server-->>-Gitaly: git upload-pack response + Gitaly -->>-GitLab Shell: SSHService.SSHUploadPack response + GitLab Shell-->>-SSH server: gitlab-shell upload-pack response + SSH server-->>-Git on client: ssh git fetch-pack response +``` + +## Related topics + +- [LICENSE](https://gitlab.com/gitlab-org/gitlab-shell/-/blob/main/LICENSE). +- [PROCESS.md](https://gitlab.com/gitlab-org/gitlab-shell/-/blob/main/PROCESS.md) +- [Using the GitLab Shell chart](https://docs.gitlab.com/charts/charts/gitlab/gitlab-shell/) diff --git a/doc/development/gitlab_shell/process.md b/doc/development/gitlab_shell/process.md new file mode 100644 index 00000000000..cc6f44b865c --- /dev/null +++ b/doc/development/gitlab_shell/process.md @@ -0,0 +1,71 @@ +--- +stage: Create +group: Source Code +info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/engineering/ux/technical-writing/#assignments +--- + +# Processes for GitLab Shell + +## Releasing a new version + +GitLab Shell is versioned by Git tags, and the version used by the Rails +application is stored in +[`GITLAB_SHELL_VERSION`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/GITLAB_SHELL_VERSION). + +For each version, there is a raw version and a tag version: + +- The **raw version** is the version number. For instance, `15.2.8`. +- The **tag version** is the raw version prefixed with `v`. For instance, `v15.2.8`. + +To release a new version of GitLab Shell and have that version available to the +Rails application: + +1. Create a merge request to update the [`CHANGELOG`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/CHANGELOG.md) with the + **tag version** and the [`VERSION`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/VERSION) file with the **raw version**. +1. Ask a maintainer to review and merge the merge request. If you're already a + maintainer, second maintainer review is not required. +1. Add a new Git tag with the **tag version**. +1. Update `GITLAB_SHELL_VERSION` in the Rails application to the **raw + version**. + + NOTE: + This can be done as a separate merge request, or in a merge request + that uses the latest GitLab Shell changes. + +## Security releases + +GitLab Shell is included in the packages we create for GitLab. Each version of +GitLab specifies the version of GitLab Shell it uses in the `GITLAB_SHELL_VERSION` +file. Because of this specification, security fixes in GitLab Shell are tightly coupled to the +[GitLab security release](https://about.gitlab.com/handbook/engineering/workflow/#security-issues) workflow. + +For a security fix in GitLab Shell, two sets of merge requests are required: + +1. The fix itself, in the `gitlab-org/security/gitlab-shell` repository and its + backports to the previous versions of GitLab Shell. +1. Merge requests to change the versions of GitLab Shell included in the GitLab + security release, in the `gitlab-org/security/gitlab` repository. + +The first step could be to create a merge request with a fix targeting `main` +in `gitlab-org/security/gitlab-shell`. When the merge request is approved by maintainers, +backports targeting previous 3 versions of GitLab Shell must be created. The stable +branches for those versions may not exist, so feel free to ask a maintainer to create +them. The stable branches must be created out of the GitLab Shell tags or versions +used by the 3 previous GitLab releases. + +To find out the GitLab Shell version used on a particular GitLab stable release, +run this command, replacing `13-9-stable-ee` with the version you're interested in. +These commands show the version used by the `13.9` version of GitLab: + +```shell +git fetch security 13-9-stable-ee +git show refs/remotes/security/13-9-stable-ee:GITLAB_SHELL_VERSION +``` + +Close to the GitLab security release, a maintainer should merge the fix and backports, +and cut all the necessary GitLab Shell versions. This allows bumping the +`GITLAB_SHELL_VERSION` for `gitlab-org/security/gitlab`. The GitLab merge request +is handled by the general GitLab security release process. + +After the security release is done, a GitLab Shell maintainer is responsible for +syncing tags and `main` to the `gitlab-org/gitlab-shell` repository. diff --git a/doc/development/go_guide/go_upgrade.md b/doc/development/go_guide/go_upgrade.md index d931140d9da..b3ec0a15c37 100644 --- a/doc/development/go_guide/go_upgrade.md +++ b/doc/development/go_guide/go_upgrade.md @@ -193,7 +193,7 @@ be included in the relevant issues as part of the definition of done: Upstream component maintainers must validate their Go-based projects using: - Established unit tests in the codebase. -- Procedures established in [Merge Request Performance Guidelines](../merge_request_performance_guidelines.md). +- Procedures established in [Merge Request Performance Guidelines](../merge_request_concepts/performance.md). - Procedures established in [Performance, Reliability, and Availability guidelines](../code_review.md#performance-reliability-and-availability). Upstream component maintainers should consider validating their Go-based diff --git a/doc/development/i18n/proofreader.md b/doc/development/i18n/proofreader.md index 8a4ad73efe3..cc7232cd793 100644 --- a/doc/development/i18n/proofreader.md +++ b/doc/development/i18n/proofreader.md @@ -86,6 +86,7 @@ are very appreciative of the work done by translators and proofreaders! - Tomo Dote - [GitLab](https://gitlab.com/fu7mu4), [Crowdin](https://crowdin.com/profile/fu7mu4) - Hiromi Nozawa - [GitLab](https://gitlab.com/hir0mi), [Crowdin](https://crowdin.com/profile/hir0mi) - Takuya Noguchi - [GitLab](https://gitlab.com/tnir), [Crowdin](https://crowdin.com/profile/tnir) + - Tsukasa Komatsubara - [GitLab](https://gitlab.com/tkomatsubara), [Crowdin](https://crowdin.com/profile/tkomatsubara) - Korean - Chang-Ho Cha - [GitLab](https://gitlab.com/changho-cha), [Crowdin](https://crowdin.com/profile/zzazang) - Ji Hun Oh - [GitLab](https://gitlab.com/Baw-Appie), [Crowdin](https://crowdin.com/profile/BawAppie) diff --git a/doc/development/image_scaling.md b/doc/development/image_scaling.md index 502a18fecd7..d182bd8333e 100644 --- a/doc/development/image_scaling.md +++ b/doc/development/image_scaling.md @@ -1,6 +1,6 @@ --- stage: Manage -group: Workspace +group: Organization info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments --- diff --git a/doc/development/img/feature-flag-metrics.png b/doc/development/img/feature-flag-metrics.png Binary files differnew file mode 100644 index 00000000000..ce8702d47eb --- /dev/null +++ b/doc/development/img/feature-flag-metrics.png diff --git a/doc/development/integrations/index.md b/doc/development/integrations/index.md index 1c9144a1163..9fd8fb7eb61 100644 --- a/doc/development/integrations/index.md +++ b/doc/development/integrations/index.md @@ -5,7 +5,7 @@ info: To determine the technical writer assigned to the Stage/Group associated w description: "GitLab's development guidelines for Integrations" --- -# Integrations development guide **(FREE)** +# Integrations development guide This page provides development guidelines for implementing [GitLab integrations](../../user/project/integrations/index.md), which are part of our [main Rails project](https://gitlab.com/gitlab-org/gitlab). diff --git a/doc/development/integrations/jenkins.md b/doc/development/integrations/jenkins.md index f314f4536e2..6baccdca327 100644 --- a/doc/development/integrations/jenkins.md +++ b/doc/development/integrations/jenkins.md @@ -4,7 +4,7 @@ group: Integrations info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments --- -# How to run Jenkins in development environment (on macOS) **(FREE)** +# How to run Jenkins in development environment (on macOS) This is a step by step guide on how to set up [Jenkins](https://www.jenkins.io/) on your local machine and connect to it from your GitLab instance. GitLab triggers webhooks on Jenkins, and Jenkins connects to GitLab using the API. By running both applications on the same machine, we can make sure they are able to access each other. diff --git a/doc/development/integrations/jira_connect.md b/doc/development/integrations/jira_connect.md index c7bb77a6a5d..5b460f8723a 100644 --- a/doc/development/integrations/jira_connect.md +++ b/doc/development/integrations/jira_connect.md @@ -4,7 +4,7 @@ group: Integrations info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments --- -# Set up a development environment **(FREE)** +# Set up a development environment The following are required to install and test the app: diff --git a/doc/development/integrations/secure.md b/doc/development/integrations/secure.md index 190a6f6eda2..f5bb2df2494 100644 --- a/doc/development/integrations/secure.md +++ b/doc/development/integrations/secure.md @@ -350,9 +350,9 @@ version is 14.0.6, the report is validated against version 14.0.6. GitLab uses the [`json_schemer`](https://www.rubydoc.info/gems/json_schemer) gem to perform validation. -Ongoing improvements to report validation are tracked [in this epic](https://gitlab.com/groups/gitlab-org/-/epics/6968). +Ongoing improvements to report validation are tracked [in this epic](https://gitlab.com/groups/gitlab-org/-/epics/8900). In the meantime, you can see which versions are supported in the -[source code](https://gitlab.com/gitlab-org/gitlab/-/blob/08dd756429731a0cca1e27ca9d59eea226398a7d/lib/gitlab/ci/parsers/security/validators/schema_validator.rb#L9-27). +[source code](https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/gitlab/ci/parsers/security/validators/schema_validator.rb#L9). Remember to pick the correct version for your instance, for example [`v15.7.3-ee`](https://gitlab.com/gitlab-org/gitlab/-/blob/v15.7.3-ee/lib/gitlab/ci/parsers/security/validators/schema_validator.rb#L9). #### Validate locally diff --git a/doc/development/internal_api/index.md b/doc/development/internal_api/index.md index 68d9b88bc05..f0fdedd801f 100644 --- a/doc/development/internal_api/index.md +++ b/doc/development/internal_api/index.md @@ -5,7 +5,7 @@ info: "To determine the technical writer assigned to the Stage/Group associated type: reference, api --- -# Internal API **(FREE)** +# Internal API The internal API is used by different GitLab components, it cannot be used by other consumers. This documentation is intended for people @@ -964,17 +964,17 @@ Example response: - CustomersDot -## SCIM API **(PREMIUM SAAS)** +## Group SCIM API **(PREMIUM SAAS)** > [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/9388) in GitLab 11.10. -The SCIM API implements the [RFC7644 protocol](https://www.rfc-editor.org/rfc/rfc7644). As this API is for +The group SCIM API implements the [RFC7644 protocol](https://www.rfc-editor.org/rfc/rfc7644). As this API is for **system** use for SCIM provider integration, it is subject to change without notice. -To use this API, [Group SSO](../../user/group/saml_sso/index.md) must be enabled for the group. +To use this API, enable [Group SSO](../../user/group/saml_sso/index.md) for the group. This API is only in use where [SCIM for Group SSO](../../user/group/saml_sso/scim_setup.md) is enabled. It's a prerequisite to the creation of SCIM identities. -Not to be confused with the [main SCIM API](../../api/scim.md). +This API is different to the [main SCIM API](../../api/scim.md) and the [instance SCIM API](#instance-scim-api). ### Get a list of SCIM provisioned users @@ -991,7 +991,7 @@ Parameters: |:----------|:--------|:---------|:----------------------------------------------------------------------------------------------------------------------------------------| | `filter` | string | no | A [filter](#available-filters) expression. | | `group_path` | string | yes | Full path to the group. | -| `startIndex` | integer | no | The 1-based index indicating where to start returning results from. A value of less than one will be interpreted as 1. | +| `startIndex` | integer | no | The 1-based index indicating where to start returning results from. A value of less than one is interpreted as 1. | | `count` | integer | no | Desired maximum number of query results. | NOTE: @@ -1185,6 +1185,219 @@ curl --verbose --request DELETE "https://gitlab.example.com/api/scim/v2/groups/t Returns an empty response with a `204` status code if successful. +## Instance SCIM API **(PREMIUM SELF)** + +> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/378599) in GitLab 15.8. + +The Instance SCIM API implements the [RFC7644 protocol](https://www.rfc-editor.org/rfc/rfc7644). As this API is for +**system** use for SCIM provider integration, it is subject to change without notice. + +To use this API, enable [SAML SSO](../../integration/saml.md) for the instance. + +This API is different to the [main SCIM API](../../api/scim.md) and the [group SCIM API](#group-scim-api). + +### Get a list of SCIM provisioned users + +This endpoint is used as part of the SCIM syncing mechanism. It only returns +a single user based on a unique ID which should match the `extern_uid` of the user. + +```plaintext +GET /api/scim/v2/application/Users +``` + +Parameters: + +| Attribute | Type | Required | Description | +|:----------|:--------|:---------|:----------------------------------------------------------------------------------------------------------------------------------------| +| `filter` | string | no | A [filter](#available-filters) expression. | +| `startIndex` | integer | no | The 1-based index indicating where to start returning results from. A value of less than one is interpreted as 1. | +| `count` | integer | no | Desired maximum number of query results. | + +NOTE: +Pagination follows the [SCIM spec](https://www.rfc-editor.org/rfc/rfc7644#section-3.4.2.4) rather than GitLab pagination as used elsewhere. If records change between requests it is possible for a page to either be missing records that have moved to a different page or repeat records from a previous request. + +Example request: + +```shell +curl "https://gitlab.example.com/api/scim/v2/application/Users?filter=id%20eq%20%220b1d561c-21ff-4092-beab-8154b17f82f2%22" \ + --header "Authorization: Bearer <your_scim_token>" \ + --header "Content-Type: application/scim+json" +``` + +Example response: + +```json +{ + "schemas": [ + "urn:ietf:params:scim:api:messages:2.0:ListResponse" + ], + "totalResults": 1, + "itemsPerPage": 20, + "startIndex": 1, + "Resources": [ + { + "schemas": [ + "urn:ietf:params:scim:schemas:core:2.0:User" + ], + "id": "0b1d561c-21ff-4092-beab-8154b17f82f2", + "active": true, + "name.formatted": "Test User", + "userName": "username", + "meta": { "resourceType":"User" }, + "emails": [ + { + "type": "work", + "value": "name@example.com", + "primary": true + } + ] + } + ] +} +``` + +### Get a single SCIM provisioned user + +```plaintext +GET /api/scim/v2/application/Users/:id +``` + +Parameters: + +| Attribute | Type | Required | Description | +|:----------|:--------|:---------|:----------------------------------------------------------------------------------------------------------------------------------------| +| `id` | string | yes | External UID of the user. | + +Example request: + +```shell +curl "https://gitlab.example.com/api/scim/v2/application/Users/f0b1d561c-21ff-4092-beab-8154b17f82f2" \ + --header "Authorization: Bearer <your_scim_token>" --header "Content-Type: application/scim+json" +``` + +Example response: + +```json +{ + "schemas": [ + "urn:ietf:params:scim:schemas:core:2.0:User" + ], + "id": "0b1d561c-21ff-4092-beab-8154b17f82f2", + "active": true, + "name.formatted": "Test User", + "userName": "username", + "meta": { "resourceType":"User" }, + "emails": [ + { + "type": "work", + "value": "name@example.com", + "primary": true + } + ] +} +``` + +### Create a SCIM provisioned user + +```plaintext +POST /api/scim/v2/application/Users +``` + +Parameters: + +| Attribute | Type | Required | Description | +|:---------------|:----------|:----|:--------------------------| +| `externalId` | string | yes | External UID of the user. | +| `userName` | string | yes | Username of the user. | +| `emails` | JSON string | yes | Work email. | +| `name` | JSON string | yes | Name of the user. | +| `meta` | string | no | Resource type (`User`). | + +Example request: + +```shell +curl --verbose --request POST "https://gitlab.example.com/api/scim/v2/application/Users" \ + --data '{"externalId":"test_uid","active":null,"userName":"username","emails":[{"primary":true,"type":"work","value":"name@example.com"}],"name":{"formatted":"Test User","familyName":"User","givenName":"Test"},"schemas":["urn:ietf:params:scim:schemas:core:2.0:User"],"meta":{"resourceType":"User"}}' \ + --header "Authorization: Bearer <your_scim_token>" --header "Content-Type: application/scim+json" +``` + +Example response: + +```json +{ + "schemas": [ + "urn:ietf:params:scim:schemas:core:2.0:User" + ], + "id": "0b1d561c-21ff-4092-beab-8154b17f82f2", + "active": true, + "name.formatted": "Test User", + "userName": "username", + "meta": { "resourceType":"User" }, + "emails": [ + { + "type": "work", + "value": "name@example.com", + "primary": true + } + ] +} +``` + +Returns a `201` status code if successful. + +### Update a single SCIM provisioned user + +Fields that can be updated are: + +| SCIM/IdP field | GitLab field | +|:---------------------------------|:-----------------------------------------------------------------------------| +| `id/externalId` | `extern_uid` | +| `active` | Identity removal if `active` = `false` | + +```plaintext +PATCH /api/scim/v2/application/Users/:id +``` + +Parameters: + +| Attribute | Type | Required | Description | +|:----------|:--------|:---------|:----------------------------------------------------------------------------------------------------------------------------------------| +| `id` | string | yes | External UID of the user. | +| `Operations` | JSON string | yes | An [operations](#available-operations) expression. | + +Example request: + +```shell +curl --verbose --request PATCH "https://gitlab.example.com/api/scim/v2/application/Users/f0b1d561c-21ff-4092-beab-8154b17f82f2" \ + --data '{ "Operations": [{"op":"Add","path":"name.formatted","value":"New Name"}] }' \ + --header "Authorization: Bearer <your_scim_token>" --header "Content-Type: application/scim+json" +``` + +Returns an empty response with a `204` status code if successful. + +### Remove a single SCIM provisioned user + +Removes the user's SSO identity. + +```plaintext +DELETE /api/scim/v2/application/Users/:id +``` + +Parameters: + +| Attribute | Type | Required | Description | +| ------------ | ------ | -------- | ------------------------- | +| `id` | string | yes | External UID of the user. | + +Example request: + +```shell +curl --verbose --request DELETE "https://gitlab.example.com/api/scim/v2/application/Users/f0b1d561c-21ff-4092-beab-8154b17f82f2" \ + --header "Authorization: Bearer <your_scim_token>" --header "Content-Type: application/scim+json" +``` + +Returns an empty response with a `204` status code if successful. + ### Available filters They match an expression as specified in [the RFC7644 filtering section](https://www.rfc-editor.org/rfc/rfc7644#section-3.4.2.2). diff --git a/doc/development/kubernetes.md b/doc/development/kubernetes.md index 332a3c4ab09..e44e2af4371 100644 --- a/doc/development/kubernetes.md +++ b/doc/development/kubernetes.md @@ -4,7 +4,7 @@ group: Configure info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments --- -# Kubernetes integration - development guidelines **(FREE)** +# Kubernetes integration - development guidelines This document provides various guidelines when developing for the GitLab [Kubernetes integration](../user/infrastructure/clusters/index.md). diff --git a/doc/development/lfs.md b/doc/development/lfs.md index dd7687cd28b..ec91f9f3c8b 100644 --- a/doc/development/lfs.md +++ b/doc/development/lfs.md @@ -4,7 +4,7 @@ group: Source Code info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments --- -# Git LFS developer information **(FREE)** +# Git LFS developer information This page contains developer-centric information for GitLab team members. For the user documentation, see [Git Large File Storage](../topics/git/lfs/index.md). diff --git a/doc/development/logging.md b/doc/development/logging.md index 44a47f96de1..6282f0f6677 100644 --- a/doc/development/logging.md +++ b/doc/development/logging.md @@ -4,7 +4,7 @@ group: Respond info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments --- -# GitLab Developers Guide to Logging **(FREE)** +# GitLab Developers Guide to Logging [GitLab Logs](../administration/logs/index.md) play a critical role for both administrators and GitLab team members to diagnose problems in the field. diff --git a/doc/development/maintenance_mode.md b/doc/development/maintenance_mode.md index b61b7472385..486519715ab 100644 --- a/doc/development/maintenance_mode.md +++ b/doc/development/maintenance_mode.md @@ -4,7 +4,7 @@ group: Geo info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments --- -# Internal workings of GitLab Maintenance Mode **(PREMIUM SELF)** +# Internal workings of GitLab Maintenance Mode ## Where is Maintenance Mode enforced? diff --git a/doc/development/merge_request_concepts/diffs/development.md b/doc/development/merge_request_concepts/diffs/development.md new file mode 100644 index 00000000000..1c22eff34db --- /dev/null +++ b/doc/development/merge_request_concepts/diffs/development.md @@ -0,0 +1,188 @@ +--- +stage: Create +group: Code Review +info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments +--- + +# Merge request diffs development guide + +This document explains the backend design and flow of merge request diffs. +It should help contributors: + +- Understand the code design. +- Identify areas for improvement through contribution. + +It's intentional that it doesn't contain too many implementation details, as they +can change often. The code better explains these details. The components +mentioned here are the major parts of the application for how merge request diffs +are generated, stored, and returned to users. + +NOTE: +This page is a living document. Update it accordingly when the parts +of the codebase touched in this document are changed or removed, or when new components +are added. + +## Data model + +Four main ActiveRecord models represent what we collectively refer to +as _diffs._ These database-backed records replicate data contained in the +project's Git repository, and are in part a cache against excessive access requests +to [Gitaly](../../gitaly.md). Additionally, they provide a logical place for: + +- Calculated and retrieved metadata about the pieces of the diff. +- General class- and instance- based logic. + +```mermaid +erDiagram + MergeRequest ||--|{ MergeRequestDiff: "" + MergeRequestDiff |{--|{ MergeRequestDiffCommit: "" + MergeRequestDiff |{--|| MergeRequestDiffDetail: "" + MergeRequestDiff |{--|{ MergeRequestDiffFile: "" + MergeRequestDiffCommit |{--|| MergeRequestDiffCommitUser: "" +``` + +### `MergeRequestDiff` + +`MergeRequestDiff` is defined in `app/models/merge_request_diff.rb`. This +class holds metadata and context related to the diff resulting from a set of +commits. It defines methods that are the primary means for interacting with diff +contents, individual commits, and the files containing changes. + +```ruby +#<MergeRequestDiff:0x00007fd1ed63b4d0 + id: 28, + state: "collected", + merge_request_id: 28, + created_at: Tue, 06 Sep 2022 18:56:02.509469000 UTC +00:00, + updated_at: Tue, 06 Sep 2022 18:56:02.754201000 UTC +00:00, + base_commit_sha: "ae73cb07c9eeaf35924a10f713b364d32b2dd34f", + real_size: "9", + head_commit_sha: "bb5206fee213d983da88c47f9cf4cc6caf9c66dc", + start_commit_sha: "0b4bc9a49b562e85de7cc9e834518ea6828729b9", + commits_count: 6, + external_diff: "diff-28", + external_diff_store: 1, + stored_externally: nil, + files_count: 9, + sorted: true, + diff_type: "regular", + verification_checksum: nil> +``` + +Diff content is usually accessed through this class. Logic is often applied +to diff, file, and commit content before it is returned to a user. + +### `MergeRequestDiffCommit` + +`MergeRequestDiffCommit` is defined in `app/models/merge_request_diff_commit.rb`. +This class corresponds to a single commit contained in its corresponding `MergeRequestDiff`, +and holds header information about the commit. + +```ruby +#<MergeRequestDiffCommit:0x00007fd1dfc6c4c0 + authored_date: Wed, 06 Aug 2022 06:35:52.000000000 UTC +00:00, + committed_date: Wed, 06 Aug 2022 06:35:52.000000000 UTC +00:00, + merge_request_diff_id: 28, + relative_order: 0, + sha: "bb5206fee213d983da88c47f9cf4cc6caf9c66dc", + message: "Feature conflcit added\n\nSigned-off-by: Sample User <sample.user@example.com>\n", + trailers: {}, + commit_author_id: 19, + committer_id: 19> +``` + +Every `MergeRequestDiffCommit` has a corresponding `MergeRequest::DiffCommitUser` +record it `:belongs_to`, in ActiveRecord parlance. These records are `:commit_author` +and `:committer`, and could be distinct individuals. + +### `MergeRequest::DiffCommitUser` + +`MergeRequest::DiffCommitUser` is defined in `app/models/merge_request/diff_commit_user.rb`. +It captures the `name` and `email` of a given commit, but contains no connection +itself to any `User` records. + +```ruby +#<MergeRequest::DiffCommitUser:0x00007fd1dff7c930 + id: 19, + name: "Sample User", + email: "sample.user@example.com"> +``` + +### `MergeRequestDiffFile` + +`MergeRequestDiffFile` is defined in `app/models/merge_request_diff_file.rb`. +This record of this class represents the diff of a single file contained in the +`MergeRequestDiff`. It holds both meta and specific information about the file's +relationship to the change, such as: + +- Whether it is added or renamed. +- Its ordering in the diff. +- The raw diff output itself. + +### `MergeRequestDiffDetail` + +`MergeRequestDiffDetail` is defined in `app/models/merge_request_diff_detail.rb`. +This class provides verification information for Geo replication, but otherwise +is not used for user-facing diffs. + +```ruby +#<MergeRequestDiffFile:0x00007fd1ef7c9048 + merge_request_diff_id: 28, + relative_order: 0, + new_file: true, + renamed_file: false, + deleted_file: false, + too_large: false, + a_mode: "0", + b_mode: "100644", + new_path: "files/ruby/feature.rb", + old_path: "files/ruby/feature.rb", + diff: + "@@ -0,0 +1,4 @@\n+# This file was changed in feature branch\n+# We put different code here to make merge conflict\n+class Conflict\n+end\n", + binary: false, + external_diff_offset: nil, + external_diff_size: nil> +``` + +## Flow + +These flowcharts should help explain the flow from the controllers down to the +models for different features. This page is not intended to document the entirety +of options for access and working with diffs, focusing solely on the most common. + +### `batch_diffs.json` + +The most common avenue for viewing diffs is the **Changes** +tab in the top navigation bar of merge request pages in the GitLab UI. When selected, the +diffs themselves are loaded via a paginated request to `/-/merge_requests/:id/batch_diffs.json`, +which is served by [`Projects::MergeRequests::DiffsController#diffs_batch`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/controllers/projects/merge_requests/diffs_controller.rb): + +<!-- Don't delete the characters below. Mermaid returns a syntax error if they aren't included.--> + +```mermaid +sequenceDiagram + Note over .#diffs_batch: Preload diffs and ivars + .#diffs_batch->>+.#define_diff_vars: + .#define_diff_vars ->>+ @merge_request: @merge_request_diffs = + Note right of @merge_request: An ordered collection of all diffs in MR + @merge_request-->>-.#define_diff_vars: + .#define_diff_vars ->>+ @merge_request: @merge_request_diff = + Note right of @merge_request: Most recent merge_request_diff (or commit) + @merge_request-->>-.#define_diff_vars: + .#define_diff_vars ->>+ .#define_diff_vars: @compare = + Note right of .#define_diff_vars:: param-filtered merge_request_diff(s) + .#define_diff_vars -->>- .#diffs_batch: + Note over .#diffs_batch: Preloading complete + .#diffs_batch->>+@merge_request: Calculate unfoldable diff lines + Note right of @merge_request: note_positions_for_paths.unfoldable + @merge_request-->>-.#diffs_batch: + Note over .#diffs_batch: Build options hash + Note over .#diffs_batch: Build cache_context + Note over .#diffs_batch: Unfold files in diff + .#diffs_batch->>+Gitlab_Diff_FileCollection_MergeRequestDiffBase: diffs.write_diff + Gitlab_Diff_FileCollection_MergeRequestDiffBase->>+Gitlab_Diff_HighlightCache: Highlight diff + Gitlab_Diff_HighlightCache -->>-Gitlab_Diff_FileCollection_MergeRequestDiffBase: Return highlighted diff + Note over Gitlab_Diff_FileCollection_MergeRequestDiffBase: Cache diff + Gitlab_Diff_FileCollection_MergeRequestDiffBase-->>-.#diffs_batch: + Note over .#diffs_batch: render JSON +``` diff --git a/doc/development/merge_request_concepts/diffs/index.md b/doc/development/merge_request_concepts/diffs/index.md new file mode 100644 index 00000000000..8ef3f01aba9 --- /dev/null +++ b/doc/development/merge_request_concepts/diffs/index.md @@ -0,0 +1,199 @@ +--- +stage: Create +group: Code Review +info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments +--- + +# Working with diffs + +We rely on different sources to present diffs. These include: + +- Gitaly service +- Database (through `merge_request_diff_files`) +- Redis (cached highlighted diffs) + +## Deep Dive + +<!-- vale gitlab.Spelling = NO --> + +In January 2019, Oswaldo Ferreira hosted a Deep Dive (GitLab team members only: +`https://gitlab.com/gitlab-org/create-stage/issues/1`) on GitLab Diffs and Commenting on Diffs +functionality to share domain-specific knowledge with anyone who may work in this part of the +codebase in the future: + +<!-- vale gitlab.Spelling = YES --> + +- <i class="fa fa-youtube-play youtube" aria-hidden="true"></i> + [Recording on YouTube](https://www.youtube.com/watch?v=K6G3gMcFyek) +- Slides on [Google Slides](https://docs.google.com/presentation/d/1bGutFH2AT3bxOPZuLMGl1ANWHqFnrxwQwjiwAZkF-TU/edit) +- [PDF slides](https://gitlab.com/gitlab-org/create-stage/uploads/b5ad2f336e0afcfe0f99db0af0ccc71a/) + +Everything covered in this deep dive was accurate as of GitLab 11.7, and while specific details may +have changed since then, it should still serve as a good introduction. + +## Architecture overview + +### Merge request diffs + +When refreshing a merge request (pushing to a source branch, force-pushing to target branch, or if the target branch now contains any commits from the MR) +we fetch the comparison information using `Gitlab::Git::Compare`, which fetches `base` and `head` data using Gitaly and diff between them through +`Gitlab::Git::Diff.between`. +The diffs fetching process _limits_ single file diff sizes and the overall size of the whole diff through a series of constant values. Raw diff files are +then persisted on `merge_request_diff_files` table. + +Even though diffs larger than 10% of the value of `ApplicationSettings#diff_max_patch_bytes` are collapsed, +we still keep them on PostgreSQL. However, diff files larger than defined _safety limits_ +(see the [Diff limits section](#diff-limits)) are _not_ persisted in the database. + +In order to present diffs information on the merge request diffs page, we: + +1. Fetch all diff files from database `merge_request_diff_files` +1. Fetch the _old_ and _new_ file blobs in batch to: + - Highlight old and new file content + - Know which viewer it should use for each file (text, image, deleted, etc) + - Know if the file content changed + - Know if it was stored externally + - Know if it had storage errors +1. If the diff file is cacheable (text-based), it's cached on Redis + using `Gitlab::Diff::FileCollection::MergeRequestDiff` + +### Note diffs + +When commenting on a diff (any comparison), we persist a truncated diff version +on `NoteDiffFile` (which is associated with the actual `DiffNote`). So instead +of hitting the repository every time we need the diff of the file, we: + +1. Check whether we have the `NoteDiffFile#diff` persisted and use it +1. Otherwise, if it's a current MR revision, use the persisted + `MergeRequestDiffFile#diff` +1. In the last scenario, go the repository and fetch the diff + +## Diff limits + +As explained above, we limit single diff files and the size of the whole diff. There are scenarios where we collapse the diff file, +and cases where the diff file is not presented at all, and the user is guided to the Blob view. + +### Diff collection limits + +Limits that act onto all diff files collection. Files number, lines number and files size are considered. + +```ruby +Gitlab::Git::DiffCollection.collection_limits[:safe_max_files] = Gitlab::Git::DiffCollection::DEFAULT_LIMITS[:max_files] = 100 +``` + +File diffs are collapsed (but are expandable) if 100 files have already been rendered. + +```ruby +Gitlab::Git::DiffCollection.collection_limits[:safe_max_lines] = Gitlab::Git::DiffCollection::DEFAULT_LIMITS[:max_lines] = 5000 +``` + +File diffs are collapsed (but be expandable) if 5000 lines have already been rendered. + +```ruby +Gitlab::Git::DiffCollection.collection_limits[:safe_max_bytes] = Gitlab::Git::DiffCollection.collection_limits[:safe_max_files] * 5.kilobytes = 500.kilobytes +``` + +File diffs are collapsed (but be expandable) if 500 kilobytes have already been rendered. + +```ruby +Gitlab::Git::DiffCollection.collection_limits[:max_files] = Commit::DIFF_HARD_LIMIT_FILES = 1000 +``` + +No more files are rendered at all if 1000 files have already been rendered. + +```ruby +Gitlab::Git::DiffCollection.collection_limits[:max_lines] = Commit::DIFF_HARD_LIMIT_LINES = 50000 +``` + +No more files are rendered at all if 50,000 lines have already been rendered. + +```ruby +Gitlab::Git::DiffCollection.collection_limits[:max_bytes] = Gitlab::Git::DiffCollection.collection_limits[:max_files] * 5.kilobytes = 5000.kilobytes +``` + +No more files are rendered at all if 5 megabytes have already been rendered. + +All collection limit parameters are sent and applied on Gitaly. That is, after the limit is surpassed, +Gitaly only returns the safe amount of data to be persisted on `merge_request_diff_files`. + +### Individual diff file limits + +Limits that act onto each diff file of a collection. Files number, lines number and files size are considered. + +#### Expandable patches (collapsed) + +Diff patches are collapsed when surpassing 10% of the value set in `ApplicationSettings#diff_max_patch_bytes`. +That is, it's equivalent to 10kb if the maximum allowed value is 100kb. +The diff is persisted and expandable if the patch size doesn't +surpass `ApplicationSettings#diff_max_patch_bytes`. + +Although this nomenclature (Collapsing) is also used on Gitaly, this limit is only used on GitLab (hardcoded - not sent to Gitaly). +Gitaly only returns `Diff.Collapsed` (RPC) when surpassing collection limits. + +#### Not expandable patches (too large) + +The patch not be rendered if it's larger than `ApplicationSettings#diff_max_patch_bytes`. +Users see a `Changes are too large to be shown.` message and a button to view only that file in that commit. + +```ruby +Commit::DIFF_SAFE_LINES = Gitlab::Git::DiffCollection::DEFAULT_LIMITS[:max_lines] = 5000 +``` + +File diff is suppressed (technically different from collapsed, but behaves the same, and is expandable) if it has more than 5000 lines. + +This limit is hardcoded and only applied on GitLab. + +## Viewers + +Diff Viewers, which can be found on `models/diff_viewer/*` are classes used to map metadata about each type of Diff File. It has information +whether it's a binary, which partial should be used to render it or which File extensions this class accounts for. + +`DiffViewer::Base` validates _blobs_ (old and new versions) content, extension and file type to check if it can be rendered. + +## Merge request diffs against the `HEAD` of the target branch + +Historically, merge request diffs have been calculated by `git diff target...source` which compares the +`HEAD` of the source branch with the merge base (or a common ancestor) of the target branch and the source's. +This solution works well until the target branch starts containing some of the +changes introduced by the source branch: Consider the following case, in which the source branch +is `feature_a` and the target is `main`: + +1. Checkout a new branch `feature_a` from `main` and remove `file_a` and `file_b` in it. +1. Add a commit that removes `file_a` to `main`. + +The merge request diff still contains the `file_a` removal while the actual diff compared to +`main`'s `HEAD` has only the `file_b` removal. The diff with such redundant +changes is harder to review. + +In order to display an up-to-date diff, in GitLab 12.9 we +[introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/27008) merge request +diffs compared against `HEAD` of the target branch: the +target branch is artificially merged into the source branch, then the resulting +merge ref is compared to the source branch to calculate an accurate +diff. + +Until we complete the epics ["use merge refs for diffs"](https://gitlab.com/groups/gitlab-org/-/epics/854) +and ["merge conflicts in diffs"](https://gitlab.com/groups/gitlab-org/-/epics/4893), +both options `main (base)` and `main (HEAD)` are available to be displayed in merge requests: + + + +The `main (HEAD)` option is meant to replace `main (base)` in the future. + +In order to support comments for both options, diff note positions are stored for +both `main (base)` and `main (HEAD)` versions ([introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/198457) in 12.10). +The position for `main (base)` version is stored in `Note#position` and +`Note#original_position` columns, for `main (HEAD)` version `DiffNotePosition` +has been introduced. + +One of the key challenges to deal with when working on merge ref diffs are merge +conflicts. If the target and source branch contains a merge conflict, the branches +cannot be automatically merged. The +<i class="fa fa-youtube-play youtube" aria-hidden="true"></i> [recording on YouTube](https://www.youtube.com/watch?v=GFXIFA4ZuZw&feature=youtu.be&ab_channel=GitLabUnfiltered) +is a quick introduction to the problem and the motivation behind the [epic](https://gitlab.com/groups/gitlab-org/-/epics/854). + +In 13.5 a solution for both-modified merge +conflict has been +[introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/232484). However, +there are more classes of merge conflicts that are to be +[addressed](https://gitlab.com/groups/gitlab-org/-/epics/4893) in the future. diff --git a/doc/development/img/merge_ref_head_options_v13_6.png b/doc/development/merge_request_concepts/img/merge_ref_head_options_v13_6.png Binary files differindex 3134092cc92..3134092cc92 100644 --- a/doc/development/img/merge_ref_head_options_v13_6.png +++ b/doc/development/merge_request_concepts/img/merge_ref_head_options_v13_6.png diff --git a/doc/development/merge_request_concepts/performance.md b/doc/development/merge_request_concepts/performance.md new file mode 100644 index 00000000000..c1bdd45891d --- /dev/null +++ b/doc/development/merge_request_concepts/performance.md @@ -0,0 +1,565 @@ +--- +stage: none +group: unassigned +info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments +--- + +# Merge Request Performance Guidelines + +Each new introduced merge request **should be performant by default**. + +To ensure a merge request does not negatively impact performance of GitLab +_every_ merge request **should** adhere to the guidelines outlined in this +document. There are no exceptions to this rule unless specifically discussed +with and agreed upon by backend maintainers and performance specialists. + +It's also highly recommended that you read the following guides: + +- [Performance Guidelines../performance.md) +- [Avoiding downtime in migrations](../database/avoiding_downtime_in_migrations.md) + +## Definition + +The term `SHOULD` per the [RFC 2119](https://www.ietf.org/rfc/rfc2119.txt) means: + +> This word, or the adjective "RECOMMENDED", mean that there +> may exist valid reasons in particular circumstances to ignore a +> particular item, but the full implications must be understood and +> carefully weighed before choosing a different course. + +Ideally, each of these tradeoffs should be documented +in the separate issues, labeled accordingly and linked +to original issue and epic. + +## Impact Analysis + +**Summary:** think about the impact your merge request may have on performance +and those maintaining a GitLab setup. + +Any change submitted can have an impact not only on the application itself but +also those maintaining it and those keeping it up and running (for example, production +engineers). As a result you should think carefully about the impact of your +merge request on not only the application but also on the people keeping it up +and running. + +Can the queries used potentially take down any critical services and result in +engineers being woken up in the night? Can a malicious user abuse the code to +take down a GitLab instance? Do my changes make loading a certain page +slower? Does execution time grow exponentially given enough load or data in the +database? + +These are all questions one should ask themselves before submitting a merge +request. It may sometimes be difficult to assess the impact, in which case you +should ask a performance specialist to review your code. See the "Reviewing" +section below for more information. + +## Performance Review + +**Summary:** ask performance specialists to review your code if you're not sure +about the impact. + +Sometimes it's hard to assess the impact of a merge request. In this case you +should ask one of the merge request reviewers to review your changes. You can +find a list of these reviewers at <https://about.gitlab.com/company/team/>. A reviewer +in turn can request a performance specialist to review the changes. + +## Think outside of the box + +Everyone has their own perception of how to use the new feature. +Always consider how users might be using the feature instead. Usually, +users test our features in a very unconventional way, +like by brute forcing or abusing edge conditions that we have. + +## Data set + +The data set the merge request processes should be known +and documented. The feature should clearly document what the expected +data set is for this feature to process, and what problems it might cause. + +If you would think about the following example that puts +a strong emphasis of data set being processed. +The problem is simple: you want to filter a list of files from +some Git repository. Your feature requests a list of all files +from the repository and perform search for the set of files. +As an author you should in context of that problem consider +the following: + +1. What repositories are planned to be supported? +1. How long it do big repositories like Linux kernel take? +1. Is there something that we can do differently to not process such a + big data set? +1. Should we build some fail-safe mechanism to contain + computational complexity? Usually it's better to degrade + the service for a single user instead of all users. + +## Query plans and database structure + +The query plan can tell us if we need additional +indexes, or expensive filtering (such as using sequential scans). + +Each query plan should be run against substantial size of data set. +For example, if you look for issues with specific conditions, +you should consider validating a query against +a small number (a few hundred) and a big number (100_000) of issues. +See how the query behaves if the result is a few +and a few thousand. + +This is needed as we have users using GitLab for very big projects and +in a very unconventional way. Even if it seems that it's unlikely +that such a big data set is used, it's still plausible that one +of our customers could encounter a problem with the feature. + +Understanding ahead of time how it behaves at scale, even if we accept it, +is the desired outcome. We should always have a plan or understanding of what is needed +to optimize the feature for higher usage patterns. + +Every database structure should be optimized and sometimes even over-described +in preparation for easy extension. The hardest part after some point is +data migration. Migrating millions of rows is always troublesome and +can have a negative impact on the application. + +To better understand how to get help with the query plan reviews +read this section on [how to prepare the merge request for a database review../database_review.md#how-to-prepare-the-merge-request-for-a-database-review). + +## Query Counts + +**Summary:** a merge request **should not** increase the total number of executed SQL +queries unless absolutely necessary. + +The total number of queries executed by the code modified or added by a merge request +must not increase unless absolutely necessary. When building features it's +entirely possible you need some extra queries, but you should try to keep +this at a minimum. + +As an example, say you introduce a feature that updates a number of database +rows with the same value. It may be very tempting (and easy) to write this using +the following pseudo code: + +```ruby +objects_to_update.each do |object| + object.some_field = some_value + object.save +end +``` + +This means running one query for every object to update. This code can +easily overload a database given enough rows to update or many instances of this +code running in parallel. This particular problem is known as the +["N+1 query problem"](https://guides.rubyonrails.org/active_record_querying.html#eager-loading-associations). You can write a test with [QueryRecorder](../database/query_recorder.md) to detect this and prevent regressions. + +In this particular case the workaround is fairly easy: + +```ruby +objects_to_update.update_all(some_field: some_value) +``` + +This uses ActiveRecord's `update_all` method to update all rows in a single +query. This in turn makes it much harder for this code to overload a database. + +## Use read replicas when possible + +In a DB cluster we have many read replicas and one primary. A classic use of scaling the DB is to have read-only actions be performed by the replicas. We use [load balancing](../../administration/postgresql/database_load_balancing.md) to distribute this load. This allows for the replicas to grow as the pressure on the DB grows. + +By default, queries use read-only replicas, but due to +[primary sticking](../../administration/postgresql/database_load_balancing.md#primary-sticking), GitLab uses the +primary for some time and reverts to secondaries after they have either caught up or after 30 seconds. +Doing this can lead to a considerable amount of unnecessary load on the primary. +To prevent switching to the primary [merge request 56849](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/56849) introduced the +`without_sticky_writes` block. Typically, this method can be applied to prevent primary stickiness +after a trivial or insignificant write which doesn't affect the following queries in the same session. + +To learn when a usage timestamp update can lead the session to stick to the primary and how to +prevent it by using `without_sticky_writes`, see [merge request 57328](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/57328) + +As a counterpart of the `without_sticky_writes` utility, +[merge request 59167](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/59167) introduced +`use_replicas_for_read_queries`. This method forces all read-only queries inside its block to read +replicas regardless of the current primary stickiness. +This utility is reserved for cases where queries can tolerate replication lag. + +Internally, our database load balancer classifies the queries based on their main statement (`select`, `update`, `delete`, and so on). When in doubt, it redirects the queries to the primary database. Hence, there are some common cases the load balancer sends the queries to the primary unnecessarily: + +- Custom queries (via `exec_query`, `execute_statement`, `execute`, and so on) +- Read-only transactions +- In-flight connection configuration set +- Sidekiq background jobs + +After the above queries are executed, GitLab +[sticks to the primary](../../administration/postgresql/database_load_balancing.md#primary-sticking). +To make the inside queries prefer using the replicas, +[merge request 59086](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/59086) introduced +`fallback_to_replicas_for_ambiguous_queries`. This MR is also an example of how we redirected a +costly, time-consuming query to the replicas. + +## Use CTEs wisely + +Read about [complex queries on the relation object../database/iterating_tables_in_batches.md#complex-queries-on-the-relation-object) for considerations on how to use CTEs. We have found in some situations that CTEs can become problematic in use (similar to the n+1 problem above). In particular, hierarchical recursive CTE queries such as the CTE in [AuthorizedProjectsWorker](https://gitlab.com/gitlab-org/gitlab/-/issues/325688) are very difficult to optimize and don't scale. We should avoid them when implementing new features that require any kind of hierarchical structure. + +CTEs have been effectively used as an optimization fence in many simpler cases, +such as this [example](https://gitlab.com/gitlab-org/gitlab-foss/-/issues/43242#note_61416277). +Beginning in PostgreSQL 12, CTEs are inlined then [optimized by default](https://paquier.xyz/postgresql-2/postgres-12-with-materialize/). +Keeping the old behavior requires marking CTEs with the keyword `MATERIALIZED`. + +When building CTE statements, use the `Gitlab::SQL::CTE` class [introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/56976) in GitLab 13.11. +By default, this `Gitlab::SQL::CTE` class forces materialization through adding the `MATERIALIZED` keyword for PostgreSQL 12 and higher. +`Gitlab::SQL::CTE` automatically omits materialization when PostgreSQL 11 is running +(this behavior is implemented using a custom Arel node `Gitlab::Database::AsWithMaterialized` under the surface). + +WARNING: +Upgrading to GitLab 14.0 requires PostgreSQL 12 or higher. + +## Cached Queries + +**Summary:** a merge request **should not** execute duplicated cached queries. + +Rails provides an [SQL Query Cache](../cached_queries.md#cached-queries-guidelines), +used to cache the results of database queries for the duration of the request. + +See [why cached queries are considered bad](../cached_queries.md#why-cached-queries-are-considered-bad) and +[how to detect them](../cached_queries.md#how-to-detect-cached-queries). + +The code introduced by a merge request, should not execute multiple duplicated cached queries. + +The total number of the queries (including cached ones) executed by the code modified or added by a merge request +should not increase unless absolutely necessary. +The number of executed queries (including cached queries) should not depend on +collection size. +You can write a test by passing the `skip_cached` variable to [QueryRecorder../database/query_recorder.md) to detect this and prevent regressions. + +As an example, say you have a CI pipeline. All pipeline builds belong to the same pipeline, +thus they also belong to the same project (`pipeline.project`): + +```ruby +pipeline_project = pipeline.project +# Project Load (0.6ms) SELECT "projects".* FROM "projects" WHERE "projects"."id" = $1 LIMIT $2 +build = pipeline.builds.first + +build.project == pipeline_project +# CACHE Project Load (0.0ms) SELECT "projects".* FROM "projects" WHERE "projects"."id" = $1 LIMIT $2 +# => true +``` + +When we call `build.project`, it doesn't hit the database, it uses the cached result, but it re-instantiates +the same pipeline project object. It turns out that associated objects do not point to the same in-memory object. + +If we try to serialize each build: + +```ruby +pipeline.builds.each do |build| + build.to_json(only: [:name], include: [project: { only: [:name]}]) +end +``` + +It re-instantiates project object for each build, instead of using the same in-memory object. + +In this particular case the workaround is fairly easy: + +```ruby +ActiveRecord::Associations::Preloader.new.preload(pipeline, [builds: :project]) + +pipeline.builds.each do |build| + build.to_json(only: [:name], include: [project: { only: [:name]}]) +end +``` + +`ActiveRecord::Associations::Preloader` uses the same in-memory object for the same project. +This avoids the cached SQL query and also avoids re-instantiation of the project object for each build. + +## Executing Queries in Loops + +**Summary:** SQL queries **must not** be executed in a loop unless absolutely +necessary. + +Executing SQL queries in a loop can result in many queries being executed +depending on the number of iterations in a loop. This may work fine for a +development environment with little data, but in a production environment this +can quickly spiral out of control. + +There are some cases where this may be needed. If this is the case this should +be clearly mentioned in the merge request description. + +## Batch process + +**Summary:** Iterating a single process to external services (for example, PostgreSQL, Redis, Object Storage) +should be executed in a **batch-style** to reduce connection overheads. + +For fetching rows from various tables in a batch-style, please see [Eager Loading](#eager-loading) section. + +### Example: Delete multiple files from Object Storage + +When you delete multiple files from object storage, like GCS, +executing a single REST API call multiple times is a quite expensive +process. Ideally, this should be done in a batch-style, for example, S3 provides +[batch deletion API](https://docs.aws.amazon.com/AmazonS3/latest/API/API_DeleteObjects.html), +so it'd be a good idea to consider such an approach. + +The `FastDestroyAll` module might help this situation. It's a +small framework when you remove a bunch of database rows and its associated data +in a batch style. + +## Timeout + +**Summary:** You should set a reasonable timeout when the system invokes HTTP calls +to external services (such as Kubernetes), and it should be executed in Sidekiq, not +in Puma threads. + +Often, GitLab needs to communicate with an external service such as Kubernetes +clusters. In this case, it's hard to estimate when the external service finishes +the requested process, for example, if it's a user-owned cluster that's inactive for some reason, +GitLab might wait for the response forever ([Example](https://gitlab.com/gitlab-org/gitlab/-/issues/31475)). +This could result in Puma timeout and should be avoided at all cost. + +You should set a reasonable timeout, gracefully handle exceptions and surface the +errors in UI or logging internally. + +Using [`ReactiveCaching`../utilities.md#reactivecaching) is one of the best solutions to fetch external data. + +## Keep database transaction minimal + +**Summary:** You should avoid accessing to external services like Gitaly during database +transactions, otherwise it leads to severe contention problems +as an open transaction basically blocks the release of a PostgreSQL backend connection. + +For keeping transaction as minimal as possible, please consider using `AfterCommitQueue` +module or `after_commit` AR hook. + +Here is [an example](https://gitlab.com/gitlab-org/gitlab/-/issues/36154#note_247228859) +that one request to Gitaly instance during transaction triggered a ~"priority::1" issue. + +## Eager Loading + +**Summary:** always eager load associations when retrieving more than one row. + +When retrieving multiple database records for which you need to use any +associations you **must** eager load these associations. For example, if you're +retrieving a list of blog posts and you want to display their authors you +**must** eager load the author associations. + +In other words, instead of this: + +```ruby +Post.all.each do |post| + puts post.author.name +end +``` + +You should use this: + +```ruby +Post.all.includes(:author).each do |post| + puts post.author.name +end +``` + +Also consider using [QueryRecoder tests](../database/query_recorder.md) to prevent a regression when eager loading. + +## Memory Usage + +**Summary:** merge requests **must not** increase memory usage unless absolutely +necessary. + +A merge request must not increase the memory usage of GitLab by more than the +absolute bare minimum required by the code. This means that if you have to parse +some large document (for example, an HTML document) it's best to parse it as a stream +whenever possible, instead of loading the entire input into memory. Sometimes +this isn't possible, in that case this should be stated explicitly in the merge +request. + +## Lazy Rendering of UI Elements + +**Summary:** only render UI elements when they are actually needed. + +Certain UI elements may not always be needed. For example, when hovering over a +diff line there's a small icon displayed that can be used to create a new +comment. Instead of always rendering these kind of elements they should only be +rendered when actually needed. This ensures we don't spend time generating +Haml/HTML when it's not used. + +## Use of Caching + +**Summary:** cache data in memory or in Redis when it's needed multiple times in +a transaction or has to be kept around for a certain time period. + +Sometimes certain bits of data have to be re-used in different places during a +transaction. In these cases this data should be cached in memory to remove the +need for running complex operations to fetch the data. You should use Redis if +data should be cached for a certain time period instead of the duration of the +transaction. + +For example, say you process multiple snippets of text containing username +mentions (for example, `Hello @alice` and `How are you doing @alice?`). By caching the +user objects for every username we can remove the need for running the same +query for every mention of `@alice`. + +Caching data per transaction can be done using +[RequestStore](https://github.com/steveklabnik/request_store) (use +`Gitlab::SafeRequestStore` to avoid having to remember to check +`RequestStore.active?`). Caching data in Redis can be done using +[Rails' caching system](https://guides.rubyonrails.org/caching_with_rails.html). + +## Pagination + +Each feature that renders a list of items as a table needs to include pagination. + +The main styles of pagination are: + +1. Offset-based pagination: user goes to a specific page, like 1. User sees the next page number, + and the total number of pages. This style is well supported by all components of GitLab. +1. Offset-based pagination, but without the count: user goes to a specific page, like 1. + User sees only the next page number, but does not see the total amount of pages. +1. Next page using keyset-based pagination: user can only go to next page, as we don't know how many pages + are available. +1. Infinite scrolling pagination: user scrolls the page and next items are loaded asynchronously. This is ideal, + as it has exact same benefits as the previous one. + +The ultimately scalable solution for pagination is to use Keyset-based pagination. +However, we don't have support for that at GitLab at that moment. You +can follow the progress looking at [API: Keyset Pagination](https://gitlab.com/groups/gitlab-org/-/epics/2039). + +Take into consideration the following when choosing a pagination strategy: + +1. It's very inefficient to calculate amount of objects that pass the filtering, + this operation usually can take seconds, and can time out, +1. It's very inefficient to get entries for page at higher ordinals, like 1000. + The database has to sort and iterate all previous items, and this operation usually + can result in substantial load put on database. + +You can find useful tips related to pagination in the [pagination guidelines../database/pagination_guidelines.md). + +## Badge counters + +Counters should always be truncated. It means that we don't want to present +the exact number over some threshold. The reason for that is for the cases where we want +to calculate exact number of items, we effectively need to filter each of them for +the purpose of knowing the exact number of items matching. + +From ~UX perspective it's often acceptable to see that you have over 1000+ pipelines, +instead of that you have 40000+ pipelines, but at a tradeoff of loading page for 2s longer. + +An example of this pattern is the list of pipelines and jobs. We truncate numbers to `1000+`, +but we show an accurate number of running pipelines, which is the most interesting information. + +There's a helper method that can be used for that purpose - `NumbersHelper.limited_counter_with_delimiter` - +that accepts an upper limit of counting rows. + +In some cases it's desired that badge counters are loaded asynchronously. +This can speed up the initial page load and give a better user experience overall. + +## Usage of feature flags + +Each feature that has performance critical elements or has a known performance deficiency +needs to come with feature flag to disable it. + +The feature flag makes our team more happy, because they can monitor the system and +quickly react without our users noticing the problem. + +Performance deficiencies should be addressed right away after we merge initial +changes. + +Read more about when and how feature flags should be used in +[Feature flags in GitLab development](https://about.gitlab.com/handbook/product-development-flow/feature-flag-lifecycle/#feature-flags-in-gitlab-development). + +## Storage + +We can consider the following types of storages: + +- **Local temporary storage** (very-very short-term storage) This type of storage is system-provided storage, like a `/tmp` folder. + This is the type of storage that you should ideally use for all your temporary tasks. + The fact that each node has its own temporary storage makes scaling significantly easier. + This storage is also very often SSD-based, thus is significantly faster. + The local storage can easily be configured for the application with + the usage of `TMPDIR` variable. + +- **Shared temporary storage** (short-term storage) This type of storage is network-based temporary storage, + usually run with a common NFS server. As of Feb 2020, we still use this type of storage + for most of our implementations. Even though this allows the above limit to be significantly larger, + it does not really mean that you can use more. The shared temporary storage is shared by + all nodes. Thus, the job that uses significant amount of that space or performs a lot + of operations creates a contention on execution of all other jobs and request + across the whole application, this can easily impact stability of the whole GitLab. + Be respectful of that. + +- **Shared persistent storage** (long-term storage) This type of storage uses + shared network-based storage (for example, NFS). This solution is mostly used by customers running small + installations consisting of a few nodes. The files on shared storage are easily accessible, + but any job that is uploading or downloading data can create a serious contention to all other jobs. + This is also an approach by default used by Omnibus. + +- **Object-based persistent storage** (long term storage) this type of storage uses external + services like [AWS S3](https://en.wikipedia.org/wiki/Amazon_S3). The Object Storage + can be treated as infinitely scalable and redundant. Accessing this storage usually requires + downloading the file to manipulate it. The Object Storage can be considered as an ultimate + solution, as by definition it can be assumed that it can handle unlimited concurrent uploads + and downloads of files. This is also ultimate solution required to ensure that application can + run in containerized deployments (Kubernetes) at ease. + +### Temporary storage + +The storage on production nodes is really sparse. The application should be built +in a way that accommodates running under very limited temporary storage. +You can expect the system on which your code runs has a total of `1G-10G` +of temporary storage. However, this storage is really shared across all +jobs being run. If your job requires to use more than `100MB` of that space +you should reconsider the approach you have taken. + +Whatever your needs are, you should clearly document if you need to process files. +If you require more than `100MB`, consider asking for help from a maintainer +to work with you to possibly discover a better solution. + +#### Local temporary storage + +The usage of local storage is a desired solution to use, +especially since we work on deploying applications to Kubernetes clusters. +When you would like to use `Dir.mktmpdir`? In a case when you want for example +to extract/create archives, perform extensive manipulation of existing data, and so on. + +```ruby +Dir.mktmpdir('designs') do |path| + # do manipulation on path + # the path will be removed once + # we go out of the block +end +``` + +#### Shared temporary storage + +The usage of shared temporary storage is required if your intent +is to persistent file for a disk-based storage, and not Object Storage. +[Workhorse direct upload](../uploads/index.md#direct-upload) when accepting file +can write it to shared storage, and later GitLab Rails can perform a move operation. +The move operation on the same destination is instantaneous. +The system instead of performing `copy` operation just re-attaches file into a new place. + +Since this introduces extra complexity into application, you should only try +to re-use well established patterns (for example, `ObjectStorage` concern) instead of re-implementing it. + +The usage of shared temporary storage is otherwise deprecated for all other usages. + +### Persistent storage + +#### Object Storage + +It is required that all features holding persistent files support saving data +to Object Storage. Having a persistent storage in the form of shared volume across nodes +is not scalable, as it creates a contention on data access all nodes. + +GitLab offers the [ObjectStorage concern](https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/uploaders/object_storage.rb) +that implements a seamless support for Shared and Object Storage-based persistent storage. + +#### Data access + +Each feature that accepts data uploads or allows to download them needs to use +[Workhorse direct upload](../uploads/index.md#direct-upload). It means that uploads needs to be +saved directly to Object Storage by Workhorse, and all downloads needs to be served +by Workhorse. + +Performing uploads/downloads via Puma is an expensive operation, +as it blocks the whole processing slot (thread) for the duration of the upload. + +Performing uploads/downloads via Puma also has a problem where the operation +can time out, which is especially problematic for slow clients. If clients take a long time +to upload/download the processing slot might be killed due to request processing +timeout (usually between 30s-60s). + +For the above reasons it is required that [Workhorse direct upload../uploads/index.md#direct-upload) is implemented +for all file uploads and downloads. diff --git a/doc/development/merge_request_diffs.md b/doc/development/merge_request_diffs.md new file mode 100644 index 00000000000..9ec7e6cae8b --- /dev/null +++ b/doc/development/merge_request_diffs.md @@ -0,0 +1,11 @@ +--- +redirect_to: 'merge_request_concepts/diffs/development.md' +remove_date: '2023-04-10' +--- + +This document was moved to [another location](merge_request_concepts/diffs/development.md). + +<!-- This redirect file can be deleted after <2023-04-10>. --> +<!-- Redirects that point to other docs in the same project expire in three months. --> +<!-- Redirects that point to docs in a different project or site (for example, link is not relative and starts with `https:`) expire in one year. --> +<!-- Before deletion, see: https://docs.gitlab.com/ee/development/documentation/redirects.html --> diff --git a/doc/development/merge_request_performance_guidelines.md b/doc/development/merge_request_performance_guidelines.md index fd78d02202f..1af81a8af9f 100644 --- a/doc/development/merge_request_performance_guidelines.md +++ b/doc/development/merge_request_performance_guidelines.md @@ -1,565 +1,11 @@ --- -stage: none -group: unassigned -info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments +redirect_to: 'merge_request_concepts/performance.md' +remove_date: '2023-04-10' --- -# Merge Request Performance Guidelines +This document was moved to [another location](merge_request_concepts/performance.md). -Each new introduced merge request **should be performant by default**. - -To ensure a merge request does not negatively impact performance of GitLab -_every_ merge request **should** adhere to the guidelines outlined in this -document. There are no exceptions to this rule unless specifically discussed -with and agreed upon by backend maintainers and performance specialists. - -It's also highly recommended that you read the following guides: - -- [Performance Guidelines](performance.md) -- [Avoiding downtime in migrations](database/avoiding_downtime_in_migrations.md) - -## Definition - -The term `SHOULD` per the [RFC 2119](https://www.ietf.org/rfc/rfc2119.txt) means: - -> This word, or the adjective "RECOMMENDED", mean that there -> may exist valid reasons in particular circumstances to ignore a -> particular item, but the full implications must be understood and -> carefully weighed before choosing a different course. - -Ideally, each of these tradeoffs should be documented -in the separate issues, labeled accordingly and linked -to original issue and epic. - -## Impact Analysis - -**Summary:** think about the impact your merge request may have on performance -and those maintaining a GitLab setup. - -Any change submitted can have an impact not only on the application itself but -also those maintaining it and those keeping it up and running (for example, production -engineers). As a result you should think carefully about the impact of your -merge request on not only the application but also on the people keeping it up -and running. - -Can the queries used potentially take down any critical services and result in -engineers being woken up in the night? Can a malicious user abuse the code to -take down a GitLab instance? Do my changes make loading a certain page -slower? Does execution time grow exponentially given enough load or data in the -database? - -These are all questions one should ask themselves before submitting a merge -request. It may sometimes be difficult to assess the impact, in which case you -should ask a performance specialist to review your code. See the "Reviewing" -section below for more information. - -## Performance Review - -**Summary:** ask performance specialists to review your code if you're not sure -about the impact. - -Sometimes it's hard to assess the impact of a merge request. In this case you -should ask one of the merge request reviewers to review your changes. You can -find a list of these reviewers at <https://about.gitlab.com/company/team/>. A reviewer -in turn can request a performance specialist to review the changes. - -## Think outside of the box - -Everyone has their own perception of how to use the new feature. -Always consider how users might be using the feature instead. Usually, -users test our features in a very unconventional way, -like by brute forcing or abusing edge conditions that we have. - -## Data set - -The data set the merge request processes should be known -and documented. The feature should clearly document what the expected -data set is for this feature to process, and what problems it might cause. - -If you would think about the following example that puts -a strong emphasis of data set being processed. -The problem is simple: you want to filter a list of files from -some Git repository. Your feature requests a list of all files -from the repository and perform search for the set of files. -As an author you should in context of that problem consider -the following: - -1. What repositories are planned to be supported? -1. How long it do big repositories like Linux kernel take? -1. Is there something that we can do differently to not process such a - big data set? -1. Should we build some fail-safe mechanism to contain - computational complexity? Usually it's better to degrade - the service for a single user instead of all users. - -## Query plans and database structure - -The query plan can tell us if we need additional -indexes, or expensive filtering (such as using sequential scans). - -Each query plan should be run against substantial size of data set. -For example, if you look for issues with specific conditions, -you should consider validating a query against -a small number (a few hundred) and a big number (100_000) of issues. -See how the query behaves if the result is a few -and a few thousand. - -This is needed as we have users using GitLab for very big projects and -in a very unconventional way. Even if it seems that it's unlikely -that such a big data set is used, it's still plausible that one -of our customers could encounter a problem with the feature. - -Understanding ahead of time how it behaves at scale, even if we accept it, -is the desired outcome. We should always have a plan or understanding of what is needed -to optimize the feature for higher usage patterns. - -Every database structure should be optimized and sometimes even over-described -in preparation for easy extension. The hardest part after some point is -data migration. Migrating millions of rows is always troublesome and -can have a negative impact on the application. - -To better understand how to get help with the query plan reviews -read this section on [how to prepare the merge request for a database review](database_review.md#how-to-prepare-the-merge-request-for-a-database-review). - -## Query Counts - -**Summary:** a merge request **should not** increase the total number of executed SQL -queries unless absolutely necessary. - -The total number of queries executed by the code modified or added by a merge request -must not increase unless absolutely necessary. When building features it's -entirely possible you need some extra queries, but you should try to keep -this at a minimum. - -As an example, say you introduce a feature that updates a number of database -rows with the same value. It may be very tempting (and easy) to write this using -the following pseudo code: - -```ruby -objects_to_update.each do |object| - object.some_field = some_value - object.save -end -``` - -This means running one query for every object to update. This code can -easily overload a database given enough rows to update or many instances of this -code running in parallel. This particular problem is known as the -["N+1 query problem"](https://guides.rubyonrails.org/active_record_querying.html#eager-loading-associations). You can write a test with [QueryRecorder](database/query_recorder.md) to detect this and prevent regressions. - -In this particular case the workaround is fairly easy: - -```ruby -objects_to_update.update_all(some_field: some_value) -``` - -This uses ActiveRecord's `update_all` method to update all rows in a single -query. This in turn makes it much harder for this code to overload a database. - -## Use read replicas when possible - -In a DB cluster we have many read replicas and one primary. A classic use of scaling the DB is to have read-only actions be performed by the replicas. We use [load balancing](../administration/postgresql/database_load_balancing.md) to distribute this load. This allows for the replicas to grow as the pressure on the DB grows. - -By default, queries use read-only replicas, but due to -[primary sticking](../administration/postgresql/database_load_balancing.md#primary-sticking), GitLab uses the -primary for some time and reverts to secondaries after they have either caught up or after 30 seconds. -Doing this can lead to a considerable amount of unnecessary load on the primary. -To prevent switching to the primary [merge request 56849](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/56849) introduced the -`without_sticky_writes` block. Typically, this method can be applied to prevent primary stickiness -after a trivial or insignificant write which doesn't affect the following queries in the same session. - -To learn when a usage timestamp update can lead the session to stick to the primary and how to -prevent it by using `without_sticky_writes`, see [merge request 57328](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/57328) - -As a counterpart of the `without_sticky_writes` utility, -[merge request 59167](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/59167) introduced -`use_replicas_for_read_queries`. This method forces all read-only queries inside its block to read -replicas regardless of the current primary stickiness. -This utility is reserved for cases where queries can tolerate replication lag. - -Internally, our database load balancer classifies the queries based on their main statement (`select`, `update`, `delete`, and so on). When in doubt, it redirects the queries to the primary database. Hence, there are some common cases the load balancer sends the queries to the primary unnecessarily: - -- Custom queries (via `exec_query`, `execute_statement`, `execute`, and so on) -- Read-only transactions -- In-flight connection configuration set -- Sidekiq background jobs - -After the above queries are executed, GitLab -[sticks to the primary](../administration/postgresql/database_load_balancing.md#primary-sticking). -To make the inside queries prefer using the replicas, -[merge request 59086](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/59086) introduced -`fallback_to_replicas_for_ambiguous_queries`. This MR is also an example of how we redirected a -costly, time-consuming query to the replicas. - -## Use CTEs wisely - -Read about [complex queries on the relation object](database/iterating_tables_in_batches.md#complex-queries-on-the-relation-object) for considerations on how to use CTEs. We have found in some situations that CTEs can become problematic in use (similar to the n+1 problem above). In particular, hierarchical recursive CTE queries such as the CTE in [AuthorizedProjectsWorker](https://gitlab.com/gitlab-org/gitlab/-/issues/325688) are very difficult to optimize and don't scale. We should avoid them when implementing new features that require any kind of hierarchical structure. - -CTEs have been effectively used as an optimization fence in many simpler cases, -such as this [example](https://gitlab.com/gitlab-org/gitlab-foss/-/issues/43242#note_61416277). -Beginning in PostgreSQL 12, CTEs are inlined then [optimized by default](https://paquier.xyz/postgresql-2/postgres-12-with-materialize/). -Keeping the old behavior requires marking CTEs with the keyword `MATERIALIZED`. - -When building CTE statements, use the `Gitlab::SQL::CTE` class [introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/56976) in GitLab 13.11. -By default, this `Gitlab::SQL::CTE` class forces materialization through adding the `MATERIALIZED` keyword for PostgreSQL 12 and higher. -`Gitlab::SQL::CTE` automatically omits materialization when PostgreSQL 11 is running -(this behavior is implemented using a custom Arel node `Gitlab::Database::AsWithMaterialized` under the surface). - -WARNING: -Upgrading to GitLab 14.0 requires PostgreSQL 12 or higher. - -## Cached Queries - -**Summary:** a merge request **should not** execute duplicated cached queries. - -Rails provides an [SQL Query Cache](cached_queries.md#cached-queries-guidelines), -used to cache the results of database queries for the duration of the request. - -See [why cached queries are considered bad](cached_queries.md#why-cached-queries-are-considered-bad) and -[how to detect them](cached_queries.md#how-to-detect-cached-queries). - -The code introduced by a merge request, should not execute multiple duplicated cached queries. - -The total number of the queries (including cached ones) executed by the code modified or added by a merge request -should not increase unless absolutely necessary. -The number of executed queries (including cached queries) should not depend on -collection size. -You can write a test by passing the `skip_cached` variable to [QueryRecorder](database/query_recorder.md) to detect this and prevent regressions. - -As an example, say you have a CI pipeline. All pipeline builds belong to the same pipeline, -thus they also belong to the same project (`pipeline.project`): - -```ruby -pipeline_project = pipeline.project -# Project Load (0.6ms) SELECT "projects".* FROM "projects" WHERE "projects"."id" = $1 LIMIT $2 -build = pipeline.builds.first - -build.project == pipeline_project -# CACHE Project Load (0.0ms) SELECT "projects".* FROM "projects" WHERE "projects"."id" = $1 LIMIT $2 -# => true -``` - -When we call `build.project`, it doesn't hit the database, it uses the cached result, but it re-instantiates -the same pipeline project object. It turns out that associated objects do not point to the same in-memory object. - -If we try to serialize each build: - -```ruby -pipeline.builds.each do |build| - build.to_json(only: [:name], include: [project: { only: [:name]}]) -end -``` - -It re-instantiates project object for each build, instead of using the same in-memory object. - -In this particular case the workaround is fairly easy: - -```ruby -ActiveRecord::Associations::Preloader.new.preload(pipeline, [builds: :project]) - -pipeline.builds.each do |build| - build.to_json(only: [:name], include: [project: { only: [:name]}]) -end -``` - -`ActiveRecord::Associations::Preloader` uses the same in-memory object for the same project. -This avoids the cached SQL query and also avoids re-instantiation of the project object for each build. - -## Executing Queries in Loops - -**Summary:** SQL queries **must not** be executed in a loop unless absolutely -necessary. - -Executing SQL queries in a loop can result in many queries being executed -depending on the number of iterations in a loop. This may work fine for a -development environment with little data, but in a production environment this -can quickly spiral out of control. - -There are some cases where this may be needed. If this is the case this should -be clearly mentioned in the merge request description. - -## Batch process - -**Summary:** Iterating a single process to external services (for example, PostgreSQL, Redis, Object Storage) -should be executed in a **batch-style** to reduce connection overheads. - -For fetching rows from various tables in a batch-style, please see [Eager Loading](#eager-loading) section. - -### Example: Delete multiple files from Object Storage - -When you delete multiple files from object storage, like GCS, -executing a single REST API call multiple times is a quite expensive -process. Ideally, this should be done in a batch-style, for example, S3 provides -[batch deletion API](https://docs.aws.amazon.com/AmazonS3/latest/API/API_DeleteObjects.html), -so it'd be a good idea to consider such an approach. - -The `FastDestroyAll` module might help this situation. It's a -small framework when you remove a bunch of database rows and its associated data -in a batch style. - -## Timeout - -**Summary:** You should set a reasonable timeout when the system invokes HTTP calls -to external services (such as Kubernetes), and it should be executed in Sidekiq, not -in Puma threads. - -Often, GitLab needs to communicate with an external service such as Kubernetes -clusters. In this case, it's hard to estimate when the external service finishes -the requested process, for example, if it's a user-owned cluster that's inactive for some reason, -GitLab might wait for the response forever ([Example](https://gitlab.com/gitlab-org/gitlab/-/issues/31475)). -This could result in Puma timeout and should be avoided at all cost. - -You should set a reasonable timeout, gracefully handle exceptions and surface the -errors in UI or logging internally. - -Using [`ReactiveCaching`](utilities.md#reactivecaching) is one of the best solutions to fetch external data. - -## Keep database transaction minimal - -**Summary:** You should avoid accessing to external services like Gitaly during database -transactions, otherwise it leads to severe contention problems -as an open transaction basically blocks the release of a PostgreSQL backend connection. - -For keeping transaction as minimal as possible, please consider using `AfterCommitQueue` -module or `after_commit` AR hook. - -Here is [an example](https://gitlab.com/gitlab-org/gitlab/-/issues/36154#note_247228859) -that one request to Gitaly instance during transaction triggered a ~"priority::1" issue. - -## Eager Loading - -**Summary:** always eager load associations when retrieving more than one row. - -When retrieving multiple database records for which you need to use any -associations you **must** eager load these associations. For example, if you're -retrieving a list of blog posts and you want to display their authors you -**must** eager load the author associations. - -In other words, instead of this: - -```ruby -Post.all.each do |post| - puts post.author.name -end -``` - -You should use this: - -```ruby -Post.all.includes(:author).each do |post| - puts post.author.name -end -``` - -Also consider using [QueryRecoder tests](database/query_recorder.md) to prevent a regression when eager loading. - -## Memory Usage - -**Summary:** merge requests **must not** increase memory usage unless absolutely -necessary. - -A merge request must not increase the memory usage of GitLab by more than the -absolute bare minimum required by the code. This means that if you have to parse -some large document (for example, an HTML document) it's best to parse it as a stream -whenever possible, instead of loading the entire input into memory. Sometimes -this isn't possible, in that case this should be stated explicitly in the merge -request. - -## Lazy Rendering of UI Elements - -**Summary:** only render UI elements when they are actually needed. - -Certain UI elements may not always be needed. For example, when hovering over a -diff line there's a small icon displayed that can be used to create a new -comment. Instead of always rendering these kind of elements they should only be -rendered when actually needed. This ensures we don't spend time generating -Haml/HTML when it's not used. - -## Use of Caching - -**Summary:** cache data in memory or in Redis when it's needed multiple times in -a transaction or has to be kept around for a certain time period. - -Sometimes certain bits of data have to be re-used in different places during a -transaction. In these cases this data should be cached in memory to remove the -need for running complex operations to fetch the data. You should use Redis if -data should be cached for a certain time period instead of the duration of the -transaction. - -For example, say you process multiple snippets of text containing username -mentions (for example, `Hello @alice` and `How are you doing @alice?`). By caching the -user objects for every username we can remove the need for running the same -query for every mention of `@alice`. - -Caching data per transaction can be done using -[RequestStore](https://github.com/steveklabnik/request_store) (use -`Gitlab::SafeRequestStore` to avoid having to remember to check -`RequestStore.active?`). Caching data in Redis can be done using -[Rails' caching system](https://guides.rubyonrails.org/caching_with_rails.html). - -## Pagination - -Each feature that renders a list of items as a table needs to include pagination. - -The main styles of pagination are: - -1. Offset-based pagination: user goes to a specific page, like 1. User sees the next page number, - and the total number of pages. This style is well supported by all components of GitLab. -1. Offset-based pagination, but without the count: user goes to a specific page, like 1. - User sees only the next page number, but does not see the total amount of pages. -1. Next page using keyset-based pagination: user can only go to next page, as we don't know how many pages - are available. -1. Infinite scrolling pagination: user scrolls the page and next items are loaded asynchronously. This is ideal, - as it has exact same benefits as the previous one. - -The ultimately scalable solution for pagination is to use Keyset-based pagination. -However, we don't have support for that at GitLab at that moment. You -can follow the progress looking at [API: Keyset Pagination](https://gitlab.com/groups/gitlab-org/-/epics/2039). - -Take into consideration the following when choosing a pagination strategy: - -1. It's very inefficient to calculate amount of objects that pass the filtering, - this operation usually can take seconds, and can time out, -1. It's very inefficient to get entries for page at higher ordinals, like 1000. - The database has to sort and iterate all previous items, and this operation usually - can result in substantial load put on database. - -You can find useful tips related to pagination in the [pagination guidelines](database/pagination_guidelines.md). - -## Badge counters - -Counters should always be truncated. It means that we don't want to present -the exact number over some threshold. The reason for that is for the cases where we want -to calculate exact number of items, we effectively need to filter each of them for -the purpose of knowing the exact number of items matching. - -From ~UX perspective it's often acceptable to see that you have over 1000+ pipelines, -instead of that you have 40000+ pipelines, but at a tradeoff of loading page for 2s longer. - -An example of this pattern is the list of pipelines and jobs. We truncate numbers to `1000+`, -but we show an accurate number of running pipelines, which is the most interesting information. - -There's a helper method that can be used for that purpose - `NumbersHelper.limited_counter_with_delimiter` - -that accepts an upper limit of counting rows. - -In some cases it's desired that badge counters are loaded asynchronously. -This can speed up the initial page load and give a better user experience overall. - -## Usage of feature flags - -Each feature that has performance critical elements or has a known performance deficiency -needs to come with feature flag to disable it. - -The feature flag makes our team more happy, because they can monitor the system and -quickly react without our users noticing the problem. - -Performance deficiencies should be addressed right away after we merge initial -changes. - -Read more about when and how feature flags should be used in -[Feature flags in GitLab development](https://about.gitlab.com/handbook/product-development-flow/feature-flag-lifecycle/#feature-flags-in-gitlab-development). - -## Storage - -We can consider the following types of storages: - -- **Local temporary storage** (very-very short-term storage) This type of storage is system-provided storage, like a `/tmp` folder. - This is the type of storage that you should ideally use for all your temporary tasks. - The fact that each node has its own temporary storage makes scaling significantly easier. - This storage is also very often SSD-based, thus is significantly faster. - The local storage can easily be configured for the application with - the usage of `TMPDIR` variable. - -- **Shared temporary storage** (short-term storage) This type of storage is network-based temporary storage, - usually run with a common NFS server. As of Feb 2020, we still use this type of storage - for most of our implementations. Even though this allows the above limit to be significantly larger, - it does not really mean that you can use more. The shared temporary storage is shared by - all nodes. Thus, the job that uses significant amount of that space or performs a lot - of operations creates a contention on execution of all other jobs and request - across the whole application, this can easily impact stability of the whole GitLab. - Be respectful of that. - -- **Shared persistent storage** (long-term storage) This type of storage uses - shared network-based storage (for example, NFS). This solution is mostly used by customers running small - installations consisting of a few nodes. The files on shared storage are easily accessible, - but any job that is uploading or downloading data can create a serious contention to all other jobs. - This is also an approach by default used by Omnibus. - -- **Object-based persistent storage** (long term storage) this type of storage uses external - services like [AWS S3](https://en.wikipedia.org/wiki/Amazon_S3). The Object Storage - can be treated as infinitely scalable and redundant. Accessing this storage usually requires - downloading the file to manipulate it. The Object Storage can be considered as an ultimate - solution, as by definition it can be assumed that it can handle unlimited concurrent uploads - and downloads of files. This is also ultimate solution required to ensure that application can - run in containerized deployments (Kubernetes) at ease. - -### Temporary storage - -The storage on production nodes is really sparse. The application should be built -in a way that accommodates running under very limited temporary storage. -You can expect the system on which your code runs has a total of `1G-10G` -of temporary storage. However, this storage is really shared across all -jobs being run. If your job requires to use more than `100MB` of that space -you should reconsider the approach you have taken. - -Whatever your needs are, you should clearly document if you need to process files. -If you require more than `100MB`, consider asking for help from a maintainer -to work with you to possibly discover a better solution. - -#### Local temporary storage - -The usage of local storage is a desired solution to use, -especially since we work on deploying applications to Kubernetes clusters. -When you would like to use `Dir.mktmpdir`? In a case when you want for example -to extract/create archives, perform extensive manipulation of existing data, and so on. - -```ruby -Dir.mktmpdir('designs') do |path| - # do manipulation on path - # the path will be removed once - # we go out of the block -end -``` - -#### Shared temporary storage - -The usage of shared temporary storage is required if your intent -is to persistent file for a disk-based storage, and not Object Storage. -[Workhorse direct upload](uploads/index.md#direct-upload) when accepting file -can write it to shared storage, and later GitLab Rails can perform a move operation. -The move operation on the same destination is instantaneous. -The system instead of performing `copy` operation just re-attaches file into a new place. - -Since this introduces extra complexity into application, you should only try -to re-use well established patterns (for example, `ObjectStorage` concern) instead of re-implementing it. - -The usage of shared temporary storage is otherwise deprecated for all other usages. - -### Persistent storage - -#### Object Storage - -It is required that all features holding persistent files support saving data -to Object Storage. Having a persistent storage in the form of shared volume across nodes -is not scalable, as it creates a contention on data access all nodes. - -GitLab offers the [ObjectStorage concern](https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/uploaders/object_storage.rb) -that implements a seamless support for Shared and Object Storage-based persistent storage. - -#### Data access - -Each feature that accepts data uploads or allows to download them needs to use -[Workhorse direct upload](uploads/index.md#direct-upload). It means that uploads needs to be -saved directly to Object Storage by Workhorse, and all downloads needs to be served -by Workhorse. - -Performing uploads/downloads via Puma is an expensive operation, -as it blocks the whole processing slot (thread) for the duration of the upload. - -Performing uploads/downloads via Puma also has a problem where the operation -can time out, which is especially problematic for slow clients. If clients take a long time -to upload/download the processing slot might be killed due to request processing -timeout (usually between 30s-60s). - -For the above reasons it is required that [Workhorse direct upload](uploads/index.md#direct-upload) is implemented -for all file uploads and downloads. +<!-- This redirect file can be deleted after <2023-04-10>. --> +<!-- Redirects that point to other docs in the same project expire in three months. --> +<!-- Redirects that point to docs in a different project or site (for example, link is not relative and starts with `https:`) expire in one year. --> +<!-- Before deletion, see: https://docs.gitlab.com/ee/development/documentation/redirects.html --> diff --git a/doc/development/migration_style_guide.md b/doc/development/migration_style_guide.md index 8f035d4aa13..4625489146e 100644 --- a/doc/development/migration_style_guide.md +++ b/doc/development/migration_style_guide.md @@ -253,21 +253,18 @@ Therefore, either: For example, if you create an empty table and need to build an index for it, it is recommended to use a regular single-transaction migration and the default -rails schema statement: [`add_index`](https://api.rubyonrails.org/v5.2/classes/ActiveRecord/ConnectionAdapters/SchemaStatements.html#method-i-add_index). +rails schema statement: [`add_index`](https://api.rubyonrails.org/classes/ActiveRecord/ConnectionAdapters/SchemaStatements.html#method-i-add_index). This is a blocking operation, but it doesn't cause problems because the table is not yet used, and therefore it does not have any records yet. ## Naming conventions -We keep column names consistent with [ActiveRecord's schema conventions](https://guides.rubyonrails.org/active_record_basics.html#schema-conventions). - -Custom index names should follow the pattern `index_#{table_name}_on_#{column_1}_and_#{column_2}_#{condition}`. +Names for database objects (such as tables, indexes, and views) must be lowercase. +Lowercase names ensure that queries with unquoted names don't cause errors. -Examples: +We keep column names consistent with [ActiveRecord's schema conventions](https://guides.rubyonrails.org/active_record_basics.html#schema-conventions). -- `index_services_on_type_and_id_and_template_when_active` -- `index_projects_on_id_service_desk_enabled` -- `index_clusters_on_enabled_cluster_type_id_and_created_at` +Custom index and constraint names should follow the [constraint naming convention guidelines](database/constraint_naming_convention.md). ### Truncate long index names @@ -430,6 +427,9 @@ end #### Changing default value for a column +Note that changing column defaults can cause application downtime if a multi-release process is not followed. +See [avoiding downtime in migrations for changing column defaults](database/avoiding_downtime_in_migrations.md#changing-column-defaults) for details. + ```ruby enable_lock_retries! diff --git a/doc/development/pages/index.md b/doc/development/pages/index.md index 5e56264330a..05eeb1965d1 100644 --- a/doc/development/pages/index.md +++ b/doc/development/pages/index.md @@ -147,7 +147,7 @@ GitLab Pages access control is disabled by default. To enable it: 1. Restart GitLab (if running through the GDK, run `gdk restart`). Running `gdk reconfigure` overwrites the value of `access_control` in `config/gitlab.yml`. 1. In your local GitLab instance, in the browser go to `http://gdk.test:3000/admin/applications`. -1. Create an [Instance-wide OAuth application](../../integration/oauth_provider.md#instance-wide-applications) +1. Create an [Instance-wide OAuth application](../../integration/oauth_provider.md#create-an-instance-wide-application) with the `api` scope. 1. Set the value of your `redirect-uri` to the `pages-domain` authorization endpoint (for example, `http://pages.gdk.test:3010/auth`). diff --git a/doc/development/performance.md b/doc/development/performance.md index 127cade9fee..21f80364358 100644 --- a/doc/development/performance.md +++ b/doc/development/performance.md @@ -14,7 +14,7 @@ consistent performance of GitLab. Refer to the [Index](#performance-documentatio - General: - [Solving performance issues](#workflow) - [Handbook performance page](https://about.gitlab.com/handbook/engineering/performance/) - - [Merge request performance guidelines](../development/merge_request_performance_guidelines.md) + - [Merge request performance guidelines](merge_request_concepts/performance.md) - Backend: - [Tooling](#tooling) - Database: @@ -51,7 +51,7 @@ The process of solving performance problems is roughly as follows: 1. Add your findings based on the measurement period (screenshots of graphs, timings, etc) to the issue mentioned in step 1. 1. Solve the problem. -1. Create a merge request, assign the "Performance" label and follow the [performance review process](merge_request_performance_guidelines.md). +1. Create a merge request, assign the "Performance" label and follow the [performance review process](merge_request_concepts/performance.md). 1. Once a change has been deployed make sure to _again_ measure for at least 24 hours to see if your changes have any impact on the production environment. 1. Repeat until you're done. diff --git a/doc/development/permissions.md b/doc/development/permissions.md index cc7c1229e50..bf7f99de1ab 100644 --- a/doc/development/permissions.md +++ b/doc/development/permissions.md @@ -33,7 +33,7 @@ See the [permissions page](../user/permissions.md) for details on how each user Groups and projects can have the following visibility levels: - public (`20`) - an entity is visible to everyone -- internal (`10`) - an entity is visible to logged in users +- internal (`10`) - an entity is visible to authenticated users - private (`0`) - an entity is visible only to the approved members of the entity By default, subgroups can **not** have higher visibility levels. diff --git a/doc/development/pipelines/index.md b/doc/development/pipelines/index.md index a7b8c99bd13..1797e082aea 100644 --- a/doc/development/pipelines/index.md +++ b/doc/development/pipelines/index.md @@ -16,16 +16,16 @@ We're striving to [dogfood](https://about.gitlab.com/handbook/engineering/develo GitLab [CI/CD features and best-practices](../../ci/yaml/index.md) as much as possible. -## Minimal test jobs before a merge request is approved +## Predictive test jobs before a merge request is approved -**To reduce the pipeline cost and shorten the job duration, before a merge request is approved, the pipeline will run a minimal set of RSpec & Jest tests that are related to the merge request changes.** +**To reduce the pipeline cost and shorten the job duration, before a merge request is approved, the pipeline will run a predictive set of RSpec & Jest tests that are likely to fail for the merge request changes.** After a merge request has been approved, the pipeline would contain the full RSpec & Jest tests. This will ensure that all tests have been run before a merge request is merged. ### Overview of the GitLab project test dependency -To understand how the minimal test jobs are executed, we need to understand the dependency between +To understand how the predictive test jobs are executed, we need to understand the dependency between GitLab code (frontend and backend) and the respective tests (Jest and RSpec). This dependency can be visualized in the following diagram: @@ -47,11 +47,11 @@ In summary: - RSpec tests are dependent on the backend code. - Jest tests are dependent on both frontend and backend code, the latter through the frontend fixtures. -### RSpec minimal jobs +### RSpec predictive jobs -#### Determining related RSpec test files in a merge request +#### Determining predictive RSpec test files in a merge request -To identify the minimal set of tests needed, we use the [`test_file_finder` gem](https://gitlab.com/gitlab-org/ci-cd/test_file_finder), with two strategies: +To identify the RSpec tests that are likely to fail in a merge request, we use the [`test_file_finder` gem](https://gitlab.com/gitlab-org/ci-cd/test_file_finder), with two strategies: - dynamic mapping from test coverage tracing (generated via the [`Crystalball` gem](https://github.com/toptal/crystalball)) ([see where it's used](https://gitlab.com/gitlab-org/gitlab/-/blob/47d507c93779675d73a05002e2ec9c3c467cd698/tooling/bin/find_tests#L15)) @@ -60,9 +60,9 @@ To identify the minimal set of tests needed, we use the [`test_file_finder` gem] The test mappings contain a map of each source files to a list of test files which is dependent of the source file. -In the `detect-tests` job, we use this mapping to identify the minimal tests needed for the current merge request. +In the `detect-tests` job, we use this mapping to identify the predictive tests needed for the current merge request. -Later on in [the `rspec fail-fast` job](#fail-fast-job-in-merge-request-pipelines), we run the minimal tests needed for the current merge request. +Later on in [the `rspec fail-fast` job](#fail-fast-job-in-merge-request-pipelines), we run the predictive tests for the current merge request. #### Exceptional cases @@ -74,11 +74,11 @@ In addition, there are a few circumstances where we would always run the full RS - when the merge request is created in a security mirror - when any CI configuration file is changed (for example, `.gitlab-ci.yml` or `.gitlab/ci/**/*`) -### Jest minimal jobs +### Jest predictive jobs -#### Determining related Jest test files in a merge request +#### Determining predictive Jest test files in a merge request -To identify the minimal set of tests needed, we pass a list of all the changed files into `jest` using the [`--findRelatedTests`](https://jestjs.io/docs/cli#--findrelatedtests-spaceseparatedlistofsourcefiles) option. +To identify the jest tests that are likely to fail in a merge request, we pass a list of all the changed files into `jest` using the [`--findRelatedTests`](https://jestjs.io/docs/cli#--findrelatedtests-spaceseparatedlistofsourcefiles) option. In this mode, `jest` would resolve all the dependencies of related to the changed files, which include test files that have these files in the dependency chain. #### Exceptional cases @@ -97,7 +97,7 @@ The `rules` definitions for full Jest tests are defined at `.frontend:rules:jest ### Fork pipelines -We run only the minimal RSpec & Jest jobs for fork pipelines, unless the `pipeline:run-all-rspec` +We run only the predictive RSpec & Jest jobs for fork pipelines, unless the `pipeline:run-all-rspec` label is set on the MR. The goal is to reduce the CI/CD minutes consumed by fork pipelines. See the [experiment issue](https://gitlab.com/gitlab-org/quality/team-tasks/-/issues/1170). @@ -176,10 +176,20 @@ graph LR A --"artifact: list of test files"--> B & C ``` -## Faster feedback for merge requests that fix a broken `master` +## Faster feedback for some merge requests + +### Broken Master Fixes When you need to [fix a broken `master`](https://about.gitlab.com/handbook/engineering/workflow/#resolution-of-broken-master), you can add the `pipeline:expedite` label to expedite the pipelines that run on the merge request. +Note that the merge request also needs to have the `master:broken` or `master:foss-broken` label set. + +### Revert MRs + +To make your Revert MRs faster, use the [revert MR template](https://gitlab.com/gitlab-org/gitlab/-/blob/master/.gitlab/merge_request_templates/Revert%20To%20Resolve%20Incident.md) **before** you create your merge request. It will apply the `pipeline:expedite` label and others that will expedite the pipelines that run on the merge request. + +### The `~pipeline:expedite` label + When this label is assigned, the following steps of the CI/CD pipeline are skipped: - The `e2e:package-and-test` job. @@ -188,8 +198,6 @@ When this label is assigned, the following steps of the CI/CD pipeline are skipp Apply the label to the merge request, and run a new pipeline for the MR. -Note that the merge request also needs to have the `master:broken` or `master:foss-broken` label set. - ## Test jobs We have dedicated jobs for each [testing level](../testing_guide/testing_levels.md) and each job runs depending on the @@ -424,17 +432,21 @@ running every day, updating cache. The default CI/CD configuration file is also set at `jh/.gitlab-ci.yml` so it runs exactly like [GitLab JH](https://jihulab.com/gitlab-cn/gitlab/-/blob/main-jh/jh/.gitlab-ci.yml). -## Ruby 3.0 jobs +## Ruby 2.7 jobs + +We're running Ruby 3.0 for the merge requests and the default branch. However, +we're still running Ruby 2.7 for GitLab.com and there are older versions that +we need to maintain. We need a way to still try out Ruby 2.7 in merge requests. -You can add the `pipeline:run-in-ruby3` label to the merge request to switch -the Ruby version used for running the whole test suite to 3.0. When you do -this, the test suite will no longer run in Ruby 2.7 (default), and an -additional job `verify-ruby-2.7` will also run and always fail to remind us to -remove the label and run in Ruby 2.7 before merging the merge request. +You can add the `pipeline:run-in-ruby2` label to the merge request to switch +the Ruby version used for running the whole test suite to 2.7. When you do +this, the test suite will no longer run in Ruby 3.0 (default), and an +additional job `verify-ruby-3.0` will also run and always fail to remind us to +remove the label and run in Ruby 3.0 before merging the merge request. This should let us: -- Test changes for Ruby 3.0 +- Test changes for Ruby 2.7 - Make sure it will not break anything when it's merged into the default branch ## `undercover` RSpec test @@ -465,9 +477,9 @@ If these commands return `undercover: ✅ No coverage is missing in latest chang ## Ruby versions testing -Our test suite runs against Ruby 2 in merge requests and default branch pipelines. +Our test suite runs against Ruby 3 in merge requests and default branch pipelines. -We also run our test suite against Ruby 3 on another 2-hourly scheduled pipelines, as GitLab.com will soon run on Ruby 3. +We also run our test suite against Ruby 2.7 on another 2-hourly scheduled pipelines, as GitLab.com still runs on Ruby 2.7. ## PostgreSQL versions testing @@ -482,26 +494,26 @@ We also run our test suite against PG11 upon specific database library changes i | Where? | PostgreSQL version | Ruby version | |------------------------------------------------------------------------------------------------|-------------------------------------------------|--------------| -| Merge requests | 12 (default version), 11 for DB library changes | 2.7 (default version) | -| `master` branch commits | 12 (default version), 11 for DB library changes | 2.7 (default version) | -| `maintenance` scheduled pipelines for the `master` branch (every even-numbered hour) | 12 (default version), 11 for DB library changes | 2.7 (default version) | -| `maintenance` scheduled pipelines for the `ruby3` branch (every odd-numbered hour), see below. | 12 (default version), 11 for DB library changes | 3.0 (coded in the branch) | -| `nightly` scheduled pipelines for the `master` branch | 12 (default version), 11, 13 | 2.7 (default version) | - -There are 2 pipeline schedules used for testing Ruby 3. One is triggering a -pipeline in `ruby3-sync` branch, which updates the `ruby3` branch with latest +| Merge requests | 12 (default version), 11 for DB library changes | 3.0 (default version) | +| `master` branch commits | 12 (default version), 11 for DB library changes | 3.0 (default version) | +| `maintenance` scheduled pipelines for the `master` branch (every even-numbered hour) | 12 (default version), 11 for DB library changes | 3.0 (default version) | +| `maintenance` scheduled pipelines for the `ruby2` branch (every odd-numbered hour), see below. | 12 (default version), 11 for DB library changes | 2.7 | +| `nightly` scheduled pipelines for the `master` branch | 12 (default version), 11, 13 | 3.0 (default version) | + +There are 2 pipeline schedules used for testing Ruby 2.7. One is triggering a +pipeline in `ruby2-sync` branch, which updates the `ruby2` branch with latest `master`, and no pipelines will be triggered by this push. The other schedule -is triggering a pipeline in `ruby3` 5 minutes after it, which is considered +is triggering a pipeline in `ruby2` 5 minutes after it, which is considered the maintenance schedule to run test suites and update cache. -Any changes in `ruby3` are only for running the pipeline. It should -never be merged back to `master`. Any other Ruby 3 changes should go into -`master` directly, which should be compatible with Ruby 2.7. +Any changes in `ruby2` are only for running the pipeline. It should +never be merged back to `master`. Any other Ruby 2.7 changes should go into +`master` directly, which should be compatible with Ruby 3. -Previously, `ruby3-sync` was using a project token stored in `RUBY3_SYNC_TOKEN` -(now backed up in `RUBY3_SYNC_TOKEN_NOT_USED`), however due to various +Previously, `ruby2-sync` was using a project token stored in `RUBY2_SYNC_TOKEN` +(now backed up in `RUBY2_SYNC_TOKEN_NOT_USED`), however due to various permissions issues, we ended up using an access token from `gitlab-bot` so now -`RUBY3_SYNC_TOKEN` is actually an access token from `gitlab-bot`. +`RUBY2_SYNC_TOKEN` is actually an access token from `gitlab-bot`. ### Long-term plan diff --git a/doc/development/pipelines/internals.md b/doc/development/pipelines/internals.md index 71492ed9f5d..9ff4e5a35ec 100644 --- a/doc/development/pipelines/internals.md +++ b/doc/development/pipelines/internals.md @@ -214,3 +214,64 @@ and included in `rules` definitions via [YAML anchors](../../ci/yaml/yaml_optimi | `code-qa-patterns` | Combination of `code-patterns` and `qa-patterns`. | | `code-backstage-qa-patterns` | Combination of `code-patterns`, `backstage-patterns`, and `qa-patterns`. | | `static-analysis-patterns` | Only create jobs for Static Analytics configuration-related changes. | + +## Best Practices + +### When to use `extends:`, `<<: *xyz` (YAML anchors), or `!reference` + +[Reference](../../ci/yaml/yaml_optimization.md) + +#### Key takeaways + +- If you need to **extend a hash**, you should use `extends` +- If you need to **extend an array**, you'll need to use `!reference`, or `YAML anchors` as last resort +- For more complex cases (e.g. extend hash inside array, extend array inside hash, ...), you'll have to use `!reference` or `YAML anchors` + +#### What can `extends` and `YAML anchors` do? + +##### `extends` + +- Deep merge for hashes +- NO merge for arrays. It overwrites ([source](../../ci/yaml/yaml_optimization.md#merge-details)) + +##### YAML anchors + +- NO deep merge for hashes, BUT it can be used to extend a hash (see the example below) +- NO merge for arrays, BUT it can be used to extend an array (see the example below) + +#### A great example + +This example shows how to extend complex YAML data structures with `!reference` and `YAML anchors`: + +```yaml +.strict-ee-only-rules: + # `rules` is an array of hashes + rules: + - if: '$CI_PROJECT_NAME !~ /^gitlab(-ee)?$/ ' + when: never + +# `if-security-merge-request` is a hash +.if-security-merge-request: &if-security-merge-request + if: '$CI_PROJECT_NAMESPACE == "gitlab-org/security"' + +# `code-qa-patterns` is an array +.code-qa-patterns: &code-qa-patterns + - "{package.json,yarn.lock}" + - ".browserslistrc" + - "babel.config.js" + - "jest.config.{base,integration,unit}.js" + +.qa:rules:as-if-foss: + rules: + # We extend the `rules` array with an array of hashes directly + - !reference [".strict-ee-only-rules", rules] + # We extend a single array entry with a hash + - <<: *if-security-merge-request + # `changes` is an array, so we pass it an entire array + changes: *code-qa-patterns + +qa:selectors-as-if-foss: + # We include the rules from .qa:rules:as-if-foss in this job + extends: + - .qa:rules:as-if-foss +``` diff --git a/doc/development/project_templates.md b/doc/development/project_templates.md index 269724c0a7f..55a63d41425 100644 --- a/doc/development/project_templates.md +++ b/doc/development/project_templates.md @@ -1,6 +1,6 @@ --- stage: Manage -group: Workspace +group: Organization info: "To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments" --- @@ -9,7 +9,7 @@ info: "To determine the technical writer assigned to the Stage/Group associated ## Adding a new built-in project template This page provides instructions about how to contribute a -[built-in project template](../user/project/working_with_projects.md#create-a-project-from-a-built-in-template). +[built-in project template](../user/project/index.md#create-a-project-from-a-built-in-template). To contribute a built-in project template, you must complete the following tasks: diff --git a/doc/development/prometheus_metrics.md b/doc/development/prometheus_metrics.md index 456f2eb50aa..834a20239fc 100644 --- a/doc/development/prometheus_metrics.md +++ b/doc/development/prometheus_metrics.md @@ -4,7 +4,7 @@ group: Respond info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments --- -# Working with Prometheus Metrics **(FREE)** +# Working with Prometheus Metrics ## Adding to the library diff --git a/doc/development/rake_tasks.md b/doc/development/rake_tasks.md index 14fbe0e875b..caea2cecf57 100644 --- a/doc/development/rake_tasks.md +++ b/doc/development/rake_tasks.md @@ -75,6 +75,54 @@ bin/rake "gitlab:seed:group_seed[subgroup_depth, username]" Group are additionally seeded with epics if GitLab instance has epics feature available. +#### Seeding a runner fleet test environment + +Use the `gitlab:seed:runner_fleet` task to seed a full runner fleet, specifically groups with subgroups and projects that contain runners and pipelines: + +```shell +bin/rake "gitlab:seed:runner_fleet[username, registration_prefix, runner_count, job_count]" +``` + +By default, the Rake task uses the `root` username to create 40 runners and 400 jobs. + +```mermaid +graph TD + G1[Top level group 1] --> G11 + G2[Top level group 2] --> G21 + G11[Group 1.1] --> G111 + G11[Group 1.1] --> G112 + G111[Group 1.1.1] --> P1111 + G112[Group 1.1.2] --> P1121 + G21[Group 2.1] --> P211 + + P1111[Project 1.1.1.1<br><i>70% of jobs, sent to first 5 runners</i>] + P1121[Project 1.1.2.1<br><i>15% of jobs, sent to first 5 runners</i>] + P211[Project 2.1.1<br><i>15% of jobs, sent to first 5 runners</i>] + + IR1[Instance runner] + P1111R1[Shared runner] + P1111R[Project 1.1.1.1 runners<br>20% total runners] + P1121R[Project 1.1.2.1 runners<br>49% total runners] + G111R[Group 1.1.1 runners<br>30% total runners<br><i>remaining jobs</i>] + G21R[Group 2.1 runners<br>1% total runners] + + P1111 --> P1111R1 + P1111 --> G111R + P1111 --> IR1 + P1111 --> P1111R + P1121 --> P1111R1 + P1121 --> IR1 + P1121 --> P1121R + P211 --> P1111R1 + P211 --> G21R + P211 --> IR1 + + classDef groups fill:#09f6,color:#000000,stroke:#333,stroke-width:3px; + classDef projects fill:#f96a,color:#000000,stroke:#333,stroke-width:2px; + class G1,G2,G11,G111,G112,G21 groups + class P1111,P1121,P211 projects +``` + #### Seeding custom metrics for the monitoring dashboard A lot of different types of metrics are supported in the monitoring dashboard. @@ -437,3 +485,26 @@ bundle exec rake gems:error_tracking_open_api:generate # Commit the changes git commit -m 'Update ErrorTrackingOpenAPI from OpenAPI definition' vendor/gems/error_tracking_open_api ``` + +## Update banned SSH keys + +You can add [banned SSH keys](../security/ssh_keys_restrictions.md#block-banned-or-compromised-keys) +from any Git repository by using the `gitlab:security:update_banned_ssh_keys` Rake task: + +1. Find a public remote Git repository containing SSH public keys. + The public key files must have the `.pub` file extension. +1. Make sure that `/tmp/` directory has enough space to store the remote Git repository. +1. To add the SSH keys to your banned-key list, run this command, replacing + `GIT_URL` and `OUTPUT_FILE` with appropriate values: + + ```shell + # @param git_url - Remote Git URL. + # @param output_file - Update keys to an output file. Default is config/security/banned_ssh_keys.yml. + + bundle exec rake "gitlab:security:update_banned_ssh_keys[GIT_URL, OUTPUT_FILE]" + ``` + +This task clones the remote repository, recursively walks the file system looking for files +ending in `.pub`, parses those files as SSH public keys, and then adds the public key fingerprints +to `output_file`. The contents of `config/security/banned_ssh_keys.yml` is read by GitLab and kept +in memory. It is not recommended to increase the size of this file beyond 1 megabyte in size. diff --git a/doc/development/reusing_abstractions.md b/doc/development/reusing_abstractions.md index e3f523fc6a7..3bacea859ef 100644 --- a/doc/development/reusing_abstractions.md +++ b/doc/development/reusing_abstractions.md @@ -248,7 +248,7 @@ response = ServiceResponse.error( if response.success? head :ok -if response.reason == :job_not_retriable +elsif response.reason == :job_not_retriable head :unprocessable_entity else head :bad_request diff --git a/doc/development/sec/analyzer_development_guide.md b/doc/development/sec/analyzer_development_guide.md index 4fb32785b9f..6edb4d1c604 100644 --- a/doc/development/sec/analyzer_development_guide.md +++ b/doc/development/sec/analyzer_development_guide.md @@ -80,7 +80,7 @@ go build -o analyzer ### Execution criteria -[Enabling SAST](../../user/application_security/sast/index.md#configure-sast-manually) requires including a pre-defined [template](https://gitlab.com/gitlab-org/gitlab/-/blob/ee4d473eb9a39f2f84b719aa0ca13d2b8e11dc7e/lib/gitlab/ci/templates/Jobs/SAST.gitlab-ci.yml) to your GitLab CI/CD configuration. +[Enabling SAST](../../user/application_security/sast/index.md#configure-sast-in-your-cicd-yaml) requires including a pre-defined [template](https://gitlab.com/gitlab-org/gitlab/-/blob/ee4d473eb9a39f2f84b719aa0ca13d2b8e11dc7e/lib/gitlab/ci/templates/Jobs/SAST.gitlab-ci.yml) to your GitLab CI/CD configuration. The following independent criteria determine which analyzer needs to be run on a project: diff --git a/doc/development/sec/index.md b/doc/development/sec/index.md index 3f52020701f..4ed0eadd92f 100644 --- a/doc/development/sec/index.md +++ b/doc/development/sec/index.md @@ -5,7 +5,7 @@ info: To determine the technical writer assigned to the Stage/Group associated w type: index, concepts, howto --- -# Sec section development **(FREE)** +# Sec section development The Sec section is responsible for GitLab application security features, the "Sec" part of DevSecOps. Development guides that are specific to the Sec section are listed here. diff --git a/doc/development/secure_coding_guidelines.md b/doc/development/secure_coding_guidelines.md index bccdda9ca04..3791cd4861e 100644 --- a/doc/development/secure_coding_guidelines.md +++ b/doc/development/secure_coding_guidelines.md @@ -1269,7 +1269,7 @@ This sensitive data must be handled carefully to avoid leaks which could lead to - Never commit credentials to repositories. - The [Gitleaks Git hook](https://gitlab.com/gitlab-com/gl-security/security-research/gitleaks-endpoint-installer) is recommended for preventing credentials from being committed. - Never log credentials under any circumstance. Issue [#353857](https://gitlab.com/gitlab-org/gitlab/-/issues/353857) is an example of credential leaks through log file. -- When credentials are required in a CI/CD job, use [masked variables](../ci/variables/index.md#mask-a-cicd-variable) to help prevent accidental exposure in the job logs. Be aware that when [debug logging](../ci/variables/index.md#debug-logging) is enabled, all masked CI/CD variables are visible in job logs. Also consider using [protected variables](../ci/variables/index.md#protected-cicd-variables) when possible so that sensitive CI/CD variables are only available to pipelines running on protected branches or protected tags. +- When credentials are required in a CI/CD job, use [masked variables](../ci/variables/index.md#mask-a-cicd-variable) to help prevent accidental exposure in the job logs. Be aware that when [debug logging](../ci/variables/index.md#enable-debug-logging) is enabled, all masked CI/CD variables are visible in job logs. Also consider using [protected variables](../ci/variables/index.md#protect-a-cicd-variable) when possible so that sensitive CI/CD variables are only available to pipelines running on protected branches or protected tags. - Proper scanners must be enabled depending on what data those credentials are protecting. See the [Application Security Inventory Policy](https://about.gitlab.com/handbook/security/security-engineering-and-research/application-security/inventory.html#policies) and our [Data Classification Standards](https://about.gitlab.com/handbook/security/data-classification-standard.html#data-classification-standards). - To store and/or share credentials between teams, refer to [1Password for Teams](https://about.gitlab.com/handbook/security/#1password-for-teams) and follow [the 1Password Guidelines](https://about.gitlab.com/handbook/security/#1password-guidelines). - If you need to share a secret with a team member, use 1Password. Do not share a secret over email, Slack, or other service on the Internet. diff --git a/doc/development/service_ping/implement.md b/doc/development/service_ping/implement.md index 70da97502bb..1e5f4191375 100644 --- a/doc/development/service_ping/implement.md +++ b/doc/development/service_ping/implement.md @@ -804,7 +804,7 @@ and run a local container instance: 1. In the downstream pipeline, wait for the `gitlab-docker` job to finish. 1. Open the job logs and locate the full container name including the version. It takes the following form: `registry.gitlab.com/gitlab-org/build/omnibus-gitlab-mirror/gitlab-ee:<VERSION>`. 1. On your local machine, make sure you are signed in to the GitLab Docker registry. You can find the instructions for this in - [Authenticate to the GitLab Container Registry](../../user/packages/container_registry/index.md#authenticate-with-the-container-registry). + [Authenticate to the GitLab Container Registry](../../user/packages/container_registry/authenticate_with_container_registry.md). 1. Once signed in, download the new image by using `docker pull registry.gitlab.com/gitlab-org/build/omnibus-gitlab-mirror/gitlab-ee:<VERSION>` 1. For more information about working with and running Omnibus GitLab containers in Docker, refer to [GitLab Docker images](../../install/docker.md) documentation. @@ -855,6 +855,8 @@ you must fulfill the following requirements: 1. All events listed at `events` attribute must have the same `aggregation` attribute. 1. `time_frame` does not include `all` value, which is unavailable for Redis sourced aggregated metrics. +While it is possible to aggregate EE-only events together with events that occur in all GitLab editions, it's important to remember that doing so may produce high variance between data collected from EE and CE GitLab instances. + ### Database sourced aggregated metrics > [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/52784) in GitLab 13.9. diff --git a/doc/development/service_ping/index.md b/doc/development/service_ping/index.md index 37e0b753448..40bb03cb5b4 100644 --- a/doc/development/service_ping/index.md +++ b/doc/development/service_ping/index.md @@ -4,7 +4,7 @@ group: Product Intelligence info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments --- -# Service Ping Guide **(FREE SELF)** +# Service Ping Guide > - Introduced in GitLab Ultimate 11.2, more statistics. > - In GitLab 14.1, [renamed from Usage Ping to Service Ping](https://gitlab.com/groups/gitlab-org/-/epics/5990). In 14.0 and earlier, use the Usage Ping documentation for the Rails commands appropriate to your version. diff --git a/doc/development/service_ping/metrics_dictionary.md b/doc/development/service_ping/metrics_dictionary.md index 49f8a5ac465..28581f81f94 100644 --- a/doc/development/service_ping/metrics_dictionary.md +++ b/doc/development/service_ping/metrics_dictionary.md @@ -45,7 +45,7 @@ Each metric is defined in a separate YAML file consisting of a number of fields: | `data_category` | yes | `string`; [categories](#data-category) of the metric, may be set to `operational`, `optional`, `subscription`, `standard`. The default value is `optional`.| | `instrumentation_class` | yes | `string`; [the class that implements the metric](metrics_instrumentation.md). | | `distribution` | yes | `array`; may be set to one of `ce, ee` or `ee`. The [distribution](https://about.gitlab.com/handbook/marketing/strategic-marketing/tiers/#definitions) where the tracked feature is available. | -| `performance_indicator_type` | no | `array`; may be set to one of [`gmau`, `smau`, `paid_gmau`, or `umau`](https://about.gitlab.com/handbook/business-technology/data-team/data-catalog/xmau-analysis/). | +| `performance_indicator_type` | no | `array`; may be set to one of [`gmau`, `smau`, `paid_gmau`, `umau` or `customer_health_score`](https://about.gitlab.com/handbook/business-technology/data-team/data-catalog/xmau-analysis/). | | `tier` | yes | `array`; may contain one or a combination of `free`, `premium` or `ultimate`. The [tier]( https://about.gitlab.com/handbook/marketing/strategic-marketing/tiers/) where the tracked feature is available. This should be verbose and contain all tiers where a metric is available. | | `milestone` | yes | The milestone when the metric is introduced and when it's available to self-managed instances with the official GitLab release. | | `milestone_removed` | no | The milestone when the metric is removed. | @@ -126,7 +126,7 @@ A metric's time frame is calculated based on the `time_frame` field and the `dat For `redis_hll` metrics, the type of aggregation is also taken into consideration. In this context, the term "aggregation" refers to [chosen events data storage interval](implement.md#add-new-events), and is **NOT** related to the Aggregated Metrics feature. For more information about the aggregation type of each feature, see the [`common.yml` file](https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/gitlab/usage_data_counters/known_events/common.yml). Weeks run from Monday to Sunday. -| data_source | time_frame | aggregation | Description | +| data_source | time_frame | aggregation | Description | |------------------------|------------|----------------|-------------------------------------------------| | any | `none` | not applicable | A type of data that’s not tracked over time, such as settings and configuration information | | `database` | `all` | not applicable | The whole time the metric has been active (all-time interval) | diff --git a/doc/development/service_ping/metrics_instrumentation.md b/doc/development/service_ping/metrics_instrumentation.md index 5cc8a7811d7..80500ccc723 100644 --- a/doc/development/service_ping/metrics_instrumentation.md +++ b/doc/development/service_ping/metrics_instrumentation.md @@ -257,10 +257,10 @@ options: ## Aggregated metrics <div class="video-fallback"> - See the video from: <a href="https://www.youtube.com/embed/22LbYqHwtUQ">Product Intelligence Office Hours Oct 6th</a> for an aggregated metrics walk-through. + See the video from: <a href="https://www.youtube.com/watch?v=22LbYqHwtUQ">Product Intelligence Office Hours Oct 6th</a> for an aggregated metrics walk-through. </div> <figure class="video-container"> - <iframe src="https://www.youtube.com/embed/22LbYqHwtUQ" frameborder="0" allowfullscreen="true"> </iframe> + <iframe src="https://www.youtube-nocookie.com/embed/22LbYqHwtUQ" frameborder="0" allowfullscreen> </iframe> </figure> The aggregated metrics feature provides insight into the number of data attributes, for example `pseudonymized_user_ids`, that occurred in a collection of events. For example, you can aggregate the number of users who perform multiple actions such as creating a new issue and opening @@ -474,5 +474,5 @@ The following pairing session video gives you an example of an investigation in See the video from: <a href="https://www.youtube.com/watch?v=y_6m2POx2ug">Product Intelligence Office Hours Oct 27th</a> to learn more about the metrics troubleshooting process. </div> <figure class="video-container"> - <iframe src="https://www.youtube.com/embed/y_6m2POx2ug" frameborder="0" allowfullscreen="true"> </iframe> + <iframe src="https://www.youtube-nocookie.com/embed/y_6m2POx2ug" frameborder="0" allowfullscreen> </iframe> </figure> diff --git a/doc/development/sidekiq/index.md b/doc/development/sidekiq/index.md index c9d783377bd..f4f98641d39 100644 --- a/doc/development/sidekiq/index.md +++ b/doc/development/sidekiq/index.md @@ -83,6 +83,19 @@ Each retry for a worker is counted as a failure in our metrics. A worker which always fails 9 times and succeeds on the 10th would have a 90% error rate. +If you want to manually retry the worker without tracking the exception in Sentry, +use an exception class inherited from `Gitlab::SidekiqMiddleware::RetryError`. + +```ruby +ServiceUnavailable = Class.new(::Gitlab::SidekiqMiddleware::RetryError) + +def perform + ... + + raise ServiceUnavailable if external_service_unavailable? +end +``` + ## Sidekiq Queues Previously, each worker had its own queue, which was automatically set based on the diff --git a/doc/development/snowplow/implementation.md b/doc/development/snowplow/implementation.md index a80d6fe70ff..40b8b7b3da8 100644 --- a/doc/development/snowplow/implementation.md +++ b/doc/development/snowplow/implementation.md @@ -115,7 +115,7 @@ You can also use it on HAML templates: ``` If you use the GitLab helper method [`nav_link`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/helpers/tab_helper.rb#L76), you must wrap `html_options` under the `html_options` keyword argument. If you -use the `ActionView` helper method [`link_to`](https://api.rubyonrails.org/v5.2.3/classes/ActionView/Helpers/UrlHelper.html#method-i-link_to), you don't need to wrap `html_options`. +use the `ActionView` helper method [`link_to`](https://api.rubyonrails.org/classes/ActionView/Helpers/UrlHelper.html#method-i-link_to), you don't need to wrap `html_options`. ```ruby # Bad diff --git a/doc/development/snowplow/index.md b/doc/development/snowplow/index.md index 32628744a23..6acbd72175e 100644 --- a/doc/development/snowplow/index.md +++ b/doc/development/snowplow/index.md @@ -27,9 +27,10 @@ Tracking can be enabled at: - The user level. User tracking can be disabled on a per user basis. GitLab respects the [Do Not Track](https://www.eff.org/issues/do-not-track) standard, so any user who has enabled the Do Not Track option in their browser is not tracked at a user level. -Snowplow tracking is enabled on GitLab.com, and we use it for most of our tracking strategy. +Snowplow tracking is configured to send data for GitLab.com to a collector configured by GitLab. By default, self-managed +instances do not have a collector configured and do not collect data via Snowplow. -To enable Snowplow tracking on a self-managed instance: +You can configure your self-managed GitLab instance to use a custom Snowplow collector. 1. On the top bar, select **Main menu > Admin**, then select **Settings > General**. Alternatively, go to `admin/application_settings/general` in your browser. diff --git a/doc/development/software_design.md b/doc/development/software_design.md index 03cbbb13d9f..d4edbaa72be 100644 --- a/doc/development/software_design.md +++ b/doc/development/software_design.md @@ -1,6 +1,6 @@ --- stage: none -+group: Engineering Productivity +group: Engineering Productivity info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments --- diff --git a/doc/development/spam_protection_and_captcha/exploratory_testing.md b/doc/development/spam_protection_and_captcha/exploratory_testing.md index 1bcd336ce93..fe5c1c3db56 100644 --- a/doc/development/spam_protection_and_captcha/exploratory_testing.md +++ b/doc/development/spam_protection_and_captcha/exploratory_testing.md @@ -1,6 +1,6 @@ --- -stage: Manage -group: Authentication and Authorization +stage: Data Science +group: Anti-Abuse info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments --- @@ -153,8 +153,8 @@ only models with full Spam and CAPTCHA support. 1. Create an API token. 1. Export it in your terminal for the REST commands: `export PRIVATE_TOKEN=<your_api_token>` -1. Ensure you are logged into GitLab development environment at `localhost:3000` before using GraphiQL explorer, - because it uses your logged-in user as authorization for running GraphQL queries. +1. Ensure you are signed into the GitLab development environment at `localhost:3000` before using GraphiQL explorer, + because it uses your authenticated user as authorization for running GraphQL queries. 1. For the GraphQL examples, use the GraphiQL explorer at `http://localhost:3000/-/graphql-explorer`. 1. Use the `--include` (`-i`) option to `curl` to print the HTTP response headers, including the status code. diff --git a/doc/development/spam_protection_and_captcha/graphql_api.md b/doc/development/spam_protection_and_captcha/graphql_api.md index 846165d35f1..383b52df1fc 100644 --- a/doc/development/spam_protection_and_captcha/graphql_api.md +++ b/doc/development/spam_protection_and_captcha/graphql_api.md @@ -1,6 +1,6 @@ --- -stage: Manage -group: Authentication and Authorization +stage: Data Science +group: Anti-Abuse info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments --- diff --git a/doc/development/spam_protection_and_captcha/index.md b/doc/development/spam_protection_and_captcha/index.md index 1ba1c1678c4..254c3401f81 100644 --- a/doc/development/spam_protection_and_captcha/index.md +++ b/doc/development/spam_protection_and_captcha/index.md @@ -1,6 +1,6 @@ --- -stage: Manage -group: Authentication and Authorization +stage: Data Science +group: Anti-Abuse info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments --- diff --git a/doc/development/spam_protection_and_captcha/model_and_services.md b/doc/development/spam_protection_and_captcha/model_and_services.md index d0ca6d350dc..9c5d389a2f5 100644 --- a/doc/development/spam_protection_and_captcha/model_and_services.md +++ b/doc/development/spam_protection_and_captcha/model_and_services.md @@ -1,6 +1,6 @@ --- -stage: Manage -group: Authentication and Authorization +stage: Data Science +group: Anti-Abuse info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments --- diff --git a/doc/development/spam_protection_and_captcha/rest_api.md b/doc/development/spam_protection_and_captcha/rest_api.md index 7a9f8b5def1..7d749944163 100644 --- a/doc/development/spam_protection_and_captcha/rest_api.md +++ b/doc/development/spam_protection_and_captcha/rest_api.md @@ -1,6 +1,6 @@ --- -stage: Manage -group: Authentication and Authorization +stage: Data Science +group: Anti-Abuse info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments --- diff --git a/doc/development/spam_protection_and_captcha/web_ui.md b/doc/development/spam_protection_and_captcha/web_ui.md index ff7049dd29f..0ae5e98f399 100644 --- a/doc/development/spam_protection_and_captcha/web_ui.md +++ b/doc/development/spam_protection_and_captcha/web_ui.md @@ -1,6 +1,6 @@ --- -stage: Manage -group: Authentication and Authorization +stage: Data Science +group: Anti-Abuse info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments --- diff --git a/doc/development/testing_guide/best_practices.md b/doc/development/testing_guide/best_practices.md index aee3e2871c2..e27d4911158 100644 --- a/doc/development/testing_guide/best_practices.md +++ b/doc/development/testing_guide/best_practices.md @@ -155,6 +155,8 @@ To avoid creation, it is worth bearing in mind that: Use [Factory Doctor](https://test-prof.evilmartians.io/#/profilers/factory_doctor) to find cases where database persistence is not needed in a given test. +Examples of factories optimization [1](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/106796), [2](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/105329). + ```shell # run test for path FDOC=1 bin/rspec spec/[path]/[to]/[spec].rb @@ -831,7 +833,7 @@ To resolve, use `let`, or change the factory to not use stubs. ### Time-sensitive tests -[`ActiveSupport::Testing::TimeHelpers`](https://api.rubyonrails.org/v6.0.3.1/classes/ActiveSupport/Testing/TimeHelpers.html) +[`ActiveSupport::Testing::TimeHelpers`](https://api.rubyonrails.org/classes/ActiveSupport/Testing/TimeHelpers.html) can be used to verify things that are time-sensitive. Any test that exercises or verifies something time-sensitive should make use of these helpers to prevent transient test failures. @@ -852,7 +854,7 @@ end #### RSpec helpers You can use the `:freeze_time` and `:time_travel_to` RSpec metadata tag helpers to help reduce the amount of -boilerplate code needed to wrap entire specs with the [`ActiveSupport::Testing::TimeHelpers`](https://api.rubyonrails.org/v6.0.3.1/classes/ActiveSupport/Testing/TimeHelpers.html) +boilerplate code needed to wrap entire specs with the [`ActiveSupport::Testing::TimeHelpers`](https://api.rubyonrails.org/classes/ActiveSupport/Testing/TimeHelpers.html) methods. ```ruby @@ -872,7 +874,7 @@ end ``` [Under the hood](https://gitlab.com/gitlab-org/gitlab/-/blob/master/spec/support/time_travel.rb), these helpers use the `around(:each)` hook and the block syntax of the -[`ActiveSupport::Testing::TimeHelpers`](https://api.rubyonrails.org/v6.0.3.1/classes/ActiveSupport/Testing/TimeHelpers.html) +[`ActiveSupport::Testing::TimeHelpers`](https://api.rubyonrails.org/classes/ActiveSupport/Testing/TimeHelpers.html) methods: ```ruby diff --git a/doc/development/testing_guide/contract/consumer_tests.md b/doc/development/testing_guide/contract/consumer_tests.md index 9c72e6835bd..39cc34d6153 100644 --- a/doc/development/testing_guide/contract/consumer_tests.md +++ b/doc/development/testing_guide/contract/consumer_tests.md @@ -6,11 +6,11 @@ info: To determine the technical writer assigned to the Stage/Group associated w # Writing consumer tests -This tutorial guides you through writing a consumer test from scratch. To start, the consumer tests are written using [`jest-pact`](https://github.com/pact-foundation/jest-pact) that builds on top of [`pact-js`](https://github.com/pact-foundation/pact-js). This tutorial shows you how to write a consumer test for the `/discussions.json` REST API endpoint, which is actually `/:namespace_name/:project_name/-/merge_requests/:id/discussions.json`. For an example of a GraphQL consumer test, see [`spec/contracts/consumer/specs/project/pipeline/show.spec.js`](https://gitlab.com/gitlab-org/gitlab/-/tree/master/spec/contracts/consumer/specs/project/pipeline/show.spec.js). +This tutorial guides you through writing a consumer test from scratch. To start, the consumer tests are written using [`jest-pact`](https://github.com/pact-foundation/jest-pact) that builds on top of [`pact-js`](https://github.com/pact-foundation/pact-js). This tutorial shows you how to write a consumer test for the `/discussions.json` REST API endpoint, which is `/:namespace_name/:project_name/-/merge_requests/:id/discussions.json`, that is called in the `MergeRequests#show` page. For an example of a GraphQL consumer test, see [`spec/contracts/consumer/specs/project/pipelines/show.spec.js`](https://gitlab.com/gitlab-org/gitlab/-/tree/master/spec/contracts/consumer/specs/project/pipelines/show.spec.js). ## Create the skeleton -Start by creating the skeleton of a consumer test. Create a file under `spec/contracts/consumer/specs/project/merge_request` called `discussions.spec.js`. +Start by creating the skeleton of a consumer test. Since this is for a request in the `MergeRequests#show` page, under `spec/contracts/consumer/specs/project/merge_requests`, create a file called `show.spec.js`. Then, populate it with the following function and parameters: - [`pactWith`](#the-pactwith-function) @@ -38,10 +38,10 @@ import { pactWith } from 'jest-pact'; pactWith( { - consumer: 'MergeRequest#show', - provider: 'Merge Request Discussions Endpoint', + consumer: 'MergeRequests#show', + provider: 'GET discussions', log: '../logs/consumer.log', - dir: '../contracts/project/merge_request/show', + dir: '../contracts/project/merge_requests/show', }, PactFn ); @@ -58,14 +58,14 @@ import { pactWith } from 'jest-pact'; pactWith( { - consumer: 'MergeRequest#show', - provider: 'Merge Request Discussions Endpoint', + consumer: 'MergeRequests#show', + provider: 'GET discussions', log: '../logs/consumer.log', - dir: '../contracts', + dir: '../contracts/project/merge_requests/show', }, (provider) => { - describe('Merge Request Discussions Endpoint', () => { + describe('GET discussions', () => { beforeEach(() => { }); @@ -97,14 +97,14 @@ import { Matchers } from '@pact-foundation/pact'; pactWith( { - consumer: 'MergeRequest#show', - provider: 'Merge Request Discussions Endpoint', + consumer: 'MergeRequests#show', + provider: 'GET discussions', log: '../logs/consumer.log', - dir: '../contracts/project/merge_request/show', + dir: '../contracts/project/merge_requests/show', }, (provider) => { - describe('Merge Request Discussions Endpoint', () => { + describe('GET discussions', () => { beforeEach(() => { const interaction = { state: 'a merge request with discussions exists', @@ -175,14 +175,14 @@ import { getDiscussions } from '../../../resources/api/project/merge_requests'; pactWith( { - consumer: 'MergeRequest#show', - provider: 'Merge Request Discussions Endpoint', + consumer: 'MergeRequests#show', + provider: 'GET discussions', log: '../logs/consumer.log', - dir: '../contracts/project/merge_request/show', + dir: '../contracts/project/merge_requests/show', }, (provider) => { - describe('Merge Request Discussions Endpoint', () => { + describe('GET discussions', () => { beforeEach(() => { const interaction = { state: 'a merge request with discussions exists', @@ -232,7 +232,7 @@ There we have it! The consumer test is now set up. You can now try [running this As you may have noticed, the request and response definitions can get large. This results in the test being difficult to read, with a lot of scrolling to find what you want. You can make the test easier to read by extracting these out to a `fixture`. -Create a file under `spec/contracts/consumer/fixtures/project/merge_request` called `discussions.fixture.js` where you will place the `request` and `response` definitions. +Create a file under `spec/contracts/consumer/fixtures/project/merge_requests` called `discussions.fixture.js` where you will place the `request` and `response` definitions. ```javascript import { Matchers } from '@pact-foundation/pact'; @@ -279,13 +279,13 @@ With all of that moved to the `fixture`, you can simplify the test to the follow ```javascript import { pactWith } from 'jest-pact'; -import { Discussions } from '../../../fixtures/project/merge_request/discussions.fixture'; +import { Discussions } from '../../../fixtures/project/merge_requests/discussions.fixture'; import { getDiscussions } from '../../../resources/api/project/merge_requests'; -const CONSUMER_NAME = 'MergeRequest#show'; -const PROVIDER_NAME = 'Merge Request Discussions Endpoint'; +const CONSUMER_NAME = 'MergeRequests#show'; +const PROVIDER_NAME = 'GET discussions'; const CONSUMER_LOG = '../logs/consumer.log'; -const CONTRACT_DIR = '../contracts/project/merge_request/show'; +const CONTRACT_DIR = '../contracts/project/merge_requests/show'; pactWith( { diff --git a/doc/development/testing_guide/contract/index.md b/doc/development/testing_guide/contract/index.md index 08a21e58a52..31d68bb9f4f 100644 --- a/doc/development/testing_guide/contract/index.md +++ b/doc/development/testing_guide/contract/index.md @@ -24,7 +24,7 @@ The contracts themselves are stored in [`/spec/contracts/contracts`](https://git ### Run the consumer tests -Before running the consumer tests, go to `spec/contracts/consumer` and run `npm install`. To run all the consumer tests, you just need to run `npm test -- /specs`. Otherwise, to run a specific spec file, replace `/specs` with the specific spec filename. +Before running the consumer tests, go to `spec/contracts/consumer` and run `npm install`. To run all the consumer tests, you just need to run `npm run jest:contract -- /specs`. Otherwise, to run a specific spec file, replace `/specs` with the specific spec filename. Running the consumer test will create the contract that the provider test uses to verify the actual API behavior. You can also run tests from the root directory of the project, using the command `yarn jest:contract`. @@ -40,6 +40,14 @@ rake contracts:merge_requests:pact:verify:discussions rake contracts:merge_requests:test:merge_requests[contract_merge_requests] # Run all merge request contract tests ``` +#### Verify the contracts in Pact Broker + +By default, the Rake tasks will verify the locally stored contracts. In order to verify the contracts published in the Pact Broker, we need to set the `PACT_BROKER` environment variable to `true`. It is important to point out here that the file path and file name of the provider test is what is used to find the contract in the Pact Broker which is why it is important to make sure the [provider test naming conventions](#provider-naming) are followed. + +## Publish contracts to Pact Broker + +The contracts generated by the consumer test can be published to a hosted Pact Broker by going to `spec/contracts` and running the `publish-contracts.sh` script. + ## Test suite folder structure and naming conventions To keep the consumer and provider test suite organized and maintainable, it's important that tests are organized, also that consumers and providers are named consistently. Therefore, it's important to adhere to the following conventions. @@ -50,31 +58,33 @@ Having an organized and sensible folder structure for the test suite makes it ea #### Consumer tests -The consumer tests are grouped according to the different pages in the application. Each file contains various types of requests found in a page. As such, the consumer test files are named using the Rails standards of how pages are referenced. For example, the project pipelines page would be the `Project::Pipeline#index` page so the equivalent consumer test would be located in `consumer/specs/project/pipelines/index.spec.js`. +The consumer tests are grouped according to the different pages in the application. Each file contains various types of requests found in a page. As such, the consumer test files are named using the Rails standards of how pages are referenced. For example, the project pipelines page would be the `Project::Pipelines#index` page so the equivalent consumer test would be located in `consumer/specs/project/pipelines/index.spec.js`. When defining the location to output the contract generated by the test, we want to follow the same file structure which would be `contracts/project/pipelines/` for this example. This is the structure in `consumer/resources` and `consumer/fixtures` as well. +The naming of the folders must also be pluralized to match how they are called in the Rails naming standard. + #### Provider tests The provider tests are grouped similarly to our controllers. Each of these tests contains various tests for an API endpoint. For example, the API endpoint to get a list of pipelines for a project would be located in `provider/pact_helpers/project/pipelines/get_list_project_pipelines_helper.rb`. The provider states are grouped according to the different pages in the application similar to the consumer tests. ### Naming conventions -When writing the consumer and provider tests, there are parts where a name is required for the consumer and provider. Since there are no restrictions imposed by Pact on how these should be named, a naming convention is important to keep it easy for us to figure out which consumer and provider tests are involved during debugging. Pact also uses the consumer and provider names to generate the generated contracts in the `#{consumer_name}-#{provider_name}` format. +When writing the consumer and provider tests, there are parts where a name is required for the consumer and provider. Since there are no restrictions imposed by Pact on how these should be named, a naming convention is important to keep it easy for us to figure out which consumer and provider tests are involved during debugging. Pact also uses the consumer and provider names to create the locally stored contract file names in the `#{consumer_name}-#{provider_name}` format. #### Consumer naming -As mentioned in the [folder structure section](#consumer-tests), consumer tests are grouped according to the different pages in the application. As such, consumer names should follow the same naming format using the Rails standard. For example, the consumer test for `Project::Pipeline#index` would be `ProjectPipeline#index` as the consumer name. Since Pact uses this name to name the contracts it generates, the colons (`::`) are dropped as colons are not valid characters in file names. +As mentioned in the [folder structure section](#consumer-tests), consumer tests are grouped according to the different pages in the application. As such, consumer names should follow the same naming format using the Rails standard. For example, the consumer test for `Project::Pipelines#index` would be under the `project` folder and will be called `Pipelines#index` as the consumer name. #### Provider naming -These are the API endpoints that provides the data to the consumer so they are named according to the API endpoint they pertain to. Be mindful that this name is as descriptive as possible. For example, if we're writing a test for the `GET /groups/:id/projects` endpoint, we don't want to name it "Projects endpoint" as there is a `GET /projects` endpoint as well that also fetches a list of projects the user has access to across all of GitLab. An easy way to name them is by checking out our [API documentation](../../../api/api_resources.md) and naming it the same way it is named in there. So the [`GET /groups/:id/projects`](../../../api/groups.md#list-a-groups-projects) would be called `List a group’s projects` and [`GET /projects`](../../../api/projects.md#list-all-projects) would be called `List all projects`. Subsequently, the test files are named `list_a_groups_projects_helper.rb` and `list_all_projects_helper.rb` respectively. +These are the API endpoints that provides the data to the consumer so they are named according to the API endpoint they pertain to. Be mindful that this begins with the HTTP request method and the rest of the name is as descriptive as possible. For example, if we're writing a test for the `GET /groups/:id/projects` endpoint, we don't want to name it "GET projects endpoint" as there is a `GET /projects` endpoint as well that also fetches a list of projects the user has access to across all of GitLab. To choose an appropriate name, we can start by checking out our [API documentation](../../../api/api_resources.md) and naming it the same way it is named in there while making sure to keep the name in sentence case. So the [`GET /groups/:id/projects`](../../../api/groups.md#list-a-groups-projects) would be called `GET list a group's projects` and [`GET /projects`](../../../api/projects.md#list-all-projects) would be called `GET list all projects`. Subsequently, the test files are named `get_list_a_groups_projects_helper.rb` and `get_list_all_projects_helper.rb` respectively. -There are some cases where the provider being tested may not be documented so, in those cases, fall back to choosing a name that is as descriptive as possible to ensure it's easy to tell what the provider is for. +There are some cases where the provider being tested may not be documented so, in those cases, fall back to starting with the HTTP request method followed by a name that is as descriptive as possible to ensure it's easy to tell what the provider is for. #### Conventions summary | Tests | Folder structure | Naming convention | | ----- | ---------------- | ----------------- | -| Consumer Test | Follows the Rails reference standards. For example, `Project::Pipeline#index` would be `consumer/specs/project/pipelines/index.spec.js` | Follows the Rails naming standard. For example, `Project::Pipeline#index` would be `ProjectPipeline#index` | -| Provider Test | Grouped like the Rails controllers. For example, [`List project pipelines` API endpoint](../../../api/pipelines.md#list-project-pipelines) would be `provider/pact_helpers/project/pipelines/provider/pact_helpers/project/pipelines/get_list_project_pipelines_helper.rb` | Follows the API documentation naming scheme. For example, [`GET /projects/:id/pipelines`](../../../api/pipelines.md#list-project-pipelines) would be called `List project pipelines`. | +| Consumer Test | Follows the Rails reference standards. For example, `Project::Pipelines#index` would be `consumer/specs/project/pipelines/index.spec.js` | Follows the Rails naming standard. For example, `Project::Pipelines#index` would be `Pipelines#index` within the `project` folder. | +| Provider Test | Grouped like the Rails controllers. For example, [`GET list project pipelines` API endpoint](../../../api/pipelines.md#list-project-pipelines) would be `provider/pact_helpers/project/pipelines/provider/pact_helpers/project/pipelines/get_list_project_pipelines_helper.rb` | Follows the API documentation naming scheme in sentence case. For example, [`GET /projects/:id/pipelines`](../../../api/pipelines.md#list-project-pipelines) would be called `GET list project pipelines`. | diff --git a/doc/development/testing_guide/contract/provider_tests.md b/doc/development/testing_guide/contract/provider_tests.md index 1772ed9384e..3ce9f91a307 100644 --- a/doc/development/testing_guide/contract/provider_tests.md +++ b/doc/development/testing_guide/contract/provider_tests.md @@ -10,7 +10,7 @@ This tutorial guides you through writing a provider test from scratch. It is a c ## Create the skeleton -Provider tests are quite simple. The goal is to set up the test data and then link that with the corresponding contract. Start by creating a file called `discussions_helper.rb` under `spec/contracts/provider/pact_helpers/project/merge_request`. Note that the files are called `helpers` to match how they are called by Pact in the Rake tasks, which are set up at the end of this tutorial. +Provider tests are quite simple. The goal is to set up the test data and then link that with the corresponding contract. Start by creating a file called `get_discussions_helper.rb` under `spec/contracts/provider/pact_helpers/project/merge_request`. Note that the files are called `helpers` to match how they are called by Pact in the Rake tasks, which are set up at the end of this tutorial. To learn more about how the contract test directory is structured, see the contract testing [test suite folder structure](index.md#test-suite-folder-structure). @@ -23,7 +23,7 @@ require_relative '../../../spec_helper' module Provider module DiscussionsHelper - Pact.service_provider 'Merge Request Discussions Endpoint' do + Pact.service_provider 'GET discussions' do end end @@ -39,8 +39,8 @@ require_relative '../../../spec_helper' module Provider module DiscussionsHelper - Pact.service_provider 'Merge Request Discussions Endpoint' do - honours_pact_with 'MergeRequest#show' do + Pact.service_provider 'GET discussions' do + honours_pact_with 'MergeRequests#show' do end end @@ -59,10 +59,10 @@ require_relative '../../../spec_helper' module Provider module DiscussionsHelper - Pact.service_provider 'Merge Request Discussions Endpoint' do + Pact.service_provider 'GET discussions' do app { Environment::Test.app } - honours_pact_with 'MergeRequest#show' do + honours_pact_with 'MergeRequests#show' do end end @@ -79,11 +79,11 @@ require_relative '../../../spec_helper' module Provider module DiscussionsHelper - Pact.service_provider 'Merge Request Discussions Endpoint' do + Pact.service_provider 'GET discussions' do app { Environment::Test.app } - honours_pact_with 'MergeRequest#show' do - pact_uri '../contracts/project/merge_request/show/mergerequest#show-merge_request_discussions_endpoint.json' + honours_pact_with 'MergeRequests#show' do + pact_uri '../contracts/project/merge_requests/show/mergerequests#show-merge_request_discussions_endpoint.json' end end end @@ -92,20 +92,25 @@ end ## Add / update the Rake tasks -Now that you have a test created, you must create Rake tasks that run this test. The Rake tasks are defined in [`lib/tasks/contracts.rake`](https://gitlab.com/gitlab-org/gitlab/-/tree/master/lib/tasks/contracts.rake) where we have individual Rake tasks to run individual specs, but also Rake tasks that run a group of tests. +Now that you have a test created, you must create Rake tasks that run this test. The Rake tasks are defined in [`lib/tasks/contracts/merge_requests.rake`](https://gitlab.com/gitlab-org/gitlab/-/tree/master/lib/tasks/contracts/merge_requests.rake) where we have individual Rake tasks to run individual tests, but also Rake tasks that run a group of tests. -Under the `contracts:mr` namespace, introduce the Rake task to run this new test specifically. In it, call `pact.uri` to define the location of the contract and the provider test that tests that contract. Notice here that `pact_uri` has a parameter called `pact_helper`. This is why the provider tests are called `_helper.rb`. +Under the `contracts:merge_requests` namespace, introduce the Rake task to run this new test specifically. In it, call `pact.uri` to define the location of the contract and the provider test that tests that contract. Notice here that `pact_uri` has a parameter called `pact_helper`. This is why the provider tests are called `_helper.rb`. ```ruby -Pact::VerificationTask.new(:discussions) do |pact| +Pact::VerificationTask.new(:get_discussions) do |pact| + provider = File.expand_path('../../../spec/contracts/provider', __dir__) + pact_helper_location = "pact_helpers/project/merge_requests/show/get_discussions_helper.rb" + pact.uri( - "#{contracts}/contracts/project/merge_request/show/merge_request#show-merge_request_discussions_endpoint.json", - pact_helper: "#{provider}/pact_helpers/project/merge_request/discussions_helper.rb" + Provider::ContractSourceHelper.contract_location(:rake, pact_helper_location), + pact_helper: "#{provider}/#{pact_helper_location}" ) end ``` -At the same time, add your new `:discussions` Rake task to be included in the `test:merge_request` Rake task. In that Rake task, there is an array defined (`%w[metadata diffs]`). You must add `discussions` in that list. +[`Provider::ContractSourceHelper`](https://gitlab.com/gitlab-org/gitlab/-/tree/master/spec/contracts/provider/helpers/contract_source_helper.rb) is a helper module that has the `#contract_location` method which parses `pact_helper_location` and determines where the contract is stored locally or on the Pact Broker depending on the `requester` passed in. + +At the same time, add your new `:get_discussions` Rake task to be included in the `test:merge_requests` Rake task. In that Rake task, there is an array defined (`%w[get_diffs_batch get_diffs_metadata]`). You must add `get_discussions` in that list. ## Create test data @@ -113,7 +118,7 @@ As the last step, create the test data that allows the provider test to return t You can read more about [provider states](https://docs.pact.io/implementation_guides/ruby/provider_states). We can do global provider states but for this tutorial, the provider state is for one specific `state`. -To create the test data, create `discussions_state.rb` under `spec/contracts/provider/states/project/merge_request`. Be sure to also import this state file in the `discussions_helper.rb` file. +To create the test data, create `show_state.rb` under `spec/contracts/provider/states/project/merge_requests`. Be sure to also import this state file in the `get_discussions_helper.rb` file. ### Default user in `spec/contracts/provider/spec_helper.rb` @@ -141,7 +146,7 @@ Any further modifications to the user that's needed can be done through the indi In the state file, you must define which consumer this provider state is for. You can do that with `provider_states_for`. Make sure that the `name` provided matches the name defined for the consumer. ```ruby -Pact.provider_states_for 'MergeRequest#show' do +Pact.provider_states_for 'MergeRequests#show' do end ``` @@ -150,7 +155,7 @@ end In the `provider_states_for` block, you then define the state the test data is for. These states are also defined in the consumer test. In this case, there is a `'a merge request with discussions exists'` state. ```ruby -Pact.provider_states_for "MergeRequest#show" do +Pact.provider_states_for "MergeRequests#show" do provider_state "a merge request with discussions exists" do end @@ -162,7 +167,7 @@ end This is where you define the test data creation steps. Use `FactoryBot` to create the data. As you create the test data, you can keep [running the provider test](index.md#run-the-provider-tests) to check on the status of the test and figure out what else is missing in your data setup. ```ruby -Pact.provider_states_for "MergeRequest#show" do +Pact.provider_states_for "MergeRequests#show" do provider_state "a merge request with discussions exists" do set_up do user = User.find_by(name: Provider::UsersHelper::CONTRACT_USER_NAME) @@ -187,20 +192,19 @@ Now that the provider state file is created, you need to import the state file t # frozen_string_literal: true require_relative '../../../spec_helper' -require_relative '../../../states/project/merge_request/discussions_state' +require_relative '../../../states/project/merge_requests/show_state' module Provider module DiscussionsHelper - Pact.service_provider "/merge_request/discussions" do + Pact.service_provider "GET discussions" do app { Environments::Test.app } honours_pact_with 'Merge Request#show' do - pact_uri '../contracts/project/merge_request/show/merge_request#show-merge_request_discussions_endpoint.json' + pact_uri '../contracts/project/merge_requests/show/mergerequests#show-merge_request_discussions_endpoint.json' end end end end - ``` -And there we have it. The provider test for `discussions_helper.rb` should now pass with this. +And there we have it. The provider test for `get_discussions_helper.rb` should now pass with this. diff --git a/doc/development/testing_guide/end_to_end/index.md b/doc/development/testing_guide/end_to_end/index.md index 55d725ba4ae..8369dcb0740 100644 --- a/doc/development/testing_guide/end_to_end/index.md +++ b/doc/development/testing_guide/end_to_end/index.md @@ -207,11 +207,10 @@ For additional test results visibility, tests that run on pipelines generate and host [Allure](https://github.com/allure-framework/allure2) test reports. The `QA` framework is using the [Allure RSpec](https://github.com/allure-framework/allure-ruby/blob/master/allure-rspec/README.md) -gem to generate source files for the `Allure` test report. An additional job -in the pipeline: +gem to generate source files for the `Allure` test report. An additional job in the pipeline: - Fetches these source files from all test jobs. -- Generates and uploads the report to the `GCS` bucket `gitlab-qa-allure-report` under the project `gitlab-qa-resources`. +- Generates and uploads the report to the `S3` bucket `gitlab-qa-allure-report` located in `AWS` group project `eng-quality-ops-ci-cd-shared-infra`. A common CI template for report uploading is stored in [`allure-report.yml`](https://gitlab.com/gitlab-org/quality/pipeline-common/-/blob/master/ci/allure-report.yml). @@ -230,13 +229,13 @@ a link to the current test report. Each type of scheduled pipeline generates a static link for the latest test report according to its stage: -- [`master`](https://storage.googleapis.com/gitlab-qa-allure-reports/e2e-package-and-test/master/index.html) -- [`staging-full`](https://storage.googleapis.com/gitlab-qa-allure-reports/staging-full/master/index.html) -- [`staging-sanity`](https://storage.googleapis.com/gitlab-qa-allure-reports/staging-sanity/master/index.html) -- [`staging-sanity-no-admin`](https://storage.googleapis.com/gitlab-qa-allure-reports/staging-sanity-no-admin/master/index.html) -- [`canary-sanity`](https://storage.googleapis.com/gitlab-qa-allure-reports/canary-sanity/master/index.html) -- [`production`](https://storage.googleapis.com/gitlab-qa-allure-reports/production-full/master/index.html) -- [`production-sanity`](https://storage.googleapis.com/gitlab-qa-allure-reports/production-sanity/master/index.html) +- [`master`](http://gitlab-qa-allure-reports.s3.amazonaws.com/e2e-package-and-test/master/index.html) +- [`staging-full`](http://gitlab-qa-allure-reports.s3.amazonaws.com/staging-full/master/index.html) +- [`staging-sanity`](http://gitlab-qa-allure-reports.s3.amazonaws.com/staging-sanity/master/index.html) +- [`staging-sanity-no-admin`](http://gitlab-qa-allure-reports.s3.amazonaws.com/staging-sanity-no-admin/master/index.html) +- [`canary-sanity`](http://gitlab-qa-allure-reports.s3.amazonaws.com/canary-sanity/master/index.html) +- [`production`](http://gitlab-qa-allure-reports.s3.amazonaws.com/production-full/master/index.html) +- [`production-sanity`](http://gitlab-qa-allure-reports.s3.amazonaws.com/production-sanity/master/index.html) ## How do you run the tests? diff --git a/doc/development/testing_guide/end_to_end/resources.md b/doc/development/testing_guide/end_to_end/resources.md index 6e29e9b9dff..37d354bf60c 100644 --- a/doc/development/testing_guide/end_to_end/resources.md +++ b/doc/development/testing_guide/end_to_end/resources.md @@ -8,17 +8,9 @@ info: To determine the technical writer assigned to the Stage/Group associated w Resources are primarily created using Browser UI steps, but can also be created via the API or the CLI. -A typical resource class is used to create a new resource that can be used in a single test. However, several tests can -end up creating the same kind of resource and use it in ways that mean it could have been -used by more than one test. Creating a new resource each time is not efficient. Therefore, we can also create reusable -resources that are created once and can then be used by many tests. - -In the following section the content focuses on single-use resources, however it also applies to reusable resources. -Information specific to [reusable resources is detailed below](#reusable-resources). - ## How to properly implement a resource class? -All non-reusable resource classes should inherit from `Resource::Base`. +All resource classes should inherit from `Resource::Base`. There is only one mandatory method to implement to define a resource class. This is the `#fabricate!` method, which is used to build the resource via the @@ -398,176 +390,6 @@ end In this case, the result is similar to calling `Resource::Shirt.fabricate!`. -## Reusable resources - -Reusable resources are created by the first test that needs a particular kind of resource, and then any test that needs -the same kind of resource can reuse it instead of creating a new one. - -The `ReusableProject` resource is an example of this class: - -```ruby -module QA - module Resource - class ReusableProject < Project # A reusable resource inherits from the resource class that we want to be able to reuse. - prepend Reusable # The Reusable module mixes in some methods that help implement reuse. - - def initialize - super # A ReusableProject is a Project so it should be initialized as one. - - # Some Project attributes aren't valid and need to be overridden. For example, a ReusableProject keeps its name once it's created, - # so we don't add a random string to the name specified. - @add_name_uuid = false - - # It has a default name, and a different name can be specified when a resource is first created. However, the same name must be - # provided any time that instance of the resource is used. - @name = "reusable_project" - - # Several instances of a ReusableProject can exists as long as each is identified via a unique value for `reuse_as`. - @reuse_as = :default_project - end - - # All reusable resource classes must validate that an instance meets the conditions that allow reuse. For example, - # by confirming that the name specified for the instance is valid and doesn't conflict with other instances. - def validate_reuse_preconditions - raise ResourceReuseError unless reused_name_valid? - end - - # Internally we identify an instance of a reusable resource by a unique value of `@reuse_as`, but in GitLab the - # resource has one or more attributes that must also be unique. This method lists those attributes and allows the - # test framework to check that each instance of a reusable resource has values that match the associated values - # in Gitlab. - def unique_identifiers - [:name, :path] - end - end - end -end -``` - -Reusable resources aren't removed immediately when `remove_via_api!` is called. Instead, they're removed after the test -suite completes. To do so each class must be registered with `QA::Resource::ReusableCollection` in `qa/spec/spec_helper.rb` -as in the example below. Registration allows `QA::Resource::ReusableCollection` to keep track of each instance of each -registered class, and to delete them all in the `config.after(:suite)` hook. - -```ruby -config.before(:suite) do |suite| - QA::Resource::ReusableCollection.register_resource_classes do |collection| - QA::Resource::ReusableProject.register(collection) - end -end -``` - -Consider some examples of how a reusable resource is used: - -```ruby -# This will create a project. -default_project = Resource::ReusableProject.fabricate_via_api! -default_project.name # => "reusable_project" -default_project.reuse_as # => :default_project -``` - -Then in another test we could reuse the project: - -```ruby -# This will fetch the project created above rather than creating a new one. -default_project_again = Resource::ReusableProject.fabricate_via_api! -default_project_again.name # => "reusable_project" -default_project_again.reuse_as # => :default_project -``` - -We can also create another project that we want to change in a way that might not be suitable for tests using the -default project: - -```ruby -project_with_member = Resource::ReusableProject.fabricate_via_api! do |project| - project.name = "project-with-member" - project.reuse_as = :project_with_member -end - -project_with_member.add_member(user) -``` - -Another test can reuse that project: - -```ruby -project_still_has_member = Resource::ReusableProject.fabricate_via_api! do |project| - project.name = "project-with-member" - project.reuse_as = :project_with_member -end - -expect(project_still_has_member).to have_member(user) -``` - -However, if we don't provide the name again an error will be raised: - -```ruby -Resource::ReusableProject.fabricate_via_api! do |project| - project.reuse_as = :project_with_member -end - -# => ResourceReuseError will be raised because it will try to use the default name, "reusable_project", which doesn't -# match the name specified when the project was first fabricated. -``` - -### Validating reusable resources - -Reusable resources can speed up test suites by avoiding the cost of creating the same resource again and again. However, -that can cause problems if a test makes changes to a resource that prevent it from being reused as expected by later -tests. That can lead to order-dependent test failures that can be difficult to troubleshoot. - -For example, the default project created by `QA::Resource::ReusableProject` has `auto_devops_enabled` set to `false` -(inherited from `QA::Resource::Project`). If a test reuses that project and enables Auto DevOps, subsequent tests that reuse -the project will fail if they expect Auto DevOps to be disabled. - -We try to avoid that kind of trouble by validating reusable resources after a test suite. If the environment variable -`QA_VALIDATE_RESOURCE_REUSE` is set to `true` the test framework will check each reusable resource to verify that none -of the attributes they were created with have been changed. It does that by creating a new resource using the same -attributes that were used to create the original resource. It then compares the new resource to the original and raises -an error if any attributes don't match. - -#### Implementation - -When you implement a new type of reusable resource there are two `private` methods you must implement so the resource -can be validated. They are: - -- `reference_resource`: creates a new instance of the resource that can be compared with the one that was used during the tests. -- `unique_identifiers`: returns an array of attributes that allow the resource to be identified (for example, name) and that are therefore -expected to differ when comparing the reference resource with the resource reused in the tests. - -The following example shows the implementation of those two methods in `QA::Resource::ReusableProject`. - -```ruby -# Creates a new project that can be compared to a reused project, using the attributes of the original. -# -# @return [QA::Resource] a new instance of Resource::ReusableProject that should be a copy of the original resource -def reference_resource - # These are the attributes that the reused resource was created with - attributes = self.class.resources[reuse_as][:attributes] - - # Two projects can't have the same path, and since we typically use the same value for the name and path, we assign - # a unique name and path to the reference resource. - name = "reference_resource_#{SecureRandom.hex(8)}_for_#{attributes.delete(:name)}" - - Project.fabricate_via_api! do |project| - self.class.resources[reuse_as][:attributes].each do |attribute_name, attribute_value| - project.instance_variable_set("@#{attribute_name}", attribute_value) if attribute_value - end - project.name = name - project.path = name - project.path_with_namespace = "#{project.group.full_path}/#{project.name}" - end -end - -# The attributes of the resource that should be the same whenever a test wants to reuse a project. -# -# @return [Array<Symbol>] the attribute names. -def unique_identifiers - # As noted above, path must be unique, and since we typically use the same value for both, we treat name and path - # as unique. These attributes are ignored when we compare the reference and reused resources. - [:name, :path] -end -``` - ### Resources cleanup We have a mechanism to [collect](https://gitlab.com/gitlab-org/gitlab/-/blob/44345381e89d6bbd440f7b4c680d03e8b75b86de/qa/qa/tools/test_resource_data_processor.rb#L32) diff --git a/doc/development/testing_guide/frontend_testing.md b/doc/development/testing_guide/frontend_testing.md index 2fa5fdeab7d..85d807eceb1 100644 --- a/doc/development/testing_guide/frontend_testing.md +++ b/doc/development/testing_guide/frontend_testing.md @@ -1368,7 +1368,7 @@ You can also prefix this command with `WEBDRIVER_HEADLESS=0` which will run the ```ruby require 'spec_helper' ``` - + Import any other relevant module. 1. Create a global scope for RSpec to define our tests, just like what we do in jest with the initial describe block. @@ -1414,7 +1414,7 @@ Most feature tests at least require you to create a user, because you want to be This creates a variable that holds the newly created user and we can use `create` because we imported the `spec_helper`. -However, we have not done anything with this user yet because it's just a variable. So, in the `before do` block of the spec, we could sign in with the user so that every spec starts with a signed in user. +However, we have not done anything with this user yet because it's just a variable. So, in the `before do` block of the spec, we could sign in with the user so that every spec starts with an authenticated user. ```ruby let(:user) { create(:user) } diff --git a/doc/development/testing_guide/img/testing_triangle.png b/doc/development/testing_guide/img/testing_triangle.png Binary files differindex 3ac4955eaff..747bad1d52d 100644 --- a/doc/development/testing_guide/img/testing_triangle.png +++ b/doc/development/testing_guide/img/testing_triangle.png diff --git a/doc/development/testing_guide/testing_migrations_guide.md b/doc/development/testing_guide/testing_migrations_guide.md index b276a7e2a3a..1b1fdcca003 100644 --- a/doc/development/testing_guide/testing_migrations_guide.md +++ b/doc/development/testing_guide/testing_migrations_guide.md @@ -17,6 +17,8 @@ a database schema. - If your migration is a data migration then it **must** have a migration test. - Other migrations may have a migration test if necessary. +We don't enforce tests on post migrations that only perform schema changes. + ## How does it work? Adding a `:migration` tag to a test signature enables some custom RSpec diff --git a/doc/development/utilities.md b/doc/development/utilities.md index 58954101890..343d03b9d68 100644 --- a/doc/development/utilities.md +++ b/doc/development/utilities.md @@ -188,7 +188,7 @@ Refer to [`strong_memoize.rb`](https://gitlab.com/gitlab-org/gitlab/-/blob/maste def enabled? Feature.enabled?(:some_feature) end - strong_memoize_attr :enabled?, :enabled + strong_memoize_attr :enabled? end ``` diff --git a/doc/development/value_stream_analytics.md b/doc/development/value_stream_analytics.md index 2d5f33b5dae..33a6744d5cd 100644 --- a/doc/development/value_stream_analytics.md +++ b/doc/development/value_stream_analytics.md @@ -261,7 +261,7 @@ considered legacy, which will be phased out at some point. - Rails Controller (`Analytics::CycleAnalytics` module): Value stream analytics exposes its data via JSON endpoints, implemented within the `analytics` workspace. Configuring the stages are also implements JSON endpoints (CRUD). - Services (`Analytics::CycleAnalytics` module): All `Stage` related actions are delegated to respective service objects. -- Models (`Analytics::CycleAnalytics` module): Models are used to persist the `Stage` objects `ProjectStage` and `GroupStage`. +- Models (`Analytics::CycleAnalytics` module): Models are used to persist the `Stage` objects `ProjectStage` and `Stage`. - Feature classes (`Gitlab::Analytics::CycleAnalytics` module): - Responsible for composing queries and define feature specific business logic. - `DataCollector`, `Event`, `StageEvents`, etc. diff --git a/doc/development/value_stream_analytics/value_stream_analytics_aggregated_backend.md b/doc/development/value_stream_analytics/value_stream_analytics_aggregated_backend.md index 5bcadc6f39b..3d1286a5809 100644 --- a/doc/development/value_stream_analytics/value_stream_analytics_aggregated_backend.md +++ b/doc/development/value_stream_analytics/value_stream_analytics_aggregated_backend.md @@ -273,7 +273,7 @@ attributes. - `stages` - Load the stages for the currently selected value stream. - `median` - For each stage, request the median duration. - `count` - For each stage, request the number of items in the stage (this is a -[limit count](../merge_request_performance_guidelines.md#badge-counters), maximum 1000 rows). +[limit count](../merge_request_concepts/performance.md#badge-counters), maximum 1000 rows). - `average_duration_chart` - Data for the duration chart. - `summary`, `time_summary` - Top-level aggregations, most of the metrics are using different APIs/ finders and not invoking the aggregated backend. diff --git a/doc/development/wikis.md b/doc/development/wikis.md index 67dc567cc5f..4541b6cca66 100644 --- a/doc/development/wikis.md +++ b/doc/development/wikis.md @@ -5,7 +5,7 @@ info: To determine the technical writer assigned to the Stage/Group associated w description: "GitLab's development guidelines for Wikis" --- -# Wikis development guide **(FREE)** +# Wikis development guide > [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/227027) in GitLab 13.5. diff --git a/doc/development/workspace/index.md b/doc/development/workspace/index.md index f4738e3fc31..0e0b6943a0b 100644 --- a/doc/development/workspace/index.md +++ b/doc/development/workspace/index.md @@ -4,19 +4,19 @@ type: index, dev stage: none group: Development info: "See the Technical Writers assigned to Development Guidelines: https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments-to-development-guidelines" -description: "Development Guidelines: learn about workspace when developing GitLab." +description: "Development Guidelines: learn about organization when developing GitLab." --- -# Workspace +# Organization -The [Workspace initiative](../../user/workspace/index.md) focuses on reaching feature parity between +The [Organization initiative](../../user/workspace/index.md) focuses on reaching feature parity between SaaS and self-managed installations. ## Consolidate groups and projects - [Architecture blueprint](../../architecture/blueprints/consolidating_groups_and_projects/index.md) -One facet of the workspace initiative is to consolidate groups and projects, +One facet of the Organization initiative is to consolidate groups and projects, addressing the feature disparity between them. Some features, such as epics, are only available at the group level. Some features, such as issues, are only available at the project level. Other features, such as milestones, are available to both groups @@ -87,7 +87,7 @@ After this work completes, we must migrate data as described in ### Phase 2 - [Phase 2 epic](https://gitlab.com/groups/gitlab-org/-/epics/6768). -- **Goal**: Make `ProjectNamespace` the front entity to interact with instead of `Project`. +- **Goal**: Link `ProjectNamespace` to other entities on the database level. In this phase: @@ -97,6 +97,10 @@ In this phase: - Raise awareness to avoid regressions, and conflicting or duplicate work that can be dealt with before phase 3. +### Phase 3 + +- [Phase 3 epic](https://gitlab.com/groups/gitlab-org/-/epics/6585). +- **Goal**: Achieve feature parity between the namespace types. Problems to solve as part of this phase: - Routes handling through `ProjectNamespace` rather than `Project`. @@ -105,16 +109,51 @@ Problems to solve as part of this phase: - Import and export. - Other interactions between project namespace and project models. -### Phase 3 - -- [Phase 3 epic](https://gitlab.com/groups/gitlab-org/-/epics/6585). -- **Goal**: Feature parity between the namespace types. - Phase 3 is when the active migration of features from `Project` to `ProjectNamespace`, or directly to `Namespace`, happens. +### How to plan features that interact with Group and ProjectNamespace + +As of now, every Project in the system has a record in the `namespaces` table. This makes it possible to +use common interface to create features that are shared between Groups and Projects. Shared behavior can be added using +a concerns mechanism. Because the `Namespace` model is responsible for `UserNamespace` methods as well, it is discouraged +to use the `Namespace` model for shared behavior for Projects and Groups. + +#### Resource-based features + +To migrate resource-based features, existing functionality will need to be supported. This can be achieved in two Phases. + +**Phase 1 - Setup** + +- Link into the namespaces table + - Add a column to the table + - For example, in issues a `project id` points to the projects table. We need to establish a link to the `namespaces` table. + - Modify code so that any new record already has the correct data in it + - Backfill + +**Phase 2 - Prerequisite work** + +- Investigate the permission model as well as any performance concerns related to that. + - Permissions need to be checked and kept in place. +- Investigate what other models need to support namespaces for functionality dependent on features you migrate in Phase 1. +- Adjust CRUD services and APIs (REST and GraphQL) to point to the new column you added in Phase 1. +- Consider performance when fetching resources. + +Introducing new functionality is very much dependent on every single team and feature. + +#### Settings-related features + +Right now, cascading settings are available for `NamespaceSettings`. By creating `ProjectNamespace`, +we can use this framework to make sure that some settings are applicable on the project level as well. + +When working on settings, we need to make sure that: + +- They are not used in `join` queries or modify those queries. +- Updating settings is taken into consideration. +- If we want to move from project to project namespace, we follow a similar database process to the one described in [Phase 1](#phase-1). + ## Related topics - [Consolidating groups and projects](../../architecture/blueprints/consolidating_groups_and_projects/index.md) architecture documentation -- [Workspace user documentation](../../user/workspace/index.md) +- [Organization user documentation](../../user/workspace/index.md) |
