summaryrefslogtreecommitdiff
path: root/doc/development
diff options
context:
space:
mode:
authorFilipa Lacerda <filipa@gitlab.com>2017-03-17 10:40:19 +0000
committerFilipa Lacerda <filipa@gitlab.com>2017-03-17 10:40:19 +0000
commitb5c80f99665f3aa9316b8667ec0fca640f3844d0 (patch)
tree3ebdd91270e27369beec13518eb56aace8763f19 /doc/development
parent6fb6632110ccec9c7ad64217647e17a29bdd66e3 (diff)
parent4bf4612cfbe73845391375bf721592426d7b4181 (diff)
downloadgitlab-ce-27574-pipelines-empty-state.tar.gz
Merge branch 'master' into 27574-pipelines-empty-state27574-pipelines-empty-state
* master: (209 commits) Fix Slack link when on notify Use Enumerable#index_by where possible Protect against unknown emojis in frequently used list Fix config option to disable Prometheus Fix double click token name fix describe block to make the karma reporter happy removes n+1 query from tags and branches indexes Fix broken links limit lines to 80 chars Add prometheus memory requirements, include additional detail on disabling prometheus in docs. Add `requirements: { id: %r{[^/]+} }` for all projects and groups namespaced API routes Expand on the good changelog/bad changelog documentation examples Add policy for closing stale merge requests Fix spec Use code icon for Raw Fix spec Add copy button to blob header and use icon for Raw button Fix archive prefix bug for refs containing dots Include routes when loading user projects Fixed search not working in issue boards modal Document a New Branch feature for Bare Projects ...
Diffstat (limited to 'doc/development')
-rw-r--r--doc/development/changelog.md109
-rw-r--r--doc/development/frontend.md49
-rw-r--r--doc/development/merge_request_performance_guidelines.md4
-rw-r--r--doc/development/performance.md1
-rw-r--r--doc/development/polling.md41
-rw-r--r--doc/development/profiling.md2
-rw-r--r--doc/development/query_recorder.md29
-rw-r--r--doc/development/testing.md4
8 files changed, 210 insertions, 29 deletions
diff --git a/doc/development/changelog.md b/doc/development/changelog.md
index c71858c6a24..ce39a379a0e 100644
--- a/doc/development/changelog.md
+++ b/doc/development/changelog.md
@@ -1,7 +1,7 @@
-# Generate a changelog entry
+# Changelog entries
-This guide contains instructions for generating a changelog entry data file, as
-well as information and history about our changelog process.
+This guide contains instructions for when and how to generate a changelog entry
+file, as well as information and history about our changelog process.
## Overview
@@ -19,19 +19,71 @@ author: Ozzy Osbourne
The `merge_request` value is a reference to a merge request that adds this
entry, and the `author` key is used to give attribution to community
-contributors. Both are optional.
+contributors. **Both are optional**.
Community contributors and core team members are encouraged to add their name to
-the `author` field. GitLab team members should not.
-
-If you're working on the GitLab EE repository, the entry will be added to
-`changelogs/unreleased-ee/` instead.
+the `author` field. GitLab team members **should not**.
[changelog.md]: https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG.md
[unreleased]: https://gitlab.com/gitlab-org/gitlab-ce/tree/master/changelogs/
[YAML]: https://en.wikipedia.org/wiki/YAML
-## Instructions
+## What warrants a changelog entry?
+
+- Any user-facing change **should** have a changelog entry. Example: "GitLab now
+ uses system fonts for all text."
+- 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.
+- Any developer-facing change (e.g., refactoring, technical debt remediation,
+ test suite changes) **should not** have a changelog entry. Example: "Reduce
+ database records created during Cycle Analytics model spec."
+- _Any_ contribution from a community member, no matter how small, **may** have
+ a changelog entry regardless of these guidelines if the contributor wants one.
+ Example: "Fixed a typo on the search results page. (Jane Smith)"
+
+## Writing good changelog entries
+
+A good changelog entry should be descriptive and concise. It should explain the
+change to a reader who has _zero context_ about the change. If you have trouble
+making it both concise and descriptive, err on the side of descriptive.
+
+- **Bad:** Go to a project order.
+- **Good:** Show a user's starred projects at the top of the "Go to project"
+ dropdown.
+
+The first example provides no context of where the change was made, or why, or
+how it benefits the user.
+
+- **Bad:** Copy [some text] to clipboard.
+- **Good:** Update the "Copy to clipboard" tooltip to indicate what's being
+ copied.
+
+Again, the first example is too vague and provides no context.
+
+- **Bad:** Fixes and Improves CSS and HTML problems in mini pipeline graph and
+ builds dropdown.
+- **Good:** Fix tooltips and hover states in mini pipeline graph and builds
+ dropdown.
+
+The first example is too focused on implementation details. The user doesn't
+care that we changed CSS and HTML, they care about the _end result_ of those
+changes.
+
+- **Bad:** Strip out `nil`s in the Array of Commit objects returned from
+ `find_commits_by_message_with_elastic`
+- **Good:** Fix 500 errors caused by elasticsearch results referencing
+ garbage-collected commits
+
+The first example focuses on _how_ we fixed something, not on _what_ it fixes.
+The rewritten version clearly describes the _end benefit_ to the user (fewer 500
+errors), and _when_ (searching commits with ElasticSearch).
+
+Use your best judgement and try to put yourself in the mindset of someone
+reading the compiled changelog. Does this entry add value? Does it offer context
+about _where_ and _why_ the change was made?
+
+## How to generate a changelog entry
A `bin/changelog` script is available to generate the changelog entry file
automatically.
@@ -55,19 +107,28 @@ title: Hey DZ, I added a feature to GitLab!
merge_request:
author:
```
+If you're working on the GitLab EE repository, the entry will be added to
+`changelogs/unreleased-ee/` instead.
+
+#### Arguments
-### Arguments
+| Argument | Shorthand | Purpose |
+| ----------------- | --------- | --------------------------------------------- |
+| [`--amend`] | | Amend the previous commit |
+| [`--force`] | `-f` | Overwrite an existing entry |
+| [`--merge-request`] | `-m` | Set merge request ID |
+| [`--dry-run`] | `-n` | Don't actually write anything, just print |
+| [`--git-username`] | `-u` | Use Git user.name configuration as the author |
+| [`--help`] | `-h` | Print help message |
-| Argument | Shorthand | Purpose |
-| ----------------- | --------- | --------------------------------------------- |
-| `--amend` | | Amend the previous commit |
-| `--force` | `-f` | Overwrite an existing entry |
-| `--merge-request` | `-m` | Merge Request ID |
-| `--dry-run` | `-n` | Don't actually write anything, just print |
-| `--git-username` | `-u` | Use Git user.name configuration as the author |
-| `--help` | `-h` | Print help message |
+[`--amend`]: #-amend
+[`--force`]: #-force-or-f
+[`--merge-request`]: #-merge-request-or-m
+[`--dry-run`]: #-dry-run-or-n
+[`--git-username`]: #-git-username-or-u
+[`--help`]: #-help
-#### `--amend`
+##### `--amend`
You can pass the **`--amend`** argument to automatically stage the generated
file and amend it to the previous commit.
@@ -88,7 +149,7 @@ merge_request:
author:
```
-#### `--force` or `-f`
+##### `--force` or `-f`
Use **`--force`** or **`-f`** to overwrite an existing changelog entry if it
already exists.
@@ -105,7 +166,7 @@ merge_request: 1983
author:
```
-#### `--merge-request` or `-m`
+##### `--merge-request` or `-m`
Use the **`--merge-request`** or **`-m`** argument to provide the
`merge_request` value:
@@ -119,7 +180,7 @@ merge_request: 1983
author:
```
-#### `--dry-run` or `-n`
+##### `--dry-run` or `-n`
Use the **`--dry-run`** or **`-n`** argument to prevent actually writing or
committing anything:
@@ -135,7 +196,7 @@ author:
$ ls changelogs/unreleased/
```
-#### `--git-username` or `-u`
+##### `--git-username` or `-u`
Use the **`--git-username`** or **`-u`** argument to automatically fill in the
`author` value with your configured Git `user.name` value:
@@ -152,7 +213,7 @@ merge_request:
author: Jane Doe
```
-## History and Reasoning
+### History and Reasoning
Our `CHANGELOG` file was previously updated manually by each contributor that
felt their change warranted an entry. When two merge requests added their own
diff --git a/doc/development/frontend.md b/doc/development/frontend.md
index d646de7c54a..766b5f1f477 100644
--- a/doc/development/frontend.md
+++ b/doc/development/frontend.md
@@ -32,6 +32,21 @@ You can find the Frontend Architecture experts on the [team page][team-page].
You can find documentation about the desired architecture for a new feature built with Vue.js in [here][vue-section].
+### Realtime
+
+When writing code for realtime features we have to keep a couple of things in mind:
+1. Do not overload the server with requests.
+1. It should feel realtime.
+
+Thus, we must strike a balance between sending requests and the feeling of realtime. Use the following rules when creating realtime solutions.
+
+1. The server will tell you how much to poll by sending `X-Poll-Interval` in the header. Use that as your polling interval. This way it is easy for system administrators to change the polling rate. A `X-Poll-Interval: -1` means you should disable polling, and this must be implemented.
+1. A response of `HTTP 429 Too Many Requests`, should disable polling as well. This must also be implemented.
+1. Use a common library for polling.
+1. Poll on active tabs only. Use a common library to find out which tab currently has eyes on it. Please use [Focus](https://gitlab.com/andrewn/focus). Specifically [Eyeballs Detector](https://gitlab.com/andrewn/focus/blob/master/lib/eyeballs-detector.js).
+1. Use regular polling intervals, do not use backoff polling, or jitter, as the interval will be controlled by the server.
+1. The backend code will most likely be using etags. You do not and should not check for status `304 Not Modified`. The browser will transform it for you.
+
### Vue
For more complex frontend features, we recommend using Vue.js. It shares
@@ -66,6 +81,8 @@ Let's look into each of them:
This is the index file of your new feature. This is where the root Vue instance
of the new feature should be.
+The Store and the Service should be imported and initialized in this file and provided as a prop to the main component.
+
Don't forget to follow [these steps.][page_specific_javascript]
**A folder for Components**
@@ -86,7 +103,7 @@ You can read more about components in Vue.js site, [Component System][component-
**A folder for the Store**
-The Store is a simple object that allows us to manage the state in a single
+The Store is a class that allows us to manage the state in a single
source of truth.
The concept we are trying to follow is better explained by Vue documentation
@@ -289,7 +306,7 @@ When exactly one object is needed for a given task, prefer to define it as a
`class` rather than as an object literal. Prefer also to explicitly restrict
instantiation, unless flexibility is important (e.g. for testing).
-```
+```javascript
// bad
gl.MyThing = {
@@ -332,6 +349,33 @@ gl.MyThing = MyThing;
```
+### Manipulating the DOM in a JS Class
+
+When writing a class that needs to manipulate the DOM guarantee a container option is provided.
+This is useful when we need that class to be instantiated more than once in the same page.
+
+Bad:
+```javascript
+class Foo {
+ constructor() {
+ document.querySelector('.bar');
+ }
+}
+new Foo();
+```
+
+Good:
+```javascript
+class Foo {
+ constructor(opts) {
+ document.querySelector(`${opts.container} .bar`);
+ }
+}
+
+new Foo({ container: '.my-element' });
+```
+You can find an example of the above in this [class][container-class-example];
+
## Supported browsers
For our currently-supported browsers, see our [requirements][requirements].
@@ -457,3 +501,4 @@ Scenario: Developer can approve merge request
[eslintrc]: https://gitlab.com/gitlab-org/gitlab-ce/blob/master/.eslintrc
[team-page]: https://about.gitlab.com/team
[vue-section]: https://docs.gitlab.com/ce/development/frontend.html#how-to-build-a-new-feature-with-vue-js
+[container-class-example]: https://gitlab.com/gitlab-org/gitlab-ce/blob/master/app/assets/javascripts/mini_pipeline_graph_dropdown.js
diff --git a/doc/development/merge_request_performance_guidelines.md b/doc/development/merge_request_performance_guidelines.md
index 8232a0a113c..2b4126b43ef 100644
--- a/doc/development/merge_request_performance_guidelines.md
+++ b/doc/development/merge_request_performance_guidelines.md
@@ -68,7 +68,7 @@ end
This will end up running one query for every object to update. This code can
easily overload a database given enough rows to update or many instances of this
code running in parallel. This particular problem is known as the
-["N+1 query problem"](http://guides.rubyonrails.org/active_record_querying.html#eager-loading-associations).
+["N+1 query problem"](http://guides.rubyonrails.org/active_record_querying.html#eager-loading-associations). You can write a test with [QueryRecoder](query_recorder.md) to detect this and prevent regressions.
In this particular case the workaround is fairly easy:
@@ -117,6 +117,8 @@ Post.all.includes(:author).each do |post|
end
```
+Also consider using [QueryRecoder tests](query_recorder.md) to prevent a regression when eager loading.
+
## Memory Usage
**Summary:** merge requests **must not** increase memory usage unless absolutely
diff --git a/doc/development/performance.md b/doc/development/performance.md
index c1f129e576c..04419650b12 100644
--- a/doc/development/performance.md
+++ b/doc/development/performance.md
@@ -39,6 +39,7 @@ GitLab provides built-in tools to aid the process of improving performance:
* [Sherlock](profiling.md#sherlock)
* [GitLab Performance Monitoring](../administration/monitoring/performance/introduction.md)
* [Request Profiling](../administration/monitoring/performance/request_profiling.md)
+* [QueryRecoder](query_recorder.md) for preventing `N+1` regressions
GitLab employees can use GitLab.com's performance monitoring systems located at
<http://performance.gitlab.net>, this requires you to log in using your
diff --git a/doc/development/polling.md b/doc/development/polling.md
new file mode 100644
index 00000000000..a7f2962acf0
--- /dev/null
+++ b/doc/development/polling.md
@@ -0,0 +1,41 @@
+# Polling with ETag caching
+
+Polling for changes (repeatedly asking server if there are any new changes)
+introduces high load on a GitLab instance, because it usually requires
+executing at least a few SQL queries. This makes scaling large GitLab
+instances (like GitLab.com) very difficult so we do not allow adding new
+features that require polling and hit the database.
+
+Instead you should use polling mechanism with ETag caching in Redis.
+
+## How to use it
+
+1. Add the path of the endpoint which you want to poll to
+ `Gitlab::EtagCaching::Middleware`.
+1. Implement cache invalidation for the path of your endpoint using
+ `Gitlab::EtagCaching::Store`. Whenever a resource changes you
+ have to invalidate the ETag for the path that depends on this
+ resource.
+1. Check that the mechanism works:
+ - requests should return status code 304
+ - there should be no SQL queries logged in `log/development.log`
+
+## How it works
+
+1. Whenever a resource changes we generate a random value and store it in
+ Redis.
+1. When a client makes a request we set the `ETag` response header to the value
+ from Redis.
+1. The client caches the response (client-side caching) and sends the ETag as
+ the `If-None-Match` header with every subsequent request for the same
+ resource.
+1. If the `If-None-Match` header matches the current value in Redis we know
+ that the resource did not change so we can send 304 response immediately,
+ without querying the database at all. The client's browser will use the
+ cached response.
+1. If the `If-None-Match` header does not match the current value in Redis
+ we have to generate a new response, because the resource changed.
+
+For more information see:
+- [RFC 7232](https://tools.ietf.org/html/rfc7232)
+- [ETag proposal](https://gitlab.com/gitlab-org/gitlab-ce/issues/26926)
diff --git a/doc/development/profiling.md b/doc/development/profiling.md
index e244ad4e881..933033a09e0 100644
--- a/doc/development/profiling.md
+++ b/doc/development/profiling.md
@@ -25,3 +25,5 @@ starting GitLab. For example:
Bullet will log query problems to both the Rails log as well as the Chrome
console.
+
+As a follow up to finding `N+1` queries with Bullet, consider writing a [QueryRecoder test](query_recorder.md) to prevent a regression.
diff --git a/doc/development/query_recorder.md b/doc/development/query_recorder.md
new file mode 100644
index 00000000000..e0127aaed4c
--- /dev/null
+++ b/doc/development/query_recorder.md
@@ -0,0 +1,29 @@
+# QueryRecorder
+
+QueryRecorder is a tool for detecting the [N+1 queries problem](http://guides.rubyonrails.org/active_record_querying.html#eager-loading-associations) from tests.
+
+> Implemented in [spec/support/query_recorder.rb](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/spec/support/query_recorder.rb) via [9c623e3e](https://gitlab.com/gitlab-org/gitlab-ce/commit/9c623e3e5d7434f2e30f7c389d13e5af4ede770a)
+
+As a rule, merge requests [should not increase query counts](merge_request_performance_guidelines.md#query-counts). If you find yourself adding something like `.includes(:author, :assignee)` to avoid having `N+1` queries, consider using QueryRecorder to enforce this with a test. Without this, a new feature which causes an additional model to be accessed will silently reintroduce the problem.
+
+## How it works
+
+This style of test works by counting the number of SQL queries executed by ActiveRecord. First a control count is taken, then you add new records to the database and rerun the count. If the number of queries has significantly increased then an `N+1` queries problem exists.
+
+```ruby
+it "avoids N+1 database queries" do
+ control_count = ActiveRecord::QueryRecorder.new { visit_some_page }.count
+ create_list(:issue, 5)
+ expect { visit_some_page }.not_to exceed_query_limit(control_count)
+end
+```
+
+As an example you might create 5 issues in between counts, which would cause the query count to increase by 5 if an N+1 problem exists.
+
+> **Note:** In some cases the query count might change slightly between runs for unrelated reasons. In this case you might need to test `exceed_query_limit(control_count + acceptable_change)`, but this should be avoided if possible.
+
+## See also
+
+- [Bullet](profiling.md#Bullet) For finding `N+1` query problems
+- [Performance guidelines](performance.md)
+- [Merge request performance guidelines](merge_request_performance_guidelines.md#query-counts) \ No newline at end of file
diff --git a/doc/development/testing.md b/doc/development/testing.md
index 9b545d7f0f1..5ac7b8dadeb 100644
--- a/doc/development/testing.md
+++ b/doc/development/testing.md
@@ -35,8 +35,8 @@ GitLab uses [Karma] to run its [Jasmine] JavaScript specs. They can be run on
the command line via `bundle exec karma`.
- JavaScript tests live in `spec/javascripts/`, matching the folder structure of
- `app/assets/javascripts/`: `app/assets/javascripts/behaviors/autosize.js.es6` has a corresponding
- `spec/javascripts/behaviors/autosize_spec.js.es6` file.
+ `app/assets/javascripts/`: `app/assets/javascripts/behaviors/autosize.js` has a corresponding
+ `spec/javascripts/behaviors/autosize_spec.js` 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.