diff options
Diffstat (limited to 'doc/development')
| -rw-r--r-- | doc/development/README.md | 4 | ||||
| -rw-r--r-- | doc/development/code_review.md | 11 | ||||
| -rw-r--r-- | doc/development/doc_styleguide.md | 33 | ||||
| -rw-r--r-- | doc/development/frontend.md | 236 | ||||
| -rw-r--r-- | doc/development/licensing.md | 2 | ||||
| -rw-r--r-- | doc/development/migration_style_guide.md | 8 | ||||
| -rw-r--r-- | doc/development/performance.md | 7 | ||||
| -rw-r--r-- | doc/development/sidekiq_style_guide.md | 38 |
8 files changed, 324 insertions, 15 deletions
diff --git a/doc/development/README.md b/doc/development/README.md index 58c00f618fa..fb6a8a5b095 100644 --- a/doc/development/README.md +++ b/doc/development/README.md @@ -13,7 +13,9 @@ - [SQL Migration Style Guide](migration_style_guide.md) for creating safe SQL migrations - [Testing standards and style guidelines](testing.md) - [UI guide](ui_guide.md) for building GitLab with existing CSS styles and elements -- [SQL guidelines](sql.md) for SQL guidelines +- [Frontend guidelines](frontend.md) +- [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/code_review.md b/doc/development/code_review.md index 40ae55ab905..c5c23b5c0b8 100644 --- a/doc/development/code_review.md +++ b/doc/development/code_review.md @@ -34,6 +34,10 @@ request is up to one of our merge request "endbosses", denoted on the ## Having your code reviewed +Please keep in mind that code review is a process that can take multiple +iterations, and reviewers may spot things later that they may not have seen the +first time. + - The first reviewer of your code is _you_. Before you perform that first push of your shiny new branch, read through the entire diff. Does it make sense? Did you include something unrelated to the overall purpose of the changes? Did @@ -55,6 +59,7 @@ request is up to one of our merge request "endbosses", denoted on the Understand why the change is necessary (fixes a bug, improves the user experience, refactors the existing code). Then: +- Try to be thorough in your reviews to reduce the number of iterations. - Communicate which ideas you feel strongly about and those you don't. - Identify ways to simplify the code while still solving the problem. - Offer alternative implementations, but assume the author already considered @@ -64,8 +69,10 @@ experience, refactors the existing code). Then: someone else would be confused by it as well. - After a round of line notes, it can be helpful to post a summary note such as "LGTM :thumbsup:", or "Just a couple things to address." -- Avoid accepting a merge request before the build succeeds ("Merge when build - succeeds" is fine). +- Avoid accepting a merge request before the build succeeds. Of course, "Merge + When Build Succeeds" (MWBS) is fine. +- If you set the MR to "Merge When Build Succeeds", you should take over + subsequent revisions for anything that would be spotted after that. ## Credits diff --git a/doc/development/doc_styleguide.md b/doc/development/doc_styleguide.md index 39b801f761d..f07d2c9af2d 100644 --- a/doc/development/doc_styleguide.md +++ b/doc/development/doc_styleguide.md @@ -95,10 +95,10 @@ merge request. someone in the Merge Request - 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`, - `@dblessing`, `@ashleys`. This is to ensure that no document - with wrong heading is going live without an audit, thus preventing dead links - and redirection issues when corrected + of the following GitLab members for a review: `@axil`, `@rspeicher`, `@marcia`, + `@SeanPackham`. This is to ensure that no document with wrong heading is going + live without an audit, thus preventing dead links and redirection issues when + corrected - Leave exactly one newline after a heading ## Links @@ -314,6 +314,29 @@ In this case: - different highlighting languages are used for each config in the code block - the [references](#references) guide is used for reconfigure/restart +## Fake tokens + +There may be times where a token is needed to demonstrate an API call using +cURL or a secret variable used in CI. It is strongly advised not to use real +tokens in documentation even if the probability of a token being exploited is +low. + +You can use the following fake tokens as examples. + +| **Token type** | **Token value** | +| --------------------- | --------------------------------- | +| Private user token | `9koXpg98eAheJpvBs5tK` | +| Personal access token | `n671WNGecHugsdEDPsyo` | +| Application ID | `2fcb195768c39e9a94cec2c2e32c59c0aad7a3365c10892e8116b5d83d4096b6` | +| Application secret | `04f294d1eaca42b8692017b426d53bbc8fe75f827734f0260710b83a556082df` | +| Secret CI variable | `Li8j-mLUVA3eZYjPfd_H` | +| Specific Runner token | `yrnZW46BrtBFqM7xDzE7dddd` | +| Shared Runner token | `6Vk7ZsosqQyfreAxXTZr` | +| Trigger token | `be20d8dcc028677c931e04f3871a9b` | +| Webhook secret token | `6XhDroRcYPM5by_h-HLY` | +| Health check token | `Tu7BgjR9qeZTEyRzGG2P` | +| Request profile token | `7VgpS4Ax5utVD2esNstz` | + ## API Here is a list of must-have items. Use them in the exact order that appears @@ -449,4 +472,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 +[gitlab-map]: https://gitlab.com/gitlab-org/gitlab-design/raw/master/production/resources/gitlab-map.png
\ No newline at end of file diff --git a/doc/development/frontend.md b/doc/development/frontend.md new file mode 100644 index 00000000000..56c8516508e --- /dev/null +++ b/doc/development/frontend.md @@ -0,0 +1,236 @@ +# Frontend Development Guidelines + +This document describes various guidelines to ensure consistency and quality +across GitLab's frontend team. + +## Overview + +GitLab is built on top of [Ruby on Rails][rails] using [Haml][haml] with +[Hamlit][hamlit]. Be wary of [the limitations that come with using +Hamlit][hamlit-limits]. We also use [SCSS][scss] and plain JavaScript with +[ES6 by way of Babel][es6]. + +The asset pipeline is [Sprockets][sprockets], which handles the concatenation, +minification, and compression of our assets. + +[jQuery][jquery] is used throughout the application's JavaScript, with +[Vue.js][vue] for particularly advanced, dynamic elements. + +### Vue + +For more complex frontend features, we recommend using Vue.js. It shares +some ideas with React.js as well as Angular. + +To get started with Vue, read through [their documentation][vue-docs]. + +## Performance + +### Resources + +- [WebPage Test][web-page-test] for testing site loading time and size. +- [Google PageSpeed Insights][pagespeed-insights] grades web pages and provides feedback to improve the page. +- [Profiling with Chrome DevTools][google-devtools-profiling] +- [Browser Diet][browser-diet] is a community-built guide that catalogues practical tips for improving web page performance. + +### Page-specific JavaScript + +Certain pages may require the use of a third party library, such as [d3][d3] for +the User Activity Calendar and [Chart.js][chartjs] for the Graphs pages. These +libraries increase the page size significantly, and impact load times due to +bandwidth bottlenecks and the browser needing to parse more JavaScript. + +In cases where libraries are only used on a few specific pages, we use +"page-specific JavaScript" to prevent the main `application.js` file from +becoming unnecessarily large. + +Steps to split page-specific JavaScript from the main `application.js`: + +1. Create a directory for the specific page(s), e.g. `graphs/`. +1. In that directory, create a `namespace_bundle.js` file, e.g. `graphs_bundle.js`. +1. In `graphs_bundle.js` add the line `//= require_tree .`, this adds all other files in the directory to the bundle. +1. Add any necessary libraries to `app/assets/javascripts/lib/`, all files directly descendant from this directory will be precompiled as separate assets, in this case `chart.js` would be added. +1. Add the new "bundle" file to the list of precompiled assets in +`config/application.rb`. + - For example: `config.assets.precompile << "graphs/graphs_bundle.js"`. +1. Move code reliant on these libraries into the `graphs` directory. +1. In the relevant views, add the scripts to the page with the following: + +```haml +- content_for :page_specific_javascripts do + = page_specific_javascript_tag('lib/chart.js') + = page_specific_javascript_tag('graphs/graphs_bundle.js') +``` + +The above loads `chart.js` and `graphs_bundle.js` for this page only. `chart.js` +is separated from the bundle file so it can be cached separately from the bundle +and reused for other pages that also rely on the library. For an example, see +[this Haml file][page-specific-js-example]. + +### Minimizing page size + +A smaller page size means the page loads faster (especially important on mobile +and poor connections), the page is parsed more quickly by the browser, and less +data is used for users with capped data plans. + +General tips: + +- Don't add new fonts. +- Prefer font formats with better compression, e.g. WOFF2 is better than WOFF, which is better than TTF. +- Compress and minify assets wherever possible (For CSS/JS, Sprockets does this for us). +- If some functionality can reasonably be achieved without adding extra libraries, avoid them. +- Use page-specific JavaScript as described above to dynamically load libraries that are only needed on certain pages. + +## Accessibility + +### Resources + +[Chrome Accessibility Developer Tools][chrome-accessibility-developer-tools] +are useful for testing for potential accessibility problems in GitLab. + +Accessibility best-practices and more in-depth information is available on +[the Audit Rules page][audit-rules] for the Chrome Accessibility Developer Tools. + +## Security + +### Resources + +[Mozilla’s HTTP Observatory CLI][observatory-cli] and the +[Qualys SSL Labs Server Test][qualys-ssl] are good resources for finding +potential problems and ensuring compliance with security best practices. + +<!-- Uncomment these sections when CSP/SRI are implemented. +### Content Security Policy (CSP) + +Content Security Policy is a web standard that intends to mitigate certain +forms of Cross-Site Scripting (XSS) as well as data injection. + +Content Security Policy rules should be taken into consideration when +implementing new features, especially those that may rely on connection with +external services. + +GitLab's CSP is used for the following: + +- Blocking plugins like Flash and Silverlight from running at all on our pages. +- Blocking the use of scripts and stylesheets downloaded from external sources. +- Upgrading `http` requests to `https` when possible. +- Preventing `iframe` elements from loading in most contexts. + +Some exceptions include: + +- Scripts from Google Analytics and Piwik if either is enabled. +- Connecting with GitHub, Bitbucket, GitLab.com, etc. to allow project importing. +- Connecting with Google, Twitter, GitHub, etc. to allow OAuth authentication. + +We use [the Secure Headers gem][secure_headers] to enable Content +Security Policy headers in the GitLab Rails app. + +Some resources on implementing Content Security Policy: + +- [MDN Article on CSP][mdn-csp] +- [GitHub’s CSP Journey on the GitHub Engineering Blog][github-eng-csp] +- The Dropbox Engineering Blog's series on CSP: [1][dropbox-csp-1], [2][dropbox-csp-2], [3][dropbox-csp-3], [4][dropbox-csp-4] + +### Subresource Integrity (SRI) + +Subresource Integrity prevents malicious assets from being provided by a CDN by +guaranteeing that the asset downloaded is identical to the asset the server +is expecting. + +The Rails app generates a unique hash of the asset, which is used as the +asset's `integrity` attribute. The browser generates the hash of the asset +on-load and will reject the asset if the hashes do not match. + +All CSS and JavaScript assets should use Subresource Integrity. For implementation details, +see the documentation for [the Sprockets implementation of SRI][sprockets-sri]. + +Some resources on implementing Subresource Integrity: + +- [MDN Article on SRI][mdn-sri] +- [Subresource Integrity on the GitHub Engineering Blog][github-eng-sri] + +--> + +### Including external resources + +External fonts, CSS, and JavaScript should never be used with the exception of +Google Analytics and Piwik - and only when the instance has enabled it. Assets +should always be hosted and served locally from the GitLab instance. Embedded +resources via `iframes` should never be used except in certain circumstances +such as with ReCaptcha, which cannot be used without an `iframe`. + +### Avoiding inline scripts and styles + +In order to protect users from [XSS vulnerabilities][xss], we will disable inline scripts in the future using Content Security Policy. + +While inline scripts can be useful, they're also a security concern. If +user-supplied content is unintentionally left un-sanitized, malicious users can +inject scripts into the web app. + +Inline styles should be avoided in almost all cases, they should only be used +when no alternatives can be found. This allows reusability of styles as well as +readability. + +## Style guides and linting + +See the relevant style guides for our guidelines and for information on linting: + +- [SCSS][scss-style-guide] + +## Testing + +Feature tests need to be written for all new features. Regression tests +also need to be written for all bug fixes to prevent them from occurring +again in the future. + +See [the Testing Standards and Style Guidelines](testing.md) for more +information. + +## Supported browsers + +For our currently-supported browsers, see our [requirements][requirements]. + +[rails]: http://rubyonrails.org/ +[haml]: http://haml.info/ +[hamlit]: https://github.com/k0kubun/hamlit +[hamlit-limits]: https://github.com/k0kubun/hamlit/blob/master/REFERENCE.md#limitations +[scss]: http://sass-lang.com/ +[es6]: https://babeljs.io/ +[sprockets]: https://github.com/rails/sprockets +[jquery]: https://jquery.com/ +[vue]: http://vuejs.org/ +[vue-docs]: http://vuejs.org/guide/index.html +[web-page-test]: http://www.webpagetest.org/ +[pagespeed-insights]: https://developers.google.com/speed/pagespeed/insights/ +[google-devtools-profiling]: https://developers.google.com/web/tools/chrome-devtools/profile/?hl=en +[browser-diet]: https://browserdiet.com/ +[d3]: https://d3js.org/ +[chartjs]: http://www.chartjs.org/ +[page-specific-js-example]: https://gitlab.com/gitlab-org/gitlab-ce/blob/13bb9ed77f405c5f6ee4fdbc964ecf635c9a223f/app/views/projects/graphs/_head.html.haml#L6-8 +[chrome-accessibility-developer-tools]: https://github.com/GoogleChrome/accessibility-developer-tools +[audit-rules]: https://github.com/GoogleChrome/accessibility-developer-tools/wiki/Audit-Rules +[observatory-cli]: https://github.com/mozilla/http-observatory-cli) +[qualys-ssl]: https://www.ssllabs.com/ssltest/analyze.html +[secure_headers]: https://github.com/twitter/secureheaders +[mdn-csp]: https://developer.mozilla.org/en-US/docs/Web/Security/CSP +[github-eng-csp]: http://githubengineering.com/githubs-csp-journey/ +[dropbox-csp-1]: https://blogs.dropbox.com/tech/2015/09/on-csp-reporting-and-filtering/ +[dropbox-csp-2]: https://blogs.dropbox.com/tech/2015/09/unsafe-inline-and-nonce-deployment/ +[dropbox-csp-3]: https://blogs.dropbox.com/tech/2015/09/csp-the-unexpected-eval/ +[dropbox-csp-4]: https://blogs.dropbox.com/tech/2015/09/csp-third-party-integrations-and-privilege-separation/ +[mdn-sri]: https://developer.mozilla.org/en-US/docs/Web/Security/Subresource_Integrity +[github-eng-sri]: http://githubengineering.com/subresource-integrity/ +[sprockets-sri]: https://github.com/rails/sprockets-rails#sri-support +[xss]: https://en.wikipedia.org/wiki/Cross-site_scripting +[scss-style-guide]: scss_styleguide.md +[requirements]: ../install/requirements.md#supported-web-browsers + +## Common Errors + +### Rspec (Capybara/Poltergeist) chokes on general 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>`). + + diff --git a/doc/development/licensing.md b/doc/development/licensing.md index 8c8c7486fff..05972b33fdb 100644 --- a/doc/development/licensing.md +++ b/doc/development/licensing.md @@ -54,6 +54,7 @@ Libraries with the following licenses are acceptable for use: - [BSD 2-Clause License][BSD-2-Clause]: A permissive (non-copyleft) license as defined by the Open Source Initiative. - [BSD 3-Clause License][BSD-3-Clause] (also known as New BSD or Modified BSD): A permissive (non-copyleft) license as defined by the Open Source Initiative - [ISC License][ISC] (also known as the OpenBSD License): A permissive (non-copyleft) license as defined by the Open Source Initiative. +- [Creative Commons Zero (CC0)][CC0]: A public domain dedication, recommended as a way to disclaim copyright on your work to the maximum extent possible. ## Unacceptable Licenses @@ -85,6 +86,7 @@ Gems which are included only in the "development" or "test" groups by Bundler ar [BSD-2-Clause]: https://opensource.org/licenses/BSD-2-Clause [BSD-3-Clause]: https://opensource.org/licenses/BSD-3-Clause [ISC]: https://opensource.org/licenses/ISC +[CC0]: https://creativecommons.org/publicdomain/zero/1.0/ [GPL]: http://choosealicense.com/licenses/gpl-3.0/ [GPLv2]: http://www.gnu.org/licenses/gpl-2.0.txt [GPLv3]: http://www.gnu.org/licenses/gpl-3.0.txt diff --git a/doc/development/migration_style_guide.md b/doc/development/migration_style_guide.md index 295eae0a88e..61b0fbc89c9 100644 --- a/doc/development/migration_style_guide.md +++ b/doc/development/migration_style_guide.md @@ -9,10 +9,10 @@ a big burden for most organizations. For this reason it is important that your migrations are written carefully, can be applied online and adhere to the style guide below. Migrations should not require GitLab installations to be taken offline unless -_absolutely_ necessary. If a migration requires downtime this should be -clearly mentioned during the review process as well as being documented in the -monthly release post. For more information see the "Downtime Tagging" section -below. +_absolutely_ necessary - see the ["What Requires Downtime?"](what_requires_downtime.md) +page. If a migration requires downtime, this should be clearly mentioned during +the review process, as well as being documented in the monthly release post. For +more information, see the "Downtime Tagging" section below. When writing your migrations, also consider that databases might have stale data or inconsistencies and guard for that. Try to make as little assumptions as possible diff --git a/doc/development/performance.md b/doc/development/performance.md index 7ff603e2c4a..8337c2d9cb3 100644 --- a/doc/development/performance.md +++ b/doc/development/performance.md @@ -34,10 +34,11 @@ graphs/dashboards. ## Tooling -GitLab provides two built-in tools to aid the process of improving performance: +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 <http://performance.gitlab.net>, this requires you to log in using your @@ -253,5 +254,5 @@ impact on runtime performance, and as such, using a constant instead of referencing an object directly may even slow code down. [#15607]: https://gitlab.com/gitlab-org/gitlab-ce/issues/15607 -[yorickpeterse]: https://gitlab.com/u/yorickpeterse +[yorickpeterse]: https://gitlab.com/yorickpeterse [anti-pattern]: https://en.wikipedia.org/wiki/Anti-pattern 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`. |