diff options
Diffstat (limited to 'doc/development')
54 files changed, 1707 insertions, 212 deletions
diff --git a/doc/development/README.md b/doc/development/README.md index 16b073045cc..66df6f46e86 100644 --- a/doc/development/README.md +++ b/doc/development/README.md @@ -99,7 +99,7 @@ description: 'Learn how to contribute to GitLab.' - [Post deployment migrations](post_deployment_migrations.md) - [Background migrations](background_migrations.md) - [Swapping tables](swapping_tables.md) -- [Deleting exiting migrations](deleting_migrations.md) +- [Deleting migrations](deleting_migrations.md) ### Best practices @@ -118,6 +118,7 @@ description: 'Learn how to contribute to GitLab.' - [Query Count Limits](query_count_limits.md) - [Database helper modules](database_helpers.md) - [Code comments](code_comments.md) +- [Creating enums](creating_enums.md) ### Case studies diff --git a/doc/development/api_graphql_styleguide.md b/doc/development/api_graphql_styleguide.md index cdd0e9b2a7b..b0d94511c6e 100644 --- a/doc/development/api_graphql_styleguide.md +++ b/doc/development/api_graphql_styleguide.md @@ -43,14 +43,14 @@ a new presenter specifically for GraphQL. The presenter is initialized using the object resolved by a field, and the context. -### Exposing Global ids +### Exposing Global IDs -When exposing an `id` field on a type, we will by default try to -expose a global id by calling `to_global_id` on the resource being +When exposing an `ID` field on a type, we will by default try to +expose a global ID by calling `to_global_id` on the resource being rendered. To override this behaviour, you can implement an `id` method on the -type for which you are exposing an id. Please make sure that when +type for which you are exposing an ID. Please make sure that when exposing a `GraphQL::ID_TYPE` using a custom method that it is globally unique. @@ -146,6 +146,10 @@ query($project_path: ID!) { } ``` +To ensure that we get consistent ordering, we will append an ordering on the primary +key, in descending order. This is usually `id`, so basically we will add `order(id: :desc)` +to the end of the relation. A primary key _must_ be available on the underlying table. + ### Exposing permissions for a type To expose permissions the current user has on a resource, you can call @@ -236,6 +240,47 @@ end ``` +## Descriptions + +All fields and arguments +[must have descriptions](https://gitlab.com/gitlab-org/gitlab/merge_requests/16438). + +A description of a field or argument is given using the `description:` +keyword. For example: + +```ruby +field :id, GraphQL::ID_TYPE, description: 'ID of the resource' +``` + +Descriptions of fields and arguments are viewable to users through: + +- The [GraphiQL explorer](../api/graphql/#graphiql). +- The [static GraphQL API reference](../api/graphql/#reference). + +### Description styleguide + +To ensure consistency, the following should be followed whenever adding or updating +descriptions: + +- Mention the name of the resource in the description. Example: + `'Labels of the issue'` (issue being the resource). +- Use `"{x} of the {y}"` where possible. Example: `'Title of the issue'`. + Do not start descriptions with `The`. +- Descriptions of `GraphQL::BOOLEAN_TYPE` fields should answer the question: "What does + this field do?". Example: `'Indicates project has a Git repository'`. +- Always include the word `"timestamp"` when describing an argument or + field of type `Types::TimeType`. This lets the reader know that the + format of the property will be `Time`, rather than just `Date`. +- No `.` at end of strings. + +Example: + +```ruby +field :id, GraphQL::ID_TYPE, description: 'ID of the Issue' +field :confidential, GraphQL::BOOLEAN_TYPE, description: 'Indicates the issue is confidential' +field :closed_at, Types::TimeType, description: 'Timestamp of when the issue was closed' +``` + ## Authorization Authorizations can be applied to both types and fields using the same @@ -350,7 +395,10 @@ To find objects to display in a field, we can add resolvers to `app/graphql/resolvers`. Arguments can be defined within the resolver, those arguments will be -made available to the fields using the resolver. +made available to the fields using the resolver. When exposing a model +that had an internal ID (`iid`), prefer using that in combination with +the namespace path as arguments in a resolver over a database +ID. Othewise use a [globally unique ID](#exposing-global-ids). We already have a `FullPathLoader` that can be included in other resolvers to quickly find Projects and Namespaces which will have a @@ -365,6 +413,10 @@ actions. In the same way a GET-request should not modify data, we cannot modify data in a regular GraphQL-query. We can however in a mutation. +To find objects for a mutation, arguments need to be specified. As with +[resolvers](#resolvers), prefer using internal ID or, if needed, a +global ID rather than the database ID. + ### Fields In the most common situations, a mutation would return 2 fields: @@ -496,6 +548,32 @@ found, we should raise a `Gitlab::Graphql::Errors::ResourceNotAvailable` error. Which will be correctly rendered to the clients. +## Gitlab's custom scalars + +### `Types::TimeType` + +[`Types::TimeType`](https://gitlab.com/gitlab-org/gitlab/blob/master/app%2Fgraphql%2Ftypes%2Ftime_type.rb) +must be used as the type for all fields and arguments that deal with Ruby +`Time` and `DateTime` objects. + +The type is +[a custom scalar](https://github.com/rmosolgo/graphql-ruby/blob/master/guides/type_definitions/scalars.md#custom-scalars) +that: + +- Converts Ruby's `Time` and `DateTime` objects into standardized + ISO-8601 formatted strings, when used as the type for our GraphQL fields. +- Converts ISO-8601 formatted time strings into Ruby `Time` objects, + when used as the type for our GraphQL arguments. + +This allows our GraphQL API to have a standardized way that it presents time +and handles time inputs. + +Example: + +```ruby +field :created_at, Types::TimeType, null: false, description: 'Timestamp of when the issue was created' +``` + ## Testing _full stack_ tests for a graphql query or mutation live in @@ -540,7 +618,7 @@ it 'returns a successful response' do end ``` -## Documentation +## Documentation and Schema -For information on generating GraphQL documentation, see -[Rake tasks related to GraphQL](rake_tasks.md#update-graphql-documentation). +For information on generating GraphQL documentation and schema files, see +[Rake tasks related to GraphQL](rake_tasks.md#update-graphql-documentation-and-schema-definitions). diff --git a/doc/development/architecture.md b/doc/development/architecture.md index ccedb96d27d..b579f812d99 100644 --- a/doc/development/architecture.md +++ b/doc/development/architecture.md @@ -59,10 +59,10 @@ graph TB Unicorn --> Gitaly Sidekiq --> Redis Sidekiq --> PgBouncer + Sidekiq --> Gitaly GitLabWorkhorse[GitLab Workhorse] --> Unicorn GitLabWorkhorse --> Redis GitLabWorkhorse --> Gitaly - Gitaly --> Redis NGINX --> GitLabWorkhorse NGINX -- TCP 8090 --> GitLabPages[GitLab Pages] NGINX --> Grafana[Grafana] diff --git a/doc/development/changelog.md b/doc/development/changelog.md index 8ded3f393ee..af2c540cca5 100644 --- a/doc/development/changelog.md +++ b/doc/development/changelog.md @@ -41,7 +41,10 @@ the `author` field. GitLab team members **should not**. a changelog entry regardless of these guidelines if the contributor wants one. Example: "Fixed a typo on the search results page." - Any docs-only changes **should not** have a changelog entry. -- Any change behind a feature flag **should not** have a changelog entry. The entry should be added [in the merge request removing the feature flags](feature_flags/development.md). +- Any change behind a feature flag **should not** have a changelog entry. The + entry should be added [in the merge request removing the feature flags](feature_flags/development.md). + If the change includes a database migration, there should be a changelog entry + for the migration change. - A fix for a regression introduced and then fixed in the same release (i.e., fixing a bug introduced during a monthly release candidate) **should not** have a changelog entry. diff --git a/doc/development/chatops_on_gitlabcom.md b/doc/development/chatops_on_gitlabcom.md index 8a313a120f1..456dd1d4b4b 100644 --- a/doc/development/chatops_on_gitlabcom.md +++ b/doc/development/chatops_on_gitlabcom.md @@ -14,7 +14,7 @@ tasks such as: To request access to Chatops on GitLab.com: 1. Log into <https://ops.gitlab.net/users/sign_in> **using the same username** as for GitLab.com (you may have to rename it). -1. Ask [an owner/maintainer in the `chatops` project](https://gitlab.com/gitlab-com/chatops/-/project_members?search=&sort=access_level_desc) to add you by running `/chatops run member add <username> gitlab-com/chatops --ops`. +1. Ask [a project member in the `chatops` project](https://ops.gitlab.net/gitlab-com/chatops/-/project_members) to add you by running `/chatops run member add <username> gitlab-com/chatops --ops`. ## See also diff --git a/doc/development/code_review.md b/doc/development/code_review.md index 421b70bd2db..77c57bb332d 100644 --- a/doc/development/code_review.md +++ b/doc/development/code_review.md @@ -170,8 +170,8 @@ Maintainers should check before merging if the merge request is approved by the required approvers. Maintainers must check before merging if the merge request is introducing new -vulnerabilities, by inspecting the list in the Merge Request [Security -Widget](../user/project/merge_requests/index.md#security-reports-ultimate). +vulnerabilities, by inspecting the list in the Merge Request +[Security Widget](../user/application_security/index.md). When in doubt, a [Security Engineer](https://about.gitlab.com/company/team/) can be involved. The list of detected vulnerabilities must be either empty or containing: @@ -368,7 +368,7 @@ Enterprise Edition instance. This has some implications: - [Background migrations](background_migrations.md) run in Sidekiq, and should only be done for migrations that would take an extreme amount of time at GitLab.com scale. -1. **Sidekiq workers** [cannot change in a backwards-incompatible way](sidekiq_style_guide.md#removing-or-renaming-queues): +1. **Sidekiq workers** [cannot change in a backwards-incompatible way](sidekiq_style_guide.md#sidekiq-compatibility-across-updates): 1. Sidekiq queues are not drained before a deploy happens, so there will be workers in the queue from the previous version of GitLab. 1. If you need to change a method signature, try to do so across two releases, diff --git a/doc/development/contributing/index.md b/doc/development/contributing/index.md index 92dd040a2bd..481a18aac3d 100644 --- a/doc/development/contributing/index.md +++ b/doc/development/contributing/index.md @@ -118,6 +118,10 @@ This [documentation](merge_request_workflow.md) outlines the current merge reque This [documentation](style_guides.md) outlines the current style guidelines. +## Getting an Enterprise Edition License + +If you need a license for contributing to an EE-feature, please [follow these instructions](https://about.gitlab.com/handbook/marketing/community-relations/code-contributor-program/#for-contributors-to-the-gitlab-enterprise-edition-ee). + --- [Return to Development documentation](../README.md) diff --git a/doc/development/contributing/issue_workflow.md b/doc/development/contributing/issue_workflow.md index 349bb371835..f32400d44a2 100644 --- a/doc/development/contributing/issue_workflow.md +++ b/doc/development/contributing/issue_workflow.md @@ -268,7 +268,7 @@ Each issue scheduled for the current milestone should be labeled ~Deliverable or ~"Stretch". Any open issue for a previous milestone should be labeled ~"Next Patch Release", or otherwise rescheduled to a different milestone. -#### Priority labels +### Priority labels Priority labels help us define the time a ~bug fix should be completed. Priority determines how quickly the defect turnaround time must be. If there are multiple defects, the priority decides which defect has to be fixed immediately versus later. diff --git a/doc/development/contributing/merge_request_workflow.md b/doc/development/contributing/merge_request_workflow.md index 86f17f4ecdb..510e90524ed 100644 --- a/doc/development/contributing/merge_request_workflow.md +++ b/doc/development/contributing/merge_request_workflow.md @@ -36,7 +36,7 @@ include a regression test are merged quickly, while new features without proper tests might be slower to receive feedback. The workflow to make a merge request is as follows: -1. [Fork](../../workflow/forking_workflow.md#creating-a-fork) the project into +1. [Fork](../../user/project/repository/forking_workflow.md) the project into your personal namespace (or group) on GitLab.com. 1. Create a feature branch in your fork (don't work off `master`). 1. Write [tests](../rake_tasks.md#run-tests) and code. @@ -69,7 +69,7 @@ request is as follows: the issue(s) once the merge request is merged. 1. If you're allowed to (Core team members, for example), set a relevant milestone and [labels](issue_workflow.md). -1. If the MR changes the UI, it should include *Before* and *After* screenshots. +1. If the MR changes the UI, you'll need approval from a Product Designer (UX), based on the appropriate [product category](https://about.gitlab.com/handbook/product/categories/). UI changes should use available components from the GitLab Design System, [Pajamas](https://design.gitlab.com/). The MR must include *Before* and *After* screenshots. 1. If the MR changes CSS classes, please include the list of affected pages, which can be found by running `grep css-class ./app -R`. 1. Be prepared to answer questions and incorporate feedback into your MR with new @@ -222,7 +222,7 @@ requirements. on the CI server. 1. Regressions and bugs are covered with tests that reduce the risk of the issue happening again. -1. Performance/scalability implications have been considered, addressed, and tested. +1. [Performance guidelines](../merge_request_performance_guidelines.md) have been followed. 1. [Documented](../documentation/index.md) in the `/doc` directory. 1. [Changelog entry added](../changelog.md), if necessary. 1. Reviewed by relevant (UX/FE/BE/tech writing) reviewers and all concerns are addressed. diff --git a/doc/development/creating_enums.md b/doc/development/creating_enums.md new file mode 100644 index 00000000000..64385a2ea79 --- /dev/null +++ b/doc/development/creating_enums.md @@ -0,0 +1,15 @@ +# Creating enums + +When creating a new enum, it should use the database type `SMALLINT`. +The `SMALLINT` type size is 2 bytes, which is sufficient for an enum. +This would help to save space in the database. + +To use this type, add `limit: 2` to the migration that creates the column. + +Example: + +```rb +def change + add_column :ci_job_artifacts, :file_format, :integer, limit: 2 +end +``` diff --git a/doc/development/database_debugging.md b/doc/development/database_debugging.md index 9947f9c16c0..65a3e518585 100644 --- a/doc/development/database_debugging.md +++ b/doc/development/database_debugging.md @@ -19,7 +19,7 @@ If you just want to delete everything and start over with an empty DB (~1 minute - `bundle exec rake db:reset RAILS_ENV=development` -If you just want to delete everything and start over with dummy data (~40 minutes). This also does `db:reset` and runs DB-specific migrations: +If you just want to delete everything and start over with dummy data (~4 minutes). This also does `db:reset` and runs DB-specific migrations: - `bundle exec rake dev:setup RAILS_ENV=development` diff --git a/doc/development/database_review.md b/doc/development/database_review.md index 39236ab1910..f3c19002417 100644 --- a/doc/development/database_review.md +++ b/doc/development/database_review.md @@ -47,6 +47,13 @@ A database **reviewer**'s role is to: reassign MR to the database **maintainer** suggested by Reviewer Roulette. +#### When there are no database maintainers available + +Currently we have a [critical shortage of database maintainers](https://gitlab.com/gitlab-org/gitlab/issues/29717). Until we are able to increase the number of database maintainers to support the volume of reviews, we have implemented this temporary solution. If the database **reviewer** cannot find an available database **maintainer** then: + +1. Assign the MR for a second review by a **database trainee maintainer** for further review. +1. Once satisfied with the review process, and if the database **maintainer** is still not available, skip the database maintainer approval step and assign the merge request to a backend maintainer for final review and approval. + A database **maintainer**'s role is to: - Perform the final database review on the MR. diff --git a/doc/development/documentation/index.md b/doc/development/documentation/index.md index fb0aa5130f8..7d575e9b0b1 100644 --- a/doc/development/documentation/index.md +++ b/doc/development/documentation/index.md @@ -16,7 +16,7 @@ In addition to this page, the following resources can help you craft and contrib ## Source files and rendered web locations -Documentation for GitLab, GitLab Runner, Omnibus GitLab and Charts are published to <https://docs.gitlab.com>. Documentation for GitLab is also published within the application at `/help` on the domain of the GitLab instance. +Documentation for GitLab, GitLab Runner, Omnibus GitLab and Charts is published to <https://docs.gitlab.com>. Documentation for GitLab is also published within the application at `/help` on the domain of the GitLab instance. At `/help`, only help for your current edition and version is included. Help for other versions is available at <https://docs.gitlab.com/archives/>. The source of the documentation exists within the codebase of each GitLab application in the following repository locations: @@ -67,8 +67,6 @@ This document was moved to [another location](path/to/new_doc.md). where `path/to/new_doc.md` is the relative path to the root directory `doc/`. ---- - For example, if you move `doc/workflow/lfs/lfs_administration.md` to `doc/administration/lfs.md`, then the steps would be: diff --git a/doc/development/documentation/site_architecture/index.md b/doc/development/documentation/site_architecture/index.md index f5a12e9c216..bf873995e54 100644 --- a/doc/development/documentation/site_architecture/index.md +++ b/doc/development/documentation/site_architecture/index.md @@ -4,14 +4,16 @@ description: "Learn how GitLab's documentation website is architectured." # Documentation site architecture -Learn how we build and architecture [`gitlab-docs`](https://gitlab.com/gitlab-org/gitlab-docs) -and deploy it to <https://docs.gitlab.com>. +The [`gitlab-docs`](https://gitlab.com/gitlab-org/gitlab-docs) project hosts +the repository which is used to generate the GitLab documentation website and +is deployed to <https://docs.gitlab.com>. It uses the [Nanoc](http://nanoc.ws) +static site generator. -## Repository +## Architecture While the source of the documentation content is stored in GitLab's respective product -repositories, the source that is used to build the documentation site _from that content_ -is located at <https://gitlab.com/gitlab-org/gitlab-docs>. +repositories, the source that is used to build the documentation +site _from that content_ is located at <https://gitlab.com/gitlab-org/gitlab-docs>. The following diagram illustrates the relationship between the repositories from where content is sourced, the `gitlab-docs` project, and the published output. @@ -43,8 +45,23 @@ from where content is sourced, the `gitlab-docs` project, and the published outp G --> L ``` -See the [README there](https://gitlab.com/gitlab-org/gitlab-docs/blob/master/README.md) -for detailed information. +You will not find any GitLab docs content in the `gitlab-docs` repository. +All documentation files are hosted in the respective repository of each +product, and all together are pulled to generate the docs website: + +- [GitLab](https://gitlab.com/gitlab-org/gitlab/tree/master/doc) +- [Omnibus GitLab](https://gitlab.com/gitlab-org/omnibus-gitlab/tree/master/doc) +- [GitLab Runner](https://gitlab.com/gitlab-org/gitlab-runner/tree/master/docs) +- [GitLab Chart](https://gitlab.com/charts/gitlab/tree/master/doc) + +NOTE: **Note:** +In September 2019, we [moved towards a single codebase](https://gitlab.com/gitlab-org/gitlab-ee/issues/2952), +as such the docs for CE and EE are now identical. For historical reasons and +in order not to break any existing links throughout the internet, we still +maintain the CE docs (`https://docs.gitlab.com/ce/`), although it is hidden +from the website, and is now a symlink to the EE docs. When +[Pages supports redirects](https://gitlab.com/gitlab-org/gitlab-pages/issues/24), +we will be able to remove this completely. ## Assets @@ -73,28 +90,112 @@ Read through [the global navigation documentation](global_nav.md) to understand: - How the global navigation is built. - How to add new navigation items. -## Deployment +<!-- +## Helpers -The docs site is deployed to production with GitLab Pages, and previewed in -merge requests with Review Apps. +TBA +--> -The deployment aspects will be soon transferred from the [original document](https://gitlab.com/gitlab-org/gitlab-docs/blob/master/README.md) -to this page. +## Using YAML data files -<!-- -## Repositories +The easiest way to achieve something similar to +[Jekyll's data files](https://jekyllrb.com/docs/datafiles/) in Nanoc is by +using the [`@items`](https://nanoc.ws/doc/reference/variables/#items-and-layouts) +variable. -TBA +The data file must be placed inside the `content/` directory and then it can +be referenced in an ERB template. -## Search engine +Suppose we have the `content/_data/versions.yaml` file with the content: -TBA +```yaml +versions: +- 10.6 +- 10.5 +- 10.4 +``` -## Versions +We can then loop over the `versions` array with something like: -TBA +```erb +<% @items['/_data/versions.yaml'][:versions].each do | version | %> -## Helpers +<h3><%= version %></h3> -TBA ---> +<% end &> +``` + +Note that the data file must have the `yaml` extension (not `yml`) and that +we reference the array with a symbol (`:versions`). + +## Bumping versions of CSS and Javascript + +Whenever the custom CSS and Javascript files under `content/assets/` change, +make sure to bump their version in the frontmatter. This method guarantees that +your changes will take effect by clearing the cache of previous files. + +Always use Nanoc's way of including those files, do not hardcode them in the +layouts. For example use: + +```erb +<script async type="application/javascript" src="<%= @items['/assets/javascripts/badges.*'].path %>"></script> + +<link rel="stylesheet" href="<%= @items['/assets/stylesheets/toc.*'].path %>"> +``` + +The links pointing to the files should be similar to: + +```erb +<%= @items['/path/to/assets/file.*'].path %> +``` + +Nanoc will then build and render those links correctly according with what's +defined in [`Rules`](https://gitlab.com/gitlab-org/gitlab-docs/blob/master/Rules). + +## Linking to source files + +A helper called [`edit_on_gitlab`](https://gitlab.com/gitlab-org/gitlab-docs/blob/master/lib/helpers/edit_on_gitlab.rb) can be used +to link to a page's source file. We can link to both the simple editor and the +web IDE. Here's how you can use it in a Nanoc layout: + +- Default editor: `<a href="<%= edit_on_gitlab(@item, editor: :simple) %>">Simple editor</a>` +- Web IDE: `<a href="<%= edit_on_gitlab(@item, editor: :webide) %>">Web IDE</a>` + +If you don't specify `editor:`, the simple one is used by default. + +## Algolia search engine + +The docs site uses [Algolia docsearch](https://community.algolia.com/docsearch/) +for its search function. This is how it works: + +1. GitLab is a member of the [docsearch program](https://community.algolia.com/docsearch/#join-docsearch-program), + which is the free tier of [Algolia](https://www.algolia.com/). +1. Algolia hosts a [doscsearch config](https://github.com/algolia/docsearch-configs/blob/master/configs/gitlab.json) + for the GitLab docs site, and we've worked together to refine it. +1. That [config](https://community.algolia.com/docsearch/config-file.html) is + parsed by their [crawler](https://community.algolia.com/docsearch/crawler-overview.html) + every 24h and [stores](https://community.algolia.com/docsearch/inside-the-engine.html) + the [docsearch index](https://community.algolia.com/docsearch/how-do-we-build-an-index.html) + on [Algolia's servers](https://community.algolia.com/docsearch/faq.html#where-is-my-data-hosted%3F). +1. On the docs side, we use a [docsearch layout](https://gitlab.com/gitlab-org/gitlab-docs/blob/master/layouts/docsearch.html) which + is present on pretty much every page except <https://docs.gitlab.com/search/>, + which uses its [own layout](https://gitlab.com/gitlab-org/gitlab-docs/blob/master/layouts/instantsearch.html). In those layouts, + there's a javascript snippet which initiates docsearch by using an API key + and an index name (`gitlab`) that are needed for Algolia to show the results. + +NOTE: **For GitLab employees:** +The credentials to access the Algolia dashboard are stored in 1Password. If you +want to receive weekly reports of the search usage, search the Google doc with +title "Email, Slack, and GitLab Groups and Aliases", search for `docsearch`, +and add a comment with your email to be added to the alias that gets the weekly +reports. + +## Monthly release process (versions) + +The docs website supports versions and each month we add the latest one to the list. +For more information, read about the [monthly release process](release_process.md). + +## Review Apps for documentation merge requests + +If you are contributing to GitLab docs read how to [create a Review App with each +merge request](../index.md#previewing-the-changes-live). diff --git a/doc/development/documentation/site_architecture/release_process.md b/doc/development/documentation/site_architecture/release_process.md new file mode 100644 index 00000000000..6f723531f4c --- /dev/null +++ b/doc/development/documentation/site_architecture/release_process.md @@ -0,0 +1,241 @@ +# GitLab Docs monthly release process + +The [`dockerfiles` directory](https://gitlab.com/gitlab-org/gitlab-docs/blob/master/dockerfiles/) +contains all needed Dockerfiles to build and deploy the versioned website. It +is heavily inspired by Docker's +[Dockerfile](https://github.com/docker/docker.github.io/blob/06ed03db13895bfe867761b6fc2ad40acf6026dd/Dockerfile). + +The following Dockerfiles are used. + +| Dockerfile | Docker image | Description | +| ---------- | ------------ | ----------- | +| [`Dockerfile.bootstrap`](https://gitlab.com/gitlab-org/gitlab-docs/blob/master/dockerfiles/Dockerfile.bootstrap) | `gitlab-docs:bootstrap` | Contains all the dependencies that are needed to build the website. If the gems are updated and `Gemfile{,.lock}` changes, the image must be rebuilt. | +| [`Dockerfile.builder.onbuild`](https://gitlab.com/gitlab-org/gitlab-docs/blob/master/dockerfiles/Dockerfile.builder.onbuild) | `gitlab-docs:builder-onbuild` | Base image to build the docs website. It uses `ONBUILD` to perform all steps and depends on `gitlab-docs:bootstrap`. | +| [`Dockerfile.nginx.onbuild`](https://gitlab.com/gitlab-org/gitlab-docs/blob/master/dockerfiles/Dockerfile.nginx.onbuild) | `gitlab-docs:nginx-onbuild` | Base image to use for building documentation archives. It uses `ONBUILD` to perform all required steps to copy the archive, and relies upon its parent `Dockerfile.builder.onbuild` that is invoked when building single documentation achives (see the `Dockerfile` of each branch. | +| [`Dockerfile.archives`](https://gitlab.com/gitlab-org/gitlab-docs/blob/master/dockerfiles/Dockerfile.archives) | `gitlab-docs:archives` | Contains all the versions of the website in one archive. It copies all generated HTML files from every version in one location. | + +## How to build the images + +Although build images are built automatically via GitLab CI/CD, you can build +and tag all tooling images locally: + +1. Make sure you have [Docker installed](https://docs.docker.com/install/). +1. Make sure you're on the `dockerfiles/` directory of the `gitlab-docs` repo. +1. Build the images: + + ```sh + docker build -t registry.gitlab.com/gitlab-org/gitlab-docs:bootstrap -f Dockerfile.bootstrap ../ + docker build -t registry.gitlab.com/gitlab-org/gitlab-docs:builder-onbuild -f Dockerfile.builder.onbuild ../ + docker build -t registry.gitlab.com/gitlab-org/gitlab-docs:nginx-onbuild -f Dockerfile.nginx.onbuild ../ + ``` + +For each image, there's a manual job under the `images` stage in +[`.gitlab-ci.yml`](https://gitlab.com/gitlab-org/gitlab-docs/blob/master/.gitlab-ci.yml) which can be invoked at will. + +## Monthly release process + +When a new GitLab version is released on the 22nd, we need to create the respective +single Docker image, and update some files so that the dropdown works correctly. + +### 1. Add the chart version + +Since the charts use a different version number than all the other GitLab +products, we need to add a +[version mapping](https://docs.gitlab.com/charts/installation/version_mappings.html): + +1. Check that there is a [stable branch created](https://gitlab.com/gitlab-org/charts/gitlab/-/branches) + for the new chart version. If you're unsure or can't find it, drop a line in + the `#g_delivery` channel. +1. Make sure you're on the root path of the `gitlab-docs` repo. +1. Open `content/_data/chart_versions.yaml` and add the new stable branch version using the + version mapping. Note that only the `major.minor` version is needed. +1. Create a new merge request and merge it. + +TIP: **Tip:** +It can be handy to create the future mappings since they are pretty much known. +In that case, when a new GitLab version is released, you don't have to repeat +this first step. + +### 2. Create an image for a single version + +The single docs version must be created before the release merge request, but +this needs to happen when the stable branches for all products have been created. + +1. Make sure you're on the root path of the `gitlab-docs` repo. +1. Run the raketask to create the single version: + + ```sh + ./bin/rake "release:single[12.0]" + ``` + + A new `Dockerfile.12.0` should have been created and committed to a new branch. + +1. Push the newly created branch, but **don't create a merge request**. + Once you push, the `image:docker-singe` job will create a new Docker image + tagged with the branch name you created in the first step. In the end, the + image will be uploaded in the [Container Registry](https://gitlab.com/gitlab-org/gitlab-docs/container_registry) + and it will be listed under the + [`registry` environment folder](https://gitlab.com/gitlab-org/gitlab-docs/environments/folders/registry). + +Optionally, you can test locally by building the image and running it: + +```sh +docker build -t docs:12.0 -f Dockerfile.12.0 . +docker run -it --rm -p 4000:4000 docs:12.0 +``` + +Visit `http://localhost:4000/12.0/` to see if everything works correctly. + +### 3. Create the release merge request + +Now it's time to create the monthly release merge request that adds the new +version and rotates the old one: + +1. Make sure you're on the root path of the `gitlab-docs` repo. +1. Create a branch `release-X-Y`: + + ```sh + git checkout -b release-12-0 + ``` + +1. **Rotate the online and offline versions:** + + At any given time, there are 4 browsable online versions: one pulled from + the upstream master branches (docs for GitLab.com) and the three latest + stable versions. + + Edit `content/_data/versions.yaml` and rotate the versions to reflect the + new changes: + + - `online`: The 3 latest stable versions. + - `offline`: All the previous versions offered as an offline archive. + +1. **Add the new offline version in the 404 page redirect script:** + + Since we're deprecating the oldest version each month, we need to redirect + those URLs in order not to create [404 entries](https://gitlab.com/gitlab-org/gitlab-docs/issues/221). + There's a temporary hack for now: + + 1. Edit `content/404.html`, making sure all offline versions under + `content/_data/versions.yaml` are in the Javascript snippet at the end of + the document. + +1. **Update the `:latest` and `:archives` Docker images:** + + The following two Dockerfiles need to be updated: + + 1. `dockerfiles/Dockerfile.archives` - Add the latest version at the top of + the list. + 1. `Dockerfile.master` - Rotate the versions (oldest gets removed and latest + is added at the top of the list). + +1. In the end, there should be four files in total that have changed. + Commit and push to create the merge request using the "Release" template: + + ```sh + git add content/ Dockerfile.master dockerfiles/Dockerfile.archives + git commit -m "Release 12.0" + git push origin release-12-0 + ``` + +### 4. Update the dropdown for all online versions + +The versions dropdown is in a way "hardcoded". When the site is built, it looks +at the contents of `content/_data/versions.yaml` and based on that, the dropdown +is populated. So, older branches will have different content, which means the +dropdown will be one or more releases behind. Remember that the new changes of +the dropdown are included in the unmerged `release-X-Y` branch. + +The content of `content/_data/versions.yaml` needs to change for all online +versions: + +1. Before creating the merge request, [disable the scheduled pipeline](https://gitlab.com/gitlab-org/gitlab-docs/pipeline_schedules/228/edit) + by unchecking the "Active" option. Since all steps must run in sequence, we need + to do this to avoid race conditions in the event some previous versions are + updated before the release merge request is merged. +1. Run the raketask that will create all the respective merge requests needed to + update the dropdowns and will be set to automatically be merged when their + pipelines succeed. The `release-X-Y` branch needs to be present locally, + otherwise the raketask will fail: + + ```sh + ./bin/rake release:dropdowns + ``` + +Once all are merged, proceed to the following and final step. + +TIP: **Tip:** +In case a pipeline fails, see [troubleshooting](#troubleshooting). + +### 5. Merge the release merge request + +The dropdown merge requests should have now been merged into their respective +version (stable branch), which will trigger another pipeline. At this point, +you need to only babysit the pipelines and make sure they don't fail: + +1. Check the [pipelines page](https://gitlab.com/gitlab-org/gitlab-docs/pipelines) + and make sure all stable branches have green pipelines. +1. After all the pipelines of the online versions succeed, merge the release merge request. +1. Finally, re-activate the [scheduled pipeline](https://gitlab.com/gitlab-org/gitlab-docs/pipeline_schedules/228/edit), + save it, and hit the play button to get it started. + +Once the scheduled pipeline succeeds, the docs site will be deployed with all +new versions online. + +## Update an old Docker image with new upstream docs content + +If there are any changes to any of the stable branches of the products that are +not included in the single Docker image, just +[rerun the pipeline](https://gitlab.com/gitlab-org/gitlab-docs/pipelines/new) +for the version in question. + +## Porting new website changes to old versions + +CAUTION: **Warning:** +Porting changes to older branches can have unintended effects as we're constantly +changing the backend of the website. Use only when you know what you're doing +and make sure to test locally. + +The website will keep changing and being improved. In order to consolidate +those changes to the stable branches, we'd need to pick certain changes +from time to time. + +If this is not possible or there are many changes, merge master into them: + +```sh +git branch 12.0 +git fetch origin master +git merge origin/master +``` + +## Troubleshooting + +Releasing a new version is a long process that involves many moving parts. + +### `test_internal_links_and_anchors` failing on dropdown merge requests + +When [updating the dropdown for the stable versions](#4-update-the-dropdown-for-all-online-versions), +there may be cases where some links might fail. The process of how the +dropdown MRs are created have a caveat, and that is that the tests run by +pulling the master branches of all products, instead of the respective stable +ones. + +In a real world scenario, the [Update 12.2 dropdown to match that of 12.4](https://gitlab.com/gitlab-org/gitlab-docs/merge_requests/604) +merge request failed because of the [`test_internal_links_and_anchors` test](https://gitlab.com/gitlab-org/gitlab-docs/-/jobs/328042431). + +This happened because there has been a rename of a product (`gitlab-monitor` to `gitlab-exporter`) +and the old name was still referenced in the 12.2 docs. If the respective stable +branches for 12.2 were used, this wouldn't have failed, but as we can see from +the [`compile_dev` job](https://gitlab.com/gitlab-org/gitlab-docs/-/jobs/328042427), +the `master` branches were pulled. + +To fix this, you need to [re-run the pipeline](https://gitlab.com/gitlab-org/gitlab-docs/pipelines/new) +for the `update-12-2-for-release-12-4` branch, by including the following environment variables: + +- `BRANCH_CE` set to `12-2-stable` +- `BRANCH_EE` set to `12-2-stable-ee` +- `BRANCH_OMNIBUS` set to `12-2-stable` +- `BRANCH_RUNNER` set to `12-2-stable` +- `BRANCH_CHARTS` set to `2-2-stable` + +This should make the MR pass. diff --git a/doc/development/documentation/styleguide.md b/doc/development/documentation/styleguide.md index b6ec7a858fa..e6d666473c3 100644 --- a/doc/development/documentation/styleguide.md +++ b/doc/development/documentation/styleguide.md @@ -604,9 +604,6 @@ Inside the document: - Always use a proper description for what the image is about. That way, when a browser fails to show the image, this text will be used as an alternative description. -- If there are consecutive images with little text between them, always add - three dashes (`---`) between the image and the text to create a horizontal - line for better clarity. - If a heading is placed right after an image, always add three dashes (`---`) between the image and the heading. @@ -1182,12 +1179,12 @@ Rendered example: - Prefer to use examples using the personal access token and don't pass data of username and password. -| Methods | Description | -|:-------------------------------------------|:------------------------------------------------------| -| `-H "PRIVATE-TOKEN: <your_access_token>"` | Use this method as is, whenever authentication needed | -| `-X POST` | Use this method when creating new objects | -| `-X PUT` | Use this method when updating existing objects | -| `-X DELETE` | Use this method when removing existing objects | +| Methods | Description | +|:------------------------------------------- |:------------------------------------------------------| +| `--header "PRIVATE-TOKEN: <your_access_token>"` | Use this method as is, whenever authentication needed | +| `--request POST` | Use this method when creating new objects | +| `--request PUT` | Use this method when updating existing objects | +| `--request DELETE` | Use this method when removing existing objects | ### cURL Examples @@ -1209,9 +1206,9 @@ Create a new project under the authenticated user's namespace: curl --request POST --header "PRIVATE-TOKEN: <your_access_token>" "https://gitlab.example.com/api/v4/projects?name=foo" ``` -#### Post data using cURL's --data +#### Post data using cURL's `--data` -Instead of using `-X POST` and appending the parameters to the URI, you can use +Instead of using `--request POST` and appending the parameters to the URI, you can use cURL's `--data` option. The example below will create a new project `foo` under the authenticated user's namespace. diff --git a/doc/development/documentation/workflow.md b/doc/development/documentation/workflow.md index 24399391b1a..c373b976453 100644 --- a/doc/development/documentation/workflow.md +++ b/doc/development/documentation/workflow.md @@ -208,7 +208,7 @@ code reviewer have ensured: Documentation [is required](../contributing/merge_request_workflow.html#definition-of-done) for a milestone when: -- A new or enhanced feature is shipped that impacts the user of administrator experience. +- A new or enhanced feature is shipped that impacts the user or administrator experience. - There are changes to the UI or API. - A process, workflow, or previously documented feature is changed. - A feature is deprecated or removed. diff --git a/doc/development/ee_features.md b/doc/development/ee_features.md index cc9df479492..d716325f332 100644 --- a/doc/development/ee_features.md +++ b/doc/development/ee_features.md @@ -245,47 +245,29 @@ end #### Use self-descriptive wrapper methods -When it's not possible/logical to modify the implementation of a -method. Wrap it in a self-descriptive method and use that method. +When it's not possible/logical to modify the implementation of a method, then +wrap it in a self-descriptive method and use that method. -For example, in CE only an `admin` is allowed to access all private -projects/groups, but in EE also an `auditor` has full private -access. It would be incorrect to override the implementation of -`User#admin?`, so instead add a method `full_private_access?` to -`app/models/users.rb`. The implementation in CE will be: +For example, in GitLab-FOSS, the only user created by the system is `User.ghost` +but in EE there are several types of bot-users that aren't really users. It would +be incorrect to override the implementation of `User#ghost?`, so instead we add +a method `#internal?` to `app/models/user.rb`. The implementation will be: ```ruby -def full_private_access? - admin? +def internal? + ghost? end ``` In EE, the implementation `ee/app/models/ee/users.rb` would be: ```ruby -override :full_private_access? -def full_private_access? - super || auditor? +override :internal? +def internal? + super || bot? end ``` -In `lib/gitlab/visibility_level.rb` this method is used to return the -allowed visibility levels: - -```ruby -def levels_for_user(user = nil) - if user.full_private_access? - [PRIVATE, INTERNAL, PUBLIC] - elsif # ... -end -``` - -See [CE MR][ce-mr-full-private] and [EE MR][ee-mr-full-private] for -full implementation details. - -[ce-mr-full-private]: https://gitlab.com/gitlab-org/gitlab-foss/merge_requests/12373 -[ee-mr-full-private]: https://gitlab.com/gitlab-org/gitlab/merge_requests/2199 - ### Code in `config/routes` When we add `draw :admin` in `config/routes.rb`, the application will try to diff --git a/doc/development/event_tracking/index.md b/doc/development/event_tracking/index.md index efc61d13cb0..ac19053320d 100644 --- a/doc/development/event_tracking/index.md +++ b/doc/development/event_tracking/index.md @@ -68,7 +68,3 @@ Once enabled, tracking events can be inspected locally by either: - Looking at the network panel of the browser's development tools - Using the [Snowplow Chrome Extension](https://chrome.google.com/webstore/detail/snowplow-inspector/maplkdomeamdlngconidoefjpogkmljm). - -## Additional libraries - -Session tracking is handled by [Pendo](https://www.pendo.io/), which is a purely client library and is a relatively minor development concern but is worth including in this documentation. diff --git a/doc/development/fe_guide/development_process.md b/doc/development/fe_guide/development_process.md index 3724bf60757..5b02098f020 100644 --- a/doc/development/fe_guide/development_process.md +++ b/doc/development/fe_guide/development_process.md @@ -73,7 +73,7 @@ With the purpose of being [respectful of others' time](https://about.gitlab.com/ - Before assigning to a maintainer, assign to a reviewer. - If you assigned a merge request, or pinged someone directly, keep in mind that we work in different timezones and asynchronously, so be patient. Unless the merge request is urgent (like fixing a broken master), please don't DM or reassign the merge request before waiting for a 24-hour window. - If you have a question regarding your merge request/issue, make it on the merge request/issue. When we DM each other, we no longer have a SSOT and [no one else is able to contribute](https://about.gitlab.com/handbook/values/#public-by-default). -- When you have a big WIP merge request with many changes, you're adivsed to get the review started before adding/removing significant code. Make sure it is assigned well before the release cut-off, as the reviewer(s)/maintainer(s) would always prioritize reviewing finished MRs before WIP ones. +- When you have a big WIP merge request with many changes, you're advised to get the review started before adding/removing significant code. Make sure it is assigned well before the release cut-off, as the reviewer(s)/maintainer(s) would always prioritize reviewing finished MRs before WIP ones. - Make sure to remove the WIP title before the last round of review. ### Share your work early diff --git a/doc/development/fe_guide/graphql.md b/doc/development/fe_guide/graphql.md index fe4f6d7bec8..894a613ec2d 100644 --- a/doc/development/fe_guide/graphql.md +++ b/doc/development/fe_guide/graphql.md @@ -19,6 +19,14 @@ To save duplicated clients getting created in different apps, we have a [default client][default-client] that should be used. This setups the Apollo client with the correct URL and also sets the CSRF headers. +Default client accepts two parameters: `resolvers` and `config`. + +- `resolvers` parameter is created to accept an object of resolvers for [local state management](#local-state-with-apollo) queries and mutations +- `config` parameter takes an object of configuration settings: + - `cacheConfig` field accepts an optional object of settings to [customize Apollo cache](https://github.com/apollographql/apollo-client/tree/master/packages/apollo-cache-inmemory#configuration) + - `baseUrl` allows us to pass a URL for GraphQL endpoint different from our main endpoint (i.e.`${gon.relative_url_root}/api/graphql`) + - `assumeImmutableResults` (set to `false` by default) - this setting, when set to `true`, will assume that every single operation on updating Apollo Cache is immutable. It also sets `freezeResults` to `true`, so any attempt on mutating Apollo Cache will throw a console warning in development environment. Please ensure you're following the immutability pattern on cache update operations before setting this option to `true`. + ## GraphQL Queries To save query compilation at runtime, webpack can directly import `.graphql` diff --git a/doc/development/fe_guide/style_guide_js.md b/doc/development/fe_guide/style_guide_js.md index 306b19c6e5d..43cd8180b6e 100644 --- a/doc/development/fe_guide/style_guide_js.md +++ b/doc/development/fe_guide/style_guide_js.md @@ -581,6 +581,18 @@ Please check this [rules][eslint-plugin-vue-rules] for more documentation. <component /> ``` +#### Component usage within templates + +1. Prefer a component's kebab-cased name over other styles when using it in a template + + ```javascript + // bad + <MyComponent /> + + // good + <my-component /> + ``` + #### Ordering 1. Tag order in `.vue` file diff --git a/doc/development/fe_guide/style_guide_scss.md b/doc/development/fe_guide/style_guide_scss.md index 07c87920dab..54d41b42c77 100644 --- a/doc/development/fe_guide/style_guide_scss.md +++ b/doc/development/fe_guide/style_guide_scss.md @@ -27,7 +27,7 @@ New utility classes should be added to [`utilities.scss`](https://gitlab.com/git | Font size | `.text-{size}` | `.text-2` | - `{variant}` is one of 'primary', 'secondary', 'success', 'warning', 'error' -- `{shade}` is on of the shades listed on [colors](https://design.gitlab.com/product-foundations/colors/) +- `{shade}` is one of the shades listed on [colors](https://design.gitlab.com/product-foundations/colors/) - `{size}` is a number from 1-6 from our [Type scale](https://design.gitlab.com/product-foundations/typography) #### When should I create component classes? diff --git a/doc/development/feature_flags/controls.md b/doc/development/feature_flags/controls.md index f22c3bb1e37..cbbbe1cd5ea 100644 --- a/doc/development/feature_flags/controls.md +++ b/doc/development/feature_flags/controls.md @@ -29,6 +29,14 @@ Monitor stage, Health group. For all production environment Chatops commands, use the `#production` channel. +Regardless of the channel in which the Chatops command is ran, any feature flag change that affects GitLab.com will automatically be logged in an issue. + +The issue is created in the [gl-infra/feature-flag-log](https://gitlab.com/gitlab-com/gl-infra/feature-flag-log/issues?scope=all&utf8=%E2%9C%93&state=closed) project, and it will at minimum log the Slack handle of person enabling a feature flag, the time, and the name of the flag being changed. + +The issue is then also posted to GitLab Inc. internal [Grafana dashboard](https://dashboards.gitlab.net/) as an annotation marker to make the change even more visible. + +Changes to the issue format can be submitted in the [Chatops project](https://gitlab.com/gitlab-com/chatops). + ## Rolling out changes When the changes are deployed to the environments it is time to start diff --git a/doc/development/feature_flags/development.md b/doc/development/feature_flags/development.md index 929c9b1c71c..c410c7eae41 100644 --- a/doc/development/feature_flags/development.md +++ b/doc/development/feature_flags/development.md @@ -25,7 +25,8 @@ end Features that are developed and are intended to be merged behind a feature flag should not include a changelog entry. The entry should be added in the merge -request removing the feature flags. +request removing the feature flag. If the feature contains any DB migration it +should include a changelog entry for DB changes. In the rare case that you need the feature flag to be on automatically, use `default_enabled: true` when checking: @@ -51,20 +52,16 @@ isn't gated by a License or Plan. [namespace-fa]: https://gitlab.com/gitlab-org/gitlab/blob/4cc1c62918aa4c31750cb21dfb1a6c3492d71080/ee/app/models/ee/namespace.rb#L71-85 [license-fa]: https://gitlab.com/gitlab-org/gitlab/blob/4cc1c62918aa4c31750cb21dfb1a6c3492d71080/ee/app/models/license.rb#L293-300 -An important side-effect of the implicit feature flags mentioned above is that +**An important side-effect of the implicit feature flags mentioned above is that unless the feature is explicitly disabled or limited to a percentage of users, -the feature flag check will default to `true`. +the feature flag check will default to `true`.** As an example, if you were to ship the backend half of a feature behind a flag, you'd want to explicitly disable that flag until the frontend half is also ready -to be shipped. [You can do this via Chatops](controls.md): - -``` -/chatops run feature set some_feature 0 -``` - -Note that you can do this at any time, even before the merge request using the -flag has been merged! +to be shipped. To make sure this feature is disabled for both GitLab.com and +self-managed instances you'd need to explicitly call `Feature.enabled?` method +before the `feature_available` method. This ensures the feature_flag is defaulting +to `false`. ## Feature groups diff --git a/doc/development/geo.md b/doc/development/geo.md index 446d85fceed..5010e44e826 100644 --- a/doc/development/geo.md +++ b/doc/development/geo.md @@ -101,15 +101,16 @@ it's successful, we replace the main repo with the newly cloned one. ### Uploads replication File uploads are also being replicated to the **secondary** node. To -track the state of syncing, the `Geo::FileRegistry` model is used. +track the state of syncing, the `Geo::UploadRegistry` model is used. -#### File Registry +#### Upload Registry Similar to the [Project Registry](#project-registry), there is a -`Geo::FileRegistry` model that tracks the synced uploads. +`Geo::UploadRegistry` model that tracks the synced uploads. -CI Job Artifacts are synced in a similar way as uploads or LFS -objects, but they are tracked by `Geo::JobArtifactRegistry` model. +CI Job Artifacts and LFS objects are synced in a similar way as uploads, +but they are tracked by `Geo::JobArtifactRegistry`, and `Geo::LfsObjectRegistry` +models respectively. #### File Download Dispatch worker @@ -490,6 +491,24 @@ When some write actions are not allowed because the node is a The database itself will already be read-only in a replicated setup, so we don't need to take any extra step for that. +## Steps needed to replicate a new data type + +As GitLab evolves, we constantly need to add new resources to the Geo replication system. +The implementation depends on resource specifics, but there are several things +that need to be taken care of: + +- Event generation on the primary site. Whenever a new resource is changed/updated, we need to + create a task for the Log Cursor. +- Event handling. The Log Cursor needs to have a handler for every event type generated by the primary site. +- Dispatch worker (cron job). Make sure the backfill condition works well. +- Sync worker. +- Registry with all possible states. +- Verification. +- Cleaner. When sync settings are changed for the secondary site, some resources need to be cleaned up. +- Geo Node Status. We need to provide API endpoints as well as some presentation in the GitLab Admin Area. +- Health Check. If we can perform some pre-cheсks and make node unhealthy if something is wrong, we should do that. + The `rake gitlab:geo:check` command has to be updated too. + ## History of communication channel The communication channel has changed since first iteration, you can diff --git a/doc/development/git_object_deduplication.md b/doc/development/git_object_deduplication.md index e8af6346524..6d9eb90d482 100644 --- a/doc/development/git_object_deduplication.md +++ b/doc/development/git_object_deduplication.md @@ -1,6 +1,6 @@ # How Git object deduplication works in GitLab -When a GitLab user [forks a project](../workflow/forking_workflow.md), +When a GitLab user [forks a project](../user/project/repository/forking_workflow.md), GitLab creates a new Project with an associated Git repository that is a copy of the original project at the time of the fork. If a large project gets forked often, this can lead to a quick increase in Git repository diff --git a/doc/development/gitaly.md b/doc/development/gitaly.md index 64f283f69d9..7d3c2b8fdf8 100644 --- a/doc/development/gitaly.md +++ b/doc/development/gitaly.md @@ -166,12 +166,11 @@ end Normally, GitLab CE/EE tests use a local clone of Gitaly in `tmp/tests/gitaly` pinned at the version specified in -`GITALY_SERVER_VERSION`. The `GITALY_SERVER_VERSION` file supports -`=my-branch` syntax to use a custom branch in <https://gitlab.com/gitlab-org/gitaly>. If +`GITALY_SERVER_VERSION`. The `GITALY_SERVER_VERSION` file supports also +branches and SHA to use a custom commit in <https://gitlab.com/gitlab-org/gitaly>. If you want to run tests locally against a modified version of Gitaly you can replace `tmp/tests/gitaly` with a symlink. This is much faster -because the `=my-branch` syntax forces a Gitaly re-install each time -you run `rspec`. +because if will avoid a Gitaly re-install each time you run `rspec`. ```shell rm -rf tmp/tests/gitaly diff --git a/doc/development/gotchas.md b/doc/development/gotchas.md index c7eed880554..da27ae9110b 100644 --- a/doc/development/gotchas.md +++ b/doc/development/gotchas.md @@ -168,3 +168,73 @@ in an initializer._ ### Further reading - Stack Overflow: [Why you should not write inline JavaScript](https://softwareengineering.stackexchange.com/questions/86589/why-should-i-avoid-inline-scripting) + +## Auto loading + +Rails auto-loading on `development` differs from the load policy in the `production` environment. +In development mode, `config.eager_load` is set to `false`, which means classes +are loaded as needed. With the classic Rails autoloader, it is known that this can lead to +[Rails resolving the wrong class](https://guides.rubyonrails.org/v5.2/autoloading_and_reloading_constants.html#when-constants-aren-t-missed-relative-references) +if the class name is ambiguous. This can be fixed by specifying the complete namespace to the class. + +### Error prone example + +```ruby +# app/controllers/application_controller.rb +class ApplicationController < ActionController::Base + ... +end + +# app/controllers/projects/application_controller.rb +class Projects::ApplicationController < ApplicationController + ... + private + + def project + ... + end +end + +# app/controllers/projects/submodule/some_controller.rb +module Projects + module Submodule + class SomeController < ApplicationController + def index + @some_id = project.id + end + end + end +end +``` + +In this case, if for any reason the top level `ApplicationController` +is loaded but `Projects::ApplicationController` is not, `ApplicationController` +would be resolved to `::ApplicationController` and then the `project` method will +be undefined and we will get an error. + +#### Solution + +```ruby +# app/controllers/projects/submodule/some_controller.rb +module Projects + module Submodule + class SomeController < Projects::ApplicationController + def index + @some_id = project.id + end + end + end +end +``` + +By specifying `Projects::`, we tell Rails exactly what class we are referring +to and we would avoid the issue. + +NOTE: **Note:** +This problem will disappear as soon as we upgrade to Rails 6 and use the Zeitwerk autoloader. + +### Further reading + +- Rails Guides: [Autoloading and Reloading Constants (Classic Mode)](https://guides.rubyonrails.org/autoloading_and_reloading_constants_classic_mode.html) +- Ruby Constant lookup: [Everything you ever wanted to know about constant lookup in Ruby](http://cirw.in/blog/constant-lookup) +- Rails 6 and Zeitwerk autoloader: [Understanding Zeitwerk in Rails 6](https://medium.com/cedarcode/understanding-zeitwerk-in-rails-6-f168a9f09a1f) diff --git a/doc/development/i18n/externalization.md b/doc/development/i18n/externalization.md index 2f067ede70f..c8960ac0f61 100644 --- a/doc/development/i18n/externalization.md +++ b/doc/development/i18n/externalization.md @@ -326,9 +326,8 @@ file in. Once the changes are on master, they will be picked up by [Crowdin](https://translate.gitlab.com) and be presented for translation. -We don't need to check in any changes to the -`locale/[language]/gitlab.po` files. Those will be updated in a [when -translations from Crowdin are merged](merging_translations.md). +We don't need to check in any changes to the `locale/[language]/gitlab.po` files. +They are updated automatically when [translations from Crowdin are merged](merging_translations.md). If there are merge conflicts in the `gitlab.pot` file, you can delete the file and regenerate it using the same command. diff --git a/doc/development/i18n/merging_translations.md b/doc/development/i18n/merging_translations.md index f5953cc5f6c..c773b187cc1 100644 --- a/doc/development/i18n/merging_translations.md +++ b/doc/development/i18n/merging_translations.md @@ -1,13 +1,12 @@ # Merging translations from Crowdin -Crowdin automatically syncs the `gitlab.pot` file presenting newly -added translations to the community of translators. +Crowdin automatically syncs the `gitlab.pot` file with the Crowdin service, presenting +newly added externalized strings to the community of translators. -At the same time, it creates a merge request to merge all newly added -& approved translations. Find the [merge request created by -`gitlab-crowdin-bot`](https://gitlab.com/gitlab-org/gitlab/merge_requests?scope=all&utf8=%E2%9C%93&state=opened&author_username=gitlab-crowdin-bot) -to see new and merged merge requests. They are created in EE and need -to be ported to CE manually. +[GitLab Crowdin Bot](https://gitlab.com/gitlab-crowdin-bot) also creates merge requests +to take newly approved translation submissions and merge them into the `locale/<language>/gitlab.po` +files. Check the [merge requests created by `gitlab-crowdin-bot`](https://gitlab.com/gitlab-org/gitlab/merge_requests?scope=all&utf8=%E2%9C%93&state=opened&author_username=gitlab-crowdin-bot) +to see new and merged merge requests. ## Validation @@ -21,7 +20,7 @@ doesn't do. Create a new pipeline at `https://gitlab.com/gitlab-org/gitlab/pipel If there are validation errors, the easiest solution is to disapprove the offending string in Crowdin, leaving a comment with what is required to fix the offense. There is an -[issue](https://gitlab.com/gitlab-org/gitlab-foss/issues/49208) +[issue](https://gitlab.com/gitlab-org/gitlab/issues/23256) suggesting to automate this process. Disapproving will exclude the invalid translation, the merge request will be updated within a few minutes. @@ -32,19 +31,16 @@ clicking `Pause sync` on the [Crowdin integration settings page](https://translate.gitlab.com/project/gitlab-ee/settings#integration). When all failures are resolved, the translations need to be double -checked once more as discussed in [confidential issue](../../user/project/issues/confidential_issues.md) `https://gitlab.com/gitlab-org/gitlab-foss/issues/37850`. +checked once more as discussed in [confidential issue](../../user/project/issues/confidential_issues.md) `https://gitlab.com/gitlab-org/gitlab/issues/19485`. ## Merging translations When all translations are found good and pipelines pass the -translations can be merged into the master branch. After that is done, -create a new merge request cherry-picking the translations from EE to -CE. When merging the translations, make sure to check the `Remove -source branch` checkbox, so Crowdin recreates the `master-i18n` from -master after the new translation was merged. - -We are discussing automating this entire process -[here](https://gitlab.com/gitlab-org/gitlab-foss/issues/39309). +translations can be merged into the master branch. When merging the translations, +make sure to check the **Remove source branch** checkbox, so Crowdin recreates the +`master-i18n` from master after the new translation was merged. + +We are discussing [automating this entire process](https://gitlab.com/gitlab-org/gitlab/issues/19896). ## Recreate the merge request diff --git a/doc/development/i18n/translation.md b/doc/development/i18n/translation.md index c1a6fd8983c..50a417e9996 100644 --- a/doc/development/i18n/translation.md +++ b/doc/development/i18n/translation.md @@ -83,7 +83,7 @@ Therefore "create a new user" would translate into "Benutzer(in) anlegen". ### Updating the glossary To propose additions to the glossary please -[open an issue](https://gitlab.com/gitlab-org/gitlab-foss/issues). +[open an issue](https://gitlab.com/gitlab-org/gitlab/issues?scope=all&utf8=✓&state=all&label_name[]=Category%3AInternationalization). ## French Translation Guidelines diff --git a/doc/development/kubernetes.md b/doc/development/kubernetes.md index 82aa02ac75d..a0dd97f2a1c 100644 --- a/doc/development/kubernetes.md +++ b/doc/development/kubernetes.md @@ -74,6 +74,50 @@ We have some Webmock stubs in [`KubernetesHelpers`](https://gitlab.com/gitlab-org/gitlab/blob/master/spec/support/helpers/kubernetes_helpers.rb) which can help with mocking out calls to Kubernetes API in your tests. +### Amazon EKS integration + +This section outlines the process for allowing a GitLab instance to create EKS clusters. + +The following prerequisites are required: + +A `Customer` AWS account. This is the account in which the +EKS cluster will be created. The following resources must be present: + +- A provisioning role that has permissions to create the cluster + and associated resources. It must list the `GitLab` AWS account + as a trusted entity. +- A VPC, management role, security group, and subnets for use by the cluster. + +A `GitLab` AWS account. This is the account which performs +the provisioning actions. The following resources must be present: + +- A service account with permissions to assume the provisioning + role in the `Customer` account above. +- Credentials for this service account configured in GitLab via + the `kubernetes` section of `gitlab.yml`. + +The process for creating a cluster is as follows: + +1. Using the `:provision_role_external_id`, GitLab assumes the role provided + by `:provision_role_arn` and stores a set of temporary credentials on the + provider record. By default these credentials are valid for one hour. +1. A CloudFormation stack is created, based on the + [`AWS CloudFormation EKS template`](https://gitlab.com/gitlab-org/gitlab/blob/master/vendor/aws/cloudformation/eks_cluster.yaml). + This triggers creation of all resources required for an EKS cluster. +1. GitLab polls the status of the stack until all resources are ready, + which takes somewhere between 10 and 15 minutes in most cases. +1. When the stack is ready, GitLab stores the cluster details and generates + another set of temporary credentials, this time to allow connecting to + the cluster via Kubeclient. These credentials are valid for one minute. +1. GitLab configures the worker nodes so that they are able to authenticate + to the cluster, and creates a service account for itself for future operations. +1. Credentials that are no longer required are removed. This deletes the following + attributes: + + - `access_key_id` + - `secret_access_key` + - `session_token` + ## Security ### SSRF @@ -123,3 +167,10 @@ they are written: ```bash kubectl logs <pod_name> --follow -n gitlab-managed-apps ``` + +## GitLab Managed Apps + +GitLab provides [GitLab Managed Apps](../user/clusters/applications.html), a one-click install for various applications which can be added directly to your configured cluster. + +**<i class="fa fa-youtube-play youtube" aria-hidden="true"></i> +For an overview of how to add a new GitLab-mananged app, see [How to add GitLab-managed-apps to Kubernetes integration](https://youtu.be/mKm-jkranEk).** diff --git a/doc/development/lfs.md b/doc/development/lfs.md index cb4c2d8967b..9139bbaca0e 100644 --- a/doc/development/lfs.md +++ b/doc/development/lfs.md @@ -5,7 +5,7 @@ In April 2019, Francisco Javier López hosted a [Deep Dive] on GitLab's [Git LFS] implementation to share his domain specific knowledge with anyone who may work in this part of the code base in the future. You can find the [recording on YouTube], and the slides on [Google Slides] and in [PDF]. Everything covered in this deep dive was accurate as of GitLab 11.10, and while specific details may have changed since then, it should still serve as a good introduction. [Deep Dive]: https://gitlab.com/gitlab-org/create-stage/issues/1 -[Git LFS]: ../workflow/lfs/manage_large_binaries_with_git_lfs.html +[Git LFS]: ../administration/lfs/manage_large_binaries_with_git_lfs.md [recording on YouTube]: https://www.youtube.com/watch?v=Yyxwcksr0Qc [Google Slides]: https://docs.google.com/presentation/d/1E-aw6-z0rYd0346YhIWE7E9A65zISL9iIMAOq2zaw9E/edit [PDF]: https://gitlab.com/gitlab-org/create-stage/uploads/07a89257a140db067bdfb484aecd35e1/Git_LFS_Deep_Dive__Create_.pdf diff --git a/doc/development/merge_request_performance_guidelines.md b/doc/development/merge_request_performance_guidelines.md index 4456e5e6d18..2e80e813a4b 100644 --- a/doc/development/merge_request_performance_guidelines.md +++ b/doc/development/merge_request_performance_guidelines.md @@ -1,7 +1,9 @@ # 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 **must** adhere to the guidelines outlined in this +_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. @@ -12,6 +14,19 @@ the following guides: - [Performance Guidelines](performance.md) - [What requires downtime?](what_requires_downtime.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, labelled accordingly and linked +to original issue and epic. + ## Impact Analysis **Summary:** think about the impact your merge request may have on performance @@ -44,6 +59,64 @@ 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 how the new feature is going to be used. +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 that will be processed by the merge request 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 going to be supported? +1. How long it will take for big repositories like Linux kernel? +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 is better to degrade + the service for a single user instead of all users. + +## Query plans and database structure + +The query plan can answer the questions whether we need additional +indexes, or whether we perform expensive filtering (i.e. using sequential scans). + +Each query plan should be run against substantional size of data set. +For example if you look for issues with specific conditions, +you should consider validating the query against +a small number (a few hundred) and a big number (100_000) of issues. +See how the query will behave if the result will be 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 is unlikely +that such big data set will be used, it is still plausible that one +of our customers will have the problem with the feature. + +Understanding ahead of time how it is going to behave at scale even if we accept it, +is the desired outcome. We should always have a plan or understanding what it takes +to optimise feature to the magnitude of higher usage patterns. + +Every database structure should be optimised and sometimes even over-described +to be prepared to be easily extended. The hardest part after some point is +data migration. Migrating millions of rows will always be troublesome and +can have negative impact on 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](https://docs.gitlab.com/ee/development/database_review.html#how-to-prepare-the-merge-request-for-a-database-review). + ## Query Counts **Summary:** a merge request **should not** increase the number of executed SQL @@ -172,3 +245,107 @@ Caching data per transaction can be done using `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 do not 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 is very inefficient to calculate amount of objects that pass the filtering, + this operation usually can take seconds, and can time out, +1. It is very inefficent 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. + +## Badge counters + +Counters should always be truncated. It means that we do not 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 is 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 is desired that badge counters are loaded asynchronously. +This can speed up the initial page load and give a better user experience overall. + +## Application/misuse limits + +Every new feature should have safe usage quotas introduced. +The quota should be optimised to a level that we consider the feature to +be performant and usable for the user, but **not limiting**. + +**We want the features to be fully usable for the users.** +**However, we want to ensure that the feature will continue to perform well if used at its limit** +**and it will not cause availability issues.** + +Consider that it is always better to start with some kind of limitation, +instead of later introducing a breaking change that would result in some +workflows breaking. + +The intent is to provide a safe usage pattern for the feature, +as our implementation decisions are optimised for the given data set. +Our feature limits should reflect the optimisations that we introduced. + +The intent of quotas could be different: + +1. We want to provide higher quotas for higher tiers of features: + we want to provide on GitLab.com more capabilities for different tiers, +1. We want to prevent misuse of the feature: someone accidentially creates + 10000 deploy tokens, because of a broken API script, +1. We want to prevent abuse of the feature: someone purposely creates + a 10000 pipelines to take advantage of the system. + +Examples: + +1. Pipeline Schedules: It is very unlikely that user will want to create + more than 50 schedules. + In such cases it is rather expected that this is either misuse + or abuse of the feature. Lack of the upper limit can result + in service degredation as the system will try to process all schedules + assigned the the project. + +1. GitLab CI includes: We started with the limit of maximum of 50 nested includes. + We understood that performance of the feature was acceptable at that level. + We received a request from the community that the limit is too small. + We had a time to understand the customer requirement, and implement an additional + fail-safe mechanism (time-based one) to increase the limit 100, and if needed increase it + further without negative impact on availability of the feature and GitLab. + +## 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://docs.gitlab.com/ee/development/feature_flags/process.html#feature-flags-in-gitlab-development). diff --git a/doc/development/migration_style_guide.md b/doc/development/migration_style_guide.md index 20d705136b2..32c4313a1ed 100644 --- a/doc/development/migration_style_guide.md +++ b/doc/development/migration_style_guide.md @@ -211,7 +211,7 @@ class MyMigration < ActiveRecord::Migration[4.2] end def down - remove_index :table, :column if index_exists?(:table, :column) + remove_concurrent_index :table, :column end end ``` diff --git a/doc/development/packages.md b/doc/development/packages.md index 2474392db62..6d4a9ea9f41 100644 --- a/doc/development/packages.md +++ b/doc/development/packages.md @@ -4,12 +4,13 @@ This document will guide you through adding another [package management system]( See already supported package types in [Packages documentation](../administration/packages/index.md) -Since GitLab packages' UI is pretty generic, it is possible to add new +Since GitLab packages' UI is pretty generic, it is possible to add basic new package system support by solely backend changes. This guide is superficial and does not cover the way the code should be written. However, you can find a good example by looking at existing merge requests with Maven and NPM support: - [NPM registry support](https://gitlab.com/gitlab-org/gitlab/merge_requests/8673). +- [Conan repository](https://gitlab.com/gitlab-org/gitlab/issues/8248). - [Maven repository](https://gitlab.com/gitlab-org/gitlab/merge_requests/6607). - [Instance level endpoint for Maven repository](https://gitlab.com/gitlab-org/gitlab/merge_requests/8757) @@ -34,7 +35,7 @@ endpoints like: - GET package file content. - PUT upload package. -Since the packages belong to a project, it's expected to have project-level endpoint +Since the packages belong to a project, it's expected to have project-level endpoint (remote) for uploading and downloading them. For example: ``` @@ -44,9 +45,48 @@ PUT https://gitlab.com/api/v4/projects/<your_project_id>/packages/npm/ Group-level and instance-level endpoints are good to have but are optional. -NOTE: **Note:** -To avoid name conflict for instance-level endpoints we use -[the package naming convention](../user/packages/npm_registry/index.md#package-naming-convention) +## Naming conventions + +To avoid name conflict for instance-level endpoints you will need to define a package naming convention +that gives a way to identify the project that the package belongs to. This generally involves using the project +id or full project path in the package name. See +[Conan's naming convention](../user/packages/conan_repository/index.md#package-recipe-naming-convention) as an example. + +For group and project-level endpoints, naming can be less constrained, and it will be up to the group and project +members to be certain that there is no conflict between two package names, however the system should prevent +a user from reusing an existing name within a given scope. + +Otherwise, naming should follow the package manager's naming conventions and include a validation in the `package.md` +model for that package type. + +## File uploads + +File uploads should be handled by GitLab workhorse using object accelerated uploads. What this means is that +the workhorse proxy that checks all incoming requests to GitLab will intercept the upload request, +upload the file, and forward a request to the main GitLab codebase only containing the metadata +and file location rather than the file itself. An overview of this process can be found in the +[development documentation](uploads.md#workhorse-object-storage-acceleration). + +In terms of code, this means a route will need to be added to the +[gitlab-workhorse project](https://gitlab.com/gitlab-org/gitlab-workhorse) for each level of remote being added +(instance, group, project). [This merge request](https://gitlab.com/gitlab-org/gitlab-workhorse/merge_requests/412/diffs) +demonstrates adding an instance-level endpoint for Conan to workhorse. You can also see the Maven project level endpoint +implemented in the same file. + +Once the route has been added, you will need to add an additional `/authorize` version of the upload endpoint to your API file. +[Here is an example](https://gitlab.com/gitlab-org/gitlab/blob/398fef1ca26ae2b2c3dc89750f6b20455a1e5507/ee/lib/api/maven_packages.rb#L164) +of the additional endpoint added for Maven. The `/authorize` endpoint verifies and authorizes the request from workhorse, +then the normal upload endpoint is implemented below, consuming the metadata that workhorse provides in order to +create the package record. Workhorse provides a variety of file metadata such as type, size, and different checksum formats. + +For testing purposes, you may want to [enable object storage](https://gitlab.com/gitlab-org/gitlab-development-kit/blob/master/doc/howto/object_storage.md) +in your local development environment. + +## Services and finders + +Logic for performing tasks such as creating package or package file records or finding packages should not live +within the API file, but should live in services and finders. Existing services and finders should be used or +extended when possible to keep the common package logic grouped as much as possible. ## Configuration @@ -56,7 +96,7 @@ to add anything there. Packages can be configured to use object storage, therefore your code must support it. -## Database +## Database and handling metadata The current database model allows you to store a name and a version for each package. Every time you upload a new package, you can either create a new record of `Package` @@ -65,4 +105,58 @@ information like the file `name`, `side`, `sha1`, etc. If there is specific data necessary to be stored for only one package system support, consider creating a separate metadata model. See `packages_maven_metadata` table -and `Packages::MavenMetadatum` model as example for package specific data. +and `Packages::MavenMetadatum` model as an example for package specific data, and `packages_conan_file_metadata` table +and `Packages::ConanFileMetadatum` model as an example for package file specific data. + +If there is package specific behavior for a given package manager, add those methods to the metadata models and +delegate from the package model. + +Note that the existing package UI only displays information within the `packages_packages` and `packages_package_files` +tables. If the data stored in the metadata tables need to be displayed, a ~frontend change will be required. + +## Authorization + +There are project and group level permissions for `read_package`, `create_package`, and `destroy_package`. Each +endpoint should +[authorize the requesting user](https://gitlab.com/gitlab-org/gitlab/blob/398fef1ca26ae2b2c3dc89750f6b20455a1e5507/ee/lib/api/conan_packages.rb#L84) +against the project or group before continuing. + +## Keep iterations small + +When implementing a new package manager, it is easy to end up creating one large merge request containing all of the +necessary endpoints and services necessary to support basic usage. If this is the case, consider putting the +API endpoints behind a [feature flag](feature_flags/development.md) and +submitting each endpoint or behavior (download, upload, etc) in different merge requests to shorten the review +process. + +### Potential MRs for any given package system + +#### MVC MRs + +These changes represent all that is needed to deliver a minimally usable package management system. + +1. Empty file structure (api file, base service for this package) +1. Authentication system for 'logging in' to the package manager +1. Identify metadata and create applicable tables +1. Workhorse route for [object storage accelerated uploads](uploads.md#workhorse-object-storage-acceleration) +1. Endpoints required for upload/publish +1. Endpoints required for install/download +1. Endpoints required for remove/delete + +#### Possible post-MVC MRs + +These updates are not essential to be able to publish and consume packages, but may be desired as the system is +released for general use. + +1. Endpoints required for search +1. Front end updates to display additional package information and metadata +1. Limits on file sizes +1. Tracking for metrics + +## Exceptions + +This documentation is just guidelines on how to implement a package manager to match the existing structure and logic +already present within GitLab. While the structure is intended to be extendable and flexible enough to allow for +any given package manager, if there is good reason to stray due to the constraints or needs of a given package +manager, then it should be raised and discussed within the implementation issue or merge request to work towards +the most efficient outcome. diff --git a/doc/development/pipelines.md b/doc/development/pipelines.md index 5954de03db4..764bd68000d 100644 --- a/doc/development/pipelines.md +++ b/doc/development/pipelines.md @@ -15,6 +15,8 @@ as much as possible. The current stages are: +- `sync`: This stage is used to synchronize changes from gitlab-org/gitlab to + gitlab-org/gitlab-foss. - `prepare`: This stage includes jobs that prepare artifacts that are needed by jobs in subsequent stages. - `quick-test`: This stage includes test jobs that should run first and fail the @@ -27,7 +29,6 @@ The current stages are: - `review`: This stage includes jobs that deploy the GitLab and Docs Review Apps. - `qa`: This stage includes jobs that perform QA tasks against the Review App that is deployed in the previous stage. -- `notification`: This stage includes jobs that sends notifications about pipeline status. - `post-test`: This stage includes jobs that build reports or gather data from the previous stages' jobs (e.g. coverage, Knapsack metadata etc.). - `pages`: This stage includes a job that deploys the various reports as @@ -38,7 +39,8 @@ The current stages are: ## Default image The default image is currently -`gitlab.com/gitlab-org/gitlab-build-images:ruby-2.6.3-golang-1.11-git-2.22-chrome-73.0-node-12.x-yarn-1.16-postgresql-9.6-graphicsmagick-1.3.33`. +`registry.gitlab.com/gitlab-org/gitlab-build-images:ruby-2.6.3-golang-1.11-git-2.22-chrome-73.0-node-12.x-yarn-1.16-postgresql-9.6-graphicsmagick-1.3.33`. + It includes Ruby 2.6.3, Go 1.11, Git 2.22, Chrome 73, Node 12, Yarn 1.16, PostgreSQL 9.6, and Graphics Magick 1.3.33. @@ -48,24 +50,13 @@ project, which is push-mirrored to <https://dev.gitlab.org/gitlab/gitlab-build-i for redundancy. The current version of the build images can be found in the -["Used by GitLab CE/EE section"](https://gitlab.com/gitlab-org/gitlab-build-images/blob/master/.gitlab-ci.yml). +["Used by GitLab section"](https://gitlab.com/gitlab-org/gitlab-build-images/blob/master/.gitlab-ci.yml). ## Default variables In addition to the [predefined variables](../ci/variables/predefined_variables.md), -each pipeline includes the following [variables](../ci/variables/README.md): - -- `RAILS_ENV: "test"` -- `NODE_ENV: "test"` -- `SIMPLECOV: "true"` -- `GIT_DEPTH: "50"` -- `GIT_SUBMODULE_STRATEGY: "none"` -- `GET_SOURCES_ATTEMPTS: "3"` -- `KNAPSACK_RSPEC_SUITE_REPORT_PATH: knapsack/${CI_PROJECT_NAME}/rspec_report-master.json` -- `FLAKY_RSPEC_SUITE_REPORT_PATH: rspec_flaky/report-suite.json` -- `BUILD_ASSETS_IMAGE: "false"` -- `ES_JAVA_OPTS: "-Xms256m -Xmx256m"` -- `ELASTIC_URL: "http://elastic:changeme@docker.elastic.co-elasticsearch-elasticsearch:9200"` +each pipeline includes default variables defined in +<https://gitlab.com/gitlab-org/gitlab/blob/master/.gitlab-ci.yml>. ## Common job definitions @@ -85,22 +76,35 @@ These common definitions are: Ruby/Rails and frontend tasks. - `.default-only`: Restricts the cases where a job is created. This currently includes `master`, `/^[\d-]+-stable(-ee)?$/` (stable branches), - `/^\d+-\d+-auto-deploy-\d+$/` (security branches), `merge_requests`, `tags`. + `/^\d+-\d+-auto-deploy-\d+$/` (auto-deploy branches), `/^security\//` (security branches), `merge_requests`, `tags`. Note that jobs won't be created for branches with this default configuration. -- `.only-review`: Only creates a job for the `gitlab-org` namespace and if - Kubernetes integration is available. Also, prevents a job from being created - for `master` and auto-deploy branches. -- `.only-review-schedules`: Same as `.only-review` but also restrict a job to - only run for [schedules](../user/project/pipelines/schedules.md). -- `.only-canonical-schedules`: Only creates a job for scheduled pipelines in - the `gitlab-org/gitlab` and `gitlab-org/gitlab-foss` projects +- `.only:variables-canonical-dot-com`: Only creates a job if the project is + located under <https://gitlab.com/gitlab-org>. +- `.only:variables_refs-canonical-dot-com-schedules`: Same as + `.only:variables-canonical-dot-com` but add the condition that pipeline is scheduled. +- `.except:refs-deploy`: Don't create a job if the `ref` is an auto-deploy branch. +- `.except:refs-master-tags-stable-deploy`: Don't create a job if the `ref` is one of: + - `master` + - a tag + - a stable branch + - an auto-deploy branch +- `.only:kubernetes`: Only creates a job if a Kubernetes integration is enabled + on the project. +- `.only-review`: This extends from: + - `.only:variables-canonical-dot-com` + - `.only:kubernetes` + - `.except:refs-master-tags-stable-deploy` +- `.only-review-schedules`: This extends from: + - `.only:variables_refs-canonical-dot-com-schedules` + - `.only:kubernetes` + - `.except:refs-deploy` - `.use-pg9`: Allows a job to use the `postgres:9.6` and `redis:alpine` services. - `.use-pg10`: Allows a job to use the `postgres:10.9` and `redis:alpine` services. - `.use-pg9-ee`: Same as `.use-pg9` but also use the `docker.elastic.co/elasticsearch/elasticsearch:5.6.12` services. - `.use-pg10-ee`: Same as `.use-pg10` but also use the `docker.elastic.co/elasticsearch/elasticsearch:5.6.12` services. -- `.only-ee`: Only creates a job for the `gitlab` project. +- `.only-ee`: Only creates a job for the `gitlab` or `gitlab-ee` project. - `.only-ee-as-if-foss`: Same as `.only-ee` but simulate the FOSS project by setting the `FOSS_ONLY='1'` environment variable. @@ -111,11 +115,13 @@ the cases where it should be created [based on the changes](../ci/yaml/README.md#onlychangesexceptchanges) from a commit or MR by extending from the following CI definitions: -- `.only-code-changes`: Allows a job to only be created upon code-related changes. -- `.only-qa-changes`: Allows a job to only be created upon QA-related changes. -- `.only-docs-changes`: Allows a job to only be created upon docs-related changes. -- `.only-code-qa-changes`: Allows a job to only be created upon code-related or QA-related changes. -- `.only-graphql-changes`: Allows a job to only be created upon graphql-related changes. +- `.only:changes-code`: Allows a job to only be created upon code-related changes. +- `.only:changes-qa`: Allows a job to only be created upon QA-related changes. +- `.only:changes-docs`: Allows a job to only be created upon docs-related changes. +- `.only:changes-graphql`: Allows a job to only be created upon GraphQL-related changes. +- `.only:changes-code-backstage`: Allows a job to only be created upon code-related or backstage-related (e.g. Danger, RuboCop, specs) changes. +- `.only:changes-code-qa`: Allows a job to only be created upon code-related or QA-related changes. +- `.only:changes-code-backstage-qa`: Allows a job to only be created upon code-related, backstage-related (e.g. Danger, RuboCop, specs) or QA-related changes. **See <https://gitlab.com/gitlab-org/gitlab/blob/master/.gitlab/ci/global.gitlab-ci.yml> for the list of exact patterns.** @@ -203,11 +209,6 @@ subgraph "`qa` stage" dast -.-> |needs and depends on| G; end -subgraph "`notification` stage" - NOTIFICATION1["schedule:package-and-qa:notify-success<br>(on_success)"] -.-> |needs| P; - NOTIFICATION2["schedule:package-and-qa:notify-failure<br>(on_failure)"] -.-> |needs| P; - end - subgraph "`post-test` stage" M end diff --git a/doc/development/policies.md b/doc/development/policies.md index 833b0acb13e..8e5ef6e57c0 100644 --- a/doc/development/policies.md +++ b/doc/development/policies.md @@ -157,3 +157,18 @@ end ``` will include all rules from `ProjectPolicy`. The delegated conditions will be evaluated with the correct delegated subject, and will be sorted along with the regular rules in the policy. Note that only the relevant rules for a particular ability will actually be considered. + +## Specifying Policy Class + +You can also override the Policy used for a given subject: + +```ruby +class Foo + + def self.declarative_policy_class + 'SomeOtherPolicy' + end +end +``` + +This will use & check permissions on the `SomeOtherPolicy` class rather than the usual calculated `FooPolicy` class. diff --git a/doc/development/profiling.md b/doc/development/profiling.md index 04897e770f8..18683fa10f8 100644 --- a/doc/development/profiling.md +++ b/doc/development/profiling.md @@ -42,6 +42,10 @@ Passing a `logger:` keyword argument to `Gitlab::Profiler.profile` will send ActiveRecord and ActionController log output to that logger. Further options are documented with the method source. +```ruby +Gitlab::Profiler.profile('/gitlab-org/gitlab-test', user: User.first, logger: Logger.new(STDOUT)) +``` + There is also a RubyProf printer available: `Gitlab::Profiler::TotalTimeFlatPrinter`. This acts like `RubyProf::FlatPrinter`, but its `min_percent` option works on the method's diff --git a/doc/development/rake_tasks.md b/doc/development/rake_tasks.md index a6d3c008686..369806d462b 100644 --- a/doc/development/rake_tasks.md +++ b/doc/development/rake_tasks.md @@ -12,6 +12,14 @@ The `setup` task is an alias for `gitlab:setup`. This tasks calls `db:reset` to create the database, and calls `db:seed_fu` to seed the database. Note: `db:setup` calls `db:seed` but this does nothing. +### Env variables + +**MASS_INSERT**: Create millions of users (2m), projects (5m) and its +relations. It's highly recommended to run the seed with it to catch slow queries +while developing. Expect the process to take up to 20 extra minutes. + +**LARGE_PROJECTS**: Create large projects (through import) from a predefined set of urls. + ### Seeding issues for all or a given project You can seed issues for all or a given project with the `gitlab:seed:issues` @@ -221,7 +229,7 @@ bundle exec rake db:obsolete_ignored_columns Feel free to remove their definitions from their `ignored_columns` definitions. -## Update GraphQL Documentation +## Update GraphQL Documentation and Schema definitions To generate GraphQL documentation based on the GitLab schema, run: @@ -243,3 +251,13 @@ The actual renderer is at `Gitlab::Graphql::Docs::Renderer`. `@parsed_schema` is an instance variable that the `graphql-docs` gem expects to have available. `Gitlab::Graphql::Docs::Helper` defines the `object` method we currently use. This is also where you should implement any new methods for new types you'd like to display. + +### Update machine-readable schema files + +To generate GraphQL schema files based on the GitLab schema, run: + +```shell +bundle exec rake gitlab:graphql:schema:dump +``` + +This uses graphql-ruby's built-in rake tasks to generate files in both [IDL](https://www.prisma.io/blog/graphql-sdl-schema-definition-language-6755bcb9ce51) and JSON formats. diff --git a/doc/development/repository_mirroring.md b/doc/development/repository_mirroring.md index 8521d6fcd30..0a0c91821cf 100644 --- a/doc/development/repository_mirroring.md +++ b/doc/development/repository_mirroring.md @@ -5,6 +5,6 @@ In December 2018, Tiago Botelho hosted a [Deep Dive] on GitLab's [Pull Repository Mirroring functionality] to share his domain specific knowledge with anyone who may work in this part of the code base in the future. You can find the [recording on YouTube], and the slides in [PDF]. Everything covered in this deep dive was accurate as of GitLab 11.6, and while specific details may have changed since then, it should still serve as a good introduction. [Deep Dive]: https://gitlab.com/gitlab-org/create-stage/issues/1 -[Pull Repository Mirroring functionality]: ../workflow/repository_mirroring.md#pulling-from-a-remote-repository-starter +[Pull Repository Mirroring functionality]: ../user/project/repository/repository_mirroring.md#pulling-from-a-remote-repository-starter [recording on YouTube]: https://www.youtube.com/watch?v=sSZq0fpdY-Y [PDF]: https://gitlab.com/gitlab-org/create-stage/uploads/8693404888a941fd851f8a8ecdec9675/Gitlab_Create_-_Pull_Mirroring_Deep_Dive.pdf diff --git a/doc/development/sidekiq_style_guide.md b/doc/development/sidekiq_style_guide.md index d52a3e652e3..e433691c1ed 100644 --- a/doc/development/sidekiq_style_guide.md +++ b/doc/development/sidekiq_style_guide.md @@ -61,6 +61,168 @@ the extra jobs will take resources away from jobs from workers that were already there, if the resources available to the Sidekiq process handling the namespace are not adjusted appropriately. +## Latency Sensitive Jobs + +If a large number of background jobs get scheduled at once, queueing of jobs may +occur while jobs wait for a worker node to be become available. This is normal +and gives the system resilience by allowing it to gracefully handle spikes in +traffic. Some jobs, however, are more sensitive to latency than others. Examples +of these jobs include: + +1. A job which updates a merge request following a push to a branch. +1. A job which invalidates a cache of known branches for a project after a push + to the branch. +1. A job which recalculates the groups and projects a user can see after a + change in permissions. +1. A job which updates the status of a CI pipeline after a state change to a job + in the pipeline. + +When these jobs are delayed, the user may perceive the delay as a bug: for +example, they may push a branch and then attempt to create a merge request for +that branch, but be told in the UI that the branch does not exist. We deem these +jobs to be `latency_sensitive`. + +Extra effort is made to ensure that these jobs are started within a very short +period of time after being scheduled. However, in order to ensure throughput, +these jobs also have very strict execution duration requirements: + +1. The median job execution time should be less than 1 second. +1. 99% of jobs should complete within 10 seconds. + +If a worker cannot meet these expectations, then it cannot be treated as a +`latency_sensitive` worker: consider redesigning the worker, or splitting the +work between two different workers, one with `latency_sensitive` code that +executes quickly, and the other with non-`latency_sensitive`, which has no +execution latency requirements (but also has lower scheduling targets). + +This can be summed up in the following table: + +| **Latency Sensitivity** | **Queue Scheduling Target** | **Execution Latency Requirement** | +|-------------------------|-----------------------------|-------------------------------------| +| Not `latency_sensitive` | 1 minute | Maximum run time of 1 hour | +| `latency_sensitive` | 100 milliseconds | p50 of 1 second, p99 of 10 seconds | + +To mark a worker as being `latency_sensitive`, use the +`latency_sensitive_worker!` attribute, as shown in this example: + +```ruby +class LatencySensitiveWorker + include ApplicationWorker + + latency_sensitive_worker! + + # ... +end +``` + +## Jobs with External Dependencies + +Most background jobs in the GitLab application communicate with other GitLab +services, eg Postgres, Redis, Gitaly and Object Storage. These are considered +to be "internal" dependencies for a job. + +However, some jobs will be dependent on external services in order to complete +successfully. Some examples include: + +1. Jobs which call web-hooks configured by a user. +1. Jobs which deploy an application to a k8s cluster configured by a user. + +These jobs have "external dependencies". This is important for the operation of +the background processing cluster in several ways: + +1. Most external dependencies (such as web-hooks) do not provide SLOs, and + therefore we cannot guarantee the execution latencies on these jobs. Since we + cannot guarantee execution latency, we cannot ensure throughput and + therefore, in high-traffic environments, we need to ensure that jobs with + external dependencies are separated from `latency_sensitive` jobs, to ensure + throughput on those queues. +1. Errors in jobs with external dependencies have higher alerting thresholds as + there is a likelihood that the cause of the error is external. + +```ruby +class ExternalDependencyWorker + include ApplicationWorker + + # Declares that this worker depends on + # third-party, external services in order + # to complete successfully + worker_has_external_dependencies! + + # ... +end +``` + +NOTE: **Note:** Note that a job cannot be both latency sensitive and have +external dependencies. + +## CPU-bound and Memory-bound Workers + +Workers that are constrained by CPU or memory resource limitations should be +annotated with the `worker_resource_boundary` method. + +Most workers tend to spend most of their time blocked, wait on network responses +from other services such as Redis, Postgres and Gitaly. Since Sidekiq is a +multithreaded environment, these jobs can be scheduled with high concurrency. + +Some workers, however, spend large amounts of time _on-cpu_ running logic in +Ruby. Ruby MRI does not support true multithreading - it relies on the +[GIL](https://thoughtbot.com/blog/untangling-ruby-threads#the-global-interpreter-lock) +to greatly simplify application development by only allowing one section of Ruby +code in a process to run at a time, no matter how many cores the machine +hosting the process has. For IO bound workers, this is not a problem, since most +of the threads are blocked in underlying libraries (which are outside of the +GIL). + +If many threads are attempting to run Ruby code simultaneously, this will lead +to contention on the GIL which will have the affect of slowing down all +processes. + +In high-traffic environments, knowing that a worker is CPU-bound allows us to +run it on a different fleet with lower concurrency. This ensures optimal +performance. + +Likewise, if a worker uses large amounts of memory, we can run these on a +bespoke low concurrency, high memory fleet. + +Note that Memory-bound workers create heavy GC workloads, with pauses of +10-50ms. This will have an impact on the latency requirements for the +worker. For this reason, `memory` bound, `latency_sensitive` jobs are not +permitted and will fail CI. In general, `memory` bound workers are +discouraged, and alternative approaches to processing the work should be +considered. + +## Declaring a Job as CPU-bound + +This example shows how to declare a job as being CPU-bound. + +```ruby +class CPUIntensiveWorker + include ApplicationWorker + + # Declares that this worker will perform a lot of + # calculations on-CPU. + worker_resource_boundary :cpu + + # ... +end +``` + +## Determining whether a worker is CPU-bound + +We use the following approach to determine whether a worker is CPU-bound: + +- In the sidekiq structured JSON logs, aggregate the worker `duration` and + `cpu_s` fields. +- `duration` refers to the total job execution duration, in seconds +- `cpu_s` is derived from the + [`Process::CLOCK_THREAD_CPUTIME_ID`](https://www.rubydoc.info/stdlib/core/Process:clock_gettime) + counter, and is a measure of time spent by the job on-CPU. +- Divide `cpu_s` by `duration` to get the percentage time spend on-CPU. +- If this ratio exceeds 33%, the worker is considered CPU-bound and should be + annotated as such. +- Note that these values should not be used over small sample sizes, but + rather over fairly large aggregates. + ## Feature Categorization Each Sidekiq worker, or one of its ancestor classes, must declare a @@ -74,7 +236,7 @@ The declaration uses the `feature_category` class method, as shown below. class SomeScheduledTaskWorker include ApplicationWorker - # Declares that this feature is part of the + # Declares that this worker is part of the # `continuous_integration` feature category feature_category :continuous_integration @@ -88,11 +250,11 @@ source](https://gitlab.com/gitlab-com/www-gitlab-com/blob/master/data/stages.yml ### Updating `config/feature_categories.yml` -Occassionally new features will be added to GitLab stages. When this occurs, you +Occasionally new features will be added to GitLab stages. When this occurs, you can automatically update `config/feature_categories.yml` by running `scripts/update-feature-categories`. This script will fetch and parse [`stages.yml`](https://gitlab.com/gitlab-com/www-gitlab-com/blob/master/data/stages.yml) -and generare a new version of the file, which needs to be checked into source control. +and generate a new version of the file, which needs to be checked into source control. ### Excluding Sidekiq workers from feature categorization @@ -116,9 +278,63 @@ end Each Sidekiq worker must be tested using RSpec, just like any other class. These tests should be placed in `spec/workers`. -## Removing or renaming queues +## Sidekiq Compatibility across Updates + +Keep in mind that the arguments for a Sidekiq job are stored in a queue while it +is scheduled for execution. During a online update, this could lead to several +possible situations: + +1. An older version of the application publishes a job, which is executed by an + upgraded Sidekiq node. +1. A job is queued before an upgrade, but executed after an upgrade. +1. A job is queued by a node running the newer version of the application, but + executed on a node running an older version of the application. + +### Changing the arguments for a worker + +Jobs need to be backwards- and forwards-compatible between consecutive versions +of the application. + +This can be done by following this process: + +1. **Do not remove arguments from the `perform` function.**. Instead, use the + following approach + 1. Provide a default value (usually `nil`) and use a comment to mark the + argument as deprecated + 1. Stop using the argument in `perform_async`. + 1. Ignore the value in the worker class, but do not remove it until the next + major release. + +### Removing workers + +Try to avoid removing workers and their queues in minor and patch +releases. -Try to avoid renaming or removing workers and their queues in minor and patch releases. During online update instance can have pending jobs and removing the queue can lead to those jobs being stuck forever. If you can't write migration for those -Sidekiq jobs, please consider doing rename or remove queue in major release only. +Sidekiq jobs, please consider removing the worker in a major release only. + +### Renaming queues + +For the same reasons that removing workers is dangerous, care should be taken +when renaming queues. + +When renaming queues, use the `sidekiq_queue_migrate` helper migration method, +as show in this example: + +```ruby +class MigrateTheRenamedSidekiqQueue < ActiveRecord::Migration[5.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def up + sidekiq_queue_migrate 'old_queue_name', to: 'new_queue_name' + end + + def down + sidekiq_queue_migrate 'new_queue_name', to: 'old_queue_name' + end +end + +``` diff --git a/doc/development/testing_guide/best_practices.md b/doc/development/testing_guide/best_practices.md index 32e079f915c..fe3989474e6 100644 --- a/doc/development/testing_guide/best_practices.md +++ b/doc/development/testing_guide/best_practices.md @@ -44,6 +44,14 @@ bundle exec rspec bundle exec rspec spec/[path]/[to]/[spec].rb ``` +Use [guard](https://github.com/guard/guard) to continuously monitor for changes and only run matching tests: + +```sh +bundle exec guard +``` + +When using spring and guard together, use `SPRING=1 bundle exec guard` instead to make use of spring. + ### General guidelines - Use a single, top-level `describe ClassName` block. @@ -61,6 +69,7 @@ bundle exec rspec spec/[path]/[to]/[spec].rb - When using `evaluate_script("$('.js-foo').testSomething()")` (or `execute_script`) which acts on a given element, use a Capyabara matcher beforehand (e.g. `find('.js-foo')`) to ensure the element actually exists. - Use `focus: true` to isolate parts of the specs you want to run. +- Use [`:aggregate_failures`](https://relishapp.com/rspec/rspec-core/docs/expectation-framework-integration/aggregating-failures) when there is more than one expectation in a test. ### System / Feature tests @@ -356,10 +365,22 @@ However, if a spec makes direct Redis calls, it should mark itself with the `:clean_gitlab_redis_cache`, `:clean_gitlab_redis_shared_state` or `:clean_gitlab_redis_queues` traits as appropriate. -Sidekiq jobs are typically not run in specs, but this behaviour can be altered -in each spec through the use of `perform_enqueued_jobs` blocks. Any spec that -causes Sidekiq jobs to be pushed to Redis should use the `:sidekiq` trait, to -ensure that they are removed once the spec completes. +#### Background jobs / Sidekiq + +By default, Sidekiq jobs are enqueued into a jobs array and aren't processed. +If a test enqueues Sidekiq jobs and need them to be processed, the +`:sidekiq_inline` trait can be used. + +The `:sidekiq_might_not_need_inline` trait was added when [Sidekiq inline mode was +changed to fake mode](https://gitlab.com/gitlab-org/gitlab/merge_requests/15479) +to all the tests that needed Sidekiq to actually process jobs. Tests with +this trait should be either fixed to not rely on Sidekiq processing jobs, or their +`:sidekiq_might_not_need_inline` trait should be updated to `:sidekiq_inline` if +the processing of background jobs is needed/expected. + +NOTE: **Note:** +The usage of `perform_enqueued_jobs` is currently useless since our +workers aren't inheriting from `ApplicationJob` / `ActiveJob::Base`. #### Filesystem diff --git a/doc/development/testing_guide/end_to_end/best_practices.md b/doc/development/testing_guide/end_to_end/best_practices.md index 042879b47aa..e2a0d267ba1 100644 --- a/doc/development/testing_guide/end_to_end/best_practices.md +++ b/doc/development/testing_guide/end_to_end/best_practices.md @@ -65,3 +65,31 @@ This library [saves the screenshots in the RSpec's `after` hook](https://github. Given this fact, we should limit the use of `before(:all)` to only those operations where a screenshot is not necessary in case of failure and QA logs would be enough for debugging. + +## Ensure tests do not leave the browser logged in + +All QA tests expect to be able to log in at the start of the test. + +That's not possible if a test leaves the browser logged in when it finishes. Normally this isn't a problem because [Capybara resets the session after each test](https://github.com/teamcapybara/capybara/blob/9ebc5033282d40c73b0286e60217515fd1bb0b5d/lib/capybara/rspec.rb#L18). But Capybara does that in an `after` block, so when a test logs in in an `after(:context)` block, the browser returns to a logged in state *after* Capybara had logged it out. And so the next test will fail. + +For an example see: <https://gitlab.com/gitlab-org/gitlab/issues/34736> + +Ideally, any actions peformed in an `after(:context)` (or [`before(:context)`](#limit-the-use-of-beforeall-hook)) block would be performed via the API. But if it's necessary to do so via the UI (e.g., if API functionality doesn't exist), make sure to log out at the end of the block. + +```ruby +after(:all) do + login unless Page::Main::Menu.perform(&:signed_in?) + + # Do something while logged in + + Page::Main::Menu.perform(&:sign_out) +end +``` + +## Tag tests that require Administrator access + +We don't run tests that require Administrator access against our Production environments. + +When you add a new test that requires Administrator access, apply the RSpec metadata `:requires_admin` so that the test will not be included in the test suites executed against Production and other environments on which we don't want to run those tests. + +Note: When running tests locally or configuring a pipeline, the environment variable `QA_CAN_TEST_ADMIN_FEATURES` can be set to `false` to skip tests that have the `:requires_admin` tag. diff --git a/doc/development/testing_guide/end_to_end/feature_flags.md b/doc/development/testing_guide/end_to_end/feature_flags.md new file mode 100644 index 00000000000..bf1e70be9cb --- /dev/null +++ b/doc/development/testing_guide/end_to_end/feature_flags.md @@ -0,0 +1,27 @@ +# Testing with feature flags + +To run a specific test with a feature flag enabled you can use the `QA::Runtime::Feature` class to enabled and disable feature flags ([via the API](../../../api/features.md)). + +Note that administrator authorization is required to change feature flags. `QA::Runtime::Feature` will automatically authenticate as an administrator as long as you provide an appropriate access token via `GITLAB_QA_ADMIN_ACCESS_TOKEN` (recommended), or provide `GITLAB_ADMIN_USERNAME` and `GITLAB_ADMIN_PASSWORD`. + +```ruby +context "with feature flag enabled" do + before do + Runtime::Feature.enable('feature_flag_name') + end + + it "feature flag test" do + # Execute a test with a feature flag enabled + end + + after do + Runtime::Feature.disable('feature_flag_name') + end +end +``` + +## Running a scenario with a feature flag enabled + +It's also possible to run an entire scenario with a feature flag enabled, without having to edit existing tests or write new ones. + +Please see the [QA readme](https://gitlab.com/gitlab-org/gitlab/tree/master/qa#running-tests-with-a-feature-flag-enabled) for details. diff --git a/doc/development/testing_guide/end_to_end/flows.md b/doc/development/testing_guide/end_to_end/flows.md new file mode 100644 index 00000000000..fb1d82914aa --- /dev/null +++ b/doc/development/testing_guide/end_to_end/flows.md @@ -0,0 +1,56 @@ +# Flows in GitLab QA + +Flows are frequently used sequences of actions. They are a higher level +of abstraction than page objects. Flows can include multiple page objects, +or any other relevant code. + +For example, the sign in flow encapsulates two steps that are included +in every browser UI test. + +```ruby +# QA::Flow::Login + +def sign_in(as: nil) + Runtime::Browser.visit(:gitlab, Page::Main::Login) + Page::Main::Login.perform { |login| login.sign_in_using_credentials(user: as) } +end + +# When used in a test + +it 'performs a test after signing in as the default user' do + Flow::Login.sign_in + + # Perform the test +end +``` + +`QA::Flow::Login` provides an even more useful flow, allowing a test to easily switch users. + +```ruby +# QA::Flow::Login + +def while_signed_in(as: nil) + Page::Main::Menu.perform(&:sign_out_if_signed_in) + + sign_in(as: as) + + yield + + Page::Main::Menu.perform(&:sign_out) +end + +# When used in a test + +it 'performs a test as one user and verifies as another' do + user1 = Resource::User.fabricate_or_use(Runtime::Env.gitlab_qa_username_1, Runtime::Env.gitlab_qa_password_1) + user2 = Resource::User.fabricate_or_use(Runtime::Env.gitlab_qa_username_2, Runtime::Env.gitlab_qa_password_2) + + Flow::Login.while_signed_in(as: user1) do + # Perform some setup as user1 + end + + Flow::Login.sign_in(as: user2) + + # Perform the rest of the test as user2 +end +``` diff --git a/doc/development/testing_guide/end_to_end/index.md b/doc/development/testing_guide/end_to_end/index.md index a9fb4be284e..19885f5756f 100644 --- a/doc/development/testing_guide/end_to_end/index.md +++ b/doc/development/testing_guide/end_to_end/index.md @@ -130,6 +130,8 @@ Continued reading: - [Quick Start Guide](quick_start_guide.md) - [Style Guide](style_guide.md) - [Best Practices](best_practices.md) +- [Testing with feature flags](feature_flags.md) +- [Flows](flows.md) ## Where can I ask for help? diff --git a/doc/development/testing_guide/end_to_end/page_objects.md b/doc/development/testing_guide/end_to_end/page_objects.md index 28111c18378..554995fa2e2 100644 --- a/doc/development/testing_guide/end_to_end/page_objects.md +++ b/doc/development/testing_guide/end_to_end/page_objects.md @@ -167,6 +167,65 @@ There are two supported methods of defining elements within a view. Any existing `.qa-selector` class should be considered deprecated and we should prefer the `data-qa-selector` method of definition. +### Dynamic element selection + +> Introduced in GitLab 12.5 + +A common occurrence in automated testing is selecting a single "one-of-many" element. +In a list of several items, how do you differentiate what you are selecting on? +The most common workaround for this is via text matching. Instead, a better practice is +by matching on that specific element by a unique identifier, rather than by text. + +We got around this by adding the `data-qa-*` extensible selection mechanism. + +#### Examples + +**Example 1** + +Given the following Rails view (using GitLab Issues as an example): + +```haml +%ul.issues-list + - @issues.each do |issue| + %li.issue{data: { qa_selector: 'issue', qa_issue_title: issue.title } }= link_to issue +``` + +We can select on that specific issue by matching on the Rails model. + +```ruby +class Page::Project::Issues::Index < Page::Base + def has_issue?(issue) + has_element? :issue, issue_title: issue + end +end +``` + +In our test, we can validate that this particular issue exists. + +```ruby +describe 'Issue' do + it 'has an issue titled "hello"' do + Page::Project::Issues::Index.perform do |index| + expect(index).to have_issue('hello') + end + end +end +``` + +**Example 2** + +*By an index...* + +```haml +%ol + - @some_model.each_with_index do |model, idx| + %li.model{ data: { qa_selector: 'model', qa_index: idx } } +``` + +```ruby +expect(the_page).to have_element(:model, index: 1) #=> select on the first model that appears in the list +``` + ### Exceptions In some cases it might not be possible or worthwhile to add a selector. diff --git a/doc/development/testing_guide/flaky_tests.md b/doc/development/testing_guide/flaky_tests.md index 0823c2e02b8..3a96f8204fc 100644 --- a/doc/development/testing_guide/flaky_tests.md +++ b/doc/development/testing_guide/flaky_tests.md @@ -83,6 +83,7 @@ This was originally implemented in: <https://gitlab.com/gitlab-org/gitlab-foss/m - In JS tests, shifting elements can cause Capybara to misclick when the element moves at the exact time Capybara sends the click - [Dropdowns rendering upward or downward due to window size and scroll position](https://gitlab.com/gitlab-org/gitlab/merge_requests/17660) - [Lazy loaded images can cause Capybara to misclick](https://gitlab.com/gitlab-org/gitlab/merge_requests/18713) +- [Triggering JS events before the event handlers are set up](https://gitlab.com/gitlab-org/gitlab/merge_requests/18742) #### Capybara viewport size related issues diff --git a/doc/development/testing_guide/frontend_testing.md b/doc/development/testing_guide/frontend_testing.md index 314995ca9b3..236f175cee5 100644 --- a/doc/development/testing_guide/frontend_testing.md +++ b/doc/development/testing_guide/frontend_testing.md @@ -119,6 +119,50 @@ Global mocks introduce magic and can affect how modules are imported in your tes When in doubt, construct mocks in your test file using [`jest.mock()`](https://jestjs.io/docs/en/jest-object#jestmockmodulename-factory-options), [`jest.spyOn()`](https://jestjs.io/docs/en/jest-object#jestspyonobject-methodname), etc. +### Data-driven tests + +Similar to [RSpec's parameterized tests](best_practices.md#table-based--parameterized-tests), +Jest supports data-driven tests for: + +- Individual tests using [`test.each`](https://jestjs.io/docs/en/api#testeachtable-name-fn-timeout) (aliased to `it.each`). +- Groups of tests using [`describe.each`](https://jestjs.io/docs/en/api#describeeachtable-name-fn-timeout). + +These can be useful for reducing repetition within tests. Each option can take an array of +data values or a tagged template literal. + +For example: + +```javascript +// function to test +const icon = status => status ? 'pipeline-passed' : 'pipeline-failed' +const message = status => status ? 'pipeline-passed' : 'pipeline-failed' + +// test with array block +it.each([ + [false, 'pipeline-failed'], + [true, 'pipeline-passed'] +])('icon with %s will return %s', + (status, icon) => { + expect(renderPipeline(status)).toEqual(icon) + } +); + +// test suite with tagged template literal block +describe.each` + status | icon | message + ${false} | ${'pipeline-failed'} | ${'Pipeline failed - boo-urns'} + ${true} | ${'pipeline-passed'} | ${'Pipeline succeeded - win!'} +`('pipeline component', ({ status, icon, message }) => { + it(`returns icon ${icon} with status ${status}`, () => { + expect(icon(status)).toEqual(message) + }) + + it(`returns message ${message} with status ${status}`, () => { + expect(message(status)).toEqual(message) + }) +}); +``` + ## Karma test suite GitLab uses the [Karma][karma] test runner with [Jasmine] as its test @@ -457,6 +501,39 @@ it('waits for an event', () => { }); ``` +#### Ensuring that tests are isolated + +Tests are normally architected in a pattern which requires a recurring setup and breakdown of the component under test. This is done by making use of the `beforeEach` and `afterEach` hooks. + +Example + +```javascript + let wrapper; + + beforeEach(() => { + wrapper = mount(Component); + }); + + afterEach(() => { + wrapper.destroy(); + }); +``` + +When looking at this initially you'd suspect that the component is setup before each test and then broken down afterwards, providing isolation between tests. + +This is however not entirely true as the `destroy` method does not remove everything which has been mutated on the `wrapper` object. For functional components, destroy only removes the rendered DOM elements from the document. + +In order to ensure that a clean wrapper object and DOM are being used in each test, the breakdown of the component should rather be performed as follows: + +```javascript + afterEach(() => { + wrapper.destroy(); + wrapper = null; + }); +``` + +See also the [Vue Test Utils documention on `destroy`](https://vue-test-utils.vuejs.org/api/wrapper/#destroy). + #### Migrating flaky Karma tests to Jest Some of our Karma tests are flaky because they access the properties of a shared scope. diff --git a/doc/development/testing_guide/review_apps.md b/doc/development/testing_guide/review_apps.md index 3dd403f148e..ecfcbc731e1 100644 --- a/doc/development/testing_guide/review_apps.md +++ b/doc/development/testing_guide/review_apps.md @@ -189,10 +189,10 @@ that the `review-apps-ce/ee` cluster is unhealthy. Leading indicators may be hea The following items may help diagnose this: -- [Instance group CPU Utilization in GCP](https://console.cloud.google.com/compute/instanceGroups/details/us-central1-b/gke-review-apps-ee-preemp-n1-standard-8affc0f5-grp?project=gitlab-review-apps&tab=monitoring&graph=GCE_CPU&duration=P30D) - helpful to identify if nodes are problematic or the entire cluster is trending towards unhealthy -- [Instance Group size in GCP](https://console.cloud.google.com/compute/instanceGroups/details/us-central1-b/gke-review-apps-ee-preemp-n1-standard-8affc0f5-grp?project=gitlab-review-apps&tab=monitoring&graph=GCE_SIZE&duration=P30D) - aids in identifying load spikes on the cluster. Kubernetes will add nodes up to 220 based on total resource requests. -- `kubectl top nodes --sort-by=cpu` - can identify if node spikes are common or load on specific nodes which may get rebalanced by the Kubernetes scheduler. -- `kubectl top pods --sort-by=cpu` - +- [Review Apps Health dashboard](https://app.google.stackdriver.com/dashboards/6798952013815386466?project=gitlab-review-apps&timeDomain=1d) + - Aids in identifying load spikes on the cluster, and if nodes are problematic or the entire cluster is trending towards unhealthy. +- `kubectl top nodes | sort --key 3 --numeric` - can identify if node spikes are common or load on specific nodes which may get rebalanced by the Kubernetes scheduler. +- `kubectl top pods | sort --key 2 --numeric` - - [K9s] - K9s is a powerful command line dashboard which allows you to filter by labels. This can help identify trends with apps exceeding the [review-app resource requests](https://gitlab.com/gitlab-org/gitlab/blob/master/scripts/review_apps/base-config.yaml). Kubernetes will schedule pods to nodes based on resource requests and allow for CPU usage up to the limits. - In K9s you can sort or add filters by typing the `/` character - `-lrelease=<review-app-slug>` - filters down to all pods for a release. This aids in determining what is having issues in a single deployment diff --git a/doc/development/understanding_explain_plans.md b/doc/development/understanding_explain_plans.md index 7c926c83a36..53b50b6332c 100644 --- a/doc/development/understanding_explain_plans.md +++ b/doc/development/understanding_explain_plans.md @@ -705,6 +705,43 @@ For more information about the available options, run: /chatops run explain --help ``` +### `#database-lab` + +Another tool GitLab employees can use is a chatbot powered by [Joe](https://gitlab.com/postgres-ai/joe), available in the [`#database-lab`](https://gitlab.slack.com/archives/CLJMDRD8C) channel on Slack. +Unlike chatops, it gives you a way to execute DDL statements (like creating indexes and tables) and get query plan not only for `SELECT` but also `UPDATE` and `DELETE`. + +For example, in order to test new index you can do the following: + +Create the index: + +``` +exec CREATE INDEX index_projects_marked_for_deletion ON projects (marked_for_deletion_at) WHERE marked_for_deletion_at IS NOT NULL +``` + +Analyze the table to update its statistics: + +``` +exec ANALYZE projects +``` + +Get the query plan: + +``` +explain SELECT * FROM projects WHERE marked_for_deletion_at < CURRENT_DATE +``` + +Once done you can rollback your changes: + +``` +reset +``` + +For more information about the available options, run: + +``` +help +``` + ## Further reading A more extensive guide on understanding query plans can be found in diff --git a/doc/development/utilities.md b/doc/development/utilities.md index 38e416d68e4..25869a0d2b5 100644 --- a/doc/development/utilities.md +++ b/doc/development/utilities.md @@ -1,6 +1,6 @@ # GitLab utilities -We developed a number of utilities to ease development. +We have developed a number of utilities to help ease development: ## `MergeHash` @@ -51,15 +51,15 @@ Refer to: <https://gitlab.com/gitlab-org/gitlab/blob/master/lib/gitlab/utils/mer Refer to <https://gitlab.com/gitlab-org/gitlab/blob/master/lib/gitlab/utils/override.rb>: -- This utility could help us check if a particular method would override - another method or not. It has the same idea of Java's `@Override` annotation - or Scala's `override` keyword. However we only do this check when +- This utility can help you check if one method would override + another or not. It is the same concept as Java's `@Override` annotation + or Scala's `override` keyword. However, you should only do this check when `ENV['STATIC_VERIFICATION']` is set to avoid production runtime overhead. - This is useful to check: + This is useful for checking: - - If we have typos in overriding methods. - - If we renamed the overridden methods, making original overriding methods - overrides nothing. + - If you have typos in overriding methods. + - If you renamed the overridden methods, which make the original override methods + irrelevant. Here's a simple example: @@ -100,11 +100,11 @@ Refer to <https://gitlab.com/gitlab-org/gitlab/blob/master/lib/gitlab/utils/stro - Memoize the value even if it is `nil` or `false`. - We often do `@value ||= compute`, however this doesn't work well if - `compute` might eventually give `nil` and we don't want to compute again. - Instead we could use `defined?` to check if the value is set or not. - However it's tedious to write such pattern, and `StrongMemoize` would - help us use such pattern. + We often do `@value ||= compute`. However, this doesn't work well if + `compute` might eventually give `nil` and you don't want to compute again. + Instead you could use `defined?` to check if the value is set or not. + It's tedious to write such pattern, and `StrongMemoize` would + help you use such pattern. Instead of writing patterns like this: @@ -118,7 +118,7 @@ Refer to <https://gitlab.com/gitlab-org/gitlab/blob/master/lib/gitlab/utils/stro end ``` - We could write it like: + You could write it like: ``` ruby class Find @@ -151,7 +151,7 @@ and the cache key would be based on the class name, method name, optionally customized instance level values, optionally customized method level values, and optional method arguments. -A simple example that only uses the instance level customised values: +A simple example that only uses the instance level customised values is: ``` ruby class UserAccess @@ -169,8 +169,8 @@ end This way, the result of `can_push_to_branch?` would be cached in `RequestStore.store` based on the cache key. If `RequestStore` is not -currently active, then it would be stored in a hash saved in an -instance variable, so the cache logic would be the same. +currently active, then it would be stored in a hash, and saved in an +instance variable so the cache logic would be the same. We can also set different strategies for different methods: @@ -184,3 +184,83 @@ class Commit request_cache(:author) { author_email } end ``` + +## `ReactiveCaching` + +The `ReactiveCaching` concern is used to fetch some data in the background and +store it in the Rails cache, keeping it up-to-date for as long as it is being +requested. If the data hasn't been requested for `reactive_cache_lifetime`, +it will stop being refreshed, and then be removed. + +Example of use: + +```ruby +class Foo < ApplicationRecord + include ReactiveCaching + + after_save :clear_reactive_cache! + + def calculate_reactive_cache + # Expensive operation here. The return value of this method is cached + end + + def result + with_reactive_cache do |data| + # ... + end + end +end +``` + +In this example, the first time `#result` is called, it will return `nil`. +However, it will enqueue a background worker to call `#calculate_reactive_cache` +and set an initial cache lifetime of ten minutes. + +The background worker needs to find or generate the object on which +`with_reactive_cache` was called. +The default behaviour can be overridden by defining a custom +`reactive_cache_worker_finder`. +Otherwise, the background worker will use the class name and primary key to get +the object using the ActiveRecord `find_by` method. + +```ruby +class Bar + include ReactiveCaching + + self.reactive_cache_key = ->() { ["bar", "thing"] } + self.reactive_cache_worker_finder = ->(_id, *args) { from_cache(*args) } + + def self.from_cache(var1, var2) + # This method will be called by the background worker with "bar1" and + # "bar2" as arguments. + new(var1, var2) + end + + def initialize(var1, var2) + # ... + end + + def calculate_reactive_cache + # Expensive operation here. The return value of this method is cached + end + + def result + with_reactive_cache("bar1", "bar2") do |data| + # ... + end + end +end +``` + +Each time the background job completes, it stores the return value of +`#calculate_reactive_cache`. It is also re-enqueued to run again after +`reactive_cache_refresh_interval`, therefore, it will keep the stored value up to date. +Calculations are never run concurrently. + +Calling `#result` while a value is cached will call the block given to +`#with_reactive_cache`, yielding the cached value. It will also extend the +lifetime by the `reactive_cache_lifetime` value. + +Once the lifetime has expired, no more background jobs will be enqueued and +calling `#result` will again return `nil` - starting the process all over +again. |