From bd6f47ac0cf36fceb2bff2e5d1721ec9e1b1db72 Mon Sep 17 00:00:00 2001 From: Jean-Pierre Huynh Date: Thu, 27 Jun 2019 10:23:12 +0000 Subject: [PATCH 01/40] Add `--globoff` flag to the curl command for Jobs API. The existing command fail in some cases with the following error `curl: (3) [globbing] bad range specification in column 56`. This was found when running cURL from a `python:3.7` container that comes with curl version `7.52.1`. ``` root@91963a56cd8f:/# curl --version curl 7.52.1 (x86_64-pc-linux-gnu) libcurl/7.52.1 OpenSSL/1.0.2r zlib/1.2.8 libidn2/0.16 libpsl/0.17.0 (+libidn2/0.16) libssh2/1.7.0 nghttp2/1.18.1 librtmp/2.3 Protocols: dict file ftp ftps gopher http https imap imaps ldap ldaps pop3 pop3s rtmp rtsp scp sftp smb smbs smtp smtps telnet tftp Features: AsynchDNS IDN IPv6 Largefile GSS-API Kerberos SPNEGO NTLM NTLM_WB SSL libz TLS-SRP HTTP2 UnixSockets HTTPS-proxy PSL ``` This doesn't seem to be an issue with `curl 7.58.0` but it doesn't hurt to have that flag enabled. --- doc/api/jobs.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/api/jobs.md b/doc/api/jobs.md index 72973b69117..223bfed91a9 100644 --- a/doc/api/jobs.md +++ b/doc/api/jobs.md @@ -14,7 +14,7 @@ GET /projects/:id/jobs | `scope` | string **or** array of strings | no | Scope of jobs to show. Either one of or an array of the following: `created`, `pending`, `running`, `failed`, `success`, `canceled`, `skipped`, or `manual`. All jobs are returned if `scope` is not provided. | ```sh -curl --header "PRIVATE-TOKEN: " 'https://gitlab.example.com/api/v4/projects/1/jobs?scope[]=pending&scope[]=running' +curl --globoff --header "PRIVATE-TOKEN: " 'https://gitlab.example.com/api/v4/projects/1/jobs?scope[]=pending&scope[]=running' ``` Example of response From 8c81fa5bc3b23f6ff65b054d665698f09afa3544 Mon Sep 17 00:00:00 2001 From: Afeique Sheikh Date: Fri, 28 Jun 2019 14:36:10 +0000 Subject: [PATCH 02/40] Update job_artifacts.md to reflect change: https://gitlab.com/gitlab-org/gitlab-ce/issues/31771 "Developers should only be able to delete jobs if they own them or are Master" --- doc/user/project/pipelines/job_artifacts.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/doc/user/project/pipelines/job_artifacts.md b/doc/user/project/pipelines/job_artifacts.md index 002addfc043..f9feae36dbc 100644 --- a/doc/user/project/pipelines/job_artifacts.md +++ b/doc/user/project/pipelines/job_artifacts.md @@ -187,7 +187,8 @@ information in the UI. DANGER: **Warning:** This is a destructive action that leads to data loss. Use with caution. -If you have at least Developer [permissions](../../permissions.md#gitlab-cicd-permissions) +If you are either the owner of a given job or have Master +[permissions](../../permissions.md#gitlab-cicd-permissions) on the project, you can erase a single job via the UI which will also remove the artifacts and the job's trace. From 11be4df3286d1f61521b2f5d3a8b7bdd9b147116 Mon Sep 17 00:00:00 2001 From: Ezekiel Kigbo Date: Mon, 24 Jun 2019 15:30:35 +1000 Subject: [PATCH 03/40] Vue-i18n: app/assets/javascripts/ide directory i18n linting for .vue files under the app/assets/javascripts/ide directory --- .../components/commit_sidebar/list_item.vue | 3 ++- .../ide/components/external_link.vue | 2 +- .../ide/components/repo_editor.vue | 21 +++++++++++-------- .../ide/components/repo_file_status_icon.vue | 5 ++++- .../javascripts/ide/components/repo_tab.vue | 5 +++-- locale/gitlab.pot | 15 +++++++++++++ 6 files changed, 37 insertions(+), 14 deletions(-) diff --git a/app/assets/javascripts/ide/components/commit_sidebar/list_item.vue b/app/assets/javascripts/ide/components/commit_sidebar/list_item.vue index 4be4b02ac1e..c8fbc3cb9f1 100644 --- a/app/assets/javascripts/ide/components/commit_sidebar/list_item.vue +++ b/app/assets/javascripts/ide/components/commit_sidebar/list_item.vue @@ -107,7 +107,8 @@ export default { @click="openFileInEditor" > - {{ file.name }} + + {{ file.name }}
diff --git a/app/assets/javascripts/ide/components/external_link.vue b/app/assets/javascripts/ide/components/external_link.vue index 954f84cea17..d1857f0176a 100644 --- a/app/assets/javascripts/ide/components/external_link.vue +++ b/app/assets/javascripts/ide/components/external_link.vue @@ -27,7 +27,7 @@ export default { target="_blank" rel="noopener noreferrer" > - Open in file view + {{ __('Open in file view') }}
diff --git a/app/assets/javascripts/ide/components/repo_editor.vue b/app/assets/javascripts/ide/components/repo_editor.vue index b0c4969c5e4..f0879baba2d 100644 --- a/app/assets/javascripts/ide/components/repo_editor.vue +++ b/app/assets/javascripts/ide/components/repo_editor.vue @@ -8,6 +8,7 @@ import { activityBarViews, viewerTypes } from '../constants'; import Editor from '../lib/editor'; import ExternalLink from './external_link.vue'; import FileTemplatesBar from './file_templates/bar.vue'; +import { __ } from '~/locale'; export default { components: { @@ -145,7 +146,14 @@ export default { this.createEditorInstance(); }) .catch(err => { - flash('Error setting up editor. Please try again.', 'alert', document, null, false, true); + flash( + __('Error setting up editor. Please try again.'), + 'alert', + document, + null, + false, + true, + ); throw err; }); }, @@ -227,12 +235,8 @@ export default { role="button" @click.prevent="setFileViewMode({ file, viewMode: 'editor' })" > - - + +
  • @@ -240,9 +244,8 @@ export default { href="javascript:void(0);" role="button" @click.prevent="setFileViewMode({ file, viewMode: 'preview' })" + >{{ file.previewMode.previewTitle }} - {{ file.previewMode.previewTitle }} -
    • diff --git a/app/assets/javascripts/ide/components/repo_file_status_icon.vue b/app/assets/javascripts/ide/components/repo_file_status_icon.vue index a964d90b090..84a962bfc7d 100644 --- a/app/assets/javascripts/ide/components/repo_file_status_icon.vue +++ b/app/assets/javascripts/ide/components/repo_file_status_icon.vue @@ -1,4 +1,5 @@ diff --git a/app/assets/javascripts/registry/components/svg_message.vue b/app/assets/javascripts/registry/components/svg_message.vue new file mode 100644 index 00000000000..d0d44bf2d14 --- /dev/null +++ b/app/assets/javascripts/registry/components/svg_message.vue @@ -0,0 +1,24 @@ + + + diff --git a/app/assets/javascripts/registry/index.js b/app/assets/javascripts/registry/index.js index 025afefe7f0..d8daec29fda 100644 --- a/app/assets/javascripts/registry/index.js +++ b/app/assets/javascripts/registry/index.js @@ -14,12 +14,22 @@ export default () => const { dataset } = document.querySelector(this.$options.el); return { endpoint: dataset.endpoint, + characterError: Boolean(dataset.characterError), + helpPagePath: dataset.helpPagePath, + noContainersImage: dataset.noContainersImage, + containersErrorImage: dataset.containersErrorImage, + repositoryUrl: dataset.repositoryUrl, }; }, render(createElement) { return createElement('registry-app', { props: { endpoint: this.endpoint, + characterError: this.characterError, + helpPagePath: this.helpPagePath, + noContainersImage: this.noContainersImage, + containersErrorImage: this.containersErrorImage, + repositoryUrl: this.repositoryUrl, }, }); }, diff --git a/app/assets/stylesheets/pages/container_registry.scss b/app/assets/stylesheets/pages/container_registry.scss index dfff3e15556..cca5214a508 100644 --- a/app/assets/stylesheets/pages/container_registry.scss +++ b/app/assets/stylesheets/pages/container_registry.scss @@ -2,6 +2,12 @@ * Container Registry */ +.container-message { + pre { + white-space: pre-line; + } +} + .container-image { border-bottom: 1px solid $white-normal; } diff --git a/app/controllers/projects/registry/repositories_controller.rb b/app/controllers/projects/registry/repositories_controller.rb index 6d60117c37d..e205e2fd4f8 100644 --- a/app/controllers/projects/registry/repositories_controller.rb +++ b/app/controllers/projects/registry/repositories_controller.rb @@ -46,6 +46,8 @@ module Projects repository.save! if repository.has_tags? end end + rescue ContainerRegistry::Path::InvalidRegistryPathError + @character_error = true end end end diff --git a/app/views/projects/registry/repositories/index.html.haml b/app/views/projects/registry/repositories/index.html.haml index db1f15f96b8..e34973f1f43 100644 --- a/app/views/projects/registry/repositories/index.html.haml +++ b/app/views/projects/registry/repositories/index.html.haml @@ -1,49 +1,9 @@ -- page_title "Container Registry" - %section - .settings-header - %h4 - = page_title - %p - = s_('ContainerRegistry|With the Docker Container Registry integrated into GitLab, every project can have its own space to store its Docker images.') - %p.append-bottom-0 - = succeed '.' do - = s_('ContainerRegistry|Learn more about') - = link_to _('Container Registry'), help_page_path('user/project/container_registry'), target: '_blank' .row.registry-placeholder.prepend-bottom-10 - .col-lg-12 - #js-vue-registry-images{ data: { endpoint: project_container_registry_index_path(@project, format: :json) } } - - .row.prepend-top-10 - .col-lg-12 - .card - .card-header - = s_('ContainerRegistry|How to use the Container Registry') - .card-body - %p - - link_token = link_to(_('personal access token'), help_page_path('user/profile/account/two_factor_authentication', anchor: 'personal-access-tokens'), target: '_blank') - - link_2fa = link_to(_('2FA enabled'), help_page_path('user/profile/account/two_factor_authentication'), target: '_blank') - = s_('ContainerRegistry|First log in to GitLab’s Container Registry using your GitLab username and password. If you have %{link_2fa} you need to use a %{link_token}:').html_safe % { link_2fa: link_2fa, link_token: link_token } - %pre - docker login #{Gitlab.config.registry.host_port} - %br - %p - - deploy_token = link_to(_('deploy token'), help_page_path('user/project/deploy_tokens/index', anchor: 'read-container-registry-images'), target: '_blank') - = s_('ContainerRegistry|You can also use a %{deploy_token} for read-only access to the registry images.').html_safe % { deploy_token: deploy_token } - %br - %p - = s_('ContainerRegistry|Once you log in, you’re free to create and upload a container image using the common %{build} and %{push} commands').html_safe % { build: "build".html_safe, push: "push".html_safe } - %pre - :plain - docker build -t #{escape_once(@project.container_registry_url)} . - docker push #{escape_once(@project.container_registry_url)} - %hr - %h5.prepend-top-default - = s_('ContainerRegistry|Use different image names') - %p.light - = s_('ContainerRegistry|GitLab supports up to 3 levels of image names. The following examples of images are valid for your project:') - %pre - :plain - #{escape_once(@project.container_registry_url)}:tag - #{escape_once(@project.container_registry_url)}/optional-image-name:tag - #{escape_once(@project.container_registry_url)}/optional-name/optional-image-name:tag + .col-12 + #js-vue-registry-images{ data: { endpoint: project_container_registry_index_path(@project, format: :json), + "help_page_path" => help_page_path('user/project/container_registry'), + "no_containers_image" => image_path('illustrations/docker-empty-state.svg'), + "containers_error_image" => image_path('illustrations/docker-error-state.svg'), + "repository_url" => escape_once(@project.container_registry_url), + character_error: @character_error.to_s } } diff --git a/changelogs/unreleased/45104-special-characters-in-project-name-path-prevent-users-from-using-the-container-registry.yml b/changelogs/unreleased/45104-special-characters-in-project-name-path-prevent-users-from-using-the-container-registry.yml new file mode 100644 index 00000000000..ddde0cc9c39 --- /dev/null +++ b/changelogs/unreleased/45104-special-characters-in-project-name-path-prevent-users-from-using-the-container-registry.yml @@ -0,0 +1,5 @@ +--- +title: Updated container registry to display error message when special characters in path. Documentation has also been updated. +merge_request: 29616 +author: +type: changed diff --git a/doc/administration/container_registry.md b/doc/administration/container_registry.md index 4d55f2357c1..2e4b4efa0ac 100644 --- a/doc/administration/container_registry.md +++ b/doc/administration/container_registry.md @@ -689,6 +689,20 @@ You can add a configuration option for backwards compatibility. 1. Restart the registry for the changes to take affect. +### Docker connection error + +A Docker connection error can occur when there are special characters in either the group, +project or branch name. Special characters can include: + +* Leading underscore +* Trailing hyphen/dash +* Double hyphen/dash + +To get around this, you can [change the group path](../user/group/index.md#changing-a-groups-path), +[change the project path](../user/project/settings/index.md#renaming-a-repository) or change the +branch name. Another option is to create a [push rule](../push_rules/push_rules.html) to prevent +this at the instance level. + [ce-18239]: https://gitlab.com/gitlab-org/gitlab-ce/issues/18239 [docker-insecure-self-signed]: https://docs.docker.com/registry/insecure/#use-self-signed-certificates diff --git a/doc/user/project/container_registry.md b/doc/user/project/container_registry.md index fdf9ce3e225..7d567da1c9a 100644 --- a/doc/user/project/container_registry.md +++ b/doc/user/project/container_registry.md @@ -168,6 +168,19 @@ curl localhost:5001/debug/health curl localhost:5001/debug/vars ``` +#### Docker connection error + +A Docker connection error can occur when there are special characters in either the group, +project or branch name. Special characters can include: + +* Leading underscore +* Trailing hyphen/dash +* Double hyphen/dash + +To get around this, you can [change the group path](../group/index.md#changing-a-groups-path), +[change the project path](../project/settings/index.md#renaming-a-repository) or chanage the branch +name. + ### Advanced Troubleshooting >**NOTE:** The following section is only recommended for experts. diff --git a/locale/gitlab.pot b/locale/gitlab.pot index c57a974fe11..d20692210fd 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -348,9 +348,6 @@ msgstr "" msgid "2FA" msgstr "" -msgid "2FA enabled" -msgstr "" - msgid "2FADevice|Registered On" msgstr "" @@ -2943,25 +2940,19 @@ msgstr "" msgid "Container registry images" msgstr "" -msgid "ContainerRegistry|First log in to GitLab’s Container Registry using your GitLab username and password. If you have %{link_2fa} you need to use a %{link_token}:" +msgid "ContainerRegistry|Container Registry" msgstr "" -msgid "ContainerRegistry|GitLab supports up to 3 levels of image names. The following examples of images are valid for your project:" -msgstr "" - -msgid "ContainerRegistry|How to use the Container Registry" +msgid "ContainerRegistry|Docker connection error" msgstr "" msgid "ContainerRegistry|Last Updated" msgstr "" -msgid "ContainerRegistry|Learn more about" -msgstr "" - msgid "ContainerRegistry|No tags in Container Registry for this container image." msgstr "" -msgid "ContainerRegistry|Once you log in, you’re free to create and upload a container image using the common %{build} and %{push} commands" +msgid "ContainerRegistry|Quick Start" msgstr "" msgid "ContainerRegistry|Remove image" @@ -2982,10 +2973,16 @@ msgstr "" msgid "ContainerRegistry|Tag ID" msgstr "" -msgid "ContainerRegistry|Use different image names" +msgid "ContainerRegistry|There are no container images stored for this project" msgstr "" -msgid "ContainerRegistry|With the Docker Container Registry integrated into GitLab, every project can have its own space to store its Docker images." +msgid "ContainerRegistry|We are having trouble connecting to Docker, which could be due to an issue with your project name or path. For more information, please review the %{docLinkStart}Container Registry documentation%{docLinkEnd}." +msgstr "" + +msgid "ContainerRegistry|With the Container Registry, every project can have its own space to store its Docker images. Learn more about the %{docLinkStart}Container Registry%{docLinkEnd}." +msgstr "" + +msgid "ContainerRegistry|With the Docker Container Registry integrated into GitLab, every project can have its own space to store its Docker images. Learn more about the %{docLinkStart}Container Registry%{docLinkEnd}." msgstr "" msgid "ContainerRegistry|You are about to delete the image %{title}. This will delete the image and all tags pointing to this image." @@ -2994,7 +2991,7 @@ msgstr "" msgid "ContainerRegistry|You are about to remove repository %{title}. Once you confirm, this repository will be permanently deleted." msgstr "" -msgid "ContainerRegistry|You can also use a %{deploy_token} for read-only access to the registry images." +msgid "ContainerRegistry|You can add an image to this registry with the following commands:" msgstr "" msgid "Contents of .gitlab-ci.yml" @@ -6750,9 +6747,6 @@ msgstr "" msgid "No connection could be made to a Gitaly Server, please check your logs!" msgstr "" -msgid "No container images stored for this project. Add one by following the instructions above." -msgstr "" - msgid "No contributions" msgstr "" @@ -12450,9 +12444,6 @@ msgstr[1] "" msgid "deleted" msgstr "" -msgid "deploy token" -msgstr "" - msgid "detached" msgstr "" @@ -12851,9 +12842,6 @@ msgstr[1] "" msgid "password" msgstr "" -msgid "personal access token" -msgstr "" - msgid "private" msgstr "" diff --git a/spec/features/container_registry_spec.rb b/spec/features/container_registry_spec.rb index 21d97aba0c5..1b5943bd5d8 100644 --- a/spec/features/container_registry_spec.rb +++ b/spec/features/container_registry_spec.rb @@ -19,7 +19,7 @@ describe "Container Registry", :js do it 'user visits container registry main page' do visit_container_registry - expect(page).to have_content 'No container images' + expect(page).to have_content 'no container images' end end diff --git a/spec/javascripts/registry/components/app_spec.js b/spec/javascripts/registry/components/app_spec.js index 76a17e6fb31..87237d2853d 100644 --- a/spec/javascripts/registry/components/app_spec.js +++ b/spec/javascripts/registry/components/app_spec.js @@ -8,6 +8,13 @@ import { reposServerResponse } from '../mock_data'; describe('Registry List', () => { const Component = Vue.extend(registry); + const props = { + endpoint: `${TEST_HOST}/foo`, + helpPagePath: 'foo', + noContainersImage: 'foo', + containersErrorImage: 'foo', + repositoryUrl: 'foo', + }; let vm; let mock; @@ -24,7 +31,7 @@ describe('Registry List', () => { beforeEach(() => { mock.onGet(`${TEST_HOST}/foo`).replyOnce(200, reposServerResponse); - vm = mountComponent(Component, { endpoint: `${TEST_HOST}/foo` }); + vm = mountComponent(Component, { ...props }); }); it('should render a list of repos', done => { @@ -72,7 +79,7 @@ describe('Registry List', () => { beforeEach(() => { mock.onGet(`${TEST_HOST}/foo`).replyOnce(200, []); - vm = mountComponent(Component, { endpoint: `${TEST_HOST}/foo` }); + vm = mountComponent(Component, { ...props }); }); it('should render empty message', done => { @@ -83,7 +90,7 @@ describe('Registry List', () => { .textContent.trim() .replace(/[\r\n]+/g, ' '), ).toEqual( - 'No container images stored for this project. Add one by following the instructions above.', + 'With the Container Registry, every project can have its own space to store its Docker images. Learn more about the Container Registry.', ); done(); }, 0); @@ -94,7 +101,7 @@ describe('Registry List', () => { beforeEach(() => { mock.onGet(`${TEST_HOST}/foo`).replyOnce(200, []); - vm = mountComponent(Component, { endpoint: `${TEST_HOST}/foo` }); + vm = mountComponent(Component, { ...props }); }); it('should render a loading spinner', done => { @@ -104,4 +111,22 @@ describe('Registry List', () => { }); }); }); + + describe('invalid characters in path', () => { + beforeEach(() => { + mock.onGet(`${TEST_HOST}/foo`).replyOnce(200, []); + + vm = mountComponent(Component, { + ...props, + characterError: true, + }); + }); + + it('should render invalid characters error message', done => { + setTimeout(() => { + expect(vm.$el.querySelector('.container-message')).not.toBe(null); + done(); + }); + }); + }); }); From 385aa46046ec83e6837c106576699f76e65876a7 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Tue, 2 Jul 2019 06:58:47 -0700 Subject: [PATCH 16/40] Cache Flipper persisted names directly to local memory storage Now that application settings are no longer dominating network traffic, we see that the Feature#persisted_names is using a significant amount of CPU and network bandwidth for Redis. Move this cache into the thread-local memory storage to reduce Redis overhead. --- .../unreleased/sh-cache-flipper-names-memory-cache.yml | 5 +++++ lib/feature.rb | 4 +++- spec/lib/feature_spec.rb | 2 +- 3 files changed, 9 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/sh-cache-flipper-names-memory-cache.yml diff --git a/changelogs/unreleased/sh-cache-flipper-names-memory-cache.yml b/changelogs/unreleased/sh-cache-flipper-names-memory-cache.yml new file mode 100644 index 00000000000..00443e81244 --- /dev/null +++ b/changelogs/unreleased/sh-cache-flipper-names-memory-cache.yml @@ -0,0 +1,5 @@ +--- +title: Cache Flipper persisted names directly to local memory storage +merge_request: 30265 +author: +type: performance diff --git a/lib/feature.rb b/lib/feature.rb index cc9c9d44005..22420e95ea2 100644 --- a/lib/feature.rb +++ b/lib/feature.rb @@ -34,7 +34,9 @@ class Feature begin # We saw on GitLab.com, this database request was called 2300 # times/s. Let's cache it for a minute to avoid that load. - Rails.cache.fetch('flipper:persisted_names', expires_in: 1.minute) { FlipperFeature.feature_names } + Gitlab::ThreadMemoryCache.cache_backend.fetch('flipper:persisted_names', expires_in: 1.minute) do + FlipperFeature.feature_names + end end end diff --git a/spec/lib/feature_spec.rb b/spec/lib/feature_spec.rb index 6f05914f915..403e0785d1b 100644 --- a/spec/lib/feature_spec.rb +++ b/spec/lib/feature_spec.rb @@ -40,7 +40,7 @@ describe Feature do .once .and_call_original - expect(Rails.cache) + expect(Gitlab::ThreadMemoryCache.cache_backend) .to receive(:fetch) .once .with('flipper:persisted_names', expires_in: 1.minute) From d5842e7490a6826bbaaaa7c76c967a48e96dd1ef Mon Sep 17 00:00:00 2001 From: Lukas Eipert Date: Tue, 2 Jul 2019 16:29:47 +0200 Subject: [PATCH 17/40] Prefer offline install for yarn --- .gitlab/ci/frontend.gitlab-ci.yml | 6 +++--- .gitlab/ci/review.gitlab-ci.yml | 2 +- lib/tasks/yarn.rake | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/.gitlab/ci/frontend.gitlab-ci.yml b/.gitlab/ci/frontend.gitlab-ci.yml index 6e80cb530f1..0c0985decdd 100644 --- a/.gitlab/ci/frontend.gitlab-ci.yml +++ b/.gitlab/ci/frontend.gitlab-ci.yml @@ -32,7 +32,7 @@ DOCKER_HOST: tcp://docker:2375 script: - node --version - - retry yarn install --frozen-lockfile --production --cache-folder .yarn-cache + - retry yarn install --frozen-lockfile --production --cache-folder .yarn-cache --prefer-offline - free -m - retry bundle exec rake gitlab:assets:compile - time scripts/build_assets_image @@ -82,7 +82,7 @@ gitlab:assets:compile pull-cache: stage: prepare script: - node --version - - retry yarn install --frozen-lockfile --cache-folder .yarn-cache + - retry yarn install --frozen-lockfile --cache-folder .yarn-cache --prefer-offline - free -m - retry bundle exec rake gitlab:assets:compile - scripts/clean-old-cached-assets @@ -231,7 +231,7 @@ qa:selectors: before_script: [] script: - date - - yarn install --frozen-lockfile --cache-folder .yarn-cache + - yarn install --frozen-lockfile --cache-folder .yarn-cache --prefer-offline - date - yarn run webpack-prod diff --git a/.gitlab/ci/review.gitlab-ci.yml b/.gitlab/ci/review.gitlab-ci.yml index 933af90c85a..61fd48fd72e 100644 --- a/.gitlab/ci/review.gitlab-ci.yml +++ b/.gitlab/ci/review.gitlab-ci.yml @@ -236,5 +236,5 @@ danger-review: script: - git version - node --version - - yarn install --frozen-lockfile --cache-folder .yarn-cache + - yarn install --frozen-lockfile --cache-folder .yarn-cache --prefer-offline - danger --fail-on-errors=true diff --git a/lib/tasks/yarn.rake b/lib/tasks/yarn.rake index 2ac88a039e7..32061ad4a57 100644 --- a/lib/tasks/yarn.rake +++ b/lib/tasks/yarn.rake @@ -24,7 +24,7 @@ namespace :yarn do desc 'Install Node dependencies with Yarn' task install: ['yarn:available'] do - unless system('yarn install --pure-lockfile --ignore-engines') + unless system('yarn install --pure-lockfile --ignore-engines --prefer-offline') abort 'Error: Unable to install node modules.'.color(:red) end end From dfdfa913ba9cb74beb7adad0352c5efadec84494 Mon Sep 17 00:00:00 2001 From: Mayra Cabrera Date: Tue, 2 Jul 2019 14:44:39 +0000 Subject: [PATCH 18/40] Includes logic to persist namespace statistics - Add two new ActiveRecord models: - RootNamespaceStoragestatistics will persist root namespace statistics - NamespaceAggregationSchedule will save information when a new update to the namespace statistics needs to be scheduled - Inject into UpdateProjectStatistics concern a new callback that will call an async job to insert a new row onto NamespaceAggregationSchedule table - When a new row is inserted a new job is scheduled. This job will update call an specific service to update the statistics and after that it will delete thee aggregated scheduled row - The RefresherServices makes heavy use of arel to build composable queries to update Namespace::RootStorageStatistics attributes. - Add an extra worker to traverse pending rows on NAmespace::AggregationSchedule table and schedule a worker for each one of this rows. - Add an extra worker to traverse pending rows on NAmespace::AggregationSchedule table and schedule a worker for each one of this rows --- .../concerns/update_project_statistics.rb | 19 +++- app/models/namespace.rb | 4 + app/models/namespace/aggregation_schedule.rb | 40 +++++++++ .../namespace/root_storage_statistics.rb | 28 ++++++ .../statistics_refresher_service.rb | 22 +++++ app/workers/all_queues.yml | 4 + .../prune_aggregation_schedules_worker.rb | 22 +++++ .../namespaces/root_statistics_worker.rb | 31 +++++++ .../namespaces/schedule_aggregation_worker.rb | 45 ++++++++++ config/initializers/1_settings.rb | 3 + config/sidekiq_queues.yml | 1 + spec/factories/project_statistics.rb | 15 ++++ .../namespace/aggregation_schedule_spec.rb | 73 ++++++++++++++- .../namespace/root_storage_statistics_spec.rb | 65 ++++++++++++++ spec/models/namespace_spec.rb | 16 ++++ .../statistics_refresher_service_spec.rb | 58 ++++++++++++ ...date_project_statistics_shared_examples.rb | 90 ++++++++++++++++++- ...prune_aggregation_schedules_worker_spec.rb | 35 ++++++++ .../namespaces/root_statistics_worker_spec.rb | 88 ++++++++++++++++++ .../schedule_aggregation_worker_spec.rb | 66 ++++++++++++++ 20 files changed, 719 insertions(+), 6 deletions(-) create mode 100644 app/services/namespaces/statistics_refresher_service.rb create mode 100644 app/workers/namespaces/prune_aggregation_schedules_worker.rb create mode 100644 app/workers/namespaces/root_statistics_worker.rb create mode 100644 app/workers/namespaces/schedule_aggregation_worker.rb create mode 100644 spec/services/namespaces/statistics_refresher_service_spec.rb create mode 100644 spec/workers/namespaces/prune_aggregation_schedules_worker_spec.rb create mode 100644 spec/workers/namespaces/root_statistics_worker_spec.rb create mode 100644 spec/workers/namespaces/schedule_aggregation_worker_spec.rb diff --git a/app/models/concerns/update_project_statistics.rb b/app/models/concerns/update_project_statistics.rb index 1f881249322..570a735973f 100644 --- a/app/models/concerns/update_project_statistics.rb +++ b/app/models/concerns/update_project_statistics.rb @@ -19,9 +19,9 @@ # # - `statistic_attribute` must be an ActiveRecord attribute # - The model must implement `project` and `project_id`. i.e. direct Project relationship or delegation -# module UpdateProjectStatistics extend ActiveSupport::Concern + include AfterCommitQueue class_methods do attr_reader :project_statistics_name, :statistic_attribute @@ -31,7 +31,6 @@ module UpdateProjectStatistics # # - project_statistics_name: A column of `ProjectStatistics` to update # - statistic_attribute: An attribute of the current model, default to `size` - # def update_project_statistics(project_statistics_name:, statistic_attribute: :size) @project_statistics_name = project_statistics_name @statistic_attribute = statistic_attribute @@ -51,6 +50,7 @@ module UpdateProjectStatistics delta = read_attribute(attr).to_i - attribute_before_last_save(attr).to_i update_project_statistics(delta) + schedule_namespace_aggregation_worker end def update_project_statistics_attribute_changed? @@ -59,6 +59,8 @@ module UpdateProjectStatistics def update_project_statistics_after_destroy update_project_statistics(-read_attribute(self.class.statistic_attribute).to_i) + + schedule_namespace_aggregation_worker end def project_destroyed? @@ -68,5 +70,18 @@ module UpdateProjectStatistics def update_project_statistics(delta) ProjectStatistics.increment_statistic(project_id, self.class.project_statistics_name, delta) end + + def schedule_namespace_aggregation_worker + run_after_commit do + next unless schedule_aggregation_worker? + + Namespaces::ScheduleAggregationWorker.perform_async(project.namespace_id) + end + end + + def schedule_aggregation_worker? + !project.nil? && + Feature.enabled?(:update_statistics_namespace, project.root_ancestor) + end end end diff --git a/app/models/namespace.rb b/app/models/namespace.rb index bfa33dc86ac..af50293a179 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -293,6 +293,10 @@ class Namespace < ApplicationRecord end end + def aggregation_scheduled? + aggregation_schedule.present? + end + private def parent_changed? diff --git a/app/models/namespace/aggregation_schedule.rb b/app/models/namespace/aggregation_schedule.rb index 43afd0b954c..355593597c6 100644 --- a/app/models/namespace/aggregation_schedule.rb +++ b/app/models/namespace/aggregation_schedule.rb @@ -1,7 +1,47 @@ # frozen_string_literal: true class Namespace::AggregationSchedule < ApplicationRecord + include AfterCommitQueue + include ExclusiveLeaseGuard + self.primary_key = :namespace_id + DEFAULT_LEASE_TIMEOUT = 3.hours + REDIS_SHARED_KEY = 'gitlab:update_namespace_statistics_delay'.freeze + belongs_to :namespace + + after_create :schedule_root_storage_statistics + + def self.delay_timeout + redis_timeout = Gitlab::Redis::SharedState.with do |redis| + redis.get(REDIS_SHARED_KEY) + end + + redis_timeout.nil? ? DEFAULT_LEASE_TIMEOUT : redis_timeout.to_i + end + + def schedule_root_storage_statistics + run_after_commit_or_now do + try_obtain_lease do + Namespaces::RootStatisticsWorker + .perform_async(namespace_id) + + Namespaces::RootStatisticsWorker + .perform_in(self.class.delay_timeout, namespace_id) + end + end + end + + private + + # Used by ExclusiveLeaseGuard + def lease_timeout + self.class.delay_timeout + end + + # Used by ExclusiveLeaseGuard + def lease_key + "namespace:namespaces_root_statistics:#{namespace_id}" + end end diff --git a/app/models/namespace/root_storage_statistics.rb b/app/models/namespace/root_storage_statistics.rb index de28eb6b37f..56c430013ee 100644 --- a/app/models/namespace/root_storage_statistics.rb +++ b/app/models/namespace/root_storage_statistics.rb @@ -1,10 +1,38 @@ # frozen_string_literal: true class Namespace::RootStorageStatistics < ApplicationRecord + STATISTICS_ATTRIBUTES = %w(storage_size repository_size wiki_size lfs_objects_size build_artifacts_size packages_size).freeze + self.primary_key = :namespace_id belongs_to :namespace has_one :route, through: :namespace delegate :all_projects, to: :namespace + + def recalculate! + update!(attributes_from_project_statistics) + end + + private + + def attributes_from_project_statistics + from_project_statistics + .take + .attributes + .slice(*STATISTICS_ATTRIBUTES) + end + + def from_project_statistics + all_projects + .joins('INNER JOIN project_statistics ps ON ps.project_id = projects.id') + .select( + 'COALESCE(SUM(ps.storage_size), 0) AS storage_size', + 'COALESCE(SUM(ps.repository_size), 0) AS repository_size', + 'COALESCE(SUM(ps.wiki_size), 0) AS wiki_size', + 'COALESCE(SUM(ps.lfs_objects_size), 0) AS lfs_objects_size', + 'COALESCE(SUM(ps.build_artifacts_size), 0) AS build_artifacts_size', + 'COALESCE(SUM(ps.packages_size), 0) AS packages_size' + ) + end end diff --git a/app/services/namespaces/statistics_refresher_service.rb b/app/services/namespaces/statistics_refresher_service.rb new file mode 100644 index 00000000000..c07b302839b --- /dev/null +++ b/app/services/namespaces/statistics_refresher_service.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +module Namespaces + class StatisticsRefresherService + RefresherError = Class.new(StandardError) + + def execute(root_namespace) + root_storage_statistics = find_or_create_root_storage_statistics(root_namespace.id) + + root_storage_statistics.recalculate! + rescue ActiveRecord::ActiveRecordError => e + raise RefresherError.new(e.message) + end + + private + + def find_or_create_root_storage_statistics(root_namespace_id) + Namespace::RootStorageStatistics + .safe_find_or_create_by!(namespace_id: root_namespace_id) + end + end +end diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index e55962b629e..3d34bfc05c7 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -26,6 +26,7 @@ - cronjob:issue_due_scheduler - cronjob:prune_web_hook_logs - cronjob:schedule_migrate_external_diffs +- cronjob:namespaces_prune_aggregation_schedules - gcp_cluster:cluster_install_app - gcp_cluster:cluster_patch_app @@ -101,6 +102,9 @@ - todos_destroyer:todos_destroyer_project_private - todos_destroyer:todos_destroyer_private_features +- update_namespace_statistics:namespaces_schedule_aggregation +- update_namespace_statistics:namespaces_root_statistics + - object_pool:object_pool_create - object_pool:object_pool_schedule_join - object_pool:object_pool_join diff --git a/app/workers/namespaces/prune_aggregation_schedules_worker.rb b/app/workers/namespaces/prune_aggregation_schedules_worker.rb new file mode 100644 index 00000000000..4e40feee702 --- /dev/null +++ b/app/workers/namespaces/prune_aggregation_schedules_worker.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +module Namespaces + class PruneAggregationSchedulesWorker + include ApplicationWorker + include CronjobQueue + + # Worker to prune pending rows on Namespace::AggregationSchedule + # It's scheduled to run once a day at 1:05am. + def perform + aggregation_schedules.find_each do |aggregation_schedule| + aggregation_schedule.schedule_root_storage_statistics + end + end + + private + + def aggregation_schedules + Namespace::AggregationSchedule.all + end + end +end diff --git a/app/workers/namespaces/root_statistics_worker.rb b/app/workers/namespaces/root_statistics_worker.rb new file mode 100644 index 00000000000..48876825564 --- /dev/null +++ b/app/workers/namespaces/root_statistics_worker.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +module Namespaces + class RootStatisticsWorker + include ApplicationWorker + + queue_namespace :update_namespace_statistics + + def perform(namespace_id) + namespace = Namespace.find(namespace_id) + + return unless update_statistics_enabled_for?(namespace) && namespace.aggregation_scheduled? + + Namespaces::StatisticsRefresherService.new.execute(namespace) + + namespace.aggregation_schedule.destroy + rescue ::Namespaces::StatisticsRefresherService::RefresherError, ActiveRecord::RecordNotFound => ex + log_error(namespace.full_path, ex.message) if namespace + end + + private + + def log_error(namespace_path, error_message) + Gitlab::SidekiqLogger.error("Namespace statistics can't be updated for #{namespace_path}: #{error_message}") + end + + def update_statistics_enabled_for?(namespace) + Feature.enabled?(:update_statistics_namespace, namespace) + end + end +end diff --git a/app/workers/namespaces/schedule_aggregation_worker.rb b/app/workers/namespaces/schedule_aggregation_worker.rb new file mode 100644 index 00000000000..a4594b84b13 --- /dev/null +++ b/app/workers/namespaces/schedule_aggregation_worker.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +module Namespaces + class ScheduleAggregationWorker + include ApplicationWorker + + queue_namespace :update_namespace_statistics + + def perform(namespace_id) + return unless aggregation_schedules_table_exists? + + namespace = Namespace.find(namespace_id) + root_ancestor = namespace.root_ancestor + + return unless update_statistics_enabled_for?(root_ancestor) && !root_ancestor.aggregation_scheduled? + + Namespace::AggregationSchedule.safe_find_or_create_by!(namespace_id: root_ancestor.id) + rescue ActiveRecord::RecordNotFound + log_error(namespace_id) + end + + private + + # On db/post_migrate/20180529152628_schedule_to_archive_legacy_traces.rb + # traces are archived through build.trace.archive, which in consequence + # calls UpdateProjectStatistics#schedule_namespace_statistics_worker. + # + # The migration and specs fails since NamespaceAggregationSchedule table + # does not exist at that point. + # https://gitlab.com/gitlab-org/gitlab-ce/issues/50712 + def aggregation_schedules_table_exists? + return true unless Rails.env.test? + + Namespace::AggregationSchedule.table_exists? + end + + def log_error(root_ancestor_id) + Gitlab::SidekiqLogger.error("Namespace can't be scheduled for aggregation: #{root_ancestor_id} does not exist") + end + + def update_statistics_enabled_for?(root_ancestor) + Feature.enabled?(:update_statistics_namespace, root_ancestor) + end + end +end diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index c803e4615b4..bf187e9a282 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -441,6 +441,9 @@ Settings.cron_jobs['prune_web_hook_logs_worker']['job_class'] = 'PruneWebHookLog Settings.cron_jobs['schedule_migrate_external_diffs_worker'] ||= Settingslogic.new({}) Settings.cron_jobs['schedule_migrate_external_diffs_worker']['cron'] ||= '15 * * * *' Settings.cron_jobs['schedule_migrate_external_diffs_worker']['job_class'] = 'ScheduleMigrateExternalDiffsWorker' +Settings.cron_jobs['namespaces_prune_aggregation_schedules_worker'] ||= Settingslogic.new({}) +Settings.cron_jobs['namespaces_prune_aggregation_schedules_worker']['cron'] ||= '5 1 * * *' +Settings.cron_jobs['namespaces_prune_aggregation_schedules_worker']['job_class'] = 'Namespaces::PruneAggregationSchedulesWorker' Gitlab.ee do Settings.cron_jobs['clear_shared_runners_minutes_worker'] ||= Settingslogic.new({}) diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index 25fd65d8644..80791795390 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -94,6 +94,7 @@ - [migrate_external_diffs, 1] - [update_project_statistics, 1] - [phabricator_import_import_tasks, 1] + - [update_namespace_statistics, 1] # EE-specific queues - [ldap_group_sync, 2] diff --git a/spec/factories/project_statistics.rb b/spec/factories/project_statistics.rb index 2d0f698475d..3d4174eb852 100644 --- a/spec/factories/project_statistics.rb +++ b/spec/factories/project_statistics.rb @@ -6,5 +6,20 @@ FactoryBot.define do # statistics are automatically created when a project is created project&.statistics || new end + + transient do + with_data { false } + size_multiplier { 1 } + end + + after(:build) do |project_statistics, evaluator| + if evaluator.with_data + project_statistics.repository_size = evaluator.size_multiplier + project_statistics.wiki_size = evaluator.size_multiplier * 2 + project_statistics.lfs_objects_size = evaluator.size_multiplier * 3 + project_statistics.build_artifacts_size = evaluator.size_multiplier * 4 + project_statistics.packages_size = evaluator.size_multiplier * 5 + end + end end end diff --git a/spec/models/namespace/aggregation_schedule_spec.rb b/spec/models/namespace/aggregation_schedule_spec.rb index 5ba7547ff4d..8ed0248e1b2 100644 --- a/spec/models/namespace/aggregation_schedule_spec.rb +++ b/spec/models/namespace/aggregation_schedule_spec.rb @@ -2,6 +2,77 @@ require 'spec_helper' -RSpec.describe Namespace::AggregationSchedule, type: :model do +RSpec.describe Namespace::AggregationSchedule, :clean_gitlab_redis_shared_state, type: :model do + include ExclusiveLeaseHelpers + it { is_expected.to belong_to :namespace } + + describe '.delay_timeout' do + context 'when timeout is set on redis' do + it 'uses personalized timeout' do + Gitlab::Redis::SharedState.with do |redis| + redis.set(described_class::REDIS_SHARED_KEY, 1.hour) + end + + expect(described_class.delay_timeout).to eq(1.hour) + end + end + + context 'when timeout is not set on redis' do + it 'uses default timeout' do + expect(described_class.delay_timeout).to eq(3.hours) + end + end + end + + describe '#schedule_root_storage_statistics' do + let(:namespace) { create(:namespace) } + let(:aggregation_schedule) { namespace.build_aggregation_schedule } + let(:lease_key) { "namespace:namespaces_root_statistics:#{namespace.id}" } + + context "when we can't obtain the lease" do + it 'does not schedule the workers' do + stub_exclusive_lease_taken(lease_key, timeout: described_class::DEFAULT_LEASE_TIMEOUT) + + expect(Namespaces::RootStatisticsWorker) + .not_to receive(:perform_async) + + expect(Namespaces::RootStatisticsWorker) + .not_to receive(:perform_in) + + aggregation_schedule.save! + end + end + + context 'when we can obtain the lease' do + it 'schedules a root storage statistics after create' do + stub_exclusive_lease(lease_key, timeout: described_class::DEFAULT_LEASE_TIMEOUT) + + expect(Namespaces::RootStatisticsWorker) + .to receive(:perform_async).once + + expect(Namespaces::RootStatisticsWorker) + .to receive(:perform_in).once + .with(described_class::DEFAULT_LEASE_TIMEOUT, aggregation_schedule.namespace_id ) + + aggregation_schedule.save! + end + end + + context 'with a personalized lease timeout' do + before do + Gitlab::Redis::SharedState.with do |redis| + redis.set(described_class::REDIS_SHARED_KEY, 1.hour) + end + end + + it 'uses a personalized time' do + expect(Namespaces::RootStatisticsWorker) + .to receive(:perform_in) + .with(1.hour, aggregation_schedule.namespace_id) + + aggregation_schedule.save! + end + end + end end diff --git a/spec/models/namespace/root_storage_statistics_spec.rb b/spec/models/namespace/root_storage_statistics_spec.rb index f6fb5af5aae..3229a32234e 100644 --- a/spec/models/namespace/root_storage_statistics_spec.rb +++ b/spec/models/namespace/root_storage_statistics_spec.rb @@ -7,4 +7,69 @@ RSpec.describe Namespace::RootStorageStatistics, type: :model do it { is_expected.to have_one(:route).through(:namespace) } it { is_expected.to delegate_method(:all_projects).to(:namespace) } + + describe '#recalculate!' do + let(:namespace) { create(:group) } + let(:root_storage_statistics) { create(:namespace_root_storage_statistics, namespace: namespace) } + + let(:project1) { create(:project, namespace: namespace) } + let(:project2) { create(:project, namespace: namespace) } + + let!(:stat1) { create(:project_statistics, project: project1, with_data: true, size_multiplier: 100) } + let!(:stat2) { create(:project_statistics, project: project2, with_data: true, size_multiplier: 200) } + + shared_examples 'data refresh' do + it 'aggregates project statistics' do + root_storage_statistics.recalculate! + + root_storage_statistics.reload + + total_repository_size = stat1.repository_size + stat2.repository_size + total_wiki_size = stat1.wiki_size + stat2.wiki_size + total_lfs_objects_size = stat1.lfs_objects_size + stat2.lfs_objects_size + total_build_artifacts_size = stat1.build_artifacts_size + stat2.build_artifacts_size + total_packages_size = stat1.packages_size + stat2.packages_size + total_storage_size = stat1.storage_size + stat2.storage_size + + expect(root_storage_statistics.repository_size).to eq(total_repository_size) + expect(root_storage_statistics.wiki_size).to eq(total_wiki_size) + expect(root_storage_statistics.lfs_objects_size).to eq(total_lfs_objects_size) + expect(root_storage_statistics.build_artifacts_size).to eq(total_build_artifacts_size) + expect(root_storage_statistics.packages_size).to eq(total_packages_size) + expect(root_storage_statistics.storage_size).to eq(total_storage_size) + end + + it 'works when there are no projects' do + Project.delete_all + + root_storage_statistics.recalculate! + + root_storage_statistics.reload + expect(root_storage_statistics.repository_size).to eq(0) + expect(root_storage_statistics.wiki_size).to eq(0) + expect(root_storage_statistics.lfs_objects_size).to eq(0) + expect(root_storage_statistics.build_artifacts_size).to eq(0) + expect(root_storage_statistics.packages_size).to eq(0) + expect(root_storage_statistics.storage_size).to eq(0) + end + end + + it_behaves_like 'data refresh' + + context 'with subgroups', :nested_groups do + let(:subgroup1) { create(:group, parent: namespace)} + let(:subgroup2) { create(:group, parent: subgroup1)} + + let(:project1) { create(:project, namespace: subgroup1) } + let(:project2) { create(:project, namespace: subgroup2) } + + it_behaves_like 'data refresh' + end + + context 'with a personal namespace' do + let(:namespace) { create(:user).namespace } + + it_behaves_like 'data refresh' + end + end end diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb index 30e49cf204f..f908f3504e0 100644 --- a/spec/models/namespace_spec.rb +++ b/spec/models/namespace_spec.rb @@ -837,4 +837,20 @@ describe Namespace do it { is_expected.to be_falsy } end end + + describe '#aggregation_scheduled?' do + let(:namespace) { create(:namespace) } + + subject { namespace.aggregation_scheduled? } + + context 'with an aggregation scheduled association' do + let(:namespace) { create(:namespace, :with_aggregation_schedule) } + + it { is_expected.to be_truthy } + end + + context 'without an aggregation scheduled association' do + it { is_expected.to be_falsy } + end + end end diff --git a/spec/services/namespaces/statistics_refresher_service_spec.rb b/spec/services/namespaces/statistics_refresher_service_spec.rb new file mode 100644 index 00000000000..f4d9c96f7f4 --- /dev/null +++ b/spec/services/namespaces/statistics_refresher_service_spec.rb @@ -0,0 +1,58 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Namespaces::StatisticsRefresherService, '#execute' do + let(:group) { create(:group) } + let(:projects) { create_list(:project, 5, namespace: group) } + let(:service) { described_class.new } + + context 'without a root storage statistics relation' do + it 'creates one' do + expect do + service.execute(group) + end.to change(Namespace::RootStorageStatistics, :count).by(1) + + expect(group.reload.root_storage_statistics).to be_present + end + + it 'recalculate the namespace statistics' do + expect_any_instance_of(Namespace::RootStorageStatistics).to receive(:recalculate!).once + + service.execute(group) + end + end + + context 'with a root storage statistics relation' do + before do + Namespace::AggregationSchedule.safe_find_or_create_by!(namespace_id: group.id) + end + + it 'does not create one' do + expect do + service.execute(group) + end.not_to change(Namespace::RootStorageStatistics, :count) + end + + it 'recalculate the namespace statistics' do + expect(Namespace::RootStorageStatistics) + .to receive(:safe_find_or_create_by!).with({ namespace_id: group.id }) + .and_return(group.root_storage_statistics) + + service.execute(group) + end + end + + context 'when something goes wrong' do + before do + allow_any_instance_of(Namespace::RootStorageStatistics) + .to receive(:recalculate!).and_raise(ActiveRecord::ActiveRecordError) + end + + it 'raises RefreshError' do + expect do + service.execute(group) + end.to raise_error(Namespaces::StatisticsRefresherService::RefresherError) + end + end +end diff --git a/spec/support/shared_examples/models/update_project_statistics_shared_examples.rb b/spec/support/shared_examples/models/update_project_statistics_shared_examples.rb index 1b09c3dd636..aad63982e7a 100644 --- a/spec/support/shared_examples/models/update_project_statistics_shared_examples.rb +++ b/spec/support/shared_examples/models/update_project_statistics_shared_examples.rb @@ -25,16 +25,36 @@ shared_examples_for 'UpdateProjectStatistics' do .to change { reload_stat } .by(delta) end + + it 'schedules a namespace statistics worker' do + expect(Namespaces::ScheduleAggregationWorker) + .to receive(:perform_async).once + + subject.save! + end + + context 'when feature flag is disabled for the namespace' do + it 'does not schedules a namespace statistics worker' do + namespace = subject.project.root_ancestor + + stub_feature_flags(update_statistics_namespace: false, namespace: namespace) + + expect(Namespaces::ScheduleAggregationWorker) + .not_to receive(:perform_async) + + subject.save! + end + end end context 'when updating' do + let(:delta) { 42 } + before do subject.save! end it 'updates project statistics' do - delta = 42 - expect(ProjectStatistics) .to receive(:increment_statistic) .and_call_original @@ -45,6 +65,42 @@ shared_examples_for 'UpdateProjectStatistics' do .to change { reload_stat } .by(delta) end + + it 'schedules a namespace statistics worker' do + expect(Namespaces::ScheduleAggregationWorker) + .to receive(:perform_async).once + + subject.write_attribute(statistic_attribute, read_attribute + delta) + subject.save! + end + + it 'avoids N + 1 queries' do + subject.write_attribute(statistic_attribute, read_attribute + delta) + + control_count = ActiveRecord::QueryRecorder.new do + subject.save! + end + + subject.write_attribute(statistic_attribute, read_attribute + delta) + + expect do + subject.save! + end.not_to exceed_query_limit(control_count) + end + + context 'when the feature flag is disabled for the namespace' do + it 'does not schedule a namespace statistics worker' do + namespace = subject.project.root_ancestor + + stub_feature_flags(update_statistics_namespace: false, namespace: namespace) + + expect(Namespaces::ScheduleAggregationWorker) + .not_to receive(:perform_async) + + subject.write_attribute(statistic_attribute, read_attribute + delta) + subject.save! + end + end end context 'when destroying' do @@ -59,11 +115,18 @@ shared_examples_for 'UpdateProjectStatistics' do .to receive(:increment_statistic) .and_call_original - expect { subject.destroy } + expect { subject.destroy! } .to change { reload_stat } .by(delta) end + it 'schedules a namespace statistics worker' do + expect(Namespaces::ScheduleAggregationWorker) + .to receive(:perform_async).once + + subject.destroy! + end + context 'when it is destroyed from the project level' do it 'does not update the project statistics' do expect(ProjectStatistics) @@ -72,6 +135,27 @@ shared_examples_for 'UpdateProjectStatistics' do project.update(pending_delete: true) project.destroy! end + + it 'does not schedule a namespace statistics worker' do + expect(Namespaces::ScheduleAggregationWorker) + .not_to receive(:perform_async) + + project.update(pending_delete: true) + project.destroy! + end + end + + context 'when feature flag is disabled for the namespace' do + it 'does not schedule a namespace statistics worker' do + namespace = subject.project.root_ancestor + + stub_feature_flags(update_statistics_namespace: false, namespace: namespace) + + expect(Namespaces::ScheduleAggregationWorker) + .not_to receive(:perform_async) + + subject.destroy! + end end end end diff --git a/spec/workers/namespaces/prune_aggregation_schedules_worker_spec.rb b/spec/workers/namespaces/prune_aggregation_schedules_worker_spec.rb new file mode 100644 index 00000000000..b069b080531 --- /dev/null +++ b/spec/workers/namespaces/prune_aggregation_schedules_worker_spec.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Namespaces::PruneAggregationSchedulesWorker, '#perform', :clean_gitlab_redis_shared_state do + include ExclusiveLeaseHelpers + + let(:namespaces) { create_list(:namespace, 5, :with_aggregation_schedule) } + let(:timeout) { Namespace::AggregationSchedule::DEFAULT_LEASE_TIMEOUT } + + subject(:worker) { described_class.new } + + before do + allow(Namespaces::RootStatisticsWorker) + .to receive(:perform_async).and_return(nil) + + allow(Namespaces::RootStatisticsWorker) + .to receive(:perform_in).and_return(nil) + + namespaces.each do |namespace| + lease_key = "namespace:namespaces_root_statistics:#{namespace.id}" + stub_exclusive_lease(lease_key, timeout: timeout) + end + end + + it 'schedules a worker per pending aggregation' do + expect(Namespaces::RootStatisticsWorker) + .to receive(:perform_async).exactly(5).times + + expect(Namespaces::RootStatisticsWorker) + .to receive(:perform_in).exactly(5).times + + worker.perform + end +end diff --git a/spec/workers/namespaces/root_statistics_worker_spec.rb b/spec/workers/namespaces/root_statistics_worker_spec.rb new file mode 100644 index 00000000000..8dd74b96d49 --- /dev/null +++ b/spec/workers/namespaces/root_statistics_worker_spec.rb @@ -0,0 +1,88 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Namespaces::RootStatisticsWorker, '#perform' do + let(:group) { create(:group, :with_aggregation_schedule) } + + subject(:worker) { described_class.new } + + context 'with a namespace' do + it 'executes refresher service' do + expect_any_instance_of(Namespaces::StatisticsRefresherService) + .to receive(:execute) + + worker.perform(group.id) + end + + it 'deletes namespace aggregated schedule row' do + worker.perform(group.id) + + expect(group.reload.aggregation_schedule).to be_nil + end + + context 'when something goes wrong when updating' do + before do + allow_any_instance_of(Namespaces::StatisticsRefresherService) + .to receive(:execute) + .and_raise(Namespaces::StatisticsRefresherService::RefresherError, 'error') + end + + it 'does not delete the aggregation schedule' do + worker.perform(group.id) + + expect(group.reload.aggregation_schedule).to be_present + end + + it 'logs the error' do + # A Namespace::RootStatisticsWorker is scheduled when + # a Namespace::AggregationSchedule is created, so having + # create(:group, :with_aggregation_schedule), will execute + # another worker + allow_any_instance_of(Namespace::AggregationSchedule) + .to receive(:schedule_root_storage_statistics).and_return(nil) + + expect(Gitlab::SidekiqLogger).to receive(:error).once + + worker.perform(group.id) + end + end + end + + context 'with no namespace' do + before do + group.destroy + end + + it 'does not execute the refresher service' do + expect_any_instance_of(Namespaces::StatisticsRefresherService) + .not_to receive(:execute) + + worker.perform(group.id) + end + end + + context 'with a namespace with no aggregation scheduled' do + before do + group.aggregation_schedule.destroy + end + + it 'does not execute the refresher service' do + expect_any_instance_of(Namespaces::StatisticsRefresherService) + .not_to receive(:execute) + + worker.perform(group.id) + end + end + + context 'when update_statistics_namespace is off' do + it 'does not create a new one' do + stub_feature_flags(update_statistics_namespace: false, namespace: group) + + expect_any_instance_of(Namespaces::StatisticsRefresherService) + .not_to receive(:execute) + + worker.perform(group.id) + end + end +end diff --git a/spec/workers/namespaces/schedule_aggregation_worker_spec.rb b/spec/workers/namespaces/schedule_aggregation_worker_spec.rb new file mode 100644 index 00000000000..7432ca12f2a --- /dev/null +++ b/spec/workers/namespaces/schedule_aggregation_worker_spec.rb @@ -0,0 +1,66 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Namespaces::ScheduleAggregationWorker, '#perform' do + let(:group) { create(:group) } + + subject(:worker) { described_class.new } + + context 'when group is the root ancestor' do + context 'when aggregation schedule exists' do + it 'does not create a new one' do + Namespace::AggregationSchedule.safe_find_or_create_by!(namespace_id: group.id) + + expect do + worker.perform(group.id) + end.not_to change(Namespace::AggregationSchedule, :count) + end + end + + context 'when update_statistics_namespace is off' do + it 'does not create a new one' do + stub_feature_flags(update_statistics_namespace: false, namespace: group) + + expect do + worker.perform(group.id) + end.not_to change(Namespace::AggregationSchedule, :count) + end + end + + context 'when aggregation schedule does not exist' do + it 'creates one' do + allow_any_instance_of(Namespace::AggregationSchedule) + .to receive(:schedule_root_storage_statistics).and_return(nil) + + expect do + worker.perform(group.id) + end.to change(Namespace::AggregationSchedule, :count).by(1) + + expect(group.aggregation_schedule).to be_present + end + end + end + + context 'when group is not the root ancestor' do + let(:parent_group) { create(:group) } + let(:group) { create(:group, parent: parent_group) } + + it 'creates an aggregation schedule for the root' do + allow_any_instance_of(Namespace::AggregationSchedule) + .to receive(:schedule_root_storage_statistics).and_return(nil) + + worker.perform(group.id) + + expect(parent_group.aggregation_schedule).to be_present + end + end + + context 'when namespace does not exist' do + it 'logs the error' do + expect(Gitlab::SidekiqLogger).to receive(:error).once + + worker.perform(12345) + end + end +end From 2f76ccd4413a5af54aa839c53f04f3760357c465 Mon Sep 17 00:00:00 2001 From: Takuya Noguchi Date: Wed, 26 Jun 2019 23:17:13 +0900 Subject: [PATCH 19/40] Use PostgreSQL 9.6.11 in CI tests Specify the minor version of postgresql for CI Signed-off-by: Takuya Noguchi --- .gitlab/ci/frontend.gitlab-ci.yml | 2 +- .gitlab/ci/rails.gitlab-ci.yml | 2 +- changelogs/unreleased/use-pg-9-6-11-on-ci.yml | 5 +++++ 3 files changed, 7 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/use-pg-9-6-11-on-ci.yml diff --git a/.gitlab/ci/frontend.gitlab-ci.yml b/.gitlab/ci/frontend.gitlab-ci.yml index cd6953a6ac2..65ff3393ebb 100644 --- a/.gitlab/ci/frontend.gitlab-ci.yml +++ b/.gitlab/ci/frontend.gitlab-ci.yml @@ -8,7 +8,7 @@ .use-pg: &use-pg services: - - name: postgres:9.6 + - name: postgres:9.6.11 command: ["postgres", "-c", "fsync=off", "-c", "synchronous_commit=off", "-c", "full_page_writes=off"] - name: redis:alpine diff --git a/.gitlab/ci/rails.gitlab-ci.yml b/.gitlab/ci/rails.gitlab-ci.yml index 6551f47d3be..dd1d46c9fa2 100644 --- a/.gitlab/ci/rails.gitlab-ci.yml +++ b/.gitlab/ci/rails.gitlab-ci.yml @@ -1,6 +1,6 @@ .use-pg: &use-pg services: - - name: postgres:9.6 + - name: postgres:9.6.11 command: ["postgres", "-c", "fsync=off", "-c", "synchronous_commit=off", "-c", "full_page_writes=off"] - name: redis:alpine diff --git a/changelogs/unreleased/use-pg-9-6-11-on-ci.yml b/changelogs/unreleased/use-pg-9-6-11-on-ci.yml new file mode 100644 index 00000000000..785eb352895 --- /dev/null +++ b/changelogs/unreleased/use-pg-9-6-11-on-ci.yml @@ -0,0 +1,5 @@ +--- +title: Use PostgreSQL 9.6.11 in CI tests +merge_request: 30270 +author: Takuya Noguchi +type: other From f312137dd319d583bbba4681e682bd9a6d863857 Mon Sep 17 00:00:00 2001 From: Winnie Hellmann Date: Tue, 2 Jul 2019 15:07:49 +0000 Subject: [PATCH 20/40] Add @pslaughter to CODEOWNERS --- .gitlab/CODEOWNERS | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.gitlab/CODEOWNERS b/.gitlab/CODEOWNERS index f65e62068d6..b865b212ac0 100644 --- a/.gitlab/CODEOWNERS +++ b/.gitlab/CODEOWNERS @@ -6,8 +6,8 @@ /doc/ @axil @marcia @eread @mikelewis # Frontend maintainers should see everything in `app/assets/` -app/assets/ @ClemMakesApps @fatihacet @filipa @iamphill @mikegreiling @timzallmann @kushalpandya -*.scss @annabeldunstone @ClemMakesApps @fatihacet @filipa @iamphill @mikegreiling @timzallmann @kushalpandya +app/assets/ @ClemMakesApps @fatihacet @filipa @iamphill @mikegreiling @timzallmann @kushalpandya @pslaughter +*.scss @annabeldunstone @ClemMakesApps @fatihacet @filipa @iamphill @mikegreiling @timzallmann @kushalpandya @pslaughter # Someone from the database team should review changes in `db/` db/ @abrandl @NikolayS From 57ae2f7e2d56a38140ae66915885567d361e240a Mon Sep 17 00:00:00 2001 From: Aleksei Lipniagov Date: Tue, 2 Jul 2019 10:36:33 +0300 Subject: [PATCH 21/40] Use separate Prometheus metrics dirs in dev/test Store Sidekiq and Web server metrics from Prometheus in separate directories in `development` and `test` environments. --- config/initializers/7_prometheus_metrics.rb | 20 +++++++++++++++---- tmp/prometheus_multiproc_dir/puma/.gitkeep | 0 tmp/prometheus_multiproc_dir/sidekiq/.gitkeep | 0 tmp/prometheus_multiproc_dir/unicorn/.gitkeep | 0 4 files changed, 16 insertions(+), 4 deletions(-) create mode 100644 tmp/prometheus_multiproc_dir/puma/.gitkeep create mode 100644 tmp/prometheus_multiproc_dir/sidekiq/.gitkeep create mode 100644 tmp/prometheus_multiproc_dir/unicorn/.gitkeep diff --git a/config/initializers/7_prometheus_metrics.rb b/config/initializers/7_prometheus_metrics.rb index 54cdefc2a10..d70d83b24d9 100644 --- a/config/initializers/7_prometheus_metrics.rb +++ b/config/initializers/7_prometheus_metrics.rb @@ -1,15 +1,27 @@ require 'prometheus/client' require 'prometheus/client/support/unicorn' +# Keep separate directories for separate processes +def prometheus_default_multiproc_dir + return unless Rails.env.development? || Rails.env.test? + + if Sidekiq.server? + Rails.root.join('tmp/prometheus_multiproc_dir/sidekiq') + elsif defined?(Unicorn::Worker) + Rails.root.join('tmp/prometheus_multiproc_dir/unicorn') + elsif defined?(::Puma) + Rails.root.join('tmp/prometheus_multiproc_dir/puma') + else + Rails.root.join('tmp/prometheus_multiproc_dir') + end +end + Prometheus::Client.configure do |config| config.logger = Rails.logger config.initial_mmap_file_size = 4 * 1024 - config.multiprocess_files_dir = ENV['prometheus_multiproc_dir'] - if Rails.env.development? || Rails.env.test? - config.multiprocess_files_dir ||= Rails.root.join('tmp/prometheus_multiproc_dir') - end + config.multiprocess_files_dir = ENV['prometheus_multiproc_dir'] || prometheus_default_multiproc_dir config.pid_provider = Prometheus::Client::Support::Unicorn.method(:worker_pid_provider) end diff --git a/tmp/prometheus_multiproc_dir/puma/.gitkeep b/tmp/prometheus_multiproc_dir/puma/.gitkeep new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tmp/prometheus_multiproc_dir/sidekiq/.gitkeep b/tmp/prometheus_multiproc_dir/sidekiq/.gitkeep new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tmp/prometheus_multiproc_dir/unicorn/.gitkeep b/tmp/prometheus_multiproc_dir/unicorn/.gitkeep new file mode 100644 index 00000000000..e69de29bb2d From c15676c4f46c3ce93a95062f770ffc22ef9eff1e Mon Sep 17 00:00:00 2001 From: Mek Stittri Date: Tue, 2 Jul 2019 16:25:32 +0000 Subject: [PATCH 22/40] Reconcile our team, group label definitions with our organization structure --- .../contributing/issue_workflow.md | 83 +++++++++++++++---- 1 file changed, 69 insertions(+), 14 deletions(-) diff --git a/doc/development/contributing/issue_workflow.md b/doc/development/contributing/issue_workflow.md index d9595bd7bba..f1015f56106 100644 --- a/doc/development/contributing/issue_workflow.md +++ b/doc/development/contributing/issue_workflow.md @@ -6,10 +6,13 @@ scheduling into milestones. Labelling is a task for everyone. Most issues will have labels for at least one of the following: -- Type: ~feature, ~bug, ~customer, etc. -- Subject: ~wiki, ~"Container Registry", ~ldap, ~api, ~frontend, etc. -- Team: ~Plan, ~Manage, ~Quality, etc. -- Stage: ~"devops:plan", ~"devops:create", etc. +- Type: ~feature, ~bug, ~backstage, etc. +- Subject: ~wiki, ~"Container Registry", ~ldap, ~api, etc. +- Team: ~Documentation, ~Delivery, etc. +- Stage: ~"devops::plan", ~"devops::create", etc. +- Group: ~"group::source code" ~"group::knowledge" ~"group::editor", etc. +- Department: ~UX, ~Quality +- Specialization: ~frontend, ~backend - Release Scoping: ~Deliverable, ~Stretch, ~"Next Patch Release" - Priority: ~P1, ~P2, ~P3, ~P4 - Severity: ~S1, ~S2, ~S3, ~S4 @@ -27,8 +30,7 @@ labels, you can _always_ add the team and type, and often also the subject. Type labels are very important. They define what kind of issue this is. Every issue should have one or more. -Examples of type labels are ~feature, ~bug, ~customer, ~security, -and ~direction. +Examples of type labels are ~feature, ~bug, ~backstage and ~security A number of type labels have a priority assigned to them, which automatically makes them float to the top, depending on their importance. @@ -56,19 +58,20 @@ issue is labeled with a subject label corresponding to your expertise. Subject labels are always all-lowercase. -## Team labels +## Team labels + +**Important**: Most of the team labels will be soon deprecated in favor of [Group labels](#group-labels). Team labels specify what team is responsible for this issue. Assigning a team label makes sure issues get the attention of the appropriate people. -The current team labels are: +The team labels planned for deprecation are: - ~Configure - ~Create - ~Defend - ~Distribution -- ~Documentation - ~Ecosystem - ~Geo - ~Gitaly @@ -77,12 +80,15 @@ The current team labels are: - ~Memory - ~Monitor - ~Plan -- ~Quality - ~Release - ~Secure -- ~UX - ~Verify +The following team labels are **true** teams per our [organization structure](https://about.gitlab.com/company/team/structure/#organizational-structure) which will remain post deprecation. + +- ~Delivery +- ~Documentation + The descriptions on the [labels page][labels-page] explain what falls under the responsibility of each team. @@ -92,6 +98,7 @@ indicate if an issue needs backend work, frontend work, or both. Team labels are always capitalized so that they show up as the first label for any issue. + ## Stage labels Stage labels specify which [DevOps stage][devops-stages] the issue belongs to. @@ -132,10 +139,44 @@ The Stage labels are used to generate the [direction pages][direction-pages] aut Group labels specify which [groups][structure-groups] the issue belongs to. -Examples include: +The current group labels are: -- ~"group::control" -- ~"group::editor" +* ~"group::access" +* ~"group::measure" +* ~"group::source code" +* ~"group::knowledge" +* ~"group::editor" +* ~"group::gitaly" +* ~"group::gitter" +* ~"group::team planning" +* ~"group::enterprise planning" +* ~"group::certify" +* ~"group::ci and runner" +* ~"group::testing" +* ~"group::package" +* ~"group::core release" +* ~"group::supporting capabilities" +* ~"group::autodevops and kubernetes" +* ~"group::serverless and paas" +* ~"group::apm" +* ~"group::health" +* ~"group::static analysis" +* ~"group::dynamic analysis" +* ~"group::software composition analysis" +* ~"group::runtime application security" +* ~"group::threat management" +* ~"group::application infrastructure security" +* ~"group::activation" +* ~"group::adoption" +* ~"group::upsell" +* ~"group::retention" +* ~"group::fulfillment" +* ~"group::telemetry" +* ~"group::distribution" +* ~"group::geo" +* ~"group::memory" +* ~"group::ecosystem" + These labels are [scoped labels](../../user/project/labels.md#scoped-labels-premium) and thus are mutually exclusive. @@ -147,6 +188,20 @@ can be applied to a single issue. You can find the groups listed in the [structure-groups]: https://about.gitlab.com/company/team/structure/#groups [product-categories]: https://about.gitlab.com/handbook/product/categories/ +## Department labels + +The current department labels are: + +* ~UX +* ~Quality + +## Specialization labels + +These labels narrow the [specialization](https://about.gitlab.com/company/team/structure/#specialist) on a unit of work. + +* ~frontend +* ~backend + ## Release Scoping labels Release Scoping labels help us clearly communicate expectations of the work for the From 2e909cb6dfd800fa4c619b8c69fa39cde42da1d1 Mon Sep 17 00:00:00 2001 From: Pat Myron Date: Thu, 20 Jun 2019 13:18:16 +0000 Subject: [PATCH 23/40] LC_ALL=C.UTF-8 --- lib/gitlab/ci/templates/Pages/Jekyll.gitlab-ci.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/gitlab/ci/templates/Pages/Jekyll.gitlab-ci.yml b/lib/gitlab/ci/templates/Pages/Jekyll.gitlab-ci.yml index 0d742aa282d..e7dacd3a1fc 100644 --- a/lib/gitlab/ci/templates/Pages/Jekyll.gitlab-ci.yml +++ b/lib/gitlab/ci/templates/Pages/Jekyll.gitlab-ci.yml @@ -4,6 +4,7 @@ image: ruby:2.3 variables: JEKYLL_ENV: production + LC_ALL: C.UTF-8 before_script: - bundle install From 3c883759c6ea9c1c60e69fd3e65fbf56b9e7af30 Mon Sep 17 00:00:00 2001 From: Jan Provaznik Date: Tue, 2 Jul 2019 17:05:56 +0000 Subject: [PATCH 24/40] Start UnicornSampler in master process Using `on_master_start` assures that the sampler is started in master process and not in worker processes. --- changelogs/unreleased/unicorn-sampler-fix.yml | 5 +++++ config/initializers/7_prometheus_metrics.rb | 10 ++++------ 2 files changed, 9 insertions(+), 6 deletions(-) create mode 100644 changelogs/unreleased/unicorn-sampler-fix.yml diff --git a/changelogs/unreleased/unicorn-sampler-fix.yml b/changelogs/unreleased/unicorn-sampler-fix.yml new file mode 100644 index 00000000000..3f0e509f15f --- /dev/null +++ b/changelogs/unreleased/unicorn-sampler-fix.yml @@ -0,0 +1,5 @@ +--- +title: Make sure UnicornSampler is started only in master process. +merge_request: 30215 +author: +type: fixed diff --git a/config/initializers/7_prometheus_metrics.rb b/config/initializers/7_prometheus_metrics.rb index 54cdefc2a10..5db72141fcd 100644 --- a/config/initializers/7_prometheus_metrics.rb +++ b/config/initializers/7_prometheus_metrics.rb @@ -29,15 +29,13 @@ if !Rails.env.test? && Gitlab::Metrics.prometheus_metrics_enabled? Gitlab::Cluster::LifecycleEvents.on_worker_start do defined?(::Prometheus::Client.reinitialize_on_pid_change) && Prometheus::Client.reinitialize_on_pid_change - if defined?(::Unicorn) - Gitlab::Metrics::Samplers::UnicornSampler.initialize_instance(Settings.monitoring.unicorn_sampler_interval).start - end - Gitlab::Metrics::Samplers::RubySampler.initialize_instance(Settings.monitoring.ruby_sampler_interval).start end - if defined?(::Puma) - Gitlab::Cluster::LifecycleEvents.on_master_start do + Gitlab::Cluster::LifecycleEvents.on_master_start do + if defined?(::Unicorn) + Gitlab::Metrics::Samplers::UnicornSampler.initialize_instance(Settings.monitoring.unicorn_sampler_interval).start + elsif defined?(::Puma) Gitlab::Metrics::Samplers::PumaSampler.initialize_instance(Settings.monitoring.puma_sampler_interval).start end end From 2302385cce79b7407d73acccd190f77e55370f04 Mon Sep 17 00:00:00 2001 From: Rajat Jain Date: Tue, 2 Jul 2019 18:03:45 +0530 Subject: [PATCH 25/40] Don't let logged out user do manual order Add a check for logged out user in the manual order so that they don't see an flash message when they try to reorder issues. --- app/assets/javascripts/manual_ordering.js | 2 +- changelogs/unreleased/rj-fix-manual-order.yml | 5 +++++ spec/features/groups/issues_spec.rb | 19 +++++++++++++++++++ 3 files changed, 25 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/rj-fix-manual-order.yml diff --git a/app/assets/javascripts/manual_ordering.js b/app/assets/javascripts/manual_ordering.js index e16ddbfef7e..012d1e70410 100644 --- a/app/assets/javascripts/manual_ordering.js +++ b/app/assets/javascripts/manual_ordering.js @@ -21,7 +21,7 @@ const updateIssue = (url, issueList, { move_before_id, move_after_id }) => const initManualOrdering = () => { const issueList = document.querySelector('.manual-ordering'); - if (!issueList || !(gon.features && gon.features.manualSorting)) { + if (!issueList || !(gon.features && gon.features.manualSorting) || !(gon.current_user_id > 0)) { return; } diff --git a/changelogs/unreleased/rj-fix-manual-order.yml b/changelogs/unreleased/rj-fix-manual-order.yml new file mode 100644 index 00000000000..ecc39b78b06 --- /dev/null +++ b/changelogs/unreleased/rj-fix-manual-order.yml @@ -0,0 +1,5 @@ +--- +title: Don't let logged out user do manual order +merge_request: 30264 +author: +type: fixed diff --git a/spec/features/groups/issues_spec.rb b/spec/features/groups/issues_spec.rb index c000165ccd9..0ada530781c 100644 --- a/spec/features/groups/issues_spec.rb +++ b/spec/features/groups/issues_spec.rb @@ -150,6 +150,25 @@ describe 'Group issues page' do check_issue_order end + it 'issues should not be draggable when user is not logged in', :js do + sign_out(user_in_group) + + visit issues_group_path(group, sort: 'relative_position') + + drag_to(selector: '.manual-ordering', + from_index: 0, + to_index: 2) + + wait_for_requests + + # Issue order should remain the same + page.within('.manual-ordering') do + expect(find('.issue:nth-child(1) .title')).to have_content('Issue #1') + expect(find('.issue:nth-child(2) .title')).to have_content('Issue #2') + expect(find('.issue:nth-child(3) .title')).to have_content('Issue #3') + end + end + def check_issue_order page.within('.manual-ordering') do expect(find('.issue:nth-child(1) .title')).to have_content('Issue #2') From d745ff0431130a760a7a59899c26410dc887f77a Mon Sep 17 00:00:00 2001 From: Krasimir Angelov Date: Tue, 2 Jul 2019 18:56:48 +0000 Subject: [PATCH 26/40] Add username to deploy tokens This new attribute is optional and used when set instead of the default format `gitlab+deploy-token-#{id}`. Empty usernames will be saved as null in the database. Related to https://gitlab.com/gitlab-org/gitlab-ce/issues/50228. --- .../settings/repository_controller.rb | 2 +- app/models/deploy_token.rb | 14 +++++++- app/services/deploy_tokens/create_service.rb | 4 ++- .../projects/deploy_tokens/_form.html.haml | 5 +++ .../50228-deploy-tokens-custom-username.yml | 5 +++ ...613044655_add_username_to_deploy_tokens.rb | 9 +++++ db/schema.rb | 1 + locale/gitlab.pot | 3 ++ .../settings/repository_controller_spec.rb | 20 +++++++++++ .../settings/repository_settings_spec.rb | 6 ++++ spec/lib/gitlab/auth_spec.rb | 9 +++++ spec/models/deploy_token_spec.rb | 35 +++++++++++++++++-- .../deploy_tokens/create_service_spec.rb | 16 +++++++++ 13 files changed, 124 insertions(+), 5 deletions(-) create mode 100644 changelogs/unreleased/50228-deploy-tokens-custom-username.yml create mode 100644 db/migrate/20190613044655_add_username_to_deploy_tokens.rb diff --git a/app/controllers/projects/settings/repository_controller.rb b/app/controllers/projects/settings/repository_controller.rb index ac3004d069f..bc2ce15286f 100644 --- a/app/controllers/projects/settings/repository_controller.rb +++ b/app/controllers/projects/settings/repository_controller.rb @@ -99,7 +99,7 @@ module Projects end def deploy_token_params - params.require(:deploy_token).permit(:name, :expires_at, :read_repository, :read_registry) + params.require(:deploy_token).permit(:name, :expires_at, :read_repository, :read_registry, :username) end end end diff --git a/app/models/deploy_token.rb b/app/models/deploy_token.rb index b0e570f52ba..33f0be91632 100644 --- a/app/models/deploy_token.rb +++ b/app/models/deploy_token.rb @@ -16,6 +16,14 @@ class DeployToken < ApplicationRecord has_many :projects, through: :project_deploy_tokens validate :ensure_at_least_one_scope + validates :username, + length: { maximum: 255 }, + allow_nil: true, + format: { + with: /\A[a-zA-Z0-9\.\+_-]+\z/, + message: "can contain only letters, digits, '_', '-', '+', and '.'" + } + before_save :ensure_token accepts_nested_attributes_for :project_deploy_tokens @@ -39,7 +47,7 @@ class DeployToken < ApplicationRecord end def username - "gitlab+deploy-token-#{id}" + super || default_username end def has_access_to?(requested_project) @@ -75,4 +83,8 @@ class DeployToken < ApplicationRecord def ensure_at_least_one_scope errors.add(:base, "Scopes can't be blank") unless read_repository || read_registry end + + def default_username + "gitlab+deploy-token-#{id}" if persisted? + end end diff --git a/app/services/deploy_tokens/create_service.rb b/app/services/deploy_tokens/create_service.rb index dc0122002e9..327a1dbf408 100644 --- a/app/services/deploy_tokens/create_service.rb +++ b/app/services/deploy_tokens/create_service.rb @@ -3,7 +3,9 @@ module DeployTokens class CreateService < BaseService def execute - @project.deploy_tokens.create(params) + @project.deploy_tokens.create(params) do |deploy_token| + deploy_token.username = params[:username].presence + end end end end diff --git a/app/views/projects/deploy_tokens/_form.html.haml b/app/views/projects/deploy_tokens/_form.html.haml index 5412fcbc9d8..f846dbd3763 100644 --- a/app/views/projects/deploy_tokens/_form.html.haml +++ b/app/views/projects/deploy_tokens/_form.html.haml @@ -12,6 +12,11 @@ = f.label :expires_at, class: 'label-bold' = f.text_field :expires_at, class: 'datepicker form-control qa-deploy-token-expires-at', value: f.object.expires_at + .form-group + = f.label :username, class: 'label-bold' + = f.text_field :username, class: 'form-control qa-deploy-token-username' + .text-secondary= s_('DeployTokens|Default format is "gitlab+deploy-token-{n}". Enter custom username if you want to change it.') + .form-group = f.label :scopes, class: 'label-bold' %fieldset.form-group.form-check diff --git a/changelogs/unreleased/50228-deploy-tokens-custom-username.yml b/changelogs/unreleased/50228-deploy-tokens-custom-username.yml new file mode 100644 index 00000000000..fdafa7b1113 --- /dev/null +++ b/changelogs/unreleased/50228-deploy-tokens-custom-username.yml @@ -0,0 +1,5 @@ +--- +title: Allow custom username for deploy tokens +merge_request: 29639 +author: +type: added diff --git a/db/migrate/20190613044655_add_username_to_deploy_tokens.rb b/db/migrate/20190613044655_add_username_to_deploy_tokens.rb new file mode 100644 index 00000000000..793553afe35 --- /dev/null +++ b/db/migrate/20190613044655_add_username_to_deploy_tokens.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class AddUsernameToDeployTokens < ActiveRecord::Migration[5.1] + DOWNTIME = false + + def change + add_column :deploy_tokens, :username, :string + end +end diff --git a/db/schema.rb b/db/schema.rb index 7948f919c57..143f6c7127e 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1047,6 +1047,7 @@ ActiveRecord::Schema.define(version: 20190628145246) do t.datetime_with_timezone "created_at", null: false t.string "name", null: false t.string "token", null: false + t.string "username" t.index ["token", "expires_at", "id"], name: "index_deploy_tokens_on_token_and_expires_at_and_id", where: "(revoked IS FALSE)", using: :btree t.index ["token"], name: "index_deploy_tokens_on_token", unique: true, using: :btree end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 60d4e199a9f..72324674bbc 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -3557,6 +3557,9 @@ msgstr "" msgid "DeployTokens|Created" msgstr "" +msgid "DeployTokens|Default format is \"gitlab+deploy-token-{n}\". Enter custom username if you want to change it." +msgstr "" + msgid "DeployTokens|Deploy Tokens" msgstr "" diff --git a/spec/controllers/projects/settings/repository_controller_spec.rb b/spec/controllers/projects/settings/repository_controller_spec.rb index b34053fc993..7f67f67e775 100644 --- a/spec/controllers/projects/settings/repository_controller_spec.rb +++ b/spec/controllers/projects/settings/repository_controller_spec.rb @@ -32,4 +32,24 @@ describe Projects::Settings::RepositoryController do expect(RepositoryCleanupWorker).to have_received(:perform_async).once end end + + describe 'POST create_deploy_token' do + let(:deploy_token_params) do + { + name: 'deployer_token', + expires_at: 1.month.from_now.to_date.to_s, + username: 'deployer', + read_repository: '1' + } + end + + subject(:create_deploy_token) { post :create_deploy_token, params: { namespace_id: project.namespace, project_id: project, deploy_token: deploy_token_params } } + + it 'creates deploy token' do + expect { create_deploy_token }.to change { DeployToken.active.count }.by(1) + + expect(response).to have_gitlab_http_status(200) + expect(response).to render_template(:show) + end + end end diff --git a/spec/features/projects/settings/repository_settings_spec.rb b/spec/features/projects/settings/repository_settings_spec.rb index 8c7bc192c50..1edfee705c8 100644 --- a/spec/features/projects/settings/repository_settings_spec.rb +++ b/spec/features/projects/settings/repository_settings_spec.rb @@ -112,11 +112,17 @@ describe 'Projects > Settings > Repository settings' do it 'add a new deploy token' do fill_in 'deploy_token_name', with: 'new_deploy_key' fill_in 'deploy_token_expires_at', with: (Date.today + 1.month).to_s + fill_in 'deploy_token_username', with: 'deployer' check 'deploy_token_read_repository' check 'deploy_token_read_registry' click_button 'Create deploy token' expect(page).to have_content('Your new project deploy token has been created') + + within('.created-deploy-token-container') do + expect(page).to have_selector("input[name='deploy-token-user'][value='deployer']") + expect(page).to have_selector("input[name='deploy-token'][readonly='readonly']") + end end end diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb index 3b5ca7c950c..d9c73cff01e 100644 --- a/spec/lib/gitlab/auth_spec.rb +++ b/spec/lib/gitlab/auth_spec.rb @@ -309,6 +309,15 @@ describe Gitlab::Auth do .to eq(auth_success) end + it 'succeeds when custom login and token are valid' do + deploy_token = create(:deploy_token, username: 'deployer', read_registry: false, projects: [project]) + auth_success = Gitlab::Auth::Result.new(deploy_token, project, :deploy_token, [:download_code]) + + expect(gl_auth).to receive(:rate_limit!).with('ip', success: true, login: 'deployer') + expect(gl_auth.find_for_git_client('deployer', deploy_token.token, project: project, ip: 'ip')) + .to eq(auth_success) + end + it 'fails when login is not valid' do expect(gl_auth).to receive(:rate_limit!).with('ip', success: false, login: 'random_login') expect(gl_auth.find_for_git_client('random_login', deploy_token.token, project: project, ip: 'ip')) diff --git a/spec/models/deploy_token_spec.rb b/spec/models/deploy_token_spec.rb index 2fe82eaa778..8d951ab6f0f 100644 --- a/spec/models/deploy_token_spec.rb +++ b/spec/models/deploy_token_spec.rb @@ -8,6 +8,15 @@ describe DeployToken do it { is_expected.to have_many :project_deploy_tokens } it { is_expected.to have_many(:projects).through(:project_deploy_tokens) } + describe 'validations' do + let(:username_format_message) { "can contain only letters, digits, '_', '-', '+', and '.'" } + + it { is_expected.to validate_length_of(:username).is_at_most(255) } + it { is_expected.to allow_value('GitLab+deploy_token-3.14').for(:username) } + it { is_expected.not_to allow_value(' - diff --git a/changelogs/unreleased/sh-improve-redis-peek.yml b/changelogs/unreleased/sh-improve-redis-peek.yml new file mode 100644 index 00000000000..940be103ab7 --- /dev/null +++ b/changelogs/unreleased/sh-improve-redis-peek.yml @@ -0,0 +1,5 @@ +--- +title: Add Redis call details in Peek performance bar +merge_request: 30191 +author: +type: changed diff --git a/lib/peek/views/redis.rb b/lib/peek/views/redis.rb new file mode 100644 index 00000000000..ad3c3c9fe01 --- /dev/null +++ b/lib/peek/views/redis.rb @@ -0,0 +1,82 @@ +# frozen_string_literal: true + +require 'redis' +require 'peek-redis' + +module Gitlab + module Peek + module RedisInstrumented + def call(*args, &block) + start = Time.now + super(*args, &block) + ensure + duration = (Time.now - start) + add_call_details(duration, args) + end + + private + + def add_call_details(duration, args) + # redis-rb passes an array (e.g. [:get, key]) + return unless args.length == 1 + + detail_store << { + cmd: args.first, + duration: duration, + backtrace: Gitlab::Profiler.clean_backtrace(caller) + } + end + + def detail_store + ::Gitlab::SafeRequestStore['redis_call_details'] ||= [] + end + end + end +end + +module Peek + module Views + module RedisDetailed + def results + super.merge(details: details) + end + + def details + detail_store + .sort { |a, b| b[:duration] <=> a[:duration] } + .map(&method(:format_call_details)) + end + + def detail_store + ::Gitlab::SafeRequestStore['redis_call_details'] ||= [] + end + + def format_call_details(call) + call.merge(cmd: format_command(call[:cmd]), + duration: (call[:duration] * 1000).round(3)) + end + + def format_command(cmd) + # Scrub out the value of the SET calls to avoid binary + # data or large data from spilling into the view + if cmd.length >= 2 && cmd.first =~ /set/i + cmd[-1] = "" + end + + cmd.join(' ') + end + end + end +end + +class Redis::Client + prepend Gitlab::Peek::RedisInstrumented +end + +module Peek + module Views + class Redis < View + prepend Peek::Views::RedisDetailed + end + end +end diff --git a/spec/javascripts/performance_bar/components/simple_metric_spec.js b/spec/javascripts/performance_bar/components/simple_metric_spec.js deleted file mode 100644 index 98b843e9711..00000000000 --- a/spec/javascripts/performance_bar/components/simple_metric_spec.js +++ /dev/null @@ -1,47 +0,0 @@ -import Vue from 'vue'; -import simpleMetric from '~/performance_bar/components/simple_metric.vue'; -import mountComponent from 'spec/helpers/vue_mount_component_helper'; - -describe('simpleMetric', () => { - let vm; - - afterEach(() => { - vm.$destroy(); - }); - - describe('when the current request has no details', () => { - beforeEach(() => { - vm = mountComponent(Vue.extend(simpleMetric), { - currentRequest: {}, - metric: 'gitaly', - }); - }); - - it('does not display details', () => { - expect(vm.$el.innerText).not.toContain('/'); - }); - - it('displays the metric name', () => { - expect(vm.$el.innerText).toContain('gitaly'); - }); - }); - - describe('when the current request has details', () => { - beforeEach(() => { - vm = mountComponent(Vue.extend(simpleMetric), { - currentRequest: { - details: { gitaly: { duration: '123ms', calls: '456' } }, - }, - metric: 'gitaly', - }); - }); - - it('diplays details', () => { - expect(vm.$el.innerText.replace(/\s+/g, ' ')).toContain('123ms / 456'); - }); - - it('displays the metric name', () => { - expect(vm.$el.innerText).toContain('gitaly'); - }); - }); -}); From 1464b1e0004c17ea56808514577233e95bd7fdd2 Mon Sep 17 00:00:00 2001 From: Marcel Amirault Date: Wed, 3 Jul 2019 07:12:04 +0000 Subject: [PATCH 36/40] Fix markdown to reduce number of kramdown warnings Add backticks around square brackets, or fix links, as necessary. --- doc/administration/auth/ldap.md | 4 +- .../geo/replication/updating_the_geo_nodes.md | 48 +++++++++---------- doc/administration/high_availability/redis.md | 4 +- .../monitoring/prometheus/gitlab_metrics.md | 2 +- doc/ci/variables/predefined_variables.md | 2 +- .../system_header_and_footer_messages.md | 3 +- doc/install/installation.md | 2 +- doc/integration/elasticsearch.md | 2 +- doc/user/clusters/applications.md | 2 +- doc/user/markdown.md | 2 +- doc/user/permissions.md | 4 +- doc/user/project/canary_deployments.md | 3 +- .../prometheus_library/haproxy.md | 4 +- .../integrations/prometheus_library/nginx.md | 6 +-- .../prometheus_library/nginx_ingress.md | 6 +-- .../prometheus_library/nginx_ingress_vts.md | 6 +-- .../work_in_progress_merge_requests.md | 4 +- doc/workflow/gitlab_flow.md | 2 +- doc/workflow/notifications.md | 24 +++++----- 19 files changed, 64 insertions(+), 66 deletions(-) diff --git a/doc/administration/auth/ldap.md b/doc/administration/auth/ldap.md index 1f2961ea39a..79ac7fe0352 100644 --- a/doc/administration/auth/ldap.md +++ b/doc/administration/auth/ldap.md @@ -1,6 +1,4 @@ -[//]: # (Do *NOT* modify this file in EE documentation. All changes in this) -[//]: # (file should happen in CE, too. If the change is EE-specific, put) -[//]: # (it in `ldap-ee.md`.) + # LDAP diff --git a/doc/administration/geo/replication/updating_the_geo_nodes.md b/doc/administration/geo/replication/updating_the_geo_nodes.md index 348b0e9e7e5..d56a59f4967 100644 --- a/doc/administration/geo/replication/updating_the_geo_nodes.md +++ b/doc/administration/geo/replication/updating_the_geo_nodes.md @@ -30,7 +30,7 @@ We now require this change as we use this password to enable the Foreign Data Wr the Geo Tracking Database. We are also improving security by disabling the use of **trust** authentication method. -1. **[primary]** Login to your **primary** node and run: +1. **(primary)** Login to your **primary** node and run: ```sh gitlab-ctl pg-password-md5 gitlab @@ -57,14 +57,14 @@ authentication method. postgresql['trust_auth_cidr_addresses'] = ['127.0.0.1/32','1.2.3.4/32'] # <- Remove this ``` -1. **[primary]** Reconfigure and restart: +1. **(primary)** Reconfigure and restart: ```sh sudo gitlab-ctl reconfigure sudo gitlab-ctl restart ``` -1. **[secondary]** Login to all **secondary** nodes and edit `/etc/gitlab/gitlab.rb`: +1. **(secondary)** Login to all **secondary** nodes and edit `/etc/gitlab/gitlab.rb`: ```ruby # Fill with the hash generated by `gitlab-ctl pg-password-md5 gitlab` @@ -88,7 +88,7 @@ authentication method. postgresql['trust_auth_cidr_addresses'] = ['127.0.0.1/32','5.6.7.8/32'] # <- Remove this ``` -1. **[secondary]** Reconfigure and restart: +1. **(secondary)** Reconfigure and restart: ```sh sudo gitlab-ctl reconfigure @@ -169,7 +169,7 @@ After you've verified that HTTP/HTTPS replication is working, you should remove the now-unused SSH keys from your secondaries, as they may cause problems if the **secondary** node if ever promoted to a **primary** node: -1. **[secondary]** Login to **all** your **secondary** nodes and run: +1. **(secondary)** Login to **all** your **secondary** nodes and run: ```ruby sudo -u git -H rm ~git/.ssh/id_rsa ~git/.ssh/id_rsa.pub @@ -260,24 +260,24 @@ Make sure to follow the steps in the exact order as they appear below and pay extra attention in what node (either **primary** or **secondary**) you execute them! Each step is prepended with the relevant node for better clarity: -1. **[secondary]** Login to **all** your **secondary** nodes and stop all services: +1. **(secondary)** Login to **all** your **secondary** nodes and stop all services: ```ruby sudo gitlab-ctl stop ``` -1. **[secondary]** Make a backup of the `recovery.conf` file on **all** +1. **(secondary)** Make a backup of the `recovery.conf` file on **all** **secondary** nodes to preserve PostgreSQL's credentials: ```sh sudo cp /var/opt/gitlab/postgresql/data/recovery.conf /var/opt/gitlab/ ``` -1. **[primary]** Update the **primary** node to GitLab 9.0 following the +1. **(primary)** Update the **primary** node to GitLab 9.0 following the [regular update docs][update]. At the end of the update, the **primary** node will be running with PostgreSQL 9.6. -1. **[primary]** To prevent a de-synchronization of the repository replication, +1. **(primary)** To prevent a de-synchronization of the repository replication, stop all services except `postgresql` as we will use it to re-initialize the **secondary** node's database: @@ -286,55 +286,55 @@ is prepended with the relevant node for better clarity: sudo gitlab-ctl start postgresql ``` -1. **[secondary]** Run the following steps on each of the **secondary** nodes: +1. **(secondary)** Run the following steps on each of the **secondary** nodes: - 1. **[secondary]** Stop all services: + 1. **(secondary)** Stop all services: ```sh sudo gitlab-ctl stop ``` - 1. **[secondary]** Prevent running database migrations: + 1. **(secondary)** Prevent running database migrations: ```sh sudo touch /etc/gitlab/skip-auto-migrations ``` - 1. **[secondary]** Move the old database to another directory: + 1. **(secondary)** Move the old database to another directory: ```sh sudo mv /var/opt/gitlab/postgresql{,.bak} ``` - 1. **[secondary]** Update to GitLab 9.0 following the [regular update docs][update]. + 1. **(secondary)** Update to GitLab 9.0 following the [regular update docs][update]. At the end of the update, the node will be running with PostgreSQL 9.6. - 1. **[secondary]** Make sure all services are up: + 1. **(secondary)** Make sure all services are up: ```sh sudo gitlab-ctl start ``` - 1. **[secondary]** Reconfigure GitLab: + 1. **(secondary)** Reconfigure GitLab: ```sh sudo gitlab-ctl reconfigure ``` - 1. **[secondary]** Run the PostgreSQL upgrade command: + 1. **(secondary)** Run the PostgreSQL upgrade command: ```sh sudo gitlab-ctl pg-upgrade ``` - 1. **[secondary]** See the stored credentials for the database that you will + 1. **(secondary)** See the stored credentials for the database that you will need to re-initialize the replication: ```sh sudo grep -s primary_conninfo /var/opt/gitlab/recovery.conf ``` - 1. **[secondary]** Save the snippet below in a file, let's say `/tmp/replica.sh`. Modify the + 1. **(secondary)** Save the snippet below in a file, let's say `/tmp/replica.sh`. Modify the embedded paths if necessary: ``` @@ -381,28 +381,28 @@ is prepended with the relevant node for better clarity: sudo service postgresql start ``` - 1. **[secondary]** Run the recovery script using the credentials from the + 1. **(secondary)** Run the recovery script using the credentials from the previous step: ```sh sudo bash /tmp/replica.sh ``` - 1. **[secondary]** Reconfigure GitLab: + 1. **(secondary)** Reconfigure GitLab: ```sh sudo gitlab-ctl reconfigure ``` - 1. **[secondary]** Start all services: + 1. **(secondary)** Start all services: ```sh sudo gitlab-ctl start ``` - 1. **[secondary]** Repeat the steps for the remaining **secondary** nodes. + 1. **(secondary)** Repeat the steps for the remaining **secondary** nodes. -1. **[primary]** After all **secondary** nodes are updated, start all services in +1. **(primary)** After all **secondary** nodes are updated, start all services in **primary** node: ```sh diff --git a/doc/administration/high_availability/redis.md b/doc/administration/high_availability/redis.md index cc338725e1b..874525dd836 100644 --- a/doc/administration/high_availability/redis.md +++ b/doc/administration/high_availability/redis.md @@ -394,7 +394,7 @@ The prerequisites for a HA Redis setup are the following: 1. [Reconfigure Omnibus GitLab][reconfigure] for the changes to take effect. > Note: You can specify multiple roles like sentinel and redis as: -> roles ['redis_sentinel_role', 'redis_master_role']. Read more about high +> `roles ['redis_sentinel_role', 'redis_master_role']`. Read more about high > availability roles at . ### Step 2. Configuring the slave Redis instances @@ -443,7 +443,7 @@ The prerequisites for a HA Redis setup are the following: 1. Go through the steps again for all the other slave nodes. > Note: You can specify multiple roles like sentinel and redis as: -> roles ['redis_sentinel_role', 'redis_slave_role']. Read more about high +> `roles ['redis_sentinel_role', 'redis_slave_role']`. Read more about high > availability roles at . --- diff --git a/doc/administration/monitoring/prometheus/gitlab_metrics.md b/doc/administration/monitoring/prometheus/gitlab_metrics.md index 84b71ae6f1c..2d9e3f7f18b 100644 --- a/doc/administration/monitoring/prometheus/gitlab_metrics.md +++ b/doc/administration/monitoring/prometheus/gitlab_metrics.md @@ -108,7 +108,7 @@ Some basic Ruby runtime metrics are available: [GC.stat]: https://ruby-doc.org/core-2.3.0/GC.html#method-c-stat -## Puma Metrics **[EXPERIMENTAL]** +## Puma Metrics **(EXPERIMENTAL)** When Puma is used instead of Unicorn, following metrics are available: diff --git a/doc/ci/variables/predefined_variables.md b/doc/ci/variables/predefined_variables.md index 7dbb9af2869..e911e97d3c8 100644 --- a/doc/ci/variables/predefined_variables.md +++ b/doc/ci/variables/predefined_variables.md @@ -78,7 +78,7 @@ future GitLab releases.** | `CI_PIPELINE_ID` | 8.10 | all | The unique id of the current pipeline that GitLab CI uses internally | | `CI_PIPELINE_IID` | 11.0 | all | The unique id of the current pipeline scoped to project | | `CI_PIPELINE_SOURCE` | 10.0 | all | Indicates how the pipeline was triggered. Possible options are: `push`, `web`, `trigger`, `schedule`, `api`, and `pipeline`. For pipelines created before GitLab 9.5, this will show as `unknown` | -| `CI_PIPELINE_TRIGGERED` | all | all | The flag to indicate that job was [triggered] | +| `CI_PIPELINE_TRIGGERED` | all | all | The flag to indicate that job was [triggered](../triggers/README.md) | | `CI_PIPELINE_URL` | 11.1 | 0.5 | Pipeline details URL | | `CI_PROJECT_DIR` | all | all | The full path where the repository is cloned and where the job is run. If the GitLab Runner `builds_dir` parameter is set, this variable is set relative to the value of `builds_dir`. For more information, see [Advanced configuration](https://docs.gitlab.com/runner/configuration/advanced-configuration.html#the-runners-section) for GitLab Runner. | | `CI_PROJECT_ID` | all | all | The unique id of the current project that GitLab CI uses internally | diff --git a/doc/customization/system_header_and_footer_messages.md b/doc/customization/system_header_and_footer_messages.md index 9d6931c730d..7eee79abc77 100644 --- a/doc/customization/system_header_and_footer_messages.md +++ b/doc/customization/system_header_and_footer_messages.md @@ -1,6 +1,7 @@ # Adding a system message to every page -> [Introduced][ee-1283] in [GitLab Premium][eep] 10.7. +> [Introduced](https://gitlab.com/gitlab-org/gitlab-ee/issues/5023) in [GitLab Premium](https://about.gitlab.com/pricing/) 10.7. +> [Added](https://gitlab.com/gitlab-org/gitlab-ce/issues/55057) to [GitLab Core](https://about.gitlab.com/pricing/) in 11.9. Navigate to the **Admin** area and go to the **Appearance** page. diff --git a/doc/install/installation.md b/doc/install/installation.md index 4c1021d097f..70e5ab28931 100644 --- a/doc/install/installation.md +++ b/doc/install/installation.md @@ -560,7 +560,7 @@ NOTE: **Note:** If you want to use HTTPS, see [Using HTTPS](#using-https) for the additional steps. NOTE: **Note:** -Make sure your hostname can be resolved on the machine itself by either a proper DNS record or an additional line in `/etc/hosts` ("127.0.0.1 hostname"). This might be necessary, for example, if you set up GitLab behind a reverse proxy. If the hostname cannot be resolved, the final installation check will fail with "Check GitLab API access: FAILED. code: 401" and pushing commits will be rejected with "[remote rejected] master -> master (hook declined)". +Make sure your hostname can be resolved on the machine itself by either a proper DNS record or an additional line in `/etc/hosts` ("127.0.0.1 hostname"). This might be necessary, for example, if you set up GitLab behind a reverse proxy. If the hostname cannot be resolved, the final installation check will fail with `Check GitLab API access: FAILED. code: 401` and pushing commits will be rejected with `[remote rejected] master -> master (hook declined)`. NOTE: **Note:** GitLab Shell application startup time can be greatly reduced by disabling RubyGems. This can be done in several ways: diff --git a/doc/integration/elasticsearch.md b/doc/integration/elasticsearch.md index f64fdb1e28b..877330b8c44 100644 --- a/doc/integration/elasticsearch.md +++ b/doc/integration/elasticsearch.md @@ -167,7 +167,7 @@ To disable the Elasticsearch integration: 1. Find the 'Elasticsearch' section and uncheck 'Search with Elasticsearch enabled' and 'Elasticsearch indexing' 1. Click **Save** for the changes to take effect -1. [Optional] Delete the existing index by running the command `sudo gitlab-rake gitlab:elastic:delete_index` +1. (Optional) Delete the existing index by running the command `sudo gitlab-rake gitlab:elastic:delete_index` ## Adding GitLab's data to the Elasticsearch index diff --git a/doc/user/clusters/applications.md b/doc/user/clusters/applications.md index 469a7c09250..2246ea8ed5a 100644 --- a/doc/user/clusters/applications.md +++ b/doc/user/clusters/applications.md @@ -111,7 +111,7 @@ file. [Ingress](https://kubernetes.github.io/ingress-nginx/) can provide load balancing, SSL termination, and name-based virtual hosting. It acts as a web proxy for your applications and is useful if you want to use [Auto -DevOps] or deploy your own web apps. +DevOps](../../topics/autodevops/index.md) or deploy your own web apps. NOTE: **Note:** The diff --git a/doc/user/markdown.md b/doc/user/markdown.md index a08b41aaecb..eaae9964367 100644 --- a/doc/user/markdown.md +++ b/doc/user/markdown.md @@ -288,7 +288,7 @@ $example = array( > If this is not rendered correctly, [view it in GitLab itself](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/user/markdown.md#inline-diff). -With inline diff tags you can display {+ additions +} or [- deletions -]. +With inline diff tags you can display `{+ additions +}` or `[- deletions -]`. The wrapping tags can be either curly braces or square brackets: diff --git a/doc/user/permissions.md b/doc/user/permissions.md index 03abef9fc62..33fff0fed74 100644 --- a/doc/user/permissions.md +++ b/doc/user/permissions.md @@ -122,8 +122,8 @@ The following table depicts the various user permission levels in a project. | Transfer project to another namespace | | | | | ✓ | | Remove project | | | | | ✓ | | Delete issues | | | | | ✓ | -| Force push to protected branches [^4] | | | | | | -| Remove protected branches [^4] | | | | | | +| Force push to protected branches (*4*) | | | | | | +| Remove protected branches (*4*) | | | | | | - (*1*): All users are able to perform this action on public and internal projects, but not private projects. - (*2*): Guest users can only view the confidential issues they created themselves diff --git a/doc/user/project/canary_deployments.md b/doc/user/project/canary_deployments.md index cce862b0911..9bb282f1b78 100644 --- a/doc/user/project/canary_deployments.md +++ b/doc/user/project/canary_deployments.md @@ -44,7 +44,7 @@ Canary deployments require that you properly configure Deploy Boards: 1. Follow the steps to [enable Deploy Boards](deploy_boards.md#enabling-deploy-boards). 1. To track canary deployments you need to label your Kubernetes deployments and - pods with `track: canary`. To get started quickly, you can use the [Auto Deploy] + pods with `track: canary`. To get started quickly, you can use the [Auto Deploy](../../ci/autodeploy/index.md) template for canary deployments that GitLab provides. Depending on the deploy, the label should be either `stable` or `canary`. @@ -62,7 +62,6 @@ can easily notice them. ![Canary deployments on Deploy Board](img/deploy_boards_canary_deployments.png) -[autodeploy]: ../../ci/autodeploy/index.md "GitLab Autodeploy" [eep]: https://about.gitlab.com/pricing/ [ee-1659]: https://gitlab.com/gitlab-org/gitlab-ee/issues/1659 [kube-canary]: https://kubernetes.io/docs/concepts/cluster-administration/manage-deployment/#canary-deployments diff --git a/doc/user/project/integrations/prometheus_library/haproxy.md b/doc/user/project/integrations/prometheus_library/haproxy.md index abb0c01ad18..6be8fc82431 100644 --- a/doc/user/project/integrations/prometheus_library/haproxy.md +++ b/doc/user/project/integrations/prometheus_library/haproxy.md @@ -11,8 +11,8 @@ The [Prometheus service](../prometheus.md) must be enabled. | Name | Query | | ---- | ----- | -| Throughput (req/sec) | sum(rate(haproxy_frontend_http_requests_total{%{environment_filter}}[2m])) by (code) | -| HTTP Error Rate (%) | sum(rate(haproxy_frontend_http_requests_total{code="5xx",%{environment_filter}}[2m])) / sum(rate(haproxy_frontend_http_requests_total{%{environment_filter}}[2m])) | +| Throughput (req/sec) | `sum(rate(haproxy_frontend_http_requests_total{%{environment_filter}}[2m])) by (code)` | +| HTTP Error Rate (%) | `sum(rate(haproxy_frontend_http_requests_total{code="5xx",%{environment_filter}}[2m])) / sum(rate(haproxy_frontend_http_requests_total{%{environment_filter}}[2m]))` | ## Configuring Prometheus to monitor for HAProxy metrics diff --git a/doc/user/project/integrations/prometheus_library/nginx.md b/doc/user/project/integrations/prometheus_library/nginx.md index c4fea178ab5..7db9629c002 100644 --- a/doc/user/project/integrations/prometheus_library/nginx.md +++ b/doc/user/project/integrations/prometheus_library/nginx.md @@ -14,9 +14,9 @@ NGINX server metrics are detected, which tracks the pages and content directly s | Name | Query | | ---- | ----- | -| Throughput (req/sec) | sum(rate(nginx_server_requests{server_zone!="*", server_zone!="_", %{environment_filter}}[2m])) by (code) | -| Latency (ms) | avg(nginx_server_requestMsec{%{environment_filter}}) | -| HTTP Error Rate (HTTP Errors / sec) | sum(rate(nginx_server_requests{code="5xx", %{environment_filter}}[2m])) | +| Throughput (req/sec) | `sum(rate(nginx_server_requests{server_zone!="*", server_zone!="_", %{environment_filter}}[2m])) by (code)` | +| Latency (ms) | `avg(nginx_server_requestMsec{%{environment_filter}})` | +| HTTP Error Rate (HTTP Errors / sec) | `sum(rate(nginx_server_requests{code="5xx", %{environment_filter}}[2m]))` | ## Configuring Prometheus to monitor for NGINX metrics diff --git a/doc/user/project/integrations/prometheus_library/nginx_ingress.md b/doc/user/project/integrations/prometheus_library/nginx_ingress.md index de7fc93f0a4..fd743855a8c 100644 --- a/doc/user/project/integrations/prometheus_library/nginx_ingress.md +++ b/doc/user/project/integrations/prometheus_library/nginx_ingress.md @@ -14,9 +14,9 @@ GitLab has support for automatically detecting and monitoring the Kubernetes NGI | Name | Query | | ---- | ----- | -| Throughput (req/sec) | sum(label_replace(rate(nginx_ingress_controller_requests{namespace="%{kube_namespace}",ingress=~".*%{ci_environment_slug}.*"}[2m]), "status_code", "${1}xx", "status", "(.)..")) by (status_code) | -| Latency (ms) | sum(rate(nginx_ingress_controller_ingress_upstream_latency_seconds_sum{namespace="%{kube_namespace}",ingress=~".*%{ci_environment_slug}.*"}[2m])) / sum(rate(nginx_ingress_controller_ingress_upstream_latency_seconds_count{namespace="%{kube_namespace}",ingress=~".*%{ci_environment_slug}.*"}[2m])) * 1000 | -| HTTP Error Rate (%) | sum(rate(nginx_ingress_controller_requests{status=~"5.*",namespace="%{kube_namespace}",ingress=~".*%{ci_environment_slug}.*"}[2m])) / sum(rate(nginx_ingress_controller_requests{namespace="%{kube_namespace}",ingress=~".*%{ci_environment_slug}.*"}[2m])) * 100 | +| Throughput (req/sec) | `sum(label_replace(rate(nginx_ingress_controller_requests{namespace="%{kube_namespace}",ingress=~".*%{ci_environment_slug}.*"}[2m]), "status_code", "${1}xx", "status", "(.)..")) by (status_code)` | +| Latency (ms) | `sum(rate(nginx_ingress_controller_ingress_upstream_latency_seconds_sum{namespace="%{kube_namespace}",ingress=~".*%{ci_environment_slug}.*"}[2m])) / sum(rate(nginx_ingress_controller_ingress_upstream_latency_seconds_count{namespace="%{kube_namespace}",ingress=~".*%{ci_environment_slug}.*"}[2m])) * 1000` | +| HTTP Error Rate (%) | `sum(rate(nginx_ingress_controller_requests{status=~"5.*",namespace="%{kube_namespace}",ingress=~".*%{ci_environment_slug}.*"}[2m])) / sum(rate(nginx_ingress_controller_requests{namespace="%{kube_namespace}",ingress=~".*%{ci_environment_slug}.*"}[2m])) * 100` | ## Configuring NGINX ingress monitoring diff --git a/doc/user/project/integrations/prometheus_library/nginx_ingress_vts.md b/doc/user/project/integrations/prometheus_library/nginx_ingress_vts.md index 31ac53c0d14..b03787b3e0d 100644 --- a/doc/user/project/integrations/prometheus_library/nginx_ingress_vts.md +++ b/doc/user/project/integrations/prometheus_library/nginx_ingress_vts.md @@ -14,9 +14,9 @@ GitLab has support for automatically detecting and monitoring the Kubernetes NGI | Name | Query | | ---- | ----- | -| Throughput (req/sec) | sum(rate(nginx_upstream_responses_total{upstream=~"%{kube_namespace}-%{ci_environment_slug}-.*"}[2m])) by (status_code) | -| Latency (ms) | avg(nginx_upstream_response_msecs_avg{upstream=~"%{kube_namespace}-%{ci_environment_slug}-.*"}) | -| HTTP Error Rate (%) | sum(rate(nginx_upstream_responses_total{status_code="5xx", upstream=~"%{kube_namespace}-%{ci_environment_slug}-.*"}[2m])) / sum(rate(nginx_upstream_responses_total{upstream=~"%{kube_namespace}-%{ci_environment_slug}-.*"}[2m])) * 100 | +| Throughput (req/sec) | `sum(rate(nginx_upstream_responses_total{upstream=~"%{kube_namespace}-%{ci_environment_slug}-.*"}[2m])) by (status_code)` | +| Latency (ms) | `avg(nginx_upstream_response_msecs_avg{upstream=~"%{kube_namespace}-%{ci_environment_slug}-.*"})` | +| HTTP Error Rate (%) | `sum(rate(nginx_upstream_responses_total{status_code="5xx", upstream=~"%{kube_namespace}-%{ci_environment_slug}-.*"}[2m])) / sum(rate(nginx_upstream_responses_total{upstream=~"%{kube_namespace}-%{ci_environment_slug}-.*"}[2m])) * 100` | ## Configuring NGINX ingress monitoring diff --git a/doc/user/project/merge_requests/work_in_progress_merge_requests.md b/doc/user/project/merge_requests/work_in_progress_merge_requests.md index 9cd868067bc..142178ba241 100644 --- a/doc/user/project/merge_requests/work_in_progress_merge_requests.md +++ b/doc/user/project/merge_requests/work_in_progress_merge_requests.md @@ -15,7 +15,7 @@ being merged, and it will stay disabled until the "WIP" flag has been removed. There are several ways to flag a merge request as a Work In Progress: -- Add "[WIP]" or "WIP:" to the start of the merge request's title. Clicking on +- Add `[WIP]` or `WIP:` to the start of the merge request's title. Clicking on **Start the title with WIP:**, under the title box, when editing the merge request's description will have the same effect. - Add the `/wip` [quick action](../quick_actions.md#quick-actions-for-issues-and-merge-requests) @@ -30,7 +30,7 @@ There are several ways to flag a merge request as a Work In Progress: Similar to above, when a Merge Request is ready to be merged, you can remove the "Work in Progress" flag in several ways: -- Remove "[WIP]" or "WIP:" from the start of the merge request's title. Clicking on +- Remove `[WIP]` or `WIP:` from the start of the merge request's title. Clicking on **Remove the WIP: prefix from the title**, under the title box, when editing the merge request's description, will have the same effect. - Add the `/wip` [quick action](../quick_actions.md#quick-actions-for-issues-and-merge-requests) diff --git a/doc/workflow/gitlab_flow.md b/doc/workflow/gitlab_flow.md index 3e24557591c..0cb390e1242 100644 --- a/doc/workflow/gitlab_flow.md +++ b/doc/workflow/gitlab_flow.md @@ -175,7 +175,7 @@ A merge request is an online place to discuss the change and review the code. If you open the merge request but do not assign it to anyone, it is a "Work In Progress" merge request. These are used to discuss the proposed implementation but are not ready for inclusion in the `master` branch yet. -Start the title of the merge request with "[WIP]" or "WIP:" to prevent it from being merged before it's ready. +Start the title of the merge request with `[WIP]` or `WIP:` to prevent it from being merged before it's ready. When you think the code is ready, assign the merge request to a reviewer. The reviewer can merge the changes when they think the code is ready for inclusion in the `master` branch. diff --git a/doc/workflow/notifications.md b/doc/workflow/notifications.md index 5d560f2e000..d49d29c805a 100644 --- a/doc/workflow/notifications.md +++ b/doc/workflow/notifications.md @@ -64,18 +64,18 @@ These settings can be configured on project page under the name of the project. Below is the table of events users can be notified of: -| Event | Sent to | Settings level | -|------------------------------|-------------------------------------------------------------------|------------------------------| -| New SSH key added | User | Security email, always sent. | -| New email added | User | Security email, always sent. | -| Email changed | User | Security email, always sent. | -| Password changed | User | Security email, always sent. | -| New user created | User | Sent on user creation, except for omniauth (LDAP)| -| User added to project | User | Sent when user is added to project | -| Project access level changed | User | Sent when user project access level is changed | -| User added to group | User | Sent when user is added to group | -| Group access level changed | User | Sent when user group access level is changed | -| Project moved | Project members [1] | [1] not disabled | +| Event | Sent to | Settings level | +|------------------------------|---------------------|------------------------------| +| New SSH key added | User | Security email, always sent. | +| New email added | User | Security email, always sent. | +| Email changed | User | Security email, always sent. | +| Password changed | User | Security email, always sent. | +| New user created | User | Sent on user creation, except for omniauth (LDAP)| +| User added to project | User | Sent when user is added to project | +| Project access level changed | User | Sent when user project access level is changed | +| User added to group | User | Sent when user is added to group | +| Group access level changed | User | Sent when user group access level is changed | +| Project moved | Project members (1) | (1) not disabled | ### Issue / Epics / Merge request events From ac14fd3d84b1fb12612dc7c8c2324c2bd1ec571c Mon Sep 17 00:00:00 2001 From: John Skarbek Date: Wed, 3 Jul 2019 08:17:42 +0000 Subject: [PATCH 37/40] Expound backporting a tad bit This bolsters the information in this document, based on questions recently raised in issue: https://gitlab.com/gitlab-org/release/framework/issues/378 --- doc/development/ee_features.md | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/doc/development/ee_features.md b/doc/development/ee_features.md index 34d41cf4958..6f4a36d4066 100644 --- a/doc/development/ee_features.md +++ b/doc/development/ee_features.md @@ -1036,7 +1036,14 @@ to avoid conflicts during CE to EE merge. ### Backporting changes from EE to CE -When working in EE-specific features, you might have to tweak a few files that are not EE-specific. Here is a workflow to make sure those changes end up backported safely into CE too. +Until the work completed to merge the ce and ee codebases, which is tracked on [epic &802](https://gitlab.com/groups/gitlab-org/-/epics/802), there exists times in which some changes for EE require specific changes to the CE +code base. Examples of backports include the following: + +* Features intended or originally built for EE that are later decided to move to CE +* Sometimes some code in CE may impact the EE feature + +Here is a workflow to make sure those changes end up backported safely into CE too. + (This approach does not refer to changes introduced via [csslab](https://gitlab.com/gitlab-org/csslab/).) 1. **Make your changes in the EE branch.** If possible, keep a separated commit (to be squashed) to help backporting and review. From 0d32d31864eff4443c538f2ead8af5bc133eec7d Mon Sep 17 00:00:00 2001 From: Luke Ward Date: Wed, 3 Jul 2019 08:20:57 +0000 Subject: [PATCH 38/40] Replace slugifyWithHyphens with improved slugify function --- app/assets/javascripts/group.js | 4 ++-- .../javascripts/lib/utils/text_utility.js | 11 +++++++++-- .../javascripts/projects/project_new.js | 4 ++-- changelogs/unreleased/slugify.yml | 5 +++++ spec/frontend/lib/utils/text_utility_spec.js | 19 +++++++++++++++++-- 5 files changed, 35 insertions(+), 8 deletions(-) create mode 100644 changelogs/unreleased/slugify.yml diff --git a/app/assets/javascripts/group.js b/app/assets/javascripts/group.js index 903c838e266..460174caf4d 100644 --- a/app/assets/javascripts/group.js +++ b/app/assets/javascripts/group.js @@ -1,5 +1,5 @@ import $ from 'jquery'; -import { slugifyWithHyphens } from './lib/utils/text_utility'; +import { slugify } from './lib/utils/text_utility'; export default class Group { constructor() { @@ -14,7 +14,7 @@ export default class Group { } update() { - const slug = slugifyWithHyphens(this.groupName.val()); + const slug = slugify(this.groupName.val()); this.groupPath.val(slug); } diff --git a/app/assets/javascripts/lib/utils/text_utility.js b/app/assets/javascripts/lib/utils/text_utility.js index cc1d85fd97d..d38f59b5861 100644 --- a/app/assets/javascripts/lib/utils/text_utility.js +++ b/app/assets/javascripts/lib/utils/text_utility.js @@ -44,11 +44,18 @@ export const pluralize = (str, count) => str + (count > 1 || count === 0 ? 's' : export const dasherize = str => str.replace(/[_\s]+/g, '-'); /** - * Replaces whitespaces with hyphens and converts to lower case + * Replaces whitespaces with hyphens, convert to lower case and remove non-allowed special characters * @param {String} str * @returns {String} */ -export const slugifyWithHyphens = str => str.toLowerCase().replace(/\s+/g, '-'); +export const slugify = str => { + const slug = str + .trim() + .toLowerCase() + .replace(/[^a-zA-Z0-9_.-]+/g, '-'); + + return slug === '-' ? '' : slug; +}; /** * Replaces whitespaces with underscore and converts to lower case diff --git a/app/assets/javascripts/projects/project_new.js b/app/assets/javascripts/projects/project_new.js index ea82ff4e340..9066844f687 100644 --- a/app/assets/javascripts/projects/project_new.js +++ b/app/assets/javascripts/projects/project_new.js @@ -1,6 +1,6 @@ import $ from 'jquery'; import { addSelectOnFocusBehaviour } from '../lib/utils/common_utils'; -import { slugifyWithHyphens } from '../lib/utils/text_utility'; +import { slugify } from '../lib/utils/text_utility'; import { s__ } from '~/locale'; let hasUserDefinedProjectPath = false; @@ -34,7 +34,7 @@ const deriveProjectPathFromUrl = $projectImportUrl => { }; const onProjectNameChange = ($projectNameInput, $projectPathInput) => { - const slug = slugifyWithHyphens($projectNameInput.val()); + const slug = slugify($projectNameInput.val()); $projectPathInput.val(slug); }; diff --git a/changelogs/unreleased/slugify.yml b/changelogs/unreleased/slugify.yml new file mode 100644 index 00000000000..853e90b8bed --- /dev/null +++ b/changelogs/unreleased/slugify.yml @@ -0,0 +1,5 @@ +--- +title: Replace slugifyWithHyphens with improved slugify function +merge_request: 30172 +author: Luke Ward +type: fixed diff --git a/spec/frontend/lib/utils/text_utility_spec.js b/spec/frontend/lib/utils/text_utility_spec.js index 9e920d59093..dc886d0db3b 100644 --- a/spec/frontend/lib/utils/text_utility_spec.js +++ b/spec/frontend/lib/utils/text_utility_spec.js @@ -55,9 +55,24 @@ describe('text_utility', () => { }); }); - describe('slugifyWithHyphens', () => { + describe('slugify', () => { + it('should remove accents and convert to lower case', () => { + expect(textUtils.slugify('João')).toEqual('jo-o'); + }); it('should replaces whitespaces with hyphens and convert to lower case', () => { - expect(textUtils.slugifyWithHyphens('My Input String')).toEqual('my-input-string'); + expect(textUtils.slugify('My Input String')).toEqual('my-input-string'); + }); + it('should remove trailing whitespace and replace whitespaces within string with a hyphen', () => { + expect(textUtils.slugify(' a new project ')).toEqual('a-new-project'); + }); + it('should only remove non-allowed special characters', () => { + expect(textUtils.slugify('test!_pro-ject~')).toEqual('test-_pro-ject-'); + }); + it('should squash multiple hypens', () => { + expect(textUtils.slugify('test!!!!_pro-ject~')).toEqual('test-_pro-ject-'); + }); + it('should return empty string if only non-allowed characters', () => { + expect(textUtils.slugify('здрасти')).toEqual(''); }); }); From d4151b14c2986db173a7a1a4d293b86bfcdaae3a Mon Sep 17 00:00:00 2001 From: Natalia Tepluhina Date: Wed, 3 Jul 2019 08:26:57 +0000 Subject: [PATCH 39/40] Rebased and squashed commits - all commits squashed to make danger review happy --- .../components/diff_discussion_reply.vue | 55 ++ .../diffs/components/diff_discussions.vue | 1 - .../diffs/components/diff_file_header.vue | 8 +- .../diffs/components/diff_gutter_avatars.vue | 27 +- .../components/diff_line_gutter_content.vue | 17 +- .../components/inline_diff_comment_row.vue | 40 +- .../components/parallel_diff_comment_row.vue | 85 ++- app/assets/javascripts/diffs/store/actions.js | 32 ++ .../javascripts/diffs/store/mutation_types.js | 2 + .../javascripts/diffs/store/mutations.js | 65 ++- app/assets/javascripts/diffs/store/utils.js | 45 ++ .../notes/components/discussion_actions.vue | 26 +- .../notes/components/discussion_notes.vue | 51 +- .../discussion_reply_placeholder.vue | 8 +- .../notes/components/noteable_discussion.vue | 10 +- .../components/notes/placeholder_note.vue | 2 +- app/assets/stylesheets/pages/diff.scss | 16 + app/assets/stylesheets/pages/notes.scss | 29 + .../unreleased/32452-multiple-discussions.yml | 5 + .../62124-new-threaded-discussion-design.yml | 5 + locale/gitlab.pot | 3 + ...single_discussion_in_merge_request_spec.rb | 2 +- ...diff_notes_and_discussions_resolve_spec.rb | 4 +- .../components/diff_discussion_reply_spec.js | 90 +++ .../components/diff_gutter_avatars_spec.js | 113 ++++ .../diffs/mock_data/diff_discussions.js | 529 ++++++++++++++++++ .../notes/components/discussion_notes_spec.js | 2 +- .../discussion_reply_placeholder_spec.js | 14 +- .../diffs/components/diff_file_header_spec.js | 6 +- .../components/diff_gutter_avatars_spec.js | 146 ----- .../diffs/components/inline_diff_view_spec.js | 3 +- spec/javascripts/diffs/store/actions_spec.js | 1 + .../components/noteable_discussion_spec.js | 6 + .../discussion_comments_shared_example.rb | 2 +- 34 files changed, 1162 insertions(+), 288 deletions(-) create mode 100644 app/assets/javascripts/diffs/components/diff_discussion_reply.vue create mode 100644 changelogs/unreleased/32452-multiple-discussions.yml create mode 100644 changelogs/unreleased/62124-new-threaded-discussion-design.yml create mode 100644 spec/frontend/diffs/components/diff_discussion_reply_spec.js create mode 100644 spec/frontend/diffs/components/diff_gutter_avatars_spec.js create mode 100644 spec/frontend/diffs/mock_data/diff_discussions.js delete mode 100644 spec/javascripts/diffs/components/diff_gutter_avatars_spec.js diff --git a/app/assets/javascripts/diffs/components/diff_discussion_reply.vue b/app/assets/javascripts/diffs/components/diff_discussion_reply.vue new file mode 100644 index 00000000000..2aa5e9b3339 --- /dev/null +++ b/app/assets/javascripts/diffs/components/diff_discussion_reply.vue @@ -0,0 +1,55 @@ + + + diff --git a/app/assets/javascripts/diffs/components/diff_discussions.vue b/app/assets/javascripts/diffs/components/diff_discussions.vue index 4c73eea4049..b0460bacff2 100644 --- a/app/assets/javascripts/diffs/components/diff_discussions.vue +++ b/app/assets/javascripts/diffs/components/diff_discussions.vue @@ -80,7 +80,6 @@ export default { v-show="isExpanded(discussion)" :discussion="discussion" :render-diff-file="false" - :always-expanded="true" :discussions-by-diff-order="true" :line="line" :help-page-path="helpPagePath" diff --git a/app/assets/javascripts/diffs/components/diff_file_header.vue b/app/assets/javascripts/diffs/components/diff_file_header.vue index eb9f1465945..4b226e30699 100644 --- a/app/assets/javascripts/diffs/components/diff_file_header.vue +++ b/app/assets/javascripts/diffs/components/diff_file_header.vue @@ -151,7 +151,11 @@ export default { stickyMonitor(this.$refs.header, contentTop() - fileHeaderHeight - 1, false); }, methods: { - ...mapActions('diffs', ['toggleFileDiscussions', 'toggleFullDiff']), + ...mapActions('diffs', [ + 'toggleFileDiscussions', + 'toggleFileDiscussionWrappers', + 'toggleFullDiff', + ]), handleToggleFile(e, checkTarget) { if ( !checkTarget || @@ -165,7 +169,7 @@ export default { this.$emit('showForkMessage'); }, handleToggleDiscussions() { - this.toggleFileDiscussions(this.diffFile); + this.toggleFileDiscussionWrappers(this.diffFile); }, handleFileNameClick(e) { const isLinkToOtherPage = diff --git a/app/assets/javascripts/diffs/components/diff_gutter_avatars.vue b/app/assets/javascripts/diffs/components/diff_gutter_avatars.vue index e28909b7be3..af5550aec3b 100644 --- a/app/assets/javascripts/diffs/components/diff_gutter_avatars.vue +++ b/app/assets/javascripts/diffs/components/diff_gutter_avatars.vue @@ -1,5 +1,4 @@ @@ -76,7 +65,7 @@ export default { type="button" :aria-label="__('Show comments')" class="diff-notes-collapse js-diff-comment-avatar js-diff-comment-button" - @click="toggleDiscussions" + @click="$emit('toggleLineDiscussions')" > @@ -87,7 +76,7 @@ export default { :img-src="note.author.avatar_url" :tooltip-text="getTooltipText(note)" class="diff-comment-avatar js-diff-comment-avatar" - @click.native="toggleDiscussions" + @click.native="$emit('toggleLineDiscussions')" /> +{{ moreCount }} diff --git a/app/assets/javascripts/diffs/components/diff_line_gutter_content.vue b/app/assets/javascripts/diffs/components/diff_line_gutter_content.vue index 1281f9b17ef..351110f0a87 100644 --- a/app/assets/javascripts/diffs/components/diff_line_gutter_content.vue +++ b/app/assets/javascripts/diffs/components/diff_line_gutter_content.vue @@ -105,7 +105,13 @@ export default { }, }, methods: { - ...mapActions('diffs', ['loadMoreLines', 'showCommentForm', 'setHighlightedRow']), + ...mapActions('diffs', [ + 'loadMoreLines', + 'showCommentForm', + 'setHighlightedRow', + 'toggleLineDiscussions', + 'toggleLineDiscussionWrappers', + ]), handleCommentButton() { this.showCommentForm({ lineCode: this.line.line_code, fileHash: this.fileHash }); }, @@ -184,7 +190,14 @@ export default { @click="setHighlightedRow(lineCode)" > - +
    diff --git a/app/assets/javascripts/diffs/components/inline_diff_comment_row.vue b/app/assets/javascripts/diffs/components/inline_diff_comment_row.vue index 1faa0493e79..ca3285e9afd 100644 --- a/app/assets/javascripts/diffs/components/inline_diff_comment_row.vue +++ b/app/assets/javascripts/diffs/components/inline_diff_comment_row.vue @@ -1,11 +1,14 @@ @@ -49,13 +54,22 @@ export default { :discussions="line.discussions" :help-page-path="helpPagePath" /> - + + + diff --git a/app/assets/javascripts/diffs/components/parallel_diff_comment_row.vue b/app/assets/javascripts/diffs/components/parallel_diff_comment_row.vue index d2e54edca85..c00b0e010ff 100644 --- a/app/assets/javascripts/diffs/components/parallel_diff_comment_row.vue +++ b/app/assets/javascripts/diffs/components/parallel_diff_comment_row.vue @@ -1,11 +1,14 @@ @@ -90,37 +115,49 @@ export default {
    - + + +
    - + + + diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js index 79ce49012f0..32e0d8f42ee 100644 --- a/app/assets/javascripts/diffs/store/actions.js +++ b/app/assets/javascripts/diffs/store/actions.js @@ -12,6 +12,7 @@ import { getNoteFormData, convertExpandLines, idleCallback, + allDiscussionWrappersExpanded, } from './utils'; import * as types from './mutation_types'; import { @@ -79,6 +80,7 @@ export const assignDiscussionsToDiff = ( discussions = rootState.notes.discussions, ) => { const diffPositionByLineCode = getDiffPositionByLineCode(state.diffFiles); + const hash = getLocationHash(); discussions .filter(discussion => discussion.diff_discussion) @@ -86,6 +88,7 @@ export const assignDiscussionsToDiff = ( commit(types.SET_LINE_DISCUSSIONS_FOR_FILE, { discussion, diffPositionByLineCode, + hash, }); }); @@ -99,6 +102,10 @@ export const removeDiscussionsFromDiff = ({ commit }, removeDiscussion) => { commit(types.REMOVE_LINE_DISCUSSIONS_FOR_FILE, { fileHash: file_hash, lineCode: line_code, id }); }; +export const toggleLineDiscussions = ({ commit }, options) => { + commit(types.TOGGLE_LINE_DISCUSSIONS, options); +}; + export const renderFileForDiscussionId = ({ commit, rootState, state }, discussionId) => { const discussion = rootState.notes.discussions.find(d => d.id === discussionId); @@ -257,6 +264,31 @@ export const toggleFileDiscussions = ({ getters, dispatch }, diff) => { }); }; +export const toggleFileDiscussionWrappers = ({ commit }, diff) => { + const discussionWrappersExpanded = allDiscussionWrappersExpanded(diff); + let linesWithDiscussions; + if (diff.highlighted_diff_lines) { + linesWithDiscussions = diff.highlighted_diff_lines.filter(line => line.discussions.length); + } + if (diff.parallel_diff_lines) { + linesWithDiscussions = diff.parallel_diff_lines.filter( + line => + (line.left && line.left.discussions.length) || + (line.right && line.right.discussions.length), + ); + } + + if (linesWithDiscussions.length) { + linesWithDiscussions.forEach(line => { + commit(types.TOGGLE_LINE_DISCUSSIONS, { + fileHash: diff.file_hash, + lineCode: line.line_code, + expanded: !discussionWrappersExpanded, + }); + }); + } +}; + export const saveDiffDiscussion = ({ state, dispatch }, { note, formData }) => { const postData = getNoteFormData({ commit: state.commit, diff --git a/app/assets/javascripts/diffs/store/mutation_types.js b/app/assets/javascripts/diffs/store/mutation_types.js index 8d6111da500..9db56331faa 100644 --- a/app/assets/javascripts/diffs/store/mutation_types.js +++ b/app/assets/javascripts/diffs/store/mutation_types.js @@ -35,3 +35,5 @@ export const ADD_CURRENT_VIEW_DIFF_FILE_LINES = 'ADD_CURRENT_VIEW_DIFF_FILE_LINE export const TOGGLE_DIFF_FILE_RENDERING_MORE = 'TOGGLE_DIFF_FILE_RENDERING_MORE'; export const SET_SHOW_SUGGEST_POPOVER = 'SET_SHOW_SUGGEST_POPOVER'; + +export const TOGGLE_LINE_DISCUSSIONS = 'TOGGLE_LINE_DISCUSSIONS'; diff --git a/app/assets/javascripts/diffs/store/mutations.js b/app/assets/javascripts/diffs/store/mutations.js index 00181a63c43..a66f205bbbd 100644 --- a/app/assets/javascripts/diffs/store/mutations.js +++ b/app/assets/javascripts/diffs/store/mutations.js @@ -6,6 +6,7 @@ import { addContextLines, prepareDiffData, isDiscussionApplicableToLine, + updateLineInFile, } from './utils'; import * as types from './mutation_types'; @@ -109,7 +110,7 @@ export default { })); }, - [types.SET_LINE_DISCUSSIONS_FOR_FILE](state, { discussion, diffPositionByLineCode }) { + [types.SET_LINE_DISCUSSIONS_FOR_FILE](state, { discussion, diffPositionByLineCode, hash }) { const { latestDiff } = state; const discussionLineCode = discussion.line_code; @@ -130,13 +131,27 @@ export default { : [], }); + const setDiscussionsExpanded = line => { + const isLineNoteTargeted = line.discussions.some( + disc => disc.notes && disc.notes.find(note => hash === `note_${note.id}`), + ); + + return { + ...line, + discussionsExpanded: + line.discussions && line.discussions.length + ? line.discussions.some(disc => !disc.resolved) || isLineNoteTargeted + : false, + }; + }; + state.diffFiles = state.diffFiles.map(diffFile => { if (diffFile.file_hash === fileHash) { const file = { ...diffFile }; if (file.highlighted_diff_lines) { file.highlighted_diff_lines = file.highlighted_diff_lines.map(line => - lineCheck(line) ? mapDiscussions(line) : line, + setDiscussionsExpanded(lineCheck(line) ? mapDiscussions(line) : line), ); } @@ -148,8 +163,10 @@ export default { if (left || right) { return { ...line, - left: line.left ? mapDiscussions(line.left) : null, - right: line.right ? mapDiscussions(line.right, () => !left) : null, + left: line.left ? setDiscussionsExpanded(mapDiscussions(line.left)) : null, + right: line.right + ? setDiscussionsExpanded(mapDiscussions(line.right, () => !left)) + : null, }; } @@ -173,32 +190,11 @@ export default { [types.REMOVE_LINE_DISCUSSIONS_FOR_FILE](state, { fileHash, lineCode }) { const selectedFile = state.diffFiles.find(f => f.file_hash === fileHash); if (selectedFile) { - if (selectedFile.parallel_diff_lines) { - const targetLine = selectedFile.parallel_diff_lines.find( - line => - (line.left && line.left.line_code === lineCode) || - (line.right && line.right.line_code === lineCode), - ); - if (targetLine) { - const side = targetLine.left && targetLine.left.line_code === lineCode ? 'left' : 'right'; - - Object.assign(targetLine[side], { - discussions: targetLine[side].discussions.filter(discussion => discussion.notes.length), - }); - } - } - - if (selectedFile.highlighted_diff_lines) { - const targetInlineLine = selectedFile.highlighted_diff_lines.find( - line => line.line_code === lineCode, - ); - - if (targetInlineLine) { - Object.assign(targetInlineLine, { - discussions: targetInlineLine.discussions.filter(discussion => discussion.notes.length), - }); - } - } + updateLineInFile(selectedFile, lineCode, line => + Object.assign(line, { + discussions: line.discussions.filter(discussion => discussion.notes.length), + }), + ); if (selectedFile.discussions && selectedFile.discussions.length) { selectedFile.discussions = selectedFile.discussions.filter( @@ -207,6 +203,15 @@ export default { } } }, + + [types.TOGGLE_LINE_DISCUSSIONS](state, { fileHash, lineCode, expanded }) { + const selectedFile = state.diffFiles.find(f => f.file_hash === fileHash); + + updateLineInFile(selectedFile, lineCode, line => + Object.assign(line, { discussionsExpanded: expanded }), + ); + }, + [types.TOGGLE_FOLDER_OPEN](state, path) { state.treeEntries[path].opened = !state.treeEntries[path].opened; }, diff --git a/app/assets/javascripts/diffs/store/utils.js b/app/assets/javascripts/diffs/store/utils.js index 71956255eef..1c3ed84001c 100644 --- a/app/assets/javascripts/diffs/store/utils.js +++ b/app/assets/javascripts/diffs/store/utils.js @@ -454,3 +454,48 @@ export const convertExpandLines = ({ }; export const idleCallback = cb => requestIdleCallback(cb); + +export const updateLineInFile = (selectedFile, lineCode, updateFn) => { + if (selectedFile.parallel_diff_lines) { + const targetLine = selectedFile.parallel_diff_lines.find( + line => + (line.left && line.left.line_code === lineCode) || + (line.right && line.right.line_code === lineCode), + ); + if (targetLine) { + const side = targetLine.left && targetLine.left.line_code === lineCode ? 'left' : 'right'; + + updateFn(targetLine[side]); + } + } + if (selectedFile.highlighted_diff_lines) { + const targetInlineLine = selectedFile.highlighted_diff_lines.find( + line => line.line_code === lineCode, + ); + + if (targetInlineLine) { + updateFn(targetInlineLine); + } + } +}; + +export const allDiscussionWrappersExpanded = diff => { + const discussionsExpandedArray = []; + if (diff.parallel_diff_lines) { + diff.parallel_diff_lines.forEach(line => { + if (line.left && line.left.discussions.length) { + discussionsExpandedArray.push(line.left.discussionsExpanded); + } + if (line.right && line.right.discussions.length) { + discussionsExpandedArray.push(line.right.discussionsExpanded); + } + }); + } else if (diff.highlighted_diff_lines) { + diff.parallel_diff_lines.forEach(line => { + if (line.discussions.length) { + discussionsExpandedArray.push(line.discussionsExpanded); + } + }); + } + return discussionsExpandedArray.every(el => el); +}; diff --git a/app/assets/javascripts/notes/components/discussion_actions.vue b/app/assets/javascripts/notes/components/discussion_actions.vue index 1357a5268d6..f4570c1292c 100644 --- a/app/assets/javascripts/notes/components/discussion_actions.vue +++ b/app/assets/javascripts/notes/components/discussion_actions.vue @@ -39,15 +39,23 @@ export default { - diff --git a/app/assets/javascripts/notes/components/discussion_reply_placeholder.vue b/app/assets/javascripts/notes/components/discussion_reply_placeholder.vue index ea590905e3c..0204169214b 100644 --- a/app/assets/javascripts/notes/components/discussion_reply_placeholder.vue +++ b/app/assets/javascripts/notes/components/discussion_reply_placeholder.vue @@ -1,6 +1,12 @@ @@ -12,6 +18,6 @@ export default { :title="s__('MergeRequests|Add a reply')" @click="$emit('onClick')" > - {{ s__('MergeRequests|Reply...') }} + {{ buttonText }} diff --git a/app/assets/javascripts/notes/components/noteable_discussion.vue b/app/assets/javascripts/notes/components/noteable_discussion.vue index b8eaff32cce..f6b5fffde29 100644 --- a/app/assets/javascripts/notes/components/noteable_discussion.vue +++ b/app/assets/javascripts/notes/components/noteable_discussion.vue @@ -132,7 +132,7 @@ export default { return this.discussion.diff_discussion && this.renderDiffFile; }, shouldGroupReplies() { - return !this.shouldRenderDiffs && !this.discussion.diff_discussion; + return !this.shouldRenderDiffs; }, wrapperComponent() { return this.shouldRenderDiffs ? diffWithNote : 'div'; @@ -250,6 +250,11 @@ export default { clearDraft(this.autosaveKey); }, saveReply(noteText, form, callback) { + if (!noteText) { + this.cancelReplyForm(); + callback(); + return; + } const postData = { in_reply_to_discussion_id: this.discussion.reply_id, target_type: this.getNoteableData.targetType, @@ -363,7 +368,6 @@ Please check your network connection and try again.`; :line="line" :should-group-replies="shouldGroupReplies" @startReplying="showReplyForm" - @toggleDiscussion="toggleDiscussionHandler" @deleteNote="deleteNoteHandler" > @@ -376,7 +380,7 @@ Please check your network connection and try again.`;