diff options
Diffstat (limited to 'doc/development')
| -rw-r--r-- | doc/development/README.md | 3 | ||||
| -rw-r--r-- | doc/development/api_styleguide.md | 96 | ||||
| -rw-r--r-- | doc/development/doc_styleguide.md | 10 | ||||
| -rw-r--r-- | doc/development/frontend.md | 25 | ||||
| -rw-r--r-- | doc/development/gotchas.md | 6 | ||||
| -rw-r--r-- | doc/development/licensing.md | 3 | ||||
| -rw-r--r-- | doc/development/post_deployment_migrations.md | 75 | ||||
| -rw-r--r-- | doc/development/testing.md | 4 |
8 files changed, 206 insertions, 16 deletions
diff --git a/doc/development/README.md b/doc/development/README.md index fb6a8a5b095..3f2151bbe8e 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 @@ -39,6 +41,7 @@ - [What requires downtime?](what_requires_downtime.md) - [Adding database indexes](adding_database_indexes.md) +- [Post Deployment Migrations](post_deployment_migrations.md) ## Compliance diff --git a/doc/development/api_styleguide.md b/doc/development/api_styleguide.md new file mode 100644 index 00000000000..ce444ebdde4 --- /dev/null +++ b/doc/development/api_styleguide.md @@ -0,0 +1,96 @@ +# API styleguide + +This styleguide recommends best practices for 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 so, 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 +`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 `params[key]` directly when we access single elements. + +For instance: + +```ruby +# good +Model.create(foo: params[:foo]) +``` + +[Entity]: https://gitlab.com/gitlab-org/gitlab-ce/blob/master/lib/api/entities.rb +[validation, and coercion of the parameters]: 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 f07d2c9af2d..2cfa30f652e 100644 --- a/doc/development/doc_styleguide.md +++ b/doc/development/doc_styleguide.md @@ -93,6 +93,8 @@ merge request. links shift too, which eventually leads to dead links. If you think it is compelling to add numbers in headings, make sure to at least discuss it with someone in the Merge Request +- Avoid adding things that show ephemeral statuses. For example, if a feature is + considered beta or experimental, put this info in a note, not in the heading. - When introducing a new document, be careful for the headings to be grammatically and syntactically correct. It is advised to mention one or all of the following GitLab members for a review: `@axil`, `@rspeicher`, `@marcia`, @@ -342,12 +344,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: ``` @@ -472,4 +468,4 @@ curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" --data "domain [doc-restart]: ../administration/restart_gitlab.md "GitLab restart documentation" [ce-3349]: https://gitlab.com/gitlab-org/gitlab-ce/issues/3349 "Documentation restructure" [graffle]: https://gitlab.com/gitlab-org/gitlab-design/blob/d8d39f4a87b90fb9ae89ca12dc565347b4900d5e/production/resources/gitlab-map.graffle -[gitlab-map]: https://gitlab.com/gitlab-org/gitlab-design/raw/master/production/resources/gitlab-map.png
\ No newline at end of file +[gitlab-map]: https://gitlab.com/gitlab-org/gitlab-design/raw/master/production/resources/gitlab-map.png diff --git a/doc/development/frontend.md b/doc/development/frontend.md index 56c8516508e..ece8f880542 100644 --- a/doc/development/frontend.md +++ b/doc/development/frontend.md @@ -185,6 +185,20 @@ again in the future. See [the Testing Standards and Style Guidelines](testing.md) for more information. +### Running frontend tests + +`rake teaspoon` runs the frontend-only (JavaScript) tests. +It consists of two subtasks: + +- `rake teaspoon:fixtures` (re-)generates fixtures +- `rake teaspoon:tests` actually executes the tests + +As long as the fixtures don't change, `rake teaspoon:tests` is sufficient +(and saves you some time). + +Please note: Not all of the frontend fixtures are generated. Some are still static +files. These will not be touched by `rake teaspoon:fixtures`. + ## Supported browsers For our currently-supported browsers, see our [requirements][requirements]. @@ -224,13 +238,18 @@ For our currently-supported browsers, see our [requirements][requirements]. [scss-style-guide]: scss_styleguide.md [requirements]: ../install/requirements.md#supported-web-browsers -## Common Errors +## Gotchas -### Rspec (Capybara/Poltergeist) chokes on general JavaScript errors +### Phantom.JS (used by Teaspoon & Rspec) chokes, returning vague JavaScript errors If you see very generic JavaScript errors (e.g. `jQuery is undefined`) being thrown in tests, but can't reproduce them manually, you may have included `ES6`-style JavaScript in files that don't have the `.js.es6` file extension. Either use ES5-friendly JavaScript or rename the file you're -working in (`git mv <file>.js> <file.js.es6>`). +working in (`git mv <file.js> <file.js.es6>`). + +Similar errors will be thrown if you're using +any of the [array methods introduced in ES6](http://www.2ality.com/2014/05/es6-array-methods.html) +whether or not you've updated the file extension. + diff --git a/doc/development/gotchas.md b/doc/development/gotchas.md index 159d5ce286d..b25ce79e89f 100644 --- a/doc/development/gotchas.md +++ b/doc/development/gotchas.md @@ -41,9 +41,9 @@ Rubocop](https://gitlab.com/gitlab-org/gitlab-ce/blob/8-4-stable/.rubocop.yml#L9 [Exception]: http://stackoverflow.com/q/10048173/223897 -## Don't use inline CoffeeScript/JavaScript in views +## Don't use inline JavaScript in views -Using the inline `:coffee` or `:coffeescript` Haml filters comes with a +Using the inline `:javascript` Haml filters comes with a performance overhead. Using inline JavaScript is not a good way to structure your code and should be avoided. _**Note:** We've [removed these two filters](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/config/initializers/hamlit.rb) @@ -51,9 +51,7 @@ in an initializer._ ### Further reading -- Pull Request: [Replace CoffeeScript block into JavaScript in Views](https://git.io/vztMu) - Stack Overflow: [Why you should not write inline JavaScript](http://programmers.stackexchange.com/questions/86589/why-should-i-avoid-inline-scripting) -- Stack Overflow: [Performance implications of using :coffescript filter inside HAML templates?](http://stackoverflow.com/a/17571242/223897) ## ID-based CSS selectors need to be a bit more specific diff --git a/doc/development/licensing.md b/doc/development/licensing.md index 05972b33fdb..5d177eb26ee 100644 --- a/doc/development/licensing.md +++ b/doc/development/licensing.md @@ -62,6 +62,7 @@ Libraries with the following licenses are unacceptable for use: - [GNU GPL][GPL] (version 1, [version 2][GPLv2], [version 3][GPLv3], or any future versions): GPL-licensed libraries cannot be linked to from non-GPL projects. - [GNU AGPLv3][AGPLv3]: AGPL-licensed libraries cannot be linked to from non-GPL projects. +- [Open Software License (OSL)][OSL]: is a copyleft license. In addition, the FSF [recommend against its use][OSL-GNU]. ## Notes @@ -93,3 +94,5 @@ Gems which are included only in the "development" or "test" groups by Bundler ar [AGPLv3]: http://choosealicense.com/licenses/agpl-3.0/ [GNU-GPL-FAQ]: http://www.gnu.org/licenses/gpl-faq.html#IfLibraryIsGPL [OSI-GPL]: https://opensource.org/faq#linking-proprietary-code +[OSL]: https://opensource.org/licenses/OSL-3.0 +[OSL-GNU]: https://www.gnu.org/licenses/license-list.en.html#OSL diff --git a/doc/development/post_deployment_migrations.md b/doc/development/post_deployment_migrations.md new file mode 100644 index 00000000000..cfc91539bee --- /dev/null +++ b/doc/development/post_deployment_migrations.md @@ -0,0 +1,75 @@ +# Post Deployment Migrations + +Post deployment migrations are regular Rails migrations that can optionally be +executed after a deployment. By default these migrations are executed alongside +the other migrations. To skip these migrations you will have to set the +environment variable `SKIP_POST_DEPLOYMENT_MIGRATIONS` to a non-empty value +when running `rake db:migrate`. + +For example, this would run all migrations including any post deployment +migrations: + +```bash +bundle exec rake db:migrate +``` + +This however will skip post deployment migrations: + +```bash +SKIP_POST_DEPLOYMENT_MIGRATIONS=true bundle exec rake db:migrate +``` + +## Deployment Integration + +Say you're using Chef for deploying new versions of GitLab and you'd like to run +post deployment migrations after deploying a new version. Let's assume you +normally use the command `chef-client` to do so. To make use of this feature +you'd have to run this command as follows: + +```bash +SKIP_POST_DEPLOYMENT_MIGRATIONS=true sudo chef-client +``` + +Once all servers have been updated you can run `chef-client` again on a single +server _without_ the environment variable. + +The process is similar for other deployment techniques: first you would deploy +with the environment variable set, then you'll essentially re-deploy a single +server but with the variable _unset_. + +## Creating Migrations + +To create a post deployment migration you can use the following Rails generator: + +```bash +bundle exec rails g post_deployment_migration migration_name_here +``` + +This will generate the migration file in `db/post_migrate`. These migrations +behave exactly like regular Rails migrations. + +## Use Cases + +Post deployment migrations can be used to perform migrations that mutate state +that an existing version of GitLab depends on. For example, say you want to +remove a column from a table. This requires downtime as a GitLab instance +depends on this column being present while it's running. Normally you'd follow +these steps in such a case: + +1. Stop the GitLab instance +2. Run the migration removing the column +3. Start the GitLab instance again + +Using post deployment migrations we can instead follow these steps: + +1. Deploy a new version of GitLab while ignoring post deployment migrations +2. Re-run `rake db:migrate` but without the environment variable set + +Here we don't need any downtime as the migration takes place _after_ a new +version (which doesn't depend on the column anymore) has been deployed. + +Some other examples where these migrations are useful: + +* Cleaning up data generated due to a bug in GitLab +* Removing tables +* Migrating jobs from one Sidekiq queue to another diff --git a/doc/development/testing.md b/doc/development/testing.md index 513457d203a..8e91ac5e3ba 100644 --- a/doc/development/testing.md +++ b/doc/development/testing.md @@ -36,8 +36,8 @@ the command line via `bundle exec teaspoon`, or via a web browser at `http://localhost:3000/teaspoon` when the Rails server is running. - JavaScript tests live in `spec/javascripts/`, matching the folder structure of - `app/assets/javascripts/`: `app/assets/javascripts/behaviors/autosize.js.coffee` has a corresponding - `spec/javascripts/behaviors/autosize_spec.js.coffee` file. + `app/assets/javascripts/`: `app/assets/javascripts/behaviors/autosize.js.es6` has a corresponding + `spec/javascripts/behaviors/autosize_spec.js.es6` file. - Haml fixtures required for JavaScript tests live in `spec/javascripts/fixtures`. They should contain the bare minimum amount of markup necessary for the test. |
