diff options
| author | Lin Jen-Shin <godfat@godfat.org> | 2017-12-15 12:14:26 +0300 |
|---|---|---|
| committer | Lin Jen-Shin <godfat@godfat.org> | 2017-12-15 12:14:26 +0300 |
| commit | 59ac184fcf64f1812fbfd88a00ea029ca3c1f4e7 (patch) | |
| tree | 5db0594a6f568f02b4f54c6bf4eabe01229a9f95 /doc/development | |
| parent | 85be6d83be4632c76760e373da131a90afb093b9 (diff) | |
| parent | 1baea77438779e74657b49ca26810d6c8f041b41 (diff) | |
Merge remote-tracking branch 'upstream/master' into no-ivar-in-modules
* upstream/master: (671 commits)
Make rubocop happy
Use guard clause
Improve language
Prettify
Use temp branch
Pass info about who started the job and which job triggered it
Docs: add indexes for monitoring and performance monitoring
clearer-documentation-on-inline-diffs
Add docs for commit diff discussion in merge requests
sorting for tags api
Clear BatchLoader after each spec to prevent holding onto records longer than necessary
Include project in BatchLoader key to prevent returning blobs for the wrong project
moved lfs_blob_ids method into ExtractsPath module
Converted JS modules into exported modules
spec fixes
Bump gitlab-shell version to 5.10.3
Clear caches before updating MR diffs
Use new Ruby version 2.4 in GitLab QA images
moved lfs blob fetch from extractspath file
Update GitLab QA dependencies
...
Diffstat (limited to 'doc/development')
| -rw-r--r-- | doc/development/README.md | 3 | ||||
| -rw-r--r-- | doc/development/automatic_ce_ee_merge.md | 93 | ||||
| -rw-r--r-- | doc/development/background_migrations.md | 10 | ||||
| -rw-r--r-- | doc/development/changelog.md | 2 | ||||
| -rw-r--r-- | doc/development/doc_styleguide.md | 6 | ||||
| -rw-r--r-- | doc/development/ee_features.md | 8 | ||||
| -rw-r--r-- | doc/development/fe_guide/dropdowns.md | 12 | ||||
| -rw-r--r-- | doc/development/fe_guide/style_guide_js.md | 122 | ||||
| -rw-r--r-- | doc/development/i18n/externalization.md | 3 | ||||
| -rw-r--r-- | doc/development/limit_ee_conflicts.md | 347 | ||||
| -rw-r--r-- | doc/development/performance.md | 2 | ||||
| -rw-r--r-- | doc/development/sidekiq_style_guide.md | 70 | ||||
| -rw-r--r-- | doc/development/ux_guide/components.md | 18 | ||||
| -rw-r--r-- | doc/development/ux_guide/copy.md | 12 | ||||
| -rw-r--r-- | doc/development/writing_documentation.md | 15 |
15 files changed, 252 insertions, 471 deletions
diff --git a/doc/development/README.md b/doc/development/README.md index c31e665e91a..6a493b4a7fa 100644 --- a/doc/development/README.md +++ b/doc/development/README.md @@ -16,7 +16,8 @@ comments: false - [GitLab core team & GitLab Inc. contribution process](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/PROCESS.md) - [Generate a changelog entry with `bin/changelog`](changelog.md) - [Code review guidelines](code_review.md) for reviewing code and having code reviewed. -- [Limit conflicts with EE when developing on CE](limit_ee_conflicts.md) +- [Automatic CE->EE merge](automatic_ce_ee_merge.md) +- [Guidelines for implementing Enterprise Edition features](ee_features.md) ## UX and frontend guides diff --git a/doc/development/automatic_ce_ee_merge.md b/doc/development/automatic_ce_ee_merge.md new file mode 100644 index 00000000000..4b9791c95bc --- /dev/null +++ b/doc/development/automatic_ce_ee_merge.md @@ -0,0 +1,93 @@ +# Automatic CE->EE merge + +GitLab Community Edition is merged automatically every 3 hours into the +Enterprise Edition (look for the [`CE Upstream` merge requests]). + +This merge is done automatically in a +[scheduled pipeline](https://gitlab.com/gitlab-org/release-tools/-/jobs/43201679). +If a merge is already in progress, the job [doesn't create a new one](https://gitlab.com/gitlab-org/release-tools/-/jobs/43157687). + +**If you are pinged in a `CE Upstream` merge request to resolve a conflict, +please resolve the conflict as soon as possible or ask someone else to do it!** + +>**Note:** +It's ok to resolve more conflicts than the one that you are asked to resolve. In +that case, it's a good habit to ask for a double-check on your resolution by +someone who is familiar with the code you touched. + +[`CE Upstream` merge requests]: https://gitlab.com/gitlab-org/gitlab-ee/merge_requests?label_name%5B%5D=CE+upstream + +## Always merge EE merge requests before their CE counterparts + +**In order to avoid conflicts in the CE->EE merge, you should always merge the +EE version of your CE merge request first, if present.** + +The rationale for this is that as CE->EE merges are done automatically every few +hours, it can happen that: + +1. A CE merge request that needs EE-specific changes is merged +1. The automatic CE->EE merge happens +1. Conflicts due to the CE merge request occur since its EE merge request isn't + merged yet +1. The automatic merge bot will ping someone to resolve the conflict **that are + already resolved in the EE merge request that isn't merged yet** + +That's a waste of time, and that's why you should merge EE merge request before +their CE counterpart. + +## Avoiding CE->EE merge conflicts beforehand + +To avoid the conflicts beforehand, check out the +[Guidelines for implementing Enterprise Edition features](ee_features.md). + +In any case, the CI `ee_compat_check` job will tell you if you need to open an +EE version of your CE merge request. + +### Conflicts detection in CE merge requests + +For each commit (except on `master`), the `ee_compat_check` CI job tries to +detect if the current branch's changes will conflict during the CE->EE merge. + +The job reports what files are conflicting and how to setup a merge request +against EE. + +#### How the job works + +1. Generates the diff between your branch and current CE `master` +1. Tries to apply it to current EE `master` +1. If it applies cleanly, the job succeeds, otherwise... +1. Detects a branch with the `ee-` prefix or `-ee` suffix in EE +1. If it exists, generate the diff between this branch and current EE `master` +1. Tries to apply it to current EE `master` +1. If it applies cleanly, the job succeeds + +In the case where the job fails, it means you should create a `ee-<ce_branch>` +or `<ce_branch>-ee` branch, push it to EE and open a merge request against EE +`master`. +At this point if you retry the failing job in your CE merge request, it should +now pass. + +Notes: + +- This task is not a silver-bullet, its current goal is to bring awareness to + developers that their work needs to be ported to EE. +- Community contributors shouldn't be required to submit merge requests against + EE, but reviewers should take actions by either creating such EE merge request + or asking a GitLab developer to do it **before the merge request is merged**. +- If you branch is too far behind `master`, the job will fail. In that case you + should rebase your branch upon latest `master`. +- Code reviews for merge requests often consist of multiple iterations of + feedback and fixes. There is no need to update your EE MR after each + iteration. Instead, create an EE MR as soon as you see the + `ee_compat_check` job failing. After you receive the final approval + from a Maintainer (but **before the CE MR is merged**) update the EE MR. + This helps to identify significant conflicts sooner, but also reduces the + number of times you have to resolve conflicts. +- Please remember to + [always have your EE merge request merged before the CE version](#always-merge-ee-merge-requests-before-their-ce-counterparts). +- You can use [`git rerere`](https://git-scm.com/blog/2010/03/08/rerere.html) + to avoid resolving the same conflicts multiple times. + +--- + +[Return to Development documentation](README.md) diff --git a/doc/development/background_migrations.md b/doc/development/background_migrations.md index 5452b0e7a2f..fd2b9d0e908 100644 --- a/doc/development/background_migrations.md +++ b/doc/development/background_migrations.md @@ -68,10 +68,10 @@ BackgroundMigrationWorker.perform_async('BackgroundMigrationClassName', [arg1, a ``` Usually it's better to enqueue jobs in bulk, for this you can use -`BackgroundMigrationWorker.perform_bulk`: +`BackgroundMigrationWorker.bulk_perform_async`: ```ruby -BackgroundMigrationWorker.perform_bulk( +BackgroundMigrationWorker.bulk_perform_async( [['BackgroundMigrationClassName', [1]], ['BackgroundMigrationClassName', [2]]] ) @@ -85,13 +85,13 @@ updates. Removals in turn can be handled by simply defining foreign keys with cascading deletes. If you would like to schedule jobs in bulk with a delay, you can use -`BackgroundMigrationWorker.perform_bulk_in`: +`BackgroundMigrationWorker.bulk_perform_in`: ```ruby jobs = [['BackgroundMigrationClassName', [1]], ['BackgroundMigrationClassName', [2]]] -BackgroundMigrationWorker.perform_bulk_in(5.minutes, jobs) +BackgroundMigrationWorker.bulk_perform_in(5.minutes, jobs) ``` ## Cleaning Up @@ -201,7 +201,7 @@ class ScheduleExtractServicesUrl < ActiveRecord::Migration ['ExtractServicesUrl', [id]] end - BackgroundMigrationWorker.perform_bulk(jobs) + BackgroundMigrationWorker.bulk_perform_async(jobs) end end diff --git a/doc/development/changelog.md b/doc/development/changelog.md index f869938fe11..48cffc0dd18 100644 --- a/doc/development/changelog.md +++ b/doc/development/changelog.md @@ -80,7 +80,7 @@ changes. 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). +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 diff --git a/doc/development/doc_styleguide.md b/doc/development/doc_styleguide.md index aaa7032cadb..db13e0e6249 100644 --- a/doc/development/doc_styleguide.md +++ b/doc/development/doc_styleguide.md @@ -170,12 +170,6 @@ You can combine one or more of the following: = link_to 'Help page', help_page_path('user/permissions'), class: 'btn btn-info' ``` -1. **Underlining a link.** - - ```haml - = link_to 'Help page', help_page_path('user/permissions'), class: 'underlined-link' - ``` - 1. **Using links inline of some text.** ```haml diff --git a/doc/development/ee_features.md b/doc/development/ee_features.md index 932a44f65e4..1af839a27e1 100644 --- a/doc/development/ee_features.md +++ b/doc/development/ee_features.md @@ -1,4 +1,4 @@ -# Guidelines for implementing Enterprise Edition feature +# Guidelines for implementing Enterprise Edition features - **Write the code and the tests.**: As with any code, EE features should have good test coverage to prevent regressions. @@ -380,3 +380,9 @@ to avoid conflicts during CE to EE merge. } } ``` + +## gitlab-svgs + +Conflicts in `app/assets/images/icons.json` or `app/assets/images/icons.svg` can +be resolved simply by regenerating those assets with +[`yarn run svg`](https://gitlab.com/gitlab-org/gitlab-svgs). diff --git a/doc/development/fe_guide/dropdowns.md b/doc/development/fe_guide/dropdowns.md index e1660ac5caa..6314f8f38d2 100644 --- a/doc/development/fe_guide/dropdowns.md +++ b/doc/development/fe_guide/dropdowns.md @@ -4,15 +4,15 @@ ## How to style a bootstrap dropdown 1. Use the HTML structure provided by the [docs][bootstrap-dropdowns] 1. Add a specific class to the top level `.dropdown` element - - + + ```Haml .dropdown.my-dropdown %button{ type: 'button', data: { toggle: 'dropdown' }, 'aria-haspopup': true, 'aria-expanded': false } %span.dropdown-toggle-text Toggle Dropdown = icon('chevron-down') - + %ul.dropdown-menu %li %a @@ -29,10 +29,4 @@ item! ``` -1. Include the mixin in CSS - - ```SCSS - @include new-style-dropdown('.my-dropdown '); - ``` - [bootstrap-dropdowns]: https://getbootstrap.com/docs/3.3/javascript/#dropdowns diff --git a/doc/development/fe_guide/style_guide_js.md b/doc/development/fe_guide/style_guide_js.md index 10f4c5a0902..1cd66f27492 100644 --- a/doc/development/fe_guide/style_guide_js.md +++ b/doc/development/fe_guide/style_guide_js.md @@ -86,34 +86,34 @@ followed by any global declarations, then a blank newline prior to any imports o #### Modules, Imports, and Exports 1. Use ES module syntax to import modules - ```javascript - // bad - const SomeClass = require('some_class'); + ```javascript + // bad + const SomeClass = require('some_class'); - // good - import SomeClass from 'some_class'; + // good + import SomeClass from 'some_class'; - // bad - module.exports = SomeClass; + // bad + module.exports = SomeClass; - // good - export default SomeClass; - ``` - - Import statements are following usual naming guidelines, for example object literals use camel case: - - ```javascript - // some_object file - export default { - key: 'value', - }; - - // bad - import ObjectLiteral from 'some_object'; + // good + export default SomeClass; + ``` + + Import statements are following usual naming guidelines, for example object literals use camel case: - // good - import objectLiteral from 'some_object'; - ``` + ```javascript + // some_object file + export default { + key: 'value', + }; + + // bad + import ObjectLiteral from 'some_object'; + + // good + import objectLiteral from 'some_object'; + ``` 1. Relative paths: when importing a module in the same directory, a child directory, or an immediate parent directory prefer relative paths. When @@ -334,33 +334,33 @@ A forEach will cause side effects, it will be mutating the array being iterated. #### Alignment 1. Follow these alignment styles for the template method: 1. With more than one attribute, all attributes should be on a new line: - ```javascript - // bad - <component v-if="bar" - param="baz" /> + ```javascript + // bad + <component v-if="bar" + param="baz" /> - <button class="btn">Click me</button> + <button class="btn">Click me</button> - // good - <component - v-if="bar" - param="baz" - /> + // good + <component + v-if="bar" + param="baz" + /> - <button class="btn"> - Click me - </button> - ``` + <button class="btn"> + Click me + </button> + ``` 1. The tag can be inline if there is only one attribute: - ```javascript - // good - <component bar="bar" /> + ```javascript + // good + <component bar="bar" /> - // good - <component - bar="bar" - /> - ``` + // good + <component + bar="bar" + /> + ``` #### Quotes 1. Always use double quotes `"` inside templates and single quotes `'` for all other JS. @@ -414,7 +414,6 @@ A forEach will cause side effects, it will be mutating the array being iterated. 1. Default key should be provided if the prop is not required. _Note:_ There are some scenarios where we need to check for the existence of the property. On those a default key should not be provided. - ```javascript // good props: { @@ -494,21 +493,20 @@ On those a default key should not be provided. #### Ordering 1. Tag order in `.vue` file - - ``` - <script> - // ... - </script> - - <template> - // ... - </template> - - // We don't use scoped styles but there are few instances of this - <style> - // ... - </style> - ``` + ``` + <script> + // ... + </script> + + <template> + // ... + </template> + + // We don't use scoped styles but there are few instances of this + <style> + // ... + </style> + ``` 1. Properties in a Vue Component: 1. `name` diff --git a/doc/development/i18n/externalization.md b/doc/development/i18n/externalization.md index 4b65a0f4a35..43b996d9395 100644 --- a/doc/development/i18n/externalization.md +++ b/doc/development/i18n/externalization.md @@ -215,6 +215,9 @@ There is also and alternative method to [translate messages from validation erro sprintf(__('Hello %{username}'), { username: 'Joe' }) => 'Hello Joe' ``` +The placeholders should match the code style of the respective source file. +For example use `%{created_at}` in Ruby but `%{createdAt}` in JavaScript. + ### Plurals - In Ruby/HAML: diff --git a/doc/development/limit_ee_conflicts.md b/doc/development/limit_ee_conflicts.md deleted file mode 100644 index ba82babb38a..00000000000 --- a/doc/development/limit_ee_conflicts.md +++ /dev/null @@ -1,347 +0,0 @@ -# Limit conflicts with EE when developing on CE - -This guide contains best-practices for avoiding conflicts between CE and EE. - -## Daily CE Upstream merge - -GitLab Community Edition is merged daily into the Enterprise Edition (look for -the [`CE Upstream` merge requests]). The daily merge is currently done manually -by four individuals. - -**If a developer pings you in a `CE Upstream` merge request for help with -resolving conflicts, please help them because it means that you didn't do your -job to reduce the conflicts nor to ease their resolution in the first place!** - -To avoid the conflicts beforehand when working on CE, there are a few tools and -techniques that can help you: - -- know what are the usual types of conflicts and how to prevent them -- the CI `rake ee_compat_check` job tells you if you need to open an EE-version - of your CE merge request - -[`CE Upstream` merge requests]: https://gitlab.com/gitlab-org/gitlab-ee/merge_requests?label_name%5B%5D=CE+upstream - -## Check the status of the CI `rake ee_compat_check` job - -For each commit (except on `master`), the `rake ee_compat_check` CI job tries to -detect if the current branch's changes will conflict during the CE->EE merge. - -The job reports what files are conflicting and how to setup a merge request -against EE. Here is roughly how it works: - -1. Generates the diff between your branch and current CE `master` -1. Tries to apply it to current EE `master` -1. If it applies cleanly, the job succeeds, otherwise... -1. Detects a branch with the `-ee` suffix in EE -1. If it exists, generate the diff between this branch and current EE `master` -1. Tries to apply it to current EE `master` -1. If it applies cleanly, the job succeeds - -In the case where the job fails, it means you should create a `<ce_branch>-ee` -branch, push it to EE and open a merge request against EE `master`. At this -point if you retry the failing job in your CE merge request, it should now pass. - -Notes: - -- This task is not a silver-bullet, its current goal is to bring awareness to - developers that their work needs to be ported to EE. -- Community contributors shouldn't submit merge requests against EE, but - reviewers should take actions by either creating such EE merge request or - asking a GitLab developer to do it once the merge request is merged. -- If you branch is more than 500 commits behind `master`, the job will fail and - you should rebase your branch upon latest `master`. -- Code reviews for merge requests often consist of multiple iterations of - feedback and fixes. There is no need to update your EE MR after each - iteration. Instead, create an EE MR as soon as you see the - `rake ee_compat_check` job failing. After you receive the final acceptance - from a Maintainer (but before the CE MR is merged) update the EE MR. - This helps to identify significant conflicts sooner, but also reduces the - number of times you have to resolve conflicts. -- You can use [`git rerere`](https://git-scm.com/blog/2010/03/08/rerere.html) - to avoid resolving the same conflicts multiple times. - -## Possible type of conflicts - -### Controllers - -#### List or arrays are augmented in EE - -In controllers, the most common type of conflict is with `before_action` that -has a list of actions in CE but EE adds some actions to that list. - -The same problem often occurs for `params.require` / `params.permit` calls. - -##### Mitigations - -Separate CE and EE actions/keywords. For instance for `params.require` in -`ProjectsController`: - -```ruby -def project_params - params.require(:project).permit(project_params_ce) - # On EE, this is always: - # params.require(:project).permit(project_params_ce << project_params_ee) -end - -# Always returns an array of symbols, created however best fits the use case. -# It _should_ be sorted alphabetically. -def project_params_ce - %i[ - description - name - path - ] -end - -# (On EE) -def project_params_ee - %i[ - approvals_before_merge - approver_group_ids - approver_ids - ... - ] -end -``` - -#### Additional condition(s) in EE - -For instance for LDAP: - -```diff - def destroy - @key = current_user.keys.find(params[:id]) - - @key.destroy - + @key.destroy unless @key.is_a? LDAPKey - - respond_to do |format| -``` - -Or for Geo: - -```diff -def after_sign_out_path_for(resource) -- current_application_settings.after_sign_out_path.presence || new_user_session_path -+ if Gitlab::Geo.secondary? -+ Gitlab::Geo.primary_node.oauth_logout_url(@geo_logout_state) -+ else -+ current_application_settings.after_sign_out_path.presence || new_user_session_path -+ end -end -``` - -Or even for audit log: - -```diff -def approve_access_request -- Members::ApproveAccessRequestService.new(membershipable, current_user, params).execute -+ member = Members::ApproveAccessRequestService.new(membershipable, current_user, params).execute -+ -+ log_audit_event(member, action: :create) - - redirect_to polymorphic_url([membershipable, :members]) -end -``` - -### Views - -#### Additional view code in EE - -A block of code added in CE conflicts because there is already another block -at the same place in EE - -##### Mitigations - -Blocks of code that are EE-specific should be moved to partials as much as -possible to avoid conflicts with big chunks of HAML code that that are not fun -to resolve when you add the indentation to the equation. - -For instance this kind of thing: - -```haml -.form-group.detail-page-description - = form.label :description, 'Description', class: 'control-label' - .col-sm-10 - = render layout: 'projects/md_preview', locals: { preview_class: "md-preview", referenced_users: true } do - = render 'projects/zen', f: form, attr: :description, - classes: 'note-textarea', - placeholder: "Write a comment or drag your files here...", - supports_quick_actions: !issuable.persisted? - = render 'projects/notes/hints', supports_quick_actions: !issuable.persisted? - .clearfix - .error-alert -- if issuable.is_a?(Issue) - .form-group - .col-sm-offset-2.col-sm-10 - .checkbox - = form.label :confidential do - = form.check_box :confidential - This issue is confidential and should only be visible to team members with at least Reporter access. -- if can?(current_user, :"admin_#{issuable.to_ability_name}", issuable.project) - - has_due_date = issuable.has_attribute?(:due_date) - %hr - .row - %div{ class: (has_due_date ? "col-lg-6" : "col-sm-12") } - .form-group.issue-assignee - = form.label :assignee_id, "Assignee", class: "control-label #{"col-lg-4" if has_due_date}" - .col-sm-10{ class: ("col-lg-8" if has_due_date) } - .issuable-form-select-holder - - if issuable.assignee_id - = form.hidden_field :assignee_id - = dropdown_tag(user_dropdown_label(issuable.assignee_id, "Assignee"), options: { toggle_class: "js-dropdown-keep-input js-user-search js-issuable-form-dropdown js-assignee-search", title: "Select assignee", filter: true, dropdown_class: "dropdown-menu-user dropdown-menu-selectable dropdown-menu-assignee js-filter-submit", - placeholder: "Search assignee", data: { first_user: current_user.try(:username), null_user: true, current_user: true, project_id: project.try(:id), selected: issuable.assignee_id, field_name: "#{issuable.class.model_name.param_key}[assignee_id]", default_label: "Assignee"} }) - .form-group.issue-milestone - = form.label :milestone_id, "Milestone", class: "control-label #{"col-lg-4" if has_due_date}" - .col-sm-10{ class: ("col-lg-8" if has_due_date) } - .issuable-form-select-holder - = render "shared/issuable/milestone_dropdown", selected: issuable.milestone, name: "#{issuable.class.model_name.param_key}[milestone_id]", show_any: false, show_upcoming: false, extra_class: "js-issuable-form-dropdown js-dropdown-keep-input", dropdown_title: "Select milestone" - .form-group - - has_labels = @labels && @labels.any? - = form.label :label_ids, "Labels", class: "control-label #{"col-lg-4" if has_due_date}" - = form.hidden_field :label_ids, multiple: true, value: '' - .col-sm-10{ class: "#{"col-lg-8" if has_due_date} #{'issuable-form-padding-top' if !has_labels}" } - .issuable-form-select-holder - = render "shared/issuable/label_dropdown", classes: ["js-issuable-form-dropdown"], selected: issuable.labels, data_options: { field_name: "#{issuable.class.model_name.param_key}[label_ids][]", show_any: false }, dropdown_title: "Select label" - - if issuable.respond_to?(:weight) - - weight_options = Issue.weight_options - - weight_options.delete(Issue::WEIGHT_ALL) - - weight_options.delete(Issue::WEIGHT_ANY) - .form-group - = form.label :label_ids, class: "control-label #{"col-lg-4" if has_due_date}" do - Weight - .col-sm-10{ class: ("col-lg-8" if has_due_date) } - .issuable-form-select-holder - - if issuable.weight - = form.hidden_field :weight - = dropdown_tag(issuable.weight || "Weight", options: { title: "Select weight", toggle_class: 'js-weight-select js-issuable-form-weight', dropdown_class: "dropdown-menu-selectable dropdown-menu-weight", - placeholder: "Search weight", data: { field_name: "#{issuable.class.model_name.param_key}[weight]" , default_label: "Weight" } }) do - %ul - - weight_options.each do |weight| - %li - %a{href: "#", data: { id: weight, none: weight === Issue::WEIGHT_NONE }, class: ("is-active" if issuable.weight == weight)} - = weight - - if has_due_date - .col-lg-6 - .form-group - = form.label :due_date, "Due date", class: "control-label" - .col-sm-10 - .issuable-form-select-holder - = form.text_field :due_date, id: "issuable-due-date", class: "datepicker form-control", placeholder: "Select due date" -``` - -could be simplified by using partials: - -```haml -= render 'shared/issuable/form/description', issuable: issuable, form: form - -- if issuable.respond_to?(:confidential) - .form-group - .col-sm-offset-2.col-sm-10 - .checkbox - = form.label :confidential do - = form.check_box :confidential - This issue is confidential and should only be visible to team members with at least Reporter access. - -= render 'shared/issuable/form/metadata', issuable: issuable, form: form -``` - -and then the `app/views/shared/issuable/form/_metadata.html.haml` could be as follows: - -```haml -- issuable = local_assigns.fetch(:issuable) - -- return unless can?(current_user, :"admin_#{issuable.to_ability_name}", issuable.project) - -- has_due_date = issuable.has_attribute?(:due_date) -- has_labels = @labels && @labels.any? -- form = local_assigns.fetch(:form) - -%hr -.row - %div{ class: (has_due_date ? "col-lg-6" : "col-sm-12") } - .form-group.issue-assignee - = form.label :assignee_id, "Assignee", class: "control-label #{"col-lg-4" if has_due_date}" - .col-sm-10{ class: ("col-lg-8" if has_due_date) } - .issuable-form-select-holder - - if issuable.assignee_id - = form.hidden_field :assignee_id - = dropdown_tag(user_dropdown_label(issuable.assignee_id, "Assignee"), options: { toggle_class: "js-dropdown-keep-input js-user-search js-issuable-form-dropdown js-assignee-search", title: "Select assignee", filter: true, dropdown_class: "dropdown-menu-user dropdown-menu-selectable dropdown-menu-assignee js-filter-submit", - placeholder: "Search assignee", data: { first_user: current_user.try(:username), null_user: true, current_user: true, project_id: issuable.project.try(:id), selected: issuable.assignee_id, field_name: "#{issuable.class.model_name.param_key}[assignee_id]", default_label: "Assignee"} }) - .form-group.issue-milestone - = form.label :milestone_id, "Milestone", class: "control-label #{"col-lg-4" if has_due_date}" - .col-sm-10{ class: ("col-lg-8" if has_due_date) } - .issuable-form-select-holder - = render "shared/issuable/milestone_dropdown", selected: issuable.milestone, name: "#{issuable.class.model_name.param_key}[milestone_id]", show_any: false, show_upcoming: false, extra_class: "js-issuable-form-dropdown js-dropdown-keep-input", dropdown_title: "Select milestone" - .form-group - - has_labels = @labels && @labels.any? - = form.label :label_ids, "Labels", class: "control-label #{"col-lg-4" if has_due_date}" - = form.hidden_field :label_ids, multiple: true, value: '' - .col-sm-10{ class: "#{"col-lg-8" if has_due_date} #{'issuable-form-padding-top' if !has_labels}" } - .issuable-form-select-holder - = render "shared/issuable/label_dropdown", classes: ["js-issuable-form-dropdown"], selected: issuable.labels, data_options: { field_name: "#{issuable.class.model_name.param_key}[label_ids][]", show_any: false }, dropdown_title: "Select label" - - = render "shared/issuable/form/weight", issuable: issuable, form: form - - - if has_due_date - .col-lg-6 - .form-group - = form.label :due_date, "Due date", class: "control-label" - .col-sm-10 - .issuable-form-select-holder - = form.text_field :due_date, id: "issuable-due-date", class: "datepicker form-control", placeholder: "Select due date" -``` - -and then the `app/views/shared/issuable/form/_weight.html.haml` could be as follows: - -```haml -- issuable = local_assigns.fetch(:issuable) - -- return unless issuable.respond_to?(:weight) - -- has_due_date = issuable.has_attribute?(:due_date) -- form = local_assigns.fetch(:form) - -.form-group - = form.label :label_ids, class: "control-label #{"col-lg-4" if has_due_date}" do - Weight - .col-sm-10{ class: ("col-lg-8" if has_due_date) } - .issuable-form-select-holder - - if issuable.weight - = form.hidden_field :weight - - = weight_dropdown_tag(issuable, toggle_class: 'js-issuable-form-weight') do - %ul - - Issue.weight_options.each do |weight| - %li - %a{ href: '#', data: { id: weight, none: weight === Issue::WEIGHT_NONE }, class: ("is-active" if issuable.weight == weight) } - = weight -``` - -Note: - -- The safeguards at the top allow to get rid of an unneccessary indentation level -- Here we only moved the 'Weight' code to a partial since this is the only - EE-specific code in that view, so it's the most likely to conflict, but you - are encouraged to use partials even for code that's in CE to logically split - big views into several smaller files. - -#### Indentation issue - -Sometimes a code block is indented more or less in EE because there's an -additional condition. - -##### Mitigations - -Blocks of code that are EE-specific should be moved to partials as much as -possible to avoid conflicts with big chunks of HAML code that that are not fun -to resolve when you add the indentation in the equation. - -### Assets - -#### gitlab-svgs - -Conflicts in `app/assets/images/icons.json` or `app/assets/images/icons.svg` can be resolved simply by regenerating those assets with [`yarn run svg`](https://gitlab.com/gitlab-org/gitlab-svgs). - ---- - -[Return to Development documentation](README.md) diff --git a/doc/development/performance.md b/doc/development/performance.md index 04419650b12..e7c5a6ca07a 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](../administration/monitoring/performance/introduction.md) +* [GitLab Performance Monitoring](../administration/monitoring/performance/index.md) * [Request Profiling](../administration/monitoring/performance/request_profiling.md) * [QueryRecoder](query_recorder.md) for preventing `N+1` regressions diff --git a/doc/development/sidekiq_style_guide.md b/doc/development/sidekiq_style_guide.md index 1e9fdbc65e2..59ebf41e09f 100644 --- a/doc/development/sidekiq_style_guide.md +++ b/doc/development/sidekiq_style_guide.md @@ -3,34 +3,60 @@ This document outlines various guidelines that should be followed when adding or modifying Sidekiq workers. -## Default Queue +## ApplicationWorker -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`. +All workers should include `ApplicationWorker` instead of `Sidekiq::Worker`, +which adds some convenience methods and automatically sets the queue based on +the worker's name. ## Dedicated Queues -Most workers should use their own queue. To ease this process a worker can -include the `DedicatedSidekiqQueue` concern as follows: +All workers should use their own queue, which is automatically set based on the +worker class name. For a worker named `ProcessSomethingWorker`, the queue name +would be `process_something`. If you're not sure what queue a worker uses, +you can find it using `SomeWorker.queue`. There is almost never a reason to +manually override the queue name using `sidekiq_options queue: :some_queue`. + +## Queue Namespaces + +While different workers cannot share a queue, they can share a queue namespace. + +Defining a queue namespace for a worker makes it possible to start a Sidekiq +process that automatically handles jobs for all workers in that namespace, +without needing to explicitly list all their queue names. If, for example, all +workers that are managed by sidekiq-cron use the `cronjob` queue namespace, we +can spin up a Sidekiq process specifically for these kinds of scheduled jobs. +If a new worker using the `cronjob` namespace is added later on, the Sidekiq +process will automatically pick up jobs for that worker too (after having been +restarted), without the need to change any configuration. + +A queue namespace can be set using the `queue_namespace` DSL class method: ```ruby -class ProcessSomethingWorker - include Sidekiq::Worker - include DedicatedSidekiqQueue +class SomeScheduledTaskWorker + include ApplicationWorker + + queue_namespace :cronjob + + # ... 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`. +Behind the scenes, this will set `SomeScheduledTaskWorker.queue` to +`cronjob:some_scheduled_task`. Commonly used namespaces will have their own +concern module that can easily be included into the worker class, and that may +set other Sidekiq options besides the queue namespace. `CronjobQueue`, for +example, sets the namespace, but also disables retries. + +`bundle exec sidekiq` is namespace-aware, and will automatically listen on all +queues in a namespace (technically: all queues prefixed with the namespace name) +when a namespace is provided instead of a simple queue name in the `--queue` +(`-q`) option, or in the `:queues:` section in `config/sidekiq_queues.yml`. -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. +Note that adding a worker to an existing namespace should be done with care, as +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. ## Tests @@ -39,7 +65,7 @@ tests should be placed in `spec/workers`. ## Removing or renaming queues -Try to avoid renaming or removing 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.
\ No newline at end of file +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. diff --git a/doc/development/ux_guide/components.md b/doc/development/ux_guide/components.md index 16a811dbc74..d396964e7c1 100644 --- a/doc/development/ux_guide/components.md +++ b/doc/development/ux_guide/components.md @@ -10,7 +10,7 @@ * [Tables](#tables) * [Blocks](#blocks) * [Panels](#panels) -* [Dialog modals](#dialog-modals) +* [Modals](#modals) * [Alerts](#alerts) * [Forms](#forms) * [Search box](#search-box) @@ -255,18 +255,18 @@ Skeleton loading can replace any existing UI elements for the period in which th --- -## Dialog modals +## Modals -Dialog modals are only used for having a conversation and confirmation with the user. The user is not able to access the features on the main page until closing the modal. +Modals are only used for having a conversation and confirmation with the user. The user is not able to access the features on the main page until closing the modal. ### Usage -* When the action is irreversible, dialog modals provide the details and confirm with the user before they take an advanced action. -* When the action will affect privacy or authorization, dialog modals provide advanced information and confirm with the user. +* When the action is irreversible, modals provide the details and confirm with the user before they take an advanced action. +* When the action will affect privacy or authorization, modals provide advanced information and confirm with the user. ### Style -* Dialog modals contain the header, body, and actions. +* Modals contain the header, body, and actions. * **Header(1):** The header title is a question instead of a descriptive phrase. * **Body(2):** The content in body should never be ambiguous and unclear. It provides specific information. * **Actions(3):** Contains a affirmative action, a dismissive action, and an extra action. The order of actions from left to right: Dismissive action → Extra action → Affirmative action @@ -277,13 +277,13 @@ Dialog modals are only used for having a conversation and confirmation with the ### Placement -* Dialog modals should always be the center of the screen horizontally and be positioned **72px** from the top. +* Modals should always be the center of the screen horizontally and be positioned **72px** from the top. -| Dialog with 2 actions | Dialog with 3 actions | Special confirmation | +| Modal with 2 actions | Modal with 3 actions | Special confirmation | | --------------------- | --------------------- | -------------------- | |  |  |  | -> TODO: Special case for dialog modal. +> TODO: Special case for modal. --- diff --git a/doc/development/ux_guide/copy.md b/doc/development/ux_guide/copy.md index 12e8d0a31bb..af842da7f62 100644 --- a/doc/development/ux_guide/copy.md +++ b/doc/development/ux_guide/copy.md @@ -46,11 +46,11 @@ Avoid using periods in solitary sentences in these elements: * Labels
* Hover text
* Bulleted lists
-* Dialog body text
+* Modal body text
Periods should be used for:
-* Lists or dialogs with multiple sentences
+* Lists or modals with multiple sentences
* Any sentence followed by a link
| :white_check_mark: **Do** place periods after sentences followed by a link | :no_entry_sign: **Don’t** place periods after a link if it‘s not followed by a sentence |
@@ -80,7 +80,7 @@ Omit punctuation after phrases and labels to create a cleaner and more readable | Punctuation mark | Copy and paste | HTML entity | Unicode | Mac shortcut | Windows shortcut | Description |
|---|---|---|---|---|---|---|
-| Period | **.** | | | | | Omit for single sentences in affordances like labels, hover text, bulleted lists, and dialog body text.<br><br>Use in lists or dialogs with multiple sentences, and any sentence followed by a link or inline code.<br><br>Place inside quotation marks unless you’re telling the reader what to enter and it’s ambiguous whether to include the period. |
+| Period | **.** | | | | | Omit for single sentences in affordances like labels, hover text, bulleted lists, and modal body text.<br><br>Use in lists or modals with multiple sentences, and any sentence followed by a link or inline code.<br><br>Place inside quotation marks unless you’re telling the reader what to enter and it’s ambiguous whether to include the period. |
| Comma | **,** | | | | | Place inside quotation marks.<br><br>Use a [serial comma][serial comma] in lists of three or more terms. |
| Exclamation point | **!** | | | | | Avoid exclamation points as they tend to come across as shouting. Some exceptions include greetings or congratulatory messages. |
| Colon | **:** | `:` | `\u003A` | | | Omit from labels, for example, in the labels for fields in a form. |
@@ -88,7 +88,7 @@ Omit punctuation after phrases and labels to create a cleaner and more readable | Quotation marks | **“**<br><br>**”**<br><br>**‘**<br><br>**’** | `“`<br><br>`”`<br><br>`‘`<br><br>`’` | `\u201C`<br><br>`\u201D`<br><br>`\u2018`<br><br>`\u2019` | <kbd>⌥ Option</kbd>+<kbd>[</kbd><br><br><kbd>⌥ Option</kbd>+<kbd>⇧ Shift</kbd>+<kbd>[</kbd><br><br><kbd>⌥ Option</kbd>+<kbd>]</kbd><br><br><kbd>⌥ Option</kbd>+<kbd>⇧ Shift</kbd>+<kbd>]</kbd> | <kbd>Alt</kbd>+<kbd>0 1 4 7</kbd><br><br><kbd>Alt</kbd>+<kbd>0 1 4 8</kbd><br><br><kbd>Alt</kbd>+<kbd>0 1 4 5</kbd><br><br><kbd>Alt</kbd>+<kbd>0 1 4 6</kbd> | Use proper quotation marks (also known as smart quotes, curly quotes, or typographer’s quotes) for quotes. Single quotation marks are used for quotes inside of quotes.<br><br>The right single quotation mark symbol is also used for apostrophes.<br><br>Don’t use primes, straight quotes, or free-standing accents for quotation marks. |
| Primes | **′**<br><br>**″** | `′`<br><br>`″` | `\u2032`<br><br>`\u2033` | | <kbd>Alt</kbd>+<kbd>8 2 4 2</kbd><br><br><kbd>Alt</kbd>+<kbd>8 2 4 3</kbd> | Use prime (′) only in abbreviations for feet, arcminutes, and minutes: 3° 15′<br><br>Use double-prime (″) only in abbreviations for inches, arcseconds, and seconds: 3° 15′ 35″<br><br>Don’t use quotation marks, straight quotes, or free-standing accents for primes. |
| Straight quotes and accents | **"**<br><br>**'**<br><br>**`**<br><br>**´** | `"`<br><br>`'`<br><br>```<br><br>`´` | `\u0022`<br><br>`\u0027`<br><br>`\u0060`<br><br>`\u00B4` | | | Don’t use straight quotes or free-standing accents for primes or quotation marks.<br><br>Proper typography never uses straight quotes. They are left over from the age of typewriters and their only modern use is for code. |
-| Ellipsis | **…** | `…` | | <kbd>⌥ Option</kbd>+<kbd>;</kbd> | <kbd>Alt</kbd>+<kbd>0 1 3 3</kbd> | Use to indicate an action in progress (“Downloading…”) or incomplete or truncated text. No space before the ellipsis.<br><br>Omit from menu items or buttons that open a dialog or start some other process. |
+| Ellipsis | **…** | `…` | | <kbd>⌥ Option</kbd>+<kbd>;</kbd> | <kbd>Alt</kbd>+<kbd>0 1 3 3</kbd> | Use to indicate an action in progress (“Downloading…”) or incomplete or truncated text. No space before the ellipsis.<br><br>Omit from menu items or buttons that open a modal or start some other process. |
| Chevrons | **«**<br><br>**»**<br><br>**‹**<br><br>**›**<br><br>**<**<br><br>**>** | `«`<br><br>`»`<br><br>`‹`<br><br>`›`<br><br>`<`<br><br>`>` | `\u00AB`<br><br>`\u00BB`<br><br>`\u2039`<br><br>`\u203A`<br><br>`\u003C`<br><br>`\u003E`<br><br> | | | Omit from links or buttons that open another page or move to the next or previous step in a process. Also known as angle brackets, angular quote brackets, or guillemets. |
| Em dash | **—** | `—` | `\u2014` | <kbd>⌥ Option</kbd>+<kbd>⇧ Shift</kbd>+<kbd>-</kbd> | <kbd>Alt</kbd>+<kbd>0 1 5 1</kbd> | Avoid using dashes to separate text. If you must use dashes for this purpose — like this — use an em dash surrounded by spaces. |
| En dash | **–** | `–` | `\u2013` | <kbd>⌥ Option</kbd>+<kbd>-</kbd> | <kbd>Alt</kbd>+<kbd>0 1 5 0</kbd> | Use an en dash without spaces instead of a hyphen to indicate a range of values, such as numbers, times, and dates: “3–5 kg”, “8:00 AM–12:30 PM”, “10–17 Jan” |
@@ -175,7 +175,7 @@ A **comment** is a written piece of text that users of GitLab can create. Commen #### Discussion
A **discussion** is a group of 1 or more comments. A discussion can include subdiscussions. Some discussions have the special capability of being able to be **resolved**. Both the comments in the discussion and the discussion itself can be resolved.
-## Confirmation dialogs
+## Modals
- Destruction buttons should be clear and always say what they are destroying.
E.g., `Delete page` instead of just `Delete`.
@@ -184,6 +184,8 @@ A **discussion** is a group of 1 or more comments. A discussion can include subd - Avoid the word `cancel` or `canceled` in the descriptive copy. It can be
confusing when you then see the `Cancel` button.
+see also: guidelines for [modal components](components.md#modals)
+
---
Portions of this page are modifications based on work created and shared by the [Android Open Source Project][android project] and used according to terms described in the [Creative Commons 2.5 Attribution License][creative commons].
diff --git a/doc/development/writing_documentation.md b/doc/development/writing_documentation.md index 68ba3dd2da3..48e04a40050 100644 --- a/doc/development/writing_documentation.md +++ b/doc/development/writing_documentation.md @@ -142,7 +142,7 @@ tests. If it doesn't, the whole test suite will run (including docs). --- When you submit a merge request to GitLab Community Edition (CE), there is an -additional job called `rake ee_compat_check` that runs against Enterprise +additional job called `ee_compat_check` that runs against Enterprise Edition (EE) and checks if your changes can apply cleanly to the EE codebase. If that job fails, read the instructions in the job log for what to do next. Contributors do not need to submit their changes to EE, GitLab Inc. employees @@ -152,12 +152,23 @@ CE and EE. ## Previewing the changes live If you want to preview the doc changes of your merge request live, you can use -the manual `review-docs-deploy` job in your merge request. +the manual `review-docs-deploy` job in your merge request. You will need at +least Master permissions to be able to run it and is currently enabled for the +following projects: + +- https://gitlab.com/gitlab-org/gitlab-ce +- https://gitlab.com/gitlab-org/gitlab-ee + +NOTE: **Note:** +You will need to push a branch to those repositories, it doesn't work for forks. TIP: **Tip:** If your branch contains only documentation changes, you can use [special branch names](#testing) to avoid long running pipelines. +In the mini pipeline graph, you should see an `>>` icon. Clicking on it will +reveal the `review-docs-deploy` job. Hit the play button for the job to start. +  This job will: |