From b2c771f45261e1484158d1304cd898e866002f07 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Thu, 13 Oct 2016 18:44:52 +0200 Subject: Add an API styleguide MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- doc/development/README.md | 2 ++ doc/development/api_styleguide.md | 58 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+) create mode 100644 doc/development/api_styleguide.md (limited to 'doc/development') diff --git a/doc/development/README.md b/doc/development/README.md index 9706cb1de7f..630fe64cee6 100644 --- a/doc/development/README.md +++ b/doc/development/README.md @@ -8,6 +8,8 @@ ## Styleguides +- [API styleguide](api_styleguide.md) Use this styleguide if you are + contributing to the API. - [Documentation styleguide](doc_styleguide.md) Use this styleguide if you are contributing to documentation. - [SQL Migration Style Guide](migration_style_guide.md) for creating safe SQL migrations diff --git a/doc/development/api_styleguide.md b/doc/development/api_styleguide.md new file mode 100644 index 00000000000..ee5e7ad3988 --- /dev/null +++ b/doc/development/api_styleguide.md @@ -0,0 +1,58 @@ +# API styleguide + +This styleguide recommends best practices for the API development. + +## Declared params + +> Grape allows you to access only the parameters that have been declared by your +`params` block. It filters out the params that have been passed, but are not +allowed. + +– https://github.com/ruby-grape/grape#declared + +### Exclude params from parent namespaces + +> By default `declared(params) `includes parameters that were defined in all +parent namespaces. + +– https://github.com/ruby-grape/grape#include-parent-namespaces + +In most cases you will want to exclude params from the parent namespaces: + +```ruby +declared(params, include_parent_namespaces: false) +``` + +### When to use `declared(params)`? + +You should always use `declared(params)` when you pass the params hash as +arguments to a method call. + +For instance: + +```ruby +# bad +User.create(params) # imagine the user submitted `admin=1`... :) + +# good +User.create(declared(params, include_parent_namespaces: false).to_h) +``` + +>**Note:** +`declared(params)` return a `Hashie::Mash` object, on which you will have to +call `.to_h`. + +But we can use directly `params[key]` when we access single elements. + +For instance: + +```ruby +# good +Model.create(foo: params[:foo]) +``` + +>**Note:** +Since you [should use Grape's DSL to declare params](doc_styleguide.md#method-description), [parameters validation and +coercion] comes for free! + +[parameters validation and coercion]: https://github.com/ruby-grape/grape#parameter-validation-and-coercion -- cgit v1.2.1 From c1dd1795ed57c9481a1a9cab81daef39dd218346 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Thu, 13 Oct 2016 19:35:57 +0200 Subject: Move the Grape DSL part from Doc styleguide to API styleguide MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Also improve API styleguide Signed-off-by: Rémy Coutable --- doc/development/api_styleguide.md | 48 +++++++++++++++++++++++++++++++++++---- doc/development/doc_styleguide.md | 6 ----- 2 files changed, 43 insertions(+), 11 deletions(-) (limited to 'doc/development') diff --git a/doc/development/api_styleguide.md b/doc/development/api_styleguide.md index ee5e7ad3988..be303fc6d39 100644 --- a/doc/development/api_styleguide.md +++ b/doc/development/api_styleguide.md @@ -2,6 +2,47 @@ This styleguide recommends best practices for the API development. +## Instance variables + +Please do not use instance variables, there is no need for them (we don't need +to access them as we do in Rails views), local variables are fine. + +## Entities + +Always use an [Entity] to present the endpoint's payload. + +## Methods and parameters description + +Every method must be described using the [Grape DSL](https://github.com/ruby-grape/grape#describing-methods) +(see https://gitlab.com/gitlab-org/gitlab-ce/blob/master/lib/api/environments.rb +for a good example): + +- `desc` for the method summary. You should pass it a block for additional + details such as: + - The GitLab version when the endpoint was added + - If the endpoint is deprecated, and if yes when will it be removed + +- `params` for the method params. This acts as description, validation, and + coercion of the parameters + +A good example is as follows: + +```ruby +desc 'Get all broadcast messages' do + detail 'This feature was introduced in GitLab 8.12.' + success Entities::BroadcastMessage +end +params do + optional :page, type: Integer, desc: 'Current page number' + optional :per_page, type: Integer, desc: 'Number of messages per page' +end +get do + messages = BroadcastMessage.all + + present paginate(messages), with: Entities::BroadcastMessage +end +``` + ## Declared params > Grape allows you to access only the parameters that have been declared by your @@ -10,7 +51,7 @@ allowed. – https://github.com/ruby-grape/grape#declared -### Exclude params from parent namespaces +### Exclude params from parent namespaces! > By default `declared(params) `includes parameters that were defined in all parent namespaces. @@ -51,8 +92,5 @@ For instance: Model.create(foo: params[:foo]) ``` ->**Note:** -Since you [should use Grape's DSL to declare params](doc_styleguide.md#method-description), [parameters validation and -coercion] comes for free! - +[Entity]: https://gitlab.com/gitlab-org/gitlab-ce/blob/master/lib/api/entities.rb [parameters validation and coercion]: https://github.com/ruby-grape/grape#parameter-validation-and-coercion diff --git a/doc/development/doc_styleguide.md b/doc/development/doc_styleguide.md index 0b725cf200c..882da2a6562 100644 --- a/doc/development/doc_styleguide.md +++ b/doc/development/doc_styleguide.md @@ -342,12 +342,6 @@ You can use the following fake tokens as examples. Here is a list of must-have items. Use them in the exact order that appears on this document. Further explanation is given below. -- Every method must be described using [Grape's DSL](https://github.com/ruby-grape/grape/tree/v0.13.0#describing-methods) - (see https://gitlab.com/gitlab-org/gitlab-ce/blob/master/lib/api/environments.rb - for a good example): - - `desc` for the method summary (you can pass it a block for additional details) - - `params` for the method params (this acts as description **and** validation - of the params) - Every method must have the REST API request. For example: ``` -- cgit v1.2.1 From 3a1d9bccd6c3ce8249e90153f4299db1c037eb56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Thu, 13 Oct 2016 19:38:38 +0200 Subject: Fix typo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- doc/development/api_styleguide.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'doc/development') diff --git a/doc/development/api_styleguide.md b/doc/development/api_styleguide.md index be303fc6d39..94047dfe173 100644 --- a/doc/development/api_styleguide.md +++ b/doc/development/api_styleguide.md @@ -1,6 +1,6 @@ # API styleguide -This styleguide recommends best practices for the API development. +This styleguide recommends best practices for API development. ## Instance variables -- cgit v1.2.1 From b4810fd2bce42d66cada56af3ef697219f176b87 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Thu, 13 Oct 2016 19:40:20 +0200 Subject: More improvements MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- doc/development/api_styleguide.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'doc/development') diff --git a/doc/development/api_styleguide.md b/doc/development/api_styleguide.md index 94047dfe173..2a41314d2db 100644 --- a/doc/development/api_styleguide.md +++ b/doc/development/api_styleguide.md @@ -22,8 +22,8 @@ for a good example): - The GitLab version when the endpoint was added - If the endpoint is deprecated, and if yes when will it be removed -- `params` for the method params. This acts as description, validation, and - coercion of the parameters +- `params` for the method params. This acts as description, + [validation, and coercion of the parameters] A good example is as follows: @@ -93,4 +93,4 @@ Model.create(foo: params[:foo]) ``` [Entity]: https://gitlab.com/gitlab-org/gitlab-ce/blob/master/lib/api/entities.rb -[parameters validation and coercion]: https://github.com/ruby-grape/grape#parameter-validation-and-coercion +[validation, and coercion of the parameters]: https://github.com/ruby-grape/grape#parameter-validation-and-coercion -- cgit v1.2.1 From 11e40c0e26e07d153cd2712bf23b5d679b54615c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Mon, 17 Oct 2016 10:10:13 +0200 Subject: Improve copy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- doc/development/api_styleguide.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'doc/development') diff --git a/doc/development/api_styleguide.md b/doc/development/api_styleguide.md index 2a41314d2db..ce444ebdde4 100644 --- a/doc/development/api_styleguide.md +++ b/doc/development/api_styleguide.md @@ -20,7 +20,7 @@ for a good example): - `desc` for the method summary. You should pass it a block for additional details such as: - The GitLab version when the endpoint was added - - If the endpoint is deprecated, and if yes when will it be removed + - If the endpoint is deprecated, and if so, when will it be removed - `params` for the method params. This acts as description, [validation, and coercion of the parameters] @@ -83,7 +83,7 @@ User.create(declared(params, include_parent_namespaces: false).to_h) `declared(params)` return a `Hashie::Mash` object, on which you will have to call `.to_h`. -But we can use directly `params[key]` when we access single elements. +But we can use `params[key]` directly when we access single elements. For instance: -- cgit v1.2.1 From 97731760d7252acf8ee94c707c0e107492b1ef24 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Fri, 21 Oct 2016 18:13:41 +0200 Subject: Re-organize queues to use for Sidekiq Dumping too many jobs in the same queue (e.g. the "default" queue) is a dangerous setup. Jobs that take a long time to process can effectively block any other work from being performed given there are enough of these jobs. Furthermore it becomes harder to monitor the jobs as a single queue could contain jobs for different workers. In such a setup the only reliable way of getting counts per job is to iterate over all jobs in a queue, which is a rather time consuming process. By using separate queues for various workers we have better control over throughput, we can add weight to queues, and we can monitor queues better. Some workers still use the same queue whenever their work is related. For example, the various CI pipeline workers use the same "pipeline" queue. This commit includes a Rails migration that moves Sidekiq jobs from the old queues to the new ones. This migration also takes care of doing the inverse if ever needed. This does require downtime as otherwise new jobs could be scheduled in the old queues after this migration completes. This commit also includes an RSpec test that blacklists the use of the "default" queue and ensures cron workers use the "cronjob" queue. Fixes gitlab-org/gitlab-ce#23370 --- doc/development/README.md | 3 ++- doc/development/sidekiq_style_guide.md | 38 ++++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) create mode 100644 doc/development/sidekiq_style_guide.md (limited to 'doc/development') diff --git a/doc/development/README.md b/doc/development/README.md index 9706cb1de7f..fb6a8a5b095 100644 --- a/doc/development/README.md +++ b/doc/development/README.md @@ -14,7 +14,8 @@ - [Testing standards and style guidelines](testing.md) - [UI guide](ui_guide.md) for building GitLab with existing CSS styles and elements - [Frontend guidelines](frontend.md) -- [SQL guidelines](sql.md) for SQL guidelines +- [SQL guidelines](sql.md) for working with SQL queries +- [Sidekiq guidelines](sidekiq_style_guide.md) for working with Sidekiq workers ## Process diff --git a/doc/development/sidekiq_style_guide.md b/doc/development/sidekiq_style_guide.md new file mode 100644 index 00000000000..e3a20f29a09 --- /dev/null +++ b/doc/development/sidekiq_style_guide.md @@ -0,0 +1,38 @@ +# Sidekiq Style Guide + +This document outlines various guidelines that should be followed when adding or +modifying Sidekiq workers. + +## Default Queue + +Use of the "default" queue is not allowed. Every worker should use a queue that +matches the worker's purpose the closest. For example, workers that are to be +executed periodically should use the "cronjob" queue. + +A list of all available queues can be found in `config/sidekiq_queues.yml`. + +## Dedicated Queues + +Most workers should use their own queue. To ease this process a worker can +include the `DedicatedSidekiqQueue` concern as follows: + +```ruby +class ProcessSomethingWorker + include Sidekiq::Worker + include DedicatedSidekiqQueue +end +``` + +This will set the queue name based on the class' name, minus the `Worker` +suffix. In the above example this would lead to the queue being +`process_something`. + +In some cases multiple workers do use the same queue. For example, the various +workers for updating CI pipelines all use the `pipeline` queue. Adding workers +to existing queues should be done with care, as adding more workers can lead to +slow jobs blocking work (even for different jobs) on the shared queue. + +## Tests + +Each Sidekiq worker must be tested using RSpec, just like any other class. These +tests should be placed in `spec/workers`. -- cgit v1.2.1 From 8b7634c2b665f6c3dc8b0f82b870fd2032f7d247 Mon Sep 17 00:00:00 2001 From: Achilleas Pipinellis Date: Mon, 24 Oct 2016 16:45:00 +0200 Subject: Fix old monitoring links to point to the new location --- doc/development/performance.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'doc/development') diff --git a/doc/development/performance.md b/doc/development/performance.md index c4a964d1da3..8337c2d9cb3 100644 --- a/doc/development/performance.md +++ b/doc/development/performance.md @@ -37,7 +37,7 @@ graphs/dashboards. GitLab provides built-in tools to aid the process of improving performance: * [Sherlock](profiling.md#sherlock) -* [GitLab Performance Monitoring](../monitoring/performance/monitoring.md) +* [GitLab Performance Monitoring](../administration/monitoring/performance/monitoring.md) * [Request Profiling](../administration/monitoring/performance/request_profiling.md) GitLab employees can use GitLab.com's performance monitoring systems located at -- cgit v1.2.1