From 07a65da1d96a71474f6997aed95bac6290d81a42 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Fri, 16 Jun 2017 22:15:40 +0800 Subject: Generate KUBECONFIG in KubernetesService#predefined_variables --- app/models/project_services/kubernetes_service.rb | 14 +++++++- lib/gitlab/kubernetes.rb | 39 +++++++++++++++++++++ spec/fixtures/config/kubeconfig-without-ca.yml | 18 ++++++++++ spec/fixtures/config/kubeconfig.yml | 19 ++++++++++ spec/lib/gitlab/kubernetes_spec.rb | 24 +++++++++++++ .../project_services/kubernetes_service_spec.rb | 40 ++++++++++++++-------- 6 files changed, 138 insertions(+), 16 deletions(-) create mode 100644 spec/fixtures/config/kubeconfig-without-ca.yml create mode 100644 spec/fixtures/config/kubeconfig.yml diff --git a/app/models/project_services/kubernetes_service.rb b/app/models/project_services/kubernetes_service.rb index 48e7802c557..f1b321139d3 100644 --- a/app/models/project_services/kubernetes_service.rb +++ b/app/models/project_services/kubernetes_service.rb @@ -96,10 +96,14 @@ class KubernetesService < DeploymentService end def predefined_variables + config = YAML.dump(kubeconfig) + variables = [ { key: 'KUBE_URL', value: api_url, public: true }, { key: 'KUBE_TOKEN', value: token, public: false }, - { key: 'KUBE_NAMESPACE', value: actual_namespace, public: true } + { key: 'KUBE_NAMESPACE', value: actual_namespace, public: true }, + { key: 'KUBECONFIG', value: config, public: false }, + { key: 'KUBECONFIG_FILE', value: config, public: false, file: true }, ] if ca_pem.present? @@ -135,6 +139,14 @@ class KubernetesService < DeploymentService private + def kubeconfig + to_kubeconfig( + url: api_url, + namespace: actual_namespace, + token: token, + ca_pem: ca_pem) + end + def namespace_placeholder default_namespace || TEMPLATE_PLACEHOLDER end diff --git a/lib/gitlab/kubernetes.rb b/lib/gitlab/kubernetes.rb index c56c1a4322f..cedef9b65ba 100644 --- a/lib/gitlab/kubernetes.rb +++ b/lib/gitlab/kubernetes.rb @@ -76,5 +76,44 @@ module Gitlab url.to_s end + + def to_kubeconfig(url:, namespace:, token:, ca_pem: nil) + config = { + apiVersion: 'v1', + clusters: [ + name: 'gitlab-deploy', + cluster: { + server: url + }, + ], + contexts: [ + name: 'gitlab-deploy', + context: { + cluster: 'gitlab-deploy', + namespace: namespace, + user: 'gitlab-deploy' + }, + ], + :'current-context' => 'gitlab-deploy', + kind: 'Config', + users: [ + { + name: 'gitlab-deploy', + user: {token: token} + } + ] + } + + kubeconfig_embed_ca_pem(config, ca_pem) if ca_pem + + config.deep_stringify_keys + end + + private + + def kubeconfig_embed_ca_pem(config, ca_pem) + cluster = config.dig(:clusters, 0, :cluster) + cluster[:'certificate-authority-data'] = ca_pem + end end end diff --git a/spec/fixtures/config/kubeconfig-without-ca.yml b/spec/fixtures/config/kubeconfig-without-ca.yml new file mode 100644 index 00000000000..b2cb989d548 --- /dev/null +++ b/spec/fixtures/config/kubeconfig-without-ca.yml @@ -0,0 +1,18 @@ +--- +apiVersion: v1 +clusters: +- name: gitlab-deploy + cluster: + server: https://kube.domain.com +contexts: +- name: gitlab-deploy + context: + cluster: gitlab-deploy + namespace: NAMESPACE + user: gitlab-deploy +current-context: gitlab-deploy +kind: Config +users: +- name: gitlab-deploy + user: + token: TOKEN diff --git a/spec/fixtures/config/kubeconfig.yml b/spec/fixtures/config/kubeconfig.yml new file mode 100644 index 00000000000..4fa52818fee --- /dev/null +++ b/spec/fixtures/config/kubeconfig.yml @@ -0,0 +1,19 @@ +--- +apiVersion: v1 +clusters: +- name: gitlab-deploy + cluster: + server: https://kube.domain.com + certificate-authority-data: PEM +contexts: +- name: gitlab-deploy + context: + cluster: gitlab-deploy + namespace: NAMESPACE + user: gitlab-deploy +current-context: gitlab-deploy +kind: Config +users: +- name: gitlab-deploy + user: + token: TOKEN diff --git a/spec/lib/gitlab/kubernetes_spec.rb b/spec/lib/gitlab/kubernetes_spec.rb index e8c599a95ee..34b33772578 100644 --- a/spec/lib/gitlab/kubernetes_spec.rb +++ b/spec/lib/gitlab/kubernetes_spec.rb @@ -46,4 +46,28 @@ describe Gitlab::Kubernetes do expect(filter_by_label(items, app: 'foo')).to eq(matching_items) end end + + describe '#to_kubeconfig' do + subject do + to_kubeconfig( + url: 'https://kube.domain.com', + namespace: 'NAMESPACE', + token: 'TOKEN', + ca_pem: ca_pem) + end + + context 'when CA PEM is provided' do + let(:ca_pem) { 'PEM' } + let(:path) { expand_fixture_path('config/kubeconfig.yml') } + + it { is_expected.to eq(YAML.load_file(path)) } + end + + context 'when CA PEM is not provided' do + let(:ca_pem) { nil } + let(:path) { expand_fixture_path('config/kubeconfig-without-ca.yml') } + + it { is_expected.to eq(YAML.load_file(path)) } + end + end end diff --git a/spec/models/project_services/kubernetes_service_spec.rb b/spec/models/project_services/kubernetes_service_spec.rb index 858ad595dbf..f69e273cd7c 100644 --- a/spec/models/project_services/kubernetes_service_spec.rb +++ b/spec/models/project_services/kubernetes_service_spec.rb @@ -129,7 +129,7 @@ describe KubernetesService, models: true, caching: true do it "returns the default namespace" do is_expected.to eq(service.send(:default_namespace)) end - + context 'when namespace is specified' do before do service.namespace = 'my-namespace' @@ -201,6 +201,13 @@ describe KubernetesService, models: true, caching: true do end describe '#predefined_variables' do + let(:kubeconfig) do + File.read(expand_fixture_path('config/kubeconfig.yml')) + .gsub('TOKEN', 'token') + .gsub('PEM', 'CA PEM DATA') + .gsub('NAMESPACE', namespace) + end + before do subject.api_url = 'https://kube.domain.com' subject.token = 'token' @@ -208,32 +215,35 @@ describe KubernetesService, models: true, caching: true do subject.project = project end - context 'namespace is provided' do - before do - subject.namespace = 'my-project' - end - + shared_examples 'setting variables' do it 'sets the variables' do expect(subject.predefined_variables).to include( { key: 'KUBE_URL', value: 'https://kube.domain.com', public: true }, { key: 'KUBE_TOKEN', value: 'token', public: false }, - { key: 'KUBE_NAMESPACE', value: 'my-project', public: true }, + { key: 'KUBE_NAMESPACE', value: namespace, public: true }, + { key: 'KUBECONFIG', value: kubeconfig, public: false }, + { key: 'KUBECONFIG_FILE', value: kubeconfig, public: false, file: true }, { key: 'KUBE_CA_PEM', value: 'CA PEM DATA', public: true }, { key: 'KUBE_CA_PEM_FILE', value: 'CA PEM DATA', public: true, file: true } ) end end - context 'no namespace provided' do - it 'sets the variables' do - expect(subject.predefined_variables).to include( - { key: 'KUBE_URL', value: 'https://kube.domain.com', public: true }, - { key: 'KUBE_TOKEN', value: 'token', public: false }, - { key: 'KUBE_CA_PEM', value: 'CA PEM DATA', public: true }, - { key: 'KUBE_CA_PEM_FILE', value: 'CA PEM DATA', public: true, file: true } - ) + context 'namespace is provided' do + let(:namespace) { 'my-project' } + + before do + subject.namespace = namespace end + it_behaves_like 'setting variables' + end + + context 'no namespace provided' do + let(:namespace) { subject.actual_namespace } + + it_behaves_like 'setting variables' + it 'sets the KUBE_NAMESPACE' do kube_namespace = subject.predefined_variables.find { |h| h[:key] == 'KUBE_NAMESPACE' } -- cgit v1.2.3 From 6eaec942e6ae89818ea1ba0da5ff00daea633c41 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Fri, 16 Jun 2017 22:26:40 +0800 Subject: Changelog entry, doc, and only pass KUBECONFIG_FILE --- app/models/project_services/kubernetes_service.rb | 3 +-- changelogs/unreleased/33360-generate-kubeconfig.yml | 4 ++++ doc/user/project/integrations/kubernetes.md | 1 + spec/models/project_services/kubernetes_service_spec.rb | 1 - 4 files changed, 6 insertions(+), 3 deletions(-) create mode 100644 changelogs/unreleased/33360-generate-kubeconfig.yml diff --git a/app/models/project_services/kubernetes_service.rb b/app/models/project_services/kubernetes_service.rb index f1b321139d3..831f4e5a3c8 100644 --- a/app/models/project_services/kubernetes_service.rb +++ b/app/models/project_services/kubernetes_service.rb @@ -102,8 +102,7 @@ class KubernetesService < DeploymentService { key: 'KUBE_URL', value: api_url, public: true }, { key: 'KUBE_TOKEN', value: token, public: false }, { key: 'KUBE_NAMESPACE', value: actual_namespace, public: true }, - { key: 'KUBECONFIG', value: config, public: false }, - { key: 'KUBECONFIG_FILE', value: config, public: false, file: true }, + { key: 'KUBECONFIG_FILE', value: config, public: false, file: true } ] if ca_pem.present? diff --git a/changelogs/unreleased/33360-generate-kubeconfig.yml b/changelogs/unreleased/33360-generate-kubeconfig.yml new file mode 100644 index 00000000000..354a8a7f9b4 --- /dev/null +++ b/changelogs/unreleased/33360-generate-kubeconfig.yml @@ -0,0 +1,4 @@ +--- +title: Provide KUBECONFIG_FILE from KubernetesService for runners +merge_request: 12223 +author: diff --git a/doc/user/project/integrations/kubernetes.md b/doc/user/project/integrations/kubernetes.md index 73fa83d72a8..d1c3e18a276 100644 --- a/doc/user/project/integrations/kubernetes.md +++ b/doc/user/project/integrations/kubernetes.md @@ -55,6 +55,7 @@ GitLab CI build environment: - `KUBE_CA_PEM_FILE` - only present if a custom CA bundle was specified. Path to a file containing PEM data. - `KUBE_CA_PEM` (deprecated)- only if a custom CA bundle was specified. Raw PEM data. +- `KUBECONFIG_FILE` - Path to a file containing kubeconfig for this deployment. CA bundle would be embedded if specified. ## Web terminals diff --git a/spec/models/project_services/kubernetes_service_spec.rb b/spec/models/project_services/kubernetes_service_spec.rb index f69e273cd7c..d4feae231bc 100644 --- a/spec/models/project_services/kubernetes_service_spec.rb +++ b/spec/models/project_services/kubernetes_service_spec.rb @@ -221,7 +221,6 @@ describe KubernetesService, models: true, caching: true do { key: 'KUBE_URL', value: 'https://kube.domain.com', public: true }, { key: 'KUBE_TOKEN', value: 'token', public: false }, { key: 'KUBE_NAMESPACE', value: namespace, public: true }, - { key: 'KUBECONFIG', value: kubeconfig, public: false }, { key: 'KUBECONFIG_FILE', value: kubeconfig, public: false, file: true }, { key: 'KUBE_CA_PEM', value: 'CA PEM DATA', public: true }, { key: 'KUBE_CA_PEM_FILE', value: 'CA PEM DATA', public: true, file: true } -- cgit v1.2.3 From 6fe2b79656cca21f9f1cea504e5cbb58a51a05ab Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Fri, 16 Jun 2017 23:17:06 +0800 Subject: Fix rubocop offense --- lib/gitlab/kubernetes.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/gitlab/kubernetes.rb b/lib/gitlab/kubernetes.rb index cedef9b65ba..88bae87211a 100644 --- a/lib/gitlab/kubernetes.rb +++ b/lib/gitlab/kubernetes.rb @@ -84,7 +84,7 @@ module Gitlab name: 'gitlab-deploy', cluster: { server: url - }, + } ], contexts: [ name: 'gitlab-deploy', @@ -92,14 +92,14 @@ module Gitlab cluster: 'gitlab-deploy', namespace: namespace, user: 'gitlab-deploy' - }, + } ], - :'current-context' => 'gitlab-deploy', + 'current-context': 'gitlab-deploy', kind: 'Config', users: [ { name: 'gitlab-deploy', - user: {token: token} + user: { token: token } } ] } -- cgit v1.2.3 From c31db46ed1c88e4680c10a04bd504c8cb64de8c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=BB=84=E6=B6=9B?= Date: Fri, 23 Jun 2017 13:43:57 +0800 Subject: Add Traditional Chinese in Taiwan translations of Commits Page [skip ci] --- locale/zh_TW/part.po | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) create mode 100644 locale/zh_TW/part.po diff --git a/locale/zh_TW/part.po b/locale/zh_TW/part.po new file mode 100644 index 00000000000..3e4ddd07cf6 --- /dev/null +++ b/locale/zh_TW/part.po @@ -0,0 +1,52 @@ +# Huang Tao , 2017. #zanata +msgid "" +msgstr "" +"Project-Id-Version: gitlab 1.0.0\n" +"Report-Msgid-Bugs-To: \n" +"POT-Creation-Date: 2017-06-15 21:59-0500\n" +"MIME-Version: 1.0\n" +"Content-Type: text/plain; charset=UTF-8\n" +"Content-Transfer-Encoding: 8bit\n" +"PO-Revision-Date: 2017-06-23 01:23-0400\n" +"Last-Translator: Huang Tao \n" +"Language-Team: Chinese (Taiwan)\n" +"Language: zh-TW\n" +"X-Generator: Zanata 3.9.6\n" +"Plural-Forms: nplurals=1; plural=0\n" + +msgid "%d additional commit have been omitted to prevent performance issues." +msgid_plural "" +"%d additional commits have been omitted to prevent performance issues." +msgstr[0] "為提高頁面加載速度及性能,已省略了 %d 次送交 (commit)。" + +msgid "%d commit" +msgid_plural "%d commits" +msgstr[0] "%d 次送交 (commit)" + +msgid "BranchSwitcherPlaceholder|Search branches" +msgstr "搜索分支 (branches)" + +msgid "BranchSwitcherTitle|Switch branch" +msgstr "切換分支 (branch)" + +msgid "Browse Directory" +msgstr "瀏覽目錄" + +msgid "Browse File" +msgstr "瀏覽檔案" + +msgid "Browse Files" +msgstr "瀏覽檔案" + +msgid "Commits feed" +msgstr "送交動態" + +msgid "Filter by commit message" +msgstr "按送交消息過濾" + +msgid "UploadLink|click to upload" +msgstr "點擊上傳" + +msgid "View open merge request" +msgstr "查看開啟的合並請求 (merge request)" + -- cgit v1.2.3 From f28a1d94a7bef0625e3a0aee3d00bcab74c97013 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=BB=84=E6=B6=9B?= Date: Fri, 23 Jun 2017 14:19:28 +0800 Subject: add changelog [skip ci] --- ...add-traditional-chinese-in-taiwan-translations-of-commits-page.yml | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changelogs/unreleased/34172-add-traditional-chinese-in-taiwan-translations-of-commits-page.yml diff --git a/changelogs/unreleased/34172-add-traditional-chinese-in-taiwan-translations-of-commits-page.yml b/changelogs/unreleased/34172-add-traditional-chinese-in-taiwan-translations-of-commits-page.yml new file mode 100644 index 00000000000..224b9e1852f --- /dev/null +++ b/changelogs/unreleased/34172-add-traditional-chinese-in-taiwan-translations-of-commits-page.yml @@ -0,0 +1,4 @@ +--- +title: Add Traditional Chinese in Taiwan translations of Commits Page +merge_request: 12407 +author: Huang Tao -- cgit v1.2.3 From 27730abe3a7b26c44a71b1d12134223186d25d5b Mon Sep 17 00:00:00 2001 From: DJ Mountney Date: Fri, 30 Jun 2017 11:54:23 -0700 Subject: Add GitLab Runner Helm Chart documenation for cucstom certificates This outlines how to provide the custom ssl certificate to the runner for accessing GitLab in the case that GitLab is using a custom/self-signed certificate. --- doc/install/kubernetes/gitlab_runner_chart.md | 53 +++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/doc/install/kubernetes/gitlab_runner_chart.md b/doc/install/kubernetes/gitlab_runner_chart.md index b8bc0795f2e..515b2841d08 100644 --- a/doc/install/kubernetes/gitlab_runner_chart.md +++ b/doc/install/kubernetes/gitlab_runner_chart.md @@ -54,6 +54,13 @@ gitlabURL: http://gitlab.your-domain.com/ ## runnerRegistrationToken: "" +## Set the certsSecretName in order to pass custom certficates for GitLab Runner to use +## Provide resource name for a Kubernetes Secret Object in the same namespace, +## this is used to populate the /etc/gitlab-runner/certs directory +## ref: https://docs.gitlab.com/runner/configuration/tls-self-signed.html#supported-options-for-self-signed-certificates +## +#certsSecretName: + ## Configure the maximum number of concurrent jobs ## ref: https://docs.gitlab.com/runner/configuration/advanced-configuration.html#the-global-section ## @@ -135,6 +142,52 @@ runners: privileged: true ``` +### Providing a custom certificate for accessing GitLab + +You can provide a [Kubernetes Secret](https://kubernetes.io/docs/concepts/configuration/secret/) +to the GitLab Runner Helm Chart, which will be used to populate the container's +`/etc/gitlab-runner/certs` directory. + +Each key name in the Secret will be used as a filename in the directory, with the +file content being the value associated with the key. + +More information on how GitLab Runner uses these certificates can be found in the +[Runner Documentation](https://docs.gitlab.com/runner/configuration/tls-self-signed.html#supported-options-for-self-signed-certificates). + + - The key/file name used should be in the format `.crt`. For example: `gitlab.your-domain.com.crt`. + - Any intermediate certificates need to be concatenated to your server certificate in the same file. + - The hostname used should be the one the certificate is registered for. + +The GitLab Runner Helm Chart does not create a secret for you. In order to create +the secret, you can prepare your certificate on you local machine, and then run +the `kubectl create secret` command from the directory with the certificate + +```bash +kubectl + --namespace + create secret generic + --from-file= +``` + +- `` is the Kubernetes namespace where you want to install the GitLab Runner. +- `` is the Kubernetes Secret resource name. For example: `gitlab-domain-cert` +- `` is the filename for the certificate in your current directory that will be imported into the secret + +You then need to provide the secret's name to the GitLab Runner chart. + +Add the following to your `values.yaml` + +```yaml +## Set the certsSecretName in order to pass custom certficates for GitLab Runner to use +## Provide resource name for a Kubernetes Secret Object in the same namespace, +## this is used to populate the /etc/gitlab-runner/certs directory +## ref: https://docs.gitlab.com/runner/configuration/tls-self-signed.html#supported-options-for-self-signed-certificates +## +certsSecretName: +``` + +- `` is the Kubernetes Secret resource name. For example: `gitlab-domain-cert` + ## Installing GitLab Runner using the Helm Chart Once you [have configured](#configuration) GitLab Runner in your `values.yml` file, -- cgit v1.2.3 From 7424eb05d7d6ead2cce2e1045f2d00d273f7540e Mon Sep 17 00:00:00 2001 From: Joshua Lambert Date: Mon, 3 Jul 2017 16:28:08 -0400 Subject: Add ELB metrics --- config/prometheus/additional_metrics.yml | 38 ++++++++++++++++++++++++++++++-- 1 file changed, 36 insertions(+), 2 deletions(-) diff --git a/config/prometheus/additional_metrics.yml b/config/prometheus/additional_metrics.yml index daecde49570..3f7e897e73a 100644 --- a/config/prometheus/additional_metrics.yml +++ b/config/prometheus/additional_metrics.yml @@ -1,5 +1,33 @@ +- group: AWS Elastic Load Balancer + priority: 10 + metrics: + - title: "Request Rate" + y_label: "Requests/Min" + required_metrics: + - aws_elb_request_count_sum + weight: 1 + queries: + - query_range: 'sum(aws_elb_request_count_sum{%{environment_filter}})' + label: Requests per minute + unit: requests + - title: "Latency" + y_label: "Average Latency" + required_metrics: + - aws_elb_latency_average + weight: 1 + queries: + - query_range: 'avg(aws_elb_latency_average{%{environment_filter}})' + unit: seconds + - title: "Error Rate" + y_label: "Percent HTTP Errors / minute" + required_metrics: + - aws_elb_request_count_sum + - aws_elb_httpcode_backend_5_xx_sum + weight: 1 + queries: + - query_range: 'sum(aws_elb_httpcode_backend_5_xx_sum{%{environment_filter}}) / sum(aws_elb_request_count_sum{%{environment_filter}})' - group: Kubernetes - priority: 1 + priority: 5 metrics: - title: "Memory usage" y_label: "Values" @@ -23,7 +51,13 @@ - container_cpu_usage_seconds_total weight: 1 queries: - - query_range: 'avg(rate(container_cpu_usage_seconds_total{%{environment_filter}}[2m])) * 100' + - query_range: 'avg(rate(container_cpu_usage_seconds_total{%{environment_filter}}[2m])) by (cpu) * 100' + series: + label: cpu + - value: cpu00 + color: red + - value: cpu01 + color: blue - title: "Current CPU usage" required_metrics: - container_cpu_usage_seconds_total -- cgit v1.2.3 From c2b2f791e4dd33d1de8305aee46d5ee5750fc7e8 Mon Sep 17 00:00:00 2001 From: Fatih Acet Date: Thu, 22 Jun 2017 00:36:21 +0300 Subject: Banzai: Add tooltip attributes to reference filter. --- lib/banzai/filter/reference_filter.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/banzai/filter/reference_filter.rb b/lib/banzai/filter/reference_filter.rb index 6640168bfa2..a6f8650ed3d 100644 --- a/lib/banzai/filter/reference_filter.rb +++ b/lib/banzai/filter/reference_filter.rb @@ -30,6 +30,8 @@ module Banzai attributes = attributes.reject { |_, v| v.nil? } attributes[:reference_type] ||= self.class.reference_type + attributes[:container] ||= 'body' + attributes[:placement] ||= 'bottom' attributes.delete(:original) if context[:no_original_data] attributes.map do |key, value| %Q(data-#{key.to_s.dasherize}="#{escape_once(value)}") -- cgit v1.2.3 From fb6144298e19a632a574bfc576f5f672e08e4324 Mon Sep 17 00:00:00 2001 From: Joshua Lambert Date: Tue, 4 Jul 2017 01:45:35 -0400 Subject: Add NGINX metrics and other minor changes --- config/prometheus/additional_metrics.yml | 90 +++++++++++++++++++------------- 1 file changed, 53 insertions(+), 37 deletions(-) diff --git a/config/prometheus/additional_metrics.yml b/config/prometheus/additional_metrics.yml index 3f7e897e73a..d33fae4182d 100644 --- a/config/prometheus/additional_metrics.yml +++ b/config/prometheus/additional_metrics.yml @@ -1,66 +1,82 @@ - group: AWS Elastic Load Balancer priority: 10 metrics: - - title: "Request Rate" - y_label: "Requests/Min" + - title: "Throughput" + y_label: "Requests / Sec" required_metrics: - aws_elb_request_count_sum weight: 1 queries: - - query_range: 'sum(aws_elb_request_count_sum{%{environment_filter}})' - label: Requests per minute - unit: requests + - query_range: 'sum(aws_elb_request_count_sum{%{environment_filter}}) * 60' + label: Total + unit: req / sec - title: "Latency" - y_label: "Average Latency" + y_label: "Latency (ms)" required_metrics: - - aws_elb_latency_average + - aws_elb_latency_average weight: 1 queries: - - query_range: 'avg(aws_elb_latency_average{%{environment_filter}})' - unit: seconds - - title: "Error Rate" - y_label: "Percent HTTP Errors / minute" + - query_range: 'avg(aws_elb_latency_average{%{environment_filter}}) * 1000' + label: Average + unit: ms + - title: "HTTP Error Rate" + y_label: "Error Rate (%)" required_metrics: - - aws_elb_request_count_sum - - aws_elb_httpcode_backend_5_xx_sum + - aws_elb_request_count_sum + - aws_elb_httpcode_backend_5_xx_sum weight: 1 queries: - query_range: 'sum(aws_elb_httpcode_backend_5_xx_sum{%{environment_filter}}) / sum(aws_elb_request_count_sum{%{environment_filter}})' -- group: Kubernetes - priority: 5 + label: HTTP Errors + unit: "%" +- group: NGINX + priority: 10 metrics: - - title: "Memory usage" - y_label: "Values" + - title: "Throughput" + y_label: "Requests / Sec" required_metrics: - - container_memory_usage_bytes + - nginx_requests_total weight: 1 queries: - - query_range: 'avg(container_memory_usage_bytes{%{environment_filter}}) / 2^20' - label: Container memory - unit: MiB - - title: "Current memory usage" + - query_range: 'sum(rate(nginx_requests_total{server_zone!="*", server_zone!="_", %{environment_filter}}[2m]))' + label: Total + unit: req / sec + - title: "Latency" + y_label: "Latency (ms)" required_metrics: - - container_memory_usage_bytes + - nginx_upstream_response_msecs_avg weight: 1 queries: - - query: 'avg(container_memory_usage_bytes{%{environment_filter}}) / 2^20' - display_empty: false - unit: MiB - - title: "CPU usage" + - query_range: 'avg(nginx_upstream_response_msecs_avg{%{environment_filter}}) * 1000' + label: Upstream + unit: ms + - title: "HTTP Error Rate" + y_label: "Error Rate (%)" required_metrics: - - container_cpu_usage_seconds_total + - nginx_responses_total + weight: 1 + queries: + - query_range: 'sum(nginx_responses_total{status_code="5xx", %{environment_filter}}) / sum(nginx_responses_total{server_zone!="*", server_zone!="_", %{environment_filter}})' + label: HTTP Errors + unit: "%" +- group: Kubernetes + priority: 5 + metrics: + - title: "Memory Usage" + y_label: "Memory Usage (MB)" + required_metrics: + - container_memory_usage_bytes weight: 1 queries: - - query_range: 'avg(rate(container_cpu_usage_seconds_total{%{environment_filter}}[2m])) by (cpu) * 100' - series: - label: cpu - - value: cpu00 - color: red - - value: cpu01 - color: blue - - title: "Current CPU usage" + - query_range: '(sum(container_memory_usage_bytes{container_name!="POD",%{environment_filter}}) / count(container_memory_usage_bytes{container_name!="POD",%{environment_filter}})) /1024/1024' + label: Average + unit: MB + - title: "CPU Utilization" + y_label: "CPU Utilization (%)" required_metrics: - container_cpu_usage_seconds_total weight: 1 queries: - - query: 'avg(rate(container_cpu_usage_seconds_total{%{environment_filter}}[2m])) * 100' + - query_range: 'sum(rate(container_cpu_usage_seconds_total{container_name!="POD",%{environment_filter}}[2m])) / count(container_cpu_usage_seconds_total{container_name!="POD",%{environment_filter}}) * 100' + label: Average + unit: "%" -- cgit v1.2.3 From 26a7fff7deb2cbbbd88875a0da5a74f0a85bc382 Mon Sep 17 00:00:00 2001 From: Nick Thomas Date: Tue, 4 Jul 2017 12:40:48 +0100 Subject: Upgrade GitLab Pages to v0.5.0 --- GITLAB_PAGES_VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/GITLAB_PAGES_VERSION b/GITLAB_PAGES_VERSION index 17b2ccd9bf9..8f0916f768f 100644 --- a/GITLAB_PAGES_VERSION +++ b/GITLAB_PAGES_VERSION @@ -1 +1 @@ -0.4.3 +0.5.0 -- cgit v1.2.3 From 65c96bfac51f5e15c447f76d8e90aac5b3287bba Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Wed, 5 Jul 2017 08:45:10 +0100 Subject: Limit the file tree view container width Closes #34670 --- app/views/projects/tree/show.html.haml | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/app/views/projects/tree/show.html.haml b/app/views/projects/tree/show.html.haml index 96a08f9f8be..75f87bf752c 100644 --- a/app/views/projects/tree/show.html.haml +++ b/app/views/projects/tree/show.html.haml @@ -5,7 +5,6 @@ = auto_discovery_link_tag(:atom, namespace_project_commits_url(@project.namespace, @project, @ref, rss_url_options), title: "#{@project.name}:#{@ref} commits") = render "projects/commits/head" -= render 'projects/last_push' - -%div{ class: container_class } +%div{ class: [container_class, ("limit-container-width" unless fluid_layout)] } + = render 'projects/last_push' = render 'projects/files', commit: @last_commit, project: @project, ref: @ref -- cgit v1.2.3 From 7f9996f66e82c29198cdd16f2aa6daf520dcde00 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=BB=84=E6=B6=9B?= Date: Fri, 23 Jun 2017 13:43:57 +0800 Subject: Add Traditional Chinese in Taiwan translations of Commits Page [skip ci] --- locale/zh_TW/part.po | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) create mode 100644 locale/zh_TW/part.po diff --git a/locale/zh_TW/part.po b/locale/zh_TW/part.po new file mode 100644 index 00000000000..3e4ddd07cf6 --- /dev/null +++ b/locale/zh_TW/part.po @@ -0,0 +1,52 @@ +# Huang Tao , 2017. #zanata +msgid "" +msgstr "" +"Project-Id-Version: gitlab 1.0.0\n" +"Report-Msgid-Bugs-To: \n" +"POT-Creation-Date: 2017-06-15 21:59-0500\n" +"MIME-Version: 1.0\n" +"Content-Type: text/plain; charset=UTF-8\n" +"Content-Transfer-Encoding: 8bit\n" +"PO-Revision-Date: 2017-06-23 01:23-0400\n" +"Last-Translator: Huang Tao \n" +"Language-Team: Chinese (Taiwan)\n" +"Language: zh-TW\n" +"X-Generator: Zanata 3.9.6\n" +"Plural-Forms: nplurals=1; plural=0\n" + +msgid "%d additional commit have been omitted to prevent performance issues." +msgid_plural "" +"%d additional commits have been omitted to prevent performance issues." +msgstr[0] "為提高頁面加載速度及性能,已省略了 %d 次送交 (commit)。" + +msgid "%d commit" +msgid_plural "%d commits" +msgstr[0] "%d 次送交 (commit)" + +msgid "BranchSwitcherPlaceholder|Search branches" +msgstr "搜索分支 (branches)" + +msgid "BranchSwitcherTitle|Switch branch" +msgstr "切換分支 (branch)" + +msgid "Browse Directory" +msgstr "瀏覽目錄" + +msgid "Browse File" +msgstr "瀏覽檔案" + +msgid "Browse Files" +msgstr "瀏覽檔案" + +msgid "Commits feed" +msgstr "送交動態" + +msgid "Filter by commit message" +msgstr "按送交消息過濾" + +msgid "UploadLink|click to upload" +msgstr "點擊上傳" + +msgid "View open merge request" +msgstr "查看開啟的合並請求 (merge request)" + -- cgit v1.2.3 From f2655ce13e25f13cf196652dcb0c85ae0b7eb705 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=BB=84=E6=B6=9B?= Date: Fri, 23 Jun 2017 14:19:28 +0800 Subject: add changelog [skip ci] --- ...add-traditional-chinese-in-taiwan-translations-of-commits-page.yml | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changelogs/unreleased/34172-add-traditional-chinese-in-taiwan-translations-of-commits-page.yml diff --git a/changelogs/unreleased/34172-add-traditional-chinese-in-taiwan-translations-of-commits-page.yml b/changelogs/unreleased/34172-add-traditional-chinese-in-taiwan-translations-of-commits-page.yml new file mode 100644 index 00000000000..224b9e1852f --- /dev/null +++ b/changelogs/unreleased/34172-add-traditional-chinese-in-taiwan-translations-of-commits-page.yml @@ -0,0 +1,4 @@ +--- +title: Add Traditional Chinese in Taiwan translations of Commits Page +merge_request: 12407 +author: Huang Tao -- cgit v1.2.3 From 8b01c1d939e33ed4cac21209d52da49cda6464ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=BB=84=E6=B6=9B?= Date: Wed, 5 Jul 2017 16:00:45 +0800 Subject: Add Traditional Chinese in Taiwan translations of Commits Page --- locale/zh_TW/gitlab.po | 38 +++++++++++++++++++++++++++++++++++- locale/zh_TW/part.po | 52 -------------------------------------------------- 2 files changed, 37 insertions(+), 53 deletions(-) delete mode 100644 locale/zh_TW/part.po diff --git a/locale/zh_TW/gitlab.po b/locale/zh_TW/gitlab.po index bb2b84c67b0..1f86a5d7021 100644 --- a/locale/zh_TW/gitlab.po +++ b/locale/zh_TW/gitlab.po @@ -7,7 +7,7 @@ msgid "" msgstr "" "Project-Id-Version: gitlab 1.0.0\n" "Report-Msgid-Bugs-To: \n" -"POT-Creation-Date: 2017-06-15 21:59-0500\n" +"POT-Creation-Date: 2017-06-19 15:50-0500\n" "MIME-Version: 1.0\n" "Content-Type: text/plain; charset=UTF-8\n" "Content-Transfer-Encoding: 8bit\n" @@ -18,6 +18,15 @@ msgstr "" "X-Generator: Zanata 3.9.6\n" "Plural-Forms: nplurals=1; plural=0\n" +msgid "%d additional commit have been omitted to prevent performance issues." +msgid_plural "" +"%d additional commits have been omitted to prevent performance issues." +msgstr[0] "為提高頁面加載速度及性能,已省略了 %d 次送交 (commit)。" + +msgid "%d commit" +msgid_plural "%d commits" +msgstr[0] "%d 次送交 (commit)" + msgid "%{commit_author_link} committed %{commit_timeago}" msgstr "%{commit_author_link} 在 %{commit_timeago} 送交" @@ -66,9 +75,24 @@ msgstr "" "已建立分支 (branch) %{branch_name} 。如要設定自動部署, 請選擇合適的 GitLab CI " "Yaml 模板,然後記得要送交 (commit) 您的編輯內容。%{link_to_autodeploy_doc}\n" +msgid "BranchSwitcherPlaceholder|Search branches" +msgstr "搜索分支 (branches)" + +msgid "BranchSwitcherTitle|Switch branch" +msgstr "切換分支 (branch)" + msgid "Branches" msgstr "分支 (branch) " +msgid "Browse Directory" +msgstr "瀏覽目錄" + +msgid "Browse File" +msgstr "瀏覽檔案" + +msgid "Browse Files" +msgstr "瀏覽檔案" + msgid "Browse files" msgstr "瀏覽檔案" @@ -175,6 +199,9 @@ msgstr "建立 %{file_name}" msgid "Commits" msgstr "更動記錄 (commit) " +msgid "Commits feed" +msgstr "送交動態" + msgid "Commits|History" msgstr "過去更動 (commit) " @@ -332,6 +359,9 @@ msgstr "無法刪除流水線 (pipeline) 排程" msgid "Files" msgstr "檔案" +msgid "Filter by commit message" +msgstr "按送交消息過濾" + msgid "Find by path" msgstr "以路徑搜尋" @@ -985,9 +1015,15 @@ msgstr "上傳新檔案" msgid "Upload file" msgstr "上傳檔案" +msgid "UploadLink|click to upload" +msgstr "點擊上傳" + msgid "Use your global notification setting" msgstr "使用全域通知設定" +msgid "View open merge request" +msgstr "查看開啟的合並請求 (merge request)" + msgid "VisibilityLevel|Internal" msgstr "內部" diff --git a/locale/zh_TW/part.po b/locale/zh_TW/part.po deleted file mode 100644 index 3e4ddd07cf6..00000000000 --- a/locale/zh_TW/part.po +++ /dev/null @@ -1,52 +0,0 @@ -# Huang Tao , 2017. #zanata -msgid "" -msgstr "" -"Project-Id-Version: gitlab 1.0.0\n" -"Report-Msgid-Bugs-To: \n" -"POT-Creation-Date: 2017-06-15 21:59-0500\n" -"MIME-Version: 1.0\n" -"Content-Type: text/plain; charset=UTF-8\n" -"Content-Transfer-Encoding: 8bit\n" -"PO-Revision-Date: 2017-06-23 01:23-0400\n" -"Last-Translator: Huang Tao \n" -"Language-Team: Chinese (Taiwan)\n" -"Language: zh-TW\n" -"X-Generator: Zanata 3.9.6\n" -"Plural-Forms: nplurals=1; plural=0\n" - -msgid "%d additional commit have been omitted to prevent performance issues." -msgid_plural "" -"%d additional commits have been omitted to prevent performance issues." -msgstr[0] "為提高頁面加載速度及性能,已省略了 %d 次送交 (commit)。" - -msgid "%d commit" -msgid_plural "%d commits" -msgstr[0] "%d 次送交 (commit)" - -msgid "BranchSwitcherPlaceholder|Search branches" -msgstr "搜索分支 (branches)" - -msgid "BranchSwitcherTitle|Switch branch" -msgstr "切換分支 (branch)" - -msgid "Browse Directory" -msgstr "瀏覽目錄" - -msgid "Browse File" -msgstr "瀏覽檔案" - -msgid "Browse Files" -msgstr "瀏覽檔案" - -msgid "Commits feed" -msgstr "送交動態" - -msgid "Filter by commit message" -msgstr "按送交消息過濾" - -msgid "UploadLink|click to upload" -msgstr "點擊上傳" - -msgid "View open merge request" -msgstr "查看開啟的合並請求 (merge request)" - -- cgit v1.2.3 From bb29a360c5ae9b4d565a9cbd5fd539e78733f1e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=BB=84=E6=B6=9B?= Date: Wed, 5 Jul 2017 17:22:24 +0800 Subject: synchronize changes in community translation of msgid --- locale/zh_TW/gitlab.po | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/locale/zh_TW/gitlab.po b/locale/zh_TW/gitlab.po index 1f86a5d7021..5ede5845bf7 100644 --- a/locale/zh_TW/gitlab.po +++ b/locale/zh_TW/gitlab.po @@ -18,7 +18,7 @@ msgstr "" "X-Generator: Zanata 3.9.6\n" "Plural-Forms: nplurals=1; plural=0\n" -msgid "%d additional commit have been omitted to prevent performance issues." +msgid "%d additional commit has been omitted to prevent performance issues." msgid_plural "" "%d additional commits have been omitted to prevent performance issues." msgstr[0] "為提高頁面加載速度及性能,已省略了 %d 次送交 (commit)。" -- cgit v1.2.3 From a45a2d47ccd1eeaf69a443755133268579185a64 Mon Sep 17 00:00:00 2001 From: Valery Sizov Date: Wed, 5 Jul 2017 15:07:03 +0300 Subject: Backport Service changes from https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/2259 --- app/models/service.rb | 8 ++++++++ app/views/projects/services/_form.html.haml | 21 +++++++++++---------- app/views/shared/_service_settings.html.haml | 9 +++++---- 3 files changed, 24 insertions(+), 14 deletions(-) diff --git a/app/models/service.rb b/app/models/service.rb index 6a0b0a5c522..e6594a9bd36 100644 --- a/app/models/service.rb +++ b/app/models/service.rb @@ -51,6 +51,14 @@ class Service < ActiveRecord::Base active end + def show_active_box? + true + end + + def editable? + true + end + def template? template end diff --git a/app/views/projects/services/_form.html.haml b/app/views/projects/services/_form.html.haml index 6dffc026392..e000792af8a 100644 --- a/app/views/projects/services/_form.html.haml +++ b/app/views/projects/services/_form.html.haml @@ -11,16 +11,17 @@ .col-lg-9 = form_for(@service, as: :service, url: namespace_project_service_path(@project.namespace, @project, @service.to_param), method: :put, html: { class: 'gl-show-field-errors form-horizontal js-integration-settings-form', data: { 'can-test' => @service.can_test?, 'test-url' => test_namespace_project_service_path } }) do |form| = render 'shared/service_settings', form: form, subject: @service - .footer-block.row-content-block - %button.btn.btn-save{ type: 'submit' } - = icon('spinner spin', class: 'hidden js-btn-spinner') - %span.js-btn-label - Save changes -   - - if @service.valid? && @service.activated? - - unless @service.can_test? - - disabled_class = 'disabled' - - disabled_title = @service.disabled_title + - if @service.editable? + .footer-block.row-content-block + %button.btn.btn-save{ type: 'submit' } + = icon('spinner spin', class: 'hidden js-btn-spinner') + %span.js-btn-label + Save changes +   + - if @service.valid? && @service.activated? + - unless @service.can_test? + - disabled_class = 'disabled' + - disabled_title = @service.disabled_title = link_to 'Cancel', namespace_project_settings_integrations_path(@project.namespace, @project), class: 'btn btn-cancel' diff --git a/app/views/shared/_service_settings.html.haml b/app/views/shared/_service_settings.html.haml index b200e5fc528..7ca14ac93cc 100644 --- a/app/views/shared/_service_settings.html.haml +++ b/app/views/shared/_service_settings.html.haml @@ -7,10 +7,11 @@ = markdown @service.help .service-settings - .form-group - = form.label :active, "Active", class: "control-label" - .col-sm-10 - = form.check_box :active + - if @service.show_active_box? + .form-group + = form.label :active, "Active", class: "control-label" + .col-sm-10 + = form.check_box :active - if @service.supported_events.present? .form-group -- cgit v1.2.3 From 6d63d766d12d92095b781cab7162ba41f23557f3 Mon Sep 17 00:00:00 2001 From: Nick Thomas Date: Wed, 5 Jul 2017 18:25:45 +0100 Subject: Fix stubbing attributes alongside cache_markdown_field --- app/models/concerns/cache_markdown_field.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/concerns/cache_markdown_field.rb b/app/models/concerns/cache_markdown_field.rb index eb32bf3d32a..95152dcd68c 100644 --- a/app/models/concerns/cache_markdown_field.rb +++ b/app/models/concerns/cache_markdown_field.rb @@ -78,7 +78,7 @@ module CacheMarkdownField def cached_html_up_to_date?(markdown_field) html_field = cached_markdown_fields.html_field(markdown_field) - cached = !cached_html_for(markdown_field).nil? && !__send__(markdown_field).nil? + cached = cached_html_for(markdown_field).present? && __send__(markdown_field).present? return false unless cached markdown_changed = attribute_changed?(markdown_field) || false -- cgit v1.2.3 From 8d44d5142ae8a5e00b8417d2db8a7627fea0ef57 Mon Sep 17 00:00:00 2001 From: vanadium23 Date: Thu, 29 Jun 2017 20:20:59 +0300 Subject: Add user projects API --- app/finders/projects_finder.rb | 9 +- changelogs/unreleased/33657-user-projects-api.yml | 4 + doc/api/projects.md | 158 ++++++++++++++++++++++ lib/api/helpers.rb | 3 +- lib/api/projects.rb | 113 +++++++++------- spec/requests/api/projects_spec.rb | 20 +++ 6 files changed, 257 insertions(+), 50 deletions(-) create mode 100644 changelogs/unreleased/33657-user-projects-api.yml diff --git a/app/finders/projects_finder.rb b/app/finders/projects_finder.rb index 8bfbe37c543..aa80dfc3f37 100644 --- a/app/finders/projects_finder.rb +++ b/app/finders/projects_finder.rb @@ -28,7 +28,14 @@ class ProjectsFinder < UnionFinder end def execute - collection = init_collection + user = params.delete(:user) + collection = + if user + PersonalProjectsFinder.new(user).execute(current_user) + else + init_collection + end + collection = by_ids(collection) collection = by_personal(collection) collection = by_starred(collection) diff --git a/changelogs/unreleased/33657-user-projects-api.yml b/changelogs/unreleased/33657-user-projects-api.yml new file mode 100644 index 00000000000..a8d485865e9 --- /dev/null +++ b/changelogs/unreleased/33657-user-projects-api.yml @@ -0,0 +1,4 @@ +--- +title: Add user projects API +merge_request: 12596 +author: Ivan Chernov diff --git a/doc/api/projects.md b/doc/api/projects.md index cc1bb3911c8..235c1493d5e 100644 --- a/doc/api/projects.md +++ b/doc/api/projects.md @@ -173,6 +173,164 @@ Parameters: ] ``` +### List a user's projects + +Get a list of visible projects for the given user. When accessed without authentication, only public projects are returned. + +``` +GET /users/:user_id/projects +``` + +Parameters: + +| Attribute | Type | Required | Description | +| --------- | ---- | -------- | ----------- | +| `user_id` | string | yes | The ID or username of the user | +| `archived` | boolean | no | Limit by archived status | +| `visibility` | string | no | Limit by visibility `public`, `internal`, or `private` | +| `order_by` | string | no | Return projects ordered by `id`, `name`, `path`, `created_at`, `updated_at`, or `last_activity_at` fields. Default is `created_at` | +| `sort` | string | no | Return projects sorted in `asc` or `desc` order. Default is `desc` | +| `search` | string | no | Return list of projects matching the search criteria | +| `simple` | boolean | no | Return only the ID, URL, name, and path of each project | +| `owned` | boolean | no | Limit by projects owned by the current user | +| `membership` | boolean | no | Limit by projects that the current user is a member of | +| `starred` | boolean | no | Limit by projects starred by the current user | +| `statistics` | boolean | no | Include project statistics | +| `with_issues_enabled` | boolean | no | Limit by enabled issues feature | +| `with_merge_requests_enabled` | boolean | no | Limit by enabled merge requests feature | + +```json +[ + { + "id": 4, + "description": null, + "default_branch": "master", + "visibility": "private", + "ssh_url_to_repo": "git@example.com:diaspora/diaspora-client.git", + "http_url_to_repo": "http://example.com/diaspora/diaspora-client.git", + "web_url": "http://example.com/diaspora/diaspora-client", + "tag_list": [ + "example", + "disapora client" + ], + "owner": { + "id": 3, + "name": "Diaspora", + "created_at": "2013-09-30T13:46:02Z" + }, + "name": "Diaspora Client", + "name_with_namespace": "Diaspora / Diaspora Client", + "path": "diaspora-client", + "path_with_namespace": "diaspora/diaspora-client", + "issues_enabled": true, + "open_issues_count": 1, + "merge_requests_enabled": true, + "jobs_enabled": true, + "wiki_enabled": true, + "snippets_enabled": false, + "container_registry_enabled": false, + "created_at": "2013-09-30T13:46:02Z", + "last_activity_at": "2013-09-30T13:46:02Z", + "creator_id": 3, + "namespace": { + "id": 3, + "name": "Diaspora", + "path": "diaspora", + "kind": "group", + "full_path": "diaspora" + }, + "import_status": "none", + "archived": false, + "avatar_url": "http://example.com/uploads/project/avatar/4/uploads/avatar.png", + "shared_runners_enabled": true, + "forks_count": 0, + "star_count": 0, + "runners_token": "b8547b1dc37721d05889db52fa2f02", + "public_jobs": true, + "shared_with_groups": [], + "only_allow_merge_if_pipeline_succeeds": false, + "only_allow_merge_if_all_discussions_are_resolved": false, + "request_access_enabled": false, + "statistics": { + "commit_count": 37, + "storage_size": 1038090, + "repository_size": 1038090, + "lfs_objects_size": 0, + "job_artifacts_size": 0 + } + }, + { + "id": 6, + "description": null, + "default_branch": "master", + "visibility": "private", + "ssh_url_to_repo": "git@example.com:brightbox/puppet.git", + "http_url_to_repo": "http://example.com/brightbox/puppet.git", + "web_url": "http://example.com/brightbox/puppet", + "tag_list": [ + "example", + "puppet" + ], + "owner": { + "id": 4, + "name": "Brightbox", + "created_at": "2013-09-30T13:46:02Z" + }, + "name": "Puppet", + "name_with_namespace": "Brightbox / Puppet", + "path": "puppet", + "path_with_namespace": "brightbox/puppet", + "issues_enabled": true, + "open_issues_count": 1, + "merge_requests_enabled": true, + "jobs_enabled": true, + "wiki_enabled": true, + "snippets_enabled": false, + "container_registry_enabled": false, + "created_at": "2013-09-30T13:46:02Z", + "last_activity_at": "2013-09-30T13:46:02Z", + "creator_id": 3, + "namespace": { + "id": 4, + "name": "Brightbox", + "path": "brightbox", + "kind": "group", + "full_path": "brightbox" + }, + "import_status": "none", + "import_error": null, + "permissions": { + "project_access": { + "access_level": 10, + "notification_level": 3 + }, + "group_access": { + "access_level": 50, + "notification_level": 3 + } + }, + "archived": false, + "avatar_url": null, + "shared_runners_enabled": true, + "forks_count": 0, + "star_count": 0, + "runners_token": "b8547b1dc37721d05889db52fa2f02", + "public_jobs": true, + "shared_with_groups": [], + "only_allow_merge_if_pipeline_succeeds": false, + "only_allow_merge_if_all_discussions_are_resolved": false, + "request_access_enabled": false, + "statistics": { + "commit_count": 12, + "storage_size": 2066080, + "repository_size": 2066080, + "lfs_objects_size": 0, + "job_artifacts_size": 0 + } + } +] +``` + ### Get single project Get a specific project. This endpoint can be accessed without authentication if diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index a2a661b205c..0f4791841d2 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -268,6 +268,7 @@ module API finder_params[:visibility_level] = Gitlab::VisibilityLevel.level_value(params[:visibility]) if params[:visibility] finder_params[:archived] = params[:archived] finder_params[:search] = params[:search] if params[:search] + finder_params[:user] = params.delete(:user) if params[:user] finder_params end @@ -313,7 +314,7 @@ module API def present_artifacts!(artifacts_file) return not_found! unless artifacts_file.exists? - + if artifacts_file.file_storage? present_file!(artifacts_file.path, artifacts_file.filename) else diff --git a/lib/api/projects.rb b/lib/api/projects.rb index d0bd64b2972..57cf3a30e10 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -35,61 +35,78 @@ module API params :statistics_params do optional :statistics, type: Boolean, default: false, desc: 'Include project statistics' end - end - resource :projects do - helpers do - params :collection_params do - use :sort_params - use :filter_params - use :pagination - - optional :simple, type: Boolean, default: false, - desc: 'Return only the ID, URL, name, and path of each project' - end + params :collection_params do + use :sort_params + use :filter_params + use :pagination - params :sort_params do - optional :order_by, type: String, values: %w[id name path created_at updated_at last_activity_at], - default: 'created_at', desc: 'Return projects ordered by field' - optional :sort, type: String, values: %w[asc desc], default: 'desc', - desc: 'Return projects sorted in ascending and descending order' - end + optional :simple, type: Boolean, default: false, + desc: 'Return only the ID, URL, name, and path of each project' + end - params :filter_params do - optional :archived, type: Boolean, default: false, desc: 'Limit by archived status' - optional :visibility, type: String, values: Gitlab::VisibilityLevel.string_values, - desc: 'Limit by visibility' - optional :search, type: String, desc: 'Return list of projects matching the search criteria' - optional :owned, type: Boolean, default: false, desc: 'Limit by owned by authenticated user' - optional :starred, type: Boolean, default: false, desc: 'Limit by starred status' - optional :membership, type: Boolean, default: false, desc: 'Limit by projects that the current user is a member of' - optional :with_issues_enabled, type: Boolean, default: false, desc: 'Limit by enabled issues feature' - optional :with_merge_requests_enabled, type: Boolean, default: false, desc: 'Limit by enabled merge requests feature' - end + params :sort_params do + optional :order_by, type: String, values: %w[id name path created_at updated_at last_activity_at], + default: 'created_at', desc: 'Return projects ordered by field' + optional :sort, type: String, values: %w[asc desc], default: 'desc', + desc: 'Return projects sorted in ascending and descending order' + end - params :create_params do - optional :namespace_id, type: Integer, desc: 'Namespace ID for the new project. Default to the user namespace.' - optional :import_url, type: String, desc: 'URL from which the project is imported' - end + params :filter_params do + optional :archived, type: Boolean, default: false, desc: 'Limit by archived status' + optional :visibility, type: String, values: Gitlab::VisibilityLevel.string_values, + desc: 'Limit by visibility' + optional :search, type: String, desc: 'Return list of projects matching the search criteria' + optional :owned, type: Boolean, default: false, desc: 'Limit by owned by authenticated user' + optional :starred, type: Boolean, default: false, desc: 'Limit by starred status' + optional :membership, type: Boolean, default: false, desc: 'Limit by projects that the current user is a member of' + optional :with_issues_enabled, type: Boolean, default: false, desc: 'Limit by enabled issues feature' + optional :with_merge_requests_enabled, type: Boolean, default: false, desc: 'Limit by enabled merge requests feature' + end - def present_projects(options = {}) - projects = ProjectsFinder.new(current_user: current_user, params: project_finder_params).execute - projects = reorder_projects(projects) - projects = projects.with_statistics if params[:statistics] - projects = projects.with_issues_enabled if params[:with_issues_enabled] - projects = projects.with_merge_requests_enabled if params[:with_merge_requests_enabled] - - options = options.reverse_merge( - with: current_user ? Entities::ProjectWithAccess : Entities::BasicProjectDetails, - statistics: params[:statistics], - current_user: current_user - ) - options[:with] = Entities::BasicProjectDetails if params[:simple] - - present paginate(projects), options - end + params :create_params do + optional :namespace_id, type: Integer, desc: 'Namespace ID for the new project. Default to the user namespace.' + optional :import_url, type: String, desc: 'URL from which the project is imported' + end + + def present_projects(options = {}) + projects = ProjectsFinder.new(current_user: current_user, params: project_finder_params).execute + projects = reorder_projects(projects) + projects = projects.with_statistics if params[:statistics] + projects = projects.with_issues_enabled if params[:with_issues_enabled] + projects = projects.with_merge_requests_enabled if params[:with_merge_requests_enabled] + + options = options.reverse_merge( + with: current_user ? Entities::ProjectWithAccess : Entities::BasicProjectDetails, + statistics: params[:statistics], + current_user: current_user + ) + options[:with] = Entities::BasicProjectDetails if params[:simple] + + present paginate(projects), options end + end + resource :users, requirements: { user_id: %r{[^/]+} } do + desc 'Get a user projects' do + success Entities::BasicProjectDetails + end + params do + requires :user_id, type: String, desc: 'The ID or username of the user' + use :collection_params + use :statistics_params + end + get ":user_id/projects" do + user = find_user(params[:user_id]) + not_found!('User') unless user + + params[:user] = user + + present_projects + end + end + + resource :projects do desc 'Get a list of visible projects for authenticated user' do success Entities::BasicProjectDetails end diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index 14dec3d45b1..049daee0ece 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -475,6 +475,26 @@ describe API::Projects do end end + describe 'GET /users/:user_id/projects/' do + let!(:public_project) { create(:empty_project, :public, name: 'public_project', creator_id: user4.id, namespace: user4.namespace) } + + it 'returns error when user not found' do + get api('/users/9999/projects/') + + expect(response).to have_http_status(404) + expect(json_response['message']).to eq('404 User Not Found') + end + + it 'returns projects filtered by user' do + get api("/users/#{user4.id}/projects/", user) + + expect(response).to have_http_status(200) + expect(response).to include_pagination_headers + expect(json_response).to be_an Array + expect(json_response.map { |project| project['id'] }).to contain_exactly(public_project.id) + end + end + describe 'POST /projects/user/:id' do before do expect(project).to be_persisted -- cgit v1.2.3 From 633793cf47b8b02bffc65976cd97c21601661504 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Wed, 7 Jun 2017 08:45:34 +0000 Subject: Implement "remember me" for OAuth-based login. - Pass a `remember_me` query parameter along with the initial OAuth request, and pick this parameter up during the omniauth callback from request.env['omniauth.params']`. - For 2FA-based login, copy the `remember_me` param from `omniauth.params` to `params`, which the 2FA process will pick up. - For non-2FA-based login, simply call the `remember_me` devise method to set the session cookie. --- app/controllers/omniauth_callbacks_controller.rb | 8 ++++++++ app/views/devise/shared/_omniauth_box.html.haml | 19 ++++++++++++++++++- 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb index b82681b197e..c5adadfa529 100644 --- a/app/controllers/omniauth_callbacks_controller.rb +++ b/app/controllers/omniauth_callbacks_controller.rb @@ -1,5 +1,6 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController include AuthenticatesWithTwoFactor + include Devise::Controllers::Rememberable protect_from_forgery except: [:kerberos, :saml, :cas3] @@ -115,8 +116,10 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController if @user.persisted? && @user.valid? log_audit_event(@user, with: oauth['provider']) if @user.two_factor_enabled? + params[:remember_me] = '1' if remember_me? prompt_for_two_factor(@user) else + remember_me(@user) if remember_me? sign_in_and_redirect(@user) end else @@ -147,4 +150,9 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController AuditEventService.new(user, user, options) .for_authentication.security_event end + + def remember_me? + request_params = request.env['omniauth.params'] + request_params['remember_me'] == '1' + end end diff --git a/app/views/devise/shared/_omniauth_box.html.haml b/app/views/devise/shared/_omniauth_box.html.haml index f92f89e73ff..acb38c300b9 100644 --- a/app/views/devise/shared/_omniauth_box.html.haml +++ b/app/views/devise/shared/_omniauth_box.html.haml @@ -6,4 +6,21 @@ - providers.each do |provider| %span.light - has_icon = provider_has_icon?(provider) - = link_to provider_image_tag(provider), omniauth_authorize_path(:user, provider), method: :post, class: (has_icon ? 'oauth-image-link' : 'btn') + = link_to provider_image_tag(provider), omniauth_authorize_path(:user, provider), method: :post, class: 'oauth-login' + (has_icon ? ' oauth-image-link' : ' btn') + %fieldset + = check_box_tag :remember_me + = label_tag :remember_me, "Remember Me" + +:javascript + $("#remember_me").click(function(event){ + var rememberMe = $(event.target).is(":checked"); + $(".oauth-login").each(function(i, element) { + var href = $(element).attr('href'); + + if (rememberMe) { + $(element).attr('href', href + '?remember_me=1'); + } else { + $(element).attr('href', href.replace('?remember_me=1', '')); + } + }); + }); -- cgit v1.2.3 From e936db963e2adb549533cfedcac6f342d7e5e32e Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Wed, 14 Jun 2017 04:30:07 +0000 Subject: Add integration tests around OAuth login. - There was previously a test for `saml` login in `login_spec`, but this didn't seem to be passing. A lot of things didn't seem right here, and I suspect that this test hasn't been running. I'll investigate this further. - It took almost a whole working day to figure out this line: OmniAuth.config.full_host = ->(request) { request['REQUEST_URI'].sub(request['REQUEST_PATH'], '') } As always, it's obvious in retrospect, but it took some digging to figure out tests were failing and returning 404s during the callback phase. - Test all OAuth providers - github, twitter, bitbucket, gitlab, google, and facebook --- app/views/devise/shared/_omniauth_box.html.haml | 2 +- spec/features/oauth_login_spec.rb | 58 +++++++++++++++++++++++++ spec/support/login_helpers.rb | 7 +++ 3 files changed, 66 insertions(+), 1 deletion(-) create mode 100644 spec/features/oauth_login_spec.rb diff --git a/app/views/devise/shared/_omniauth_box.html.haml b/app/views/devise/shared/_omniauth_box.html.haml index acb38c300b9..e06b804e349 100644 --- a/app/views/devise/shared/_omniauth_box.html.haml +++ b/app/views/devise/shared/_omniauth_box.html.haml @@ -6,7 +6,7 @@ - providers.each do |provider| %span.light - has_icon = provider_has_icon?(provider) - = link_to provider_image_tag(provider), omniauth_authorize_path(:user, provider), method: :post, class: 'oauth-login' + (has_icon ? ' oauth-image-link' : ' btn') + = link_to provider_image_tag(provider), omniauth_authorize_path(:user, provider), method: :post, class: 'oauth-login' + (has_icon ? ' oauth-image-link' : ' btn'), id: "oauth-login-#{provider}" %fieldset = check_box_tag :remember_me = label_tag :remember_me, "Remember Me" diff --git a/spec/features/oauth_login_spec.rb b/spec/features/oauth_login_spec.rb new file mode 100644 index 00000000000..f960dacdcac --- /dev/null +++ b/spec/features/oauth_login_spec.rb @@ -0,0 +1,58 @@ +require 'spec_helper' + +feature 'OAuth Login', feature: true, js: true do + def enter_code(code) + fill_in 'user_otp_attempt', with: code + click_button 'Verify code' + end + + def provider_config(provider) + OpenStruct.new(name: provider.to_s, app_id: 'app_id', app_secret: 'app_secret') + end + + def stub_omniauth_config(provider) + OmniAuth.config.add_mock(provider, OmniAuth::AuthHash.new({ provider: provider.to_s, uid: "12345" })) + Rails.application.env_config['devise.mapping'] = Devise.mappings[:user] + Rails.application.env_config["omniauth.auth"] = OmniAuth.config.mock_auth[provider] + end + + providers = [:github, :twitter, :bitbucket, :gitlab, :google_oauth2, :facebook] + + before do + OmniAuth.config.full_host = ->(request) { request['REQUEST_URI'].sub(request['REQUEST_PATH'], '') } + + messages = { + enabled: true, + allow_single_sign_on: providers.map(&:to_s), + providers: providers.map { |provider| provider_config(provider) } + } + + allow(Gitlab.config.omniauth).to receive_messages(messages) + end + + providers.each do |provider| + context "when the user logs in using the #{provider} provider" do + context "when two-factor authentication is disabled" do + it 'logs the user in' do + stub_omniauth_config(provider) + user = create(:omniauth_user, extern_uid: 'my-uid', provider: provider.to_s) + login_via(provider.to_s, user, 'my-uid') + + expect(current_path).to eq root_path + save_screenshot + end + end + + context "when two-factor authentication is enabled" do + it 'logs the user in' do + stub_omniauth_config(provider) + user = create(:omniauth_user, :two_factor, extern_uid: 'my-uid', provider: provider.to_s) + login_via(provider.to_s, user, 'my-uid') + + enter_code(user.current_otp) + expect(current_path).to eq root_path + end + end + end + end +end diff --git a/spec/support/login_helpers.rb b/spec/support/login_helpers.rb index 4c88958264b..27f12cacc62 100644 --- a/spec/support/login_helpers.rb +++ b/spec/support/login_helpers.rb @@ -62,6 +62,13 @@ module LoginHelpers Thread.current[:current_user] = user end + def login_via(provider, user, uid) + mock_auth_hash(provider, uid, user.email) + visit new_user_session_path + expect(page).to have_content('Sign in with') + click_link "oauth-login-#{provider}" + end + def mock_auth_hash(provider, uid, email) # The mock_auth configuration allows you to set per-provider (or default) # authentication hashes to return during integration testing. -- cgit v1.2.3 From 43337c120de9f88b8141b0f8073bfa04a4e23776 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Thu, 15 Jun 2017 04:40:47 +0000 Subject: Test the "Remember Me" flow for OAuth-based login. --- spec/features/oauth_login_spec.rb | 61 +++++++++++++++++++++++++++++++++++++-- spec/support/capybara_helpers.rb | 5 ++++ spec/support/login_helpers.rb | 5 +++- 3 files changed, 68 insertions(+), 3 deletions(-) diff --git a/spec/features/oauth_login_spec.rb b/spec/features/oauth_login_spec.rb index f960dacdcac..2d51abd0e97 100644 --- a/spec/features/oauth_login_spec.rb +++ b/spec/features/oauth_login_spec.rb @@ -19,7 +19,7 @@ feature 'OAuth Login', feature: true, js: true do providers = [:github, :twitter, :bitbucket, :gitlab, :google_oauth2, :facebook] before do - OmniAuth.config.full_host = ->(request) { request['REQUEST_URI'].sub(request['REQUEST_PATH'], '') } + OmniAuth.config.full_host = ->(request) { request['REQUEST_URI'].sub(/#{request['REQUEST_PATH']}.*/, '') } messages = { enabled: true, @@ -39,7 +39,6 @@ feature 'OAuth Login', feature: true, js: true do login_via(provider.to_s, user, 'my-uid') expect(current_path).to eq root_path - save_screenshot end end @@ -53,6 +52,64 @@ feature 'OAuth Login', feature: true, js: true do expect(current_path).to eq root_path end end + + context 'when "remember me" is checked' do + context "when two-factor authentication is disabled" do + it 'remembers the user after a browser restart' do + stub_omniauth_config(provider) + user = create(:omniauth_user, extern_uid: 'my-uid', provider: provider.to_s) + login_via(provider.to_s, user, 'my-uid', remember_me: true) + + restart_browser + + visit(root_path) + expect(current_path).to eq root_path + end + end + + context "when two-factor authentication is enabled" do + it 'remembers the user after a browser restart' do + stub_omniauth_config(provider) + user = create(:omniauth_user, :two_factor, extern_uid: 'my-uid', provider: provider.to_s) + login_via(provider.to_s, user, 'my-uid', remember_me: true) + enter_code(user.current_otp) + + restart_browser + + visit(root_path) + expect(current_path).to eq root_path + end + end + end + + context 'when "remember me" is not checked' do + context "when two-factor authentication is disabled" do + it 'does not remember the user after a browser restart' do + stub_omniauth_config(provider) + user = create(:omniauth_user, extern_uid: 'my-uid', provider: provider.to_s) + login_via(provider.to_s, user, 'my-uid', remember_me: false) + + restart_browser + + visit(root_path) + expect(current_path).to eq new_user_session_path + end + end + + context "when two-factor authentication is enabled" do + it 'remembers the user after a browser restart' do + stub_omniauth_config(provider) + user = create(:omniauth_user, :two_factor, extern_uid: 'my-uid', provider: provider.to_s) + login_via(provider.to_s, user, 'my-uid', remember_me: false) + enter_code(user.current_otp) + + restart_browser + + visit(root_path) + expect(current_path).to eq new_user_session_path + end + end + end end end end diff --git a/spec/support/capybara_helpers.rb b/spec/support/capybara_helpers.rb index b57a3493aff..1037e9def8c 100644 --- a/spec/support/capybara_helpers.rb +++ b/spec/support/capybara_helpers.rb @@ -35,6 +35,11 @@ module CapybaraHelpers visit 'about:blank' visit url end + + # Simulate a browser restart by clearing the session cookie. + def restart_browser + page.driver.remove_cookie('_gitlab_session') + end end RSpec.configure do |config| diff --git a/spec/support/login_helpers.rb b/spec/support/login_helpers.rb index 27f12cacc62..789cf9baae2 100644 --- a/spec/support/login_helpers.rb +++ b/spec/support/login_helpers.rb @@ -62,10 +62,13 @@ module LoginHelpers Thread.current[:current_user] = user end - def login_via(provider, user, uid) + def login_via(provider, user, uid, remember_me: false) mock_auth_hash(provider, uid, user.email) visit new_user_session_path expect(page).to have_content('Sign in with') + + check "Remember Me" if remember_me + click_link "oauth-login-#{provider}" end -- cgit v1.2.3 From d705a2548ceebe0fc63356e3e5b0afc54a109d9f Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Thu, 15 Jun 2017 12:13:30 +0000 Subject: Move OAuth "remember me" javascript logic into a class. --- app/assets/javascripts/dispatcher.js | 2 ++ app/assets/javascripts/oauth_remember_me.js | 31 +++++++++++++++++++++++++ app/views/devise/shared/_omniauth_box.html.haml | 14 ----------- 3 files changed, 33 insertions(+), 14 deletions(-) create mode 100644 app/assets/javascripts/oauth_remember_me.js diff --git a/app/assets/javascripts/dispatcher.js b/app/assets/javascripts/dispatcher.js index 4247540de22..a58d1be68b5 100644 --- a/app/assets/javascripts/dispatcher.js +++ b/app/assets/javascripts/dispatcher.js @@ -56,6 +56,7 @@ import GfmAutoComplete from './gfm_auto_complete'; import ShortcutsBlob from './shortcuts_blob'; import initSettingsPanels from './settings_panels'; import initExperimentalFlags from './experimental_flags'; +import OAuthRememberMe from './oauth_remember_me'; (function() { var Dispatcher; @@ -127,6 +128,7 @@ import initExperimentalFlags from './experimental_flags'; case 'sessions:new': new UsernameValidator(); new ActiveTabMemoizer(); + new OAuthRememberMe({ container: $("#remember_me") }).bindEvents(); break; case 'projects:boards:show': case 'projects:boards:index': diff --git a/app/assets/javascripts/oauth_remember_me.js b/app/assets/javascripts/oauth_remember_me.js new file mode 100644 index 00000000000..3048f6fec90 --- /dev/null +++ b/app/assets/javascripts/oauth_remember_me.js @@ -0,0 +1,31 @@ +/** + * OAuth-based login buttons have a separate "remember me" checkbox. + * + * Toggling this checkbox adds/removes a `remember_me` parameter to the + * login buttons' href, which is passed on to the omniauth callback. + **/ + +export default class OAuthRememberMe { + constructor(opts = {}) { + this.container = opts.container || ''; + this.loginLinkSelector = '.oauth-login'; + } + + bindEvents() { + this.container.on('click', this.toggleRememberMe); + } + + toggleRememberMe(event) { + var rememberMe = $(event.target).is(":checked"); + + $('.oauth-login').each(function(i, element) { + var href = $(element).attr('href'); + + if (rememberMe) { + $(element).attr('href', href + '?remember_me=1'); + } else { + $(element).attr('href', href.replace('?remember_me=1', '')); + } + }); + } +} diff --git a/app/views/devise/shared/_omniauth_box.html.haml b/app/views/devise/shared/_omniauth_box.html.haml index e06b804e349..493e18565c0 100644 --- a/app/views/devise/shared/_omniauth_box.html.haml +++ b/app/views/devise/shared/_omniauth_box.html.haml @@ -10,17 +10,3 @@ %fieldset = check_box_tag :remember_me = label_tag :remember_me, "Remember Me" - -:javascript - $("#remember_me").click(function(event){ - var rememberMe = $(event.target).is(":checked"); - $(".oauth-login").each(function(i, element) { - var href = $(element).attr('href'); - - if (rememberMe) { - $(element).attr('href', href + '?remember_me=1'); - } else { - $(element).attr('href', href.replace('?remember_me=1', '')); - } - }); - }); -- cgit v1.2.3 From fd94855893b96ccab2227330ffd3134a92f4cb45 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Mon, 19 Jun 2017 04:40:24 +0000 Subject: Add more providers to the OAuth login integration tests. - Added saml, authentiq, cas3, and auth0 - Crowd seems to be a special case that will be handled separately. --- spec/features/oauth_login_spec.rb | 43 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 41 insertions(+), 2 deletions(-) diff --git a/spec/features/oauth_login_spec.rb b/spec/features/oauth_login_spec.rb index 2d51abd0e97..b37c14bd638 100644 --- a/spec/features/oauth_login_spec.rb +++ b/spec/features/oauth_login_spec.rb @@ -7,7 +7,20 @@ feature 'OAuth Login', feature: true, js: true do end def provider_config(provider) - OpenStruct.new(name: provider.to_s, app_id: 'app_id', app_secret: 'app_secret') + if provider == :saml + OpenStruct.new( + name: 'saml', label: 'saml', + args: { + assertion_consumer_service_url: 'https://localhost:3443/users/auth/saml/callback', + idp_cert_fingerprint: '26:43:2C:47:AF:F0:6B:D0:07:9C:AD:A3:74:FE:5D:94:5F:4E:9E:52', + idp_sso_target_url: 'https://idp.example.com/sso/saml', + issuer: 'https://localhost:3443/', + name_identifier_format: 'urn:oasis:names:tc:SAML:2.0:nameid-format:transient' + } + ) + else + OpenStruct.new(name: provider.to_s, app_id: 'app_id', app_secret: 'app_secret') + end end def stub_omniauth_config(provider) @@ -16,7 +29,8 @@ feature 'OAuth Login', feature: true, js: true do Rails.application.env_config["omniauth.auth"] = OmniAuth.config.mock_auth[provider] end - providers = [:github, :twitter, :bitbucket, :gitlab, :google_oauth2, :facebook] + providers = [:github, :twitter, :bitbucket, :gitlab, :google_oauth2, + :facebook, :authentiq, :cas3, :auth0] before do OmniAuth.config.full_host = ->(request) { request['REQUEST_URI'].sub(/#{request['REQUEST_PATH']}.*/, '') } @@ -24,12 +38,37 @@ feature 'OAuth Login', feature: true, js: true do messages = { enabled: true, allow_single_sign_on: providers.map(&:to_s), + auto_link_saml_user: true, providers: providers.map { |provider| provider_config(provider) } } allow(Gitlab.config.omniauth).to receive_messages(messages) end + # context 'logging in via OAuth' do + # def saml_config + + # end + # def stub_omniauth_config(messages) + # Rails.application.env_config['devise.mapping'] = Devise.mappings[:user] + # Rails.application.routes.disable_clear_and_finalize = true + # Rails.application.routes.draw do + # post '/users/auth/saml' => 'omniauth_callbacks#saml' + # end + # allow(Gitlab::OAuth::Provider).to receive_messages(providers: [:saml], config_for: saml_config) + # allow(Gitlab.config.omniauth).to receive_messages(messages) + # expect_any_instance_of(Object).to receive(:omniauth_authorize_path).with(:user, "saml").and_return('/users/auth/saml') + # end + # it 'shows 2FA prompt after OAuth login' do + # stub_omniauth_config(enabled: true, auto_link_saml_user: true, allow_single_sign_on: ['saml'], providers: [saml_config]) + # user = create(:omniauth_user, :two_factor, extern_uid: 'my-uid', provider: 'saml') + # login_via('saml', user, 'my-uid') + # expect(page).to have_content('Two-Factor Authentication') + # enter_code(user.current_otp) + # expect(current_path).to eq root_path + # end + # end + providers.each do |provider| context "when the user logs in using the #{provider} provider" do context "when two-factor authentication is disabled" do -- cgit v1.2.3 From 15dba34c9a469c95ea6112419dca33c2c63c6247 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Mon, 19 Jun 2017 07:55:09 +0000 Subject: Add Omniauth OAuth config to the test section of `gitlab.yml` - I tried to get this to work by stubbing out portions of the config within the test. This didn't work as expected because Devise/Omniauth loaded before the stub could run, and the stubbed config was ignored. - I attempted to fix this by reloading Devise/Omniauth after stubbing the config. This successfully got Devise to load the stubbed providers, but failed while trying to access a route such as `user_gitlab_omniauth_authorize_path`. - I spent a while trying to figure this out (even trying `Rails.application.reload_routes!`), but nothing seemed to work. - I settled for adding this config directly to `gitlab.yml` rather than go down this path any further. --- config/gitlab.yml.example | 66 +++++++++++++++++++++++++++++++++++++++ spec/features/oauth_login_spec.rb | 52 +----------------------------- 2 files changed, 67 insertions(+), 51 deletions(-) diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example index 43a8c0078ca..b58a173bccb 100644 --- a/config/gitlab.yml.example +++ b/config/gitlab.yml.example @@ -615,6 +615,72 @@ test: title: "JIRA" url: https://sample_company.atlassian.net project_key: PROJECT + + omniauth: + enabled: true + allow_single_sign_on: true + block_auto_created_users: false + auto_link_saml_user: true + external_providers: [] + + providers: + - { name: 'cas3', + label: 'cas3', + args: { + url: 'https://sso.example.com', + disable_ssl_verification: false, + login_url: '/cas/login', + service_validate_url: '/cas/p3/serviceValidate', + logout_url: '/cas/logout'} } + - { name: 'authentiq', + app_id: 'YOUR_CLIENT_ID', + app_secret: 'YOUR_CLIENT_SECRET', + args: { + scope: 'aq:name email~rs address aq:push' + } + } + + - { name: 'github', + app_id: 'YOUR_APP_ID', + app_secret: 'YOUR_APP_SECRET', + url: "https://github.com/", + verify_ssl: false, + args: { scope: 'user:email' } } + - { name: 'bitbucket', + app_id: 'YOUR_APP_ID', + app_secret: 'YOUR_APP_SECRET' } + - { name: 'gitlab', + app_id: 'YOUR_APP_ID', + app_secret: 'YOUR_APP_SECRET', + args: { scope: 'api' } } + - { name: 'google_oauth2', + app_id: 'YOUR_APP_ID', + app_secret: 'YOUR_APP_SECRET', + args: { access_type: 'offline', approval_prompt: '' } } + - { name: 'facebook', + app_id: 'YOUR_APP_ID', + app_secret: 'YOUR_APP_SECRET' } + - { name: 'twitter', + app_id: 'YOUR_APP_ID', + app_secret: 'YOUR_APP_SECRET' } + + - { name: 'saml', + label: 'Our SAML Provider', + groups_attribute: 'Groups', + external_groups: ['Contractors', 'Freelancers'], + args: { + assertion_consumer_service_url: 'https://gitlab.example.com/users/auth/saml/callback', + idp_cert_fingerprint: '43:51:43:a1:b5:fc:8b:b7:0a:3a:a9:b1:0f:66:73:a8', + idp_sso_target_url: 'https://login.example.com/idp', + issuer: 'https://gitlab.example.com', + name_identifier_format: 'urn:oasis:names:tc:SAML:2.0:nameid-format:transient' + } } + + - { name: 'auth0', + args: { + client_id: 'YOUR_AUTH0_CLIENT_ID', + client_secret: 'YOUR_AUTH0_CLIENT_SECRET', + namespace: 'YOUR_AUTH0_DOMAIN' } } ldap: enabled: false servers: diff --git a/spec/features/oauth_login_spec.rb b/spec/features/oauth_login_spec.rb index b37c14bd638..8e02bc88fad 100644 --- a/spec/features/oauth_login_spec.rb +++ b/spec/features/oauth_login_spec.rb @@ -6,23 +6,6 @@ feature 'OAuth Login', feature: true, js: true do click_button 'Verify code' end - def provider_config(provider) - if provider == :saml - OpenStruct.new( - name: 'saml', label: 'saml', - args: { - assertion_consumer_service_url: 'https://localhost:3443/users/auth/saml/callback', - idp_cert_fingerprint: '26:43:2C:47:AF:F0:6B:D0:07:9C:AD:A3:74:FE:5D:94:5F:4E:9E:52', - idp_sso_target_url: 'https://idp.example.com/sso/saml', - issuer: 'https://localhost:3443/', - name_identifier_format: 'urn:oasis:names:tc:SAML:2.0:nameid-format:transient' - } - ) - else - OpenStruct.new(name: provider.to_s, app_id: 'app_id', app_secret: 'app_secret') - end - end - def stub_omniauth_config(provider) OmniAuth.config.add_mock(provider, OmniAuth::AuthHash.new({ provider: provider.to_s, uid: "12345" })) Rails.application.env_config['devise.mapping'] = Devise.mappings[:user] @@ -32,43 +15,10 @@ feature 'OAuth Login', feature: true, js: true do providers = [:github, :twitter, :bitbucket, :gitlab, :google_oauth2, :facebook, :authentiq, :cas3, :auth0] - before do + before(:all) do OmniAuth.config.full_host = ->(request) { request['REQUEST_URI'].sub(/#{request['REQUEST_PATH']}.*/, '') } - - messages = { - enabled: true, - allow_single_sign_on: providers.map(&:to_s), - auto_link_saml_user: true, - providers: providers.map { |provider| provider_config(provider) } - } - - allow(Gitlab.config.omniauth).to receive_messages(messages) end - # context 'logging in via OAuth' do - # def saml_config - - # end - # def stub_omniauth_config(messages) - # Rails.application.env_config['devise.mapping'] = Devise.mappings[:user] - # Rails.application.routes.disable_clear_and_finalize = true - # Rails.application.routes.draw do - # post '/users/auth/saml' => 'omniauth_callbacks#saml' - # end - # allow(Gitlab::OAuth::Provider).to receive_messages(providers: [:saml], config_for: saml_config) - # allow(Gitlab.config.omniauth).to receive_messages(messages) - # expect_any_instance_of(Object).to receive(:omniauth_authorize_path).with(:user, "saml").and_return('/users/auth/saml') - # end - # it 'shows 2FA prompt after OAuth login' do - # stub_omniauth_config(enabled: true, auto_link_saml_user: true, allow_single_sign_on: ['saml'], providers: [saml_config]) - # user = create(:omniauth_user, :two_factor, extern_uid: 'my-uid', provider: 'saml') - # login_via('saml', user, 'my-uid') - # expect(page).to have_content('Two-Factor Authentication') - # enter_code(user.current_otp) - # expect(current_path).to eq root_path - # end - # end - providers.each do |provider| context "when the user logs in using the #{provider} provider" do context "when two-factor authentication is disabled" do -- cgit v1.2.3 From 738789d2cbe04453f96fa86b6f46d9d8213a271b Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Mon, 19 Jun 2017 13:10:34 +0000 Subject: Get ESLint spec passing for the `OAuthRememberMe` class. --- app/assets/javascripts/oauth_remember_me.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/app/assets/javascripts/oauth_remember_me.js b/app/assets/javascripts/oauth_remember_me.js index 3048f6fec90..8f4796f2ede 100644 --- a/app/assets/javascripts/oauth_remember_me.js +++ b/app/assets/javascripts/oauth_remember_me.js @@ -12,17 +12,17 @@ export default class OAuthRememberMe { } bindEvents() { - this.container.on('click', this.toggleRememberMe); + this.container.on('click', this.constructor.toggleRememberMe); } - toggleRememberMe(event) { - var rememberMe = $(event.target).is(":checked"); + static toggleRememberMe(event) { + const rememberMe = $(event.target).is(':checked'); - $('.oauth-login').each(function(i, element) { - var href = $(element).attr('href'); + $('.oauth-login').each((i, element) => { + const href = $(element).attr('href'); if (rememberMe) { - $(element).attr('href', href + '?remember_me=1'); + $(element).attr('href', `${href}?remember_me=1`); } else { $(element).attr('href', href.replace('?remember_me=1', '')); } -- cgit v1.2.3 From 1e9dfb73992e7586c4ee0503357df31925be593f Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Mon, 19 Jun 2017 13:15:01 +0000 Subject: Add CHANGELOG entry for CE MR 11963 --- changelogs/unreleased/18000-remember-me-for-oauth-login.yml | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changelogs/unreleased/18000-remember-me-for-oauth-login.yml diff --git a/changelogs/unreleased/18000-remember-me-for-oauth-login.yml b/changelogs/unreleased/18000-remember-me-for-oauth-login.yml new file mode 100644 index 00000000000..1ef92756a76 --- /dev/null +++ b/changelogs/unreleased/18000-remember-me-for-oauth-login.yml @@ -0,0 +1,4 @@ +--- +title: Honor the "Remember me" parameter for OAuth-based login +merge_request: 11963 +author: -- cgit v1.2.3 From 56754848dddb5460500ab056e5ac1b9a61c7ff89 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Tue, 20 Jun 2017 03:11:16 +0000 Subject: Don't allow the `gitlab:env:info` rake task to mutate the list of omniauth providers. - The test for `rake gitlab:env:info` executed the rake task, which mutated the list of omniauth providers, breaking subsequent tests relying on this list. - I've changed the rake task to duplicate the providers list before modifying it. --- lib/tasks/gitlab/info.rake | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/tasks/gitlab/info.rake b/lib/tasks/gitlab/info.rake index e3883278886..2245dfb7828 100644 --- a/lib/tasks/gitlab/info.rake +++ b/lib/tasks/gitlab/info.rake @@ -42,7 +42,7 @@ namespace :gitlab do http_clone_url = project.http_url_to_repo ssh_clone_url = project.ssh_url_to_repo - omniauth_providers = Gitlab.config.omniauth.providers + omniauth_providers = Gitlab.config.omniauth.providers.dup omniauth_providers.map! { |provider| provider['name'] } puts "" -- cgit v1.2.3 From 8fa08ea3cd81e906c4f4951c70e3571defeab7c7 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Fri, 30 Jun 2017 16:36:36 +0000 Subject: Implement review comments for !11963 from @adamniedzielski. - Change double quotes to single quotes. - Why is `OmniAuth.config.full_host` being reassigned in the integration test? - Use `map` over `map!` to avoid `dup` in the `gitlab:info` rake task - Other minor changes --- app/views/devise/shared/_omniauth_box.html.haml | 2 +- lib/tasks/gitlab/info.rake | 3 +-- spec/features/oauth_login_spec.rb | 36 +++++++++++++++---------- spec/support/capybara_helpers.rb | 2 +- spec/support/login_helpers.rb | 2 +- 5 files changed, 26 insertions(+), 19 deletions(-) diff --git a/app/views/devise/shared/_omniauth_box.html.haml b/app/views/devise/shared/_omniauth_box.html.haml index 493e18565c0..e80d10dc8f1 100644 --- a/app/views/devise/shared/_omniauth_box.html.haml +++ b/app/views/devise/shared/_omniauth_box.html.haml @@ -9,4 +9,4 @@ = link_to provider_image_tag(provider), omniauth_authorize_path(:user, provider), method: :post, class: 'oauth-login' + (has_icon ? ' oauth-image-link' : ' btn'), id: "oauth-login-#{provider}" %fieldset = check_box_tag :remember_me - = label_tag :remember_me, "Remember Me" + = label_tag :remember_me, 'Remember Me' diff --git a/lib/tasks/gitlab/info.rake b/lib/tasks/gitlab/info.rake index 2245dfb7828..e9fb6a008b0 100644 --- a/lib/tasks/gitlab/info.rake +++ b/lib/tasks/gitlab/info.rake @@ -42,8 +42,7 @@ namespace :gitlab do http_clone_url = project.http_url_to_repo ssh_clone_url = project.ssh_url_to_repo - omniauth_providers = Gitlab.config.omniauth.providers.dup - omniauth_providers.map! { |provider| provider['name'] } + omniauth_providers = Gitlab.config.omniauth.providers.map { |provider| provider['name'] } puts "" puts "GitLab information".color(:yellow) diff --git a/spec/features/oauth_login_spec.rb b/spec/features/oauth_login_spec.rb index 8e02bc88fad..452b920307c 100644 --- a/spec/features/oauth_login_spec.rb +++ b/spec/features/oauth_login_spec.rb @@ -1,27 +1,35 @@ require 'spec_helper' -feature 'OAuth Login', feature: true, js: true do +feature 'OAuth Login', js: true do def enter_code(code) fill_in 'user_otp_attempt', with: code click_button 'Verify code' end def stub_omniauth_config(provider) - OmniAuth.config.add_mock(provider, OmniAuth::AuthHash.new({ provider: provider.to_s, uid: "12345" })) + OmniAuth.config.add_mock(provider, OmniAuth::AuthHash.new(provider: provider.to_s, uid: "12345")) Rails.application.env_config['devise.mapping'] = Devise.mappings[:user] - Rails.application.env_config["omniauth.auth"] = OmniAuth.config.mock_auth[provider] + Rails.application.env_config['omniauth.auth'] = OmniAuth.config.mock_auth[provider] end providers = [:github, :twitter, :bitbucket, :gitlab, :google_oauth2, :facebook, :authentiq, :cas3, :auth0] before(:all) do + # The OmniAuth `full_host` parameter doesn't get set correctly (it gets set to something like `http://localhost` + # here), and causes integration tests to fail with 404s. We set the `full_host` by removing the request path (and + # anything after it) from the request URI. + @omniauth_config_full_host = OmniAuth.config.full_host OmniAuth.config.full_host = ->(request) { request['REQUEST_URI'].sub(/#{request['REQUEST_PATH']}.*/, '') } end + after(:all) do + OmniAuth.config.full_host = @omniauth_config_full_host + end + providers.each do |provider| context "when the user logs in using the #{provider} provider" do - context "when two-factor authentication is disabled" do + context 'when two-factor authentication is disabled' do it 'logs the user in' do stub_omniauth_config(provider) user = create(:omniauth_user, extern_uid: 'my-uid', provider: provider.to_s) @@ -31,7 +39,7 @@ feature 'OAuth Login', feature: true, js: true do end end - context "when two-factor authentication is enabled" do + context 'when two-factor authentication is enabled' do it 'logs the user in' do stub_omniauth_config(provider) user = create(:omniauth_user, :two_factor, extern_uid: 'my-uid', provider: provider.to_s) @@ -43,27 +51,27 @@ feature 'OAuth Login', feature: true, js: true do end context 'when "remember me" is checked' do - context "when two-factor authentication is disabled" do + context 'when two-factor authentication is disabled' do it 'remembers the user after a browser restart' do stub_omniauth_config(provider) user = create(:omniauth_user, extern_uid: 'my-uid', provider: provider.to_s) login_via(provider.to_s, user, 'my-uid', remember_me: true) - restart_browser + clear_browser_session visit(root_path) expect(current_path).to eq root_path end end - context "when two-factor authentication is enabled" do + context 'when two-factor authentication is enabled' do it 'remembers the user after a browser restart' do stub_omniauth_config(provider) user = create(:omniauth_user, :two_factor, extern_uid: 'my-uid', provider: provider.to_s) login_via(provider.to_s, user, 'my-uid', remember_me: true) enter_code(user.current_otp) - restart_browser + clear_browser_session visit(root_path) expect(current_path).to eq root_path @@ -72,27 +80,27 @@ feature 'OAuth Login', feature: true, js: true do end context 'when "remember me" is not checked' do - context "when two-factor authentication is disabled" do + context 'when two-factor authentication is disabled' do it 'does not remember the user after a browser restart' do stub_omniauth_config(provider) user = create(:omniauth_user, extern_uid: 'my-uid', provider: provider.to_s) login_via(provider.to_s, user, 'my-uid', remember_me: false) - restart_browser + clear_browser_session visit(root_path) expect(current_path).to eq new_user_session_path end end - context "when two-factor authentication is enabled" do - it 'remembers the user after a browser restart' do + context 'when two-factor authentication is enabled' do + it 'does not remember the user after a browser restart' do stub_omniauth_config(provider) user = create(:omniauth_user, :two_factor, extern_uid: 'my-uid', provider: provider.to_s) login_via(provider.to_s, user, 'my-uid', remember_me: false) enter_code(user.current_otp) - restart_browser + clear_browser_session visit(root_path) expect(current_path).to eq new_user_session_path diff --git a/spec/support/capybara_helpers.rb b/spec/support/capybara_helpers.rb index 1037e9def8c..3eb7bea3227 100644 --- a/spec/support/capybara_helpers.rb +++ b/spec/support/capybara_helpers.rb @@ -37,7 +37,7 @@ module CapybaraHelpers end # Simulate a browser restart by clearing the session cookie. - def restart_browser + def clear_browser_session page.driver.remove_cookie('_gitlab_session') end end diff --git a/spec/support/login_helpers.rb b/spec/support/login_helpers.rb index 789cf9baae2..184c7b5125a 100644 --- a/spec/support/login_helpers.rb +++ b/spec/support/login_helpers.rb @@ -67,7 +67,7 @@ module LoginHelpers visit new_user_session_path expect(page).to have_content('Sign in with') - check "Remember Me" if remember_me + check 'Remember Me' if remember_me click_link "oauth-login-#{provider}" end -- cgit v1.2.3 From f1caa0b316c0be7c957e34a4bcc9f392023379d3 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Mon, 3 Jul 2017 16:23:28 +0000 Subject: Implement review comments for !11963 from @filipa. - Disable an ESLint check rather than work around it (by converting `OAuthRememberMe` from a regular class to a static class. - Scope `$` calls inside `OAuthRememberMe` --- app/assets/javascripts/dispatcher.js | 2 +- app/assets/javascripts/oauth_remember_me.js | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/app/assets/javascripts/dispatcher.js b/app/assets/javascripts/dispatcher.js index a58d1be68b5..e924fde60bf 100644 --- a/app/assets/javascripts/dispatcher.js +++ b/app/assets/javascripts/dispatcher.js @@ -128,7 +128,7 @@ import OAuthRememberMe from './oauth_remember_me'; case 'sessions:new': new UsernameValidator(); new ActiveTabMemoizer(); - new OAuthRememberMe({ container: $("#remember_me") }).bindEvents(); + new OAuthRememberMe({ container: $(".omniauth-container") }).bindEvents(); break; case 'projects:boards:show': case 'projects:boards:index': diff --git a/app/assets/javascripts/oauth_remember_me.js b/app/assets/javascripts/oauth_remember_me.js index 8f4796f2ede..ffc2dd6bbca 100644 --- a/app/assets/javascripts/oauth_remember_me.js +++ b/app/assets/javascripts/oauth_remember_me.js @@ -12,13 +12,14 @@ export default class OAuthRememberMe { } bindEvents() { - this.container.on('click', this.constructor.toggleRememberMe); + $('#remember_me', this.container).on('click', this.toggleRememberMe); } - static toggleRememberMe(event) { + // eslint-disable-next-line class-methods-use-this + toggleRememberMe(event) { const rememberMe = $(event.target).is(':checked'); - $('.oauth-login').each((i, element) => { + $('.oauth-login', this.container).each((i, element) => { const href = $(element).attr('href'); if (rememberMe) { -- cgit v1.2.3 From 7c2f5bb48d98426b8458782216311f24aa705209 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Mon, 3 Jul 2017 19:37:37 +0000 Subject: Fix build for !11963. - Don't use `request.env['omniauth.params']` if it isn't present. - Remove the `saml` section from the `gitlab.yml` test section. Some tests depend on this section not being initially present, so it can be overridden in the test. This MR doesn't add any tests for SAML, so we didn't really need this in the first place anyway. - Clean up the test -> omniauth section of `gitlab.yml` --- app/controllers/omniauth_callbacks_controller.rb | 2 +- config/gitlab.yml.example | 25 +++--------------------- spec/support/login_helpers.rb | 3 ++- 3 files changed, 6 insertions(+), 24 deletions(-) diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb index c5adadfa529..323d5d26eb6 100644 --- a/app/controllers/omniauth_callbacks_controller.rb +++ b/app/controllers/omniauth_callbacks_controller.rb @@ -153,6 +153,6 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController def remember_me? request_params = request.env['omniauth.params'] - request_params['remember_me'] == '1' + (request_params['remember_me'] == '1') if request_params.present? end end diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example index b58a173bccb..950a58bb0dd 100644 --- a/config/gitlab.yml.example +++ b/config/gitlab.yml.example @@ -619,15 +619,12 @@ test: omniauth: enabled: true allow_single_sign_on: true - block_auto_created_users: false - auto_link_saml_user: true external_providers: [] providers: - { name: 'cas3', label: 'cas3', - args: { - url: 'https://sso.example.com', + args: { url: 'https://sso.example.com', disable_ssl_verification: false, login_url: '/cas/login', service_validate_url: '/cas/p3/serviceValidate', @@ -635,11 +632,7 @@ test: - { name: 'authentiq', app_id: 'YOUR_CLIENT_ID', app_secret: 'YOUR_CLIENT_SECRET', - args: { - scope: 'aq:name email~rs address aq:push' - } - } - + args: { scope: 'aq:name email~rs address aq:push' } } - { name: 'github', app_id: 'YOUR_APP_ID', app_secret: 'YOUR_APP_SECRET', @@ -663,24 +656,12 @@ test: - { name: 'twitter', app_id: 'YOUR_APP_ID', app_secret: 'YOUR_APP_SECRET' } - - - { name: 'saml', - label: 'Our SAML Provider', - groups_attribute: 'Groups', - external_groups: ['Contractors', 'Freelancers'], - args: { - assertion_consumer_service_url: 'https://gitlab.example.com/users/auth/saml/callback', - idp_cert_fingerprint: '43:51:43:a1:b5:fc:8b:b7:0a:3a:a9:b1:0f:66:73:a8', - idp_sso_target_url: 'https://login.example.com/idp', - issuer: 'https://gitlab.example.com', - name_identifier_format: 'urn:oasis:names:tc:SAML:2.0:nameid-format:transient' - } } - - { name: 'auth0', args: { client_id: 'YOUR_AUTH0_CLIENT_ID', client_secret: 'YOUR_AUTH0_CLIENT_SECRET', namespace: 'YOUR_AUTH0_DOMAIN' } } + ldap: enabled: false servers: diff --git a/spec/support/login_helpers.rb b/spec/support/login_helpers.rb index 184c7b5125a..99e7806353d 100644 --- a/spec/support/login_helpers.rb +++ b/spec/support/login_helpers.rb @@ -118,6 +118,7 @@ module LoginHelpers end allow(Gitlab::OAuth::Provider).to receive_messages(providers: [:saml], config_for: mock_saml_config) stub_omniauth_setting(messages) - expect_any_instance_of(Object).to receive(:omniauth_authorize_path).with(:user, "saml").and_return('/users/auth/saml') + allow_any_instance_of(Object).to receive(:user_saml_omniauth_authorize_path).and_return('/users/auth/saml') + allow_any_instance_of(Object).to receive(:omniauth_authorize_path).with(:user, "saml").and_return('/users/auth/saml') end end -- cgit v1.2.3 From 8d49a9fd580212b12bd719e6b310646e285c36b7 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Tue, 4 Jul 2017 11:59:41 +0000 Subject: Add Jasmine tests for `OAuthRememberMe` --- .../fixtures/oauth_remember_me.html.haml | 5 +++++ spec/javascripts/oauth_remember_me_spec.js | 26 ++++++++++++++++++++++ 2 files changed, 31 insertions(+) create mode 100644 spec/javascripts/fixtures/oauth_remember_me.html.haml create mode 100644 spec/javascripts/oauth_remember_me_spec.js diff --git a/spec/javascripts/fixtures/oauth_remember_me.html.haml b/spec/javascripts/fixtures/oauth_remember_me.html.haml new file mode 100644 index 00000000000..7886e995e57 --- /dev/null +++ b/spec/javascripts/fixtures/oauth_remember_me.html.haml @@ -0,0 +1,5 @@ +#oauth-container + %input#remember_me{ type: "checkbox" } + + %a.oauth-login.twitter{ href: "http://example.com/" } + %a.oauth-login.github{ href: "http://example.com/" } diff --git a/spec/javascripts/oauth_remember_me_spec.js b/spec/javascripts/oauth_remember_me_spec.js new file mode 100644 index 00000000000..f90e0093d25 --- /dev/null +++ b/spec/javascripts/oauth_remember_me_spec.js @@ -0,0 +1,26 @@ +import OAuthRememberMe from '~/oauth_remember_me'; + +describe('OAuthRememberMe', () => { + preloadFixtures('static/oauth_remember_me.html.raw'); + + beforeEach(() => { + loadFixtures('static/oauth_remember_me.html.raw'); + + new OAuthRememberMe({ container: $('#oauth-container') }).bindEvents(); + }); + + it('adds the "remember_me" query parameter to all OAuth login buttons', () => { + $('#oauth-container #remember_me').click(); + + expect($('#oauth-container .oauth-login.twitter').attr('href')).toBe('http://example.com/?remember_me=1'); + expect($('#oauth-container .oauth-login.github').attr('href')).toBe('http://example.com/?remember_me=1'); + }); + + it('removes the "remember_me" query parameter from all OAuth login buttons', () => { + $('#oauth-container #remember_me').click(); + $('#oauth-container #remember_me').click(); + + expect($('#oauth-container .oauth-login.twitter').attr('href')).toBe('http://example.com/'); + expect($('#oauth-container .oauth-login.github').attr('href')).toBe('http://example.com/'); + }); +}); -- cgit v1.2.3 From 89b0c987fcba5692842f83cfaba90a9004ac91de Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Thu, 6 Jul 2017 06:23:20 +0000 Subject: Remove Authentiq from the OAuth login integration tests. - This is causing autoload-related errors in the `migration:path` builds. We need to find a better way of testing this provider. --- config/gitlab.yml.example | 4 ---- spec/features/oauth_login_spec.rb | 2 +- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example index 950a58bb0dd..bdf8bcf4931 100644 --- a/config/gitlab.yml.example +++ b/config/gitlab.yml.example @@ -629,10 +629,6 @@ test: login_url: '/cas/login', service_validate_url: '/cas/p3/serviceValidate', logout_url: '/cas/logout'} } - - { name: 'authentiq', - app_id: 'YOUR_CLIENT_ID', - app_secret: 'YOUR_CLIENT_SECRET', - args: { scope: 'aq:name email~rs address aq:push' } } - { name: 'github', app_id: 'YOUR_APP_ID', app_secret: 'YOUR_APP_SECRET', diff --git a/spec/features/oauth_login_spec.rb b/spec/features/oauth_login_spec.rb index 452b920307c..1b6d1f3415f 100644 --- a/spec/features/oauth_login_spec.rb +++ b/spec/features/oauth_login_spec.rb @@ -13,7 +13,7 @@ feature 'OAuth Login', js: true do end providers = [:github, :twitter, :bitbucket, :gitlab, :google_oauth2, - :facebook, :authentiq, :cas3, :auth0] + :facebook, :cas3, :auth0] before(:all) do # The OmniAuth `full_host` parameter doesn't get set correctly (it gets set to something like `http://localhost` -- cgit v1.2.3 From e7acc88156116bbfc20d13b5d897492cc415ee38 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Thu, 6 Jul 2017 15:55:40 +0800 Subject: Rename KUBECONFIG_FILE to KUBECONFIG --- app/models/project_services/kubernetes_service.rb | 2 +- changelogs/unreleased/33360-generate-kubeconfig.yml | 2 +- doc/user/project/integrations/kubernetes.md | 2 +- spec/models/project_services/kubernetes_service_spec.rb | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/models/project_services/kubernetes_service.rb b/app/models/project_services/kubernetes_service.rb index 831f4e5a3c8..62f7c057c5b 100644 --- a/app/models/project_services/kubernetes_service.rb +++ b/app/models/project_services/kubernetes_service.rb @@ -102,7 +102,7 @@ class KubernetesService < DeploymentService { key: 'KUBE_URL', value: api_url, public: true }, { key: 'KUBE_TOKEN', value: token, public: false }, { key: 'KUBE_NAMESPACE', value: actual_namespace, public: true }, - { key: 'KUBECONFIG_FILE', value: config, public: false, file: true } + { key: 'KUBECONFIG', value: config, public: false, file: true } ] if ca_pem.present? diff --git a/changelogs/unreleased/33360-generate-kubeconfig.yml b/changelogs/unreleased/33360-generate-kubeconfig.yml index 354a8a7f9b4..96f0b1bc93f 100644 --- a/changelogs/unreleased/33360-generate-kubeconfig.yml +++ b/changelogs/unreleased/33360-generate-kubeconfig.yml @@ -1,4 +1,4 @@ --- -title: Provide KUBECONFIG_FILE from KubernetesService for runners +title: Provide KUBECONFIG from KubernetesService for runners merge_request: 12223 author: diff --git a/doc/user/project/integrations/kubernetes.md b/doc/user/project/integrations/kubernetes.md index d1c3e18a276..bfe2672e098 100644 --- a/doc/user/project/integrations/kubernetes.md +++ b/doc/user/project/integrations/kubernetes.md @@ -55,7 +55,7 @@ GitLab CI build environment: - `KUBE_CA_PEM_FILE` - only present if a custom CA bundle was specified. Path to a file containing PEM data. - `KUBE_CA_PEM` (deprecated)- only if a custom CA bundle was specified. Raw PEM data. -- `KUBECONFIG_FILE` - Path to a file containing kubeconfig for this deployment. CA bundle would be embedded if specified. +- `KUBECONFIG` - Path to a file containing kubeconfig for this deployment. CA bundle would be embedded if specified. ## Web terminals diff --git a/spec/models/project_services/kubernetes_service_spec.rb b/spec/models/project_services/kubernetes_service_spec.rb index d4feae231bc..7ec2ea5ba95 100644 --- a/spec/models/project_services/kubernetes_service_spec.rb +++ b/spec/models/project_services/kubernetes_service_spec.rb @@ -221,7 +221,7 @@ describe KubernetesService, models: true, caching: true do { key: 'KUBE_URL', value: 'https://kube.domain.com', public: true }, { key: 'KUBE_TOKEN', value: 'token', public: false }, { key: 'KUBE_NAMESPACE', value: namespace, public: true }, - { key: 'KUBECONFIG_FILE', value: kubeconfig, public: false, file: true }, + { key: 'KUBECONFIG', value: kubeconfig, public: false, file: true }, { key: 'KUBE_CA_PEM', value: 'CA PEM DATA', public: true }, { key: 'KUBE_CA_PEM_FILE', value: 'CA PEM DATA', public: true, file: true } ) -- cgit v1.2.3 From 3eba6ad09381f7ac0ee34e59136786f65010995a Mon Sep 17 00:00:00 2001 From: Huang Tao Date: Thu, 6 Jul 2017 08:11:48 +0000 Subject: Synchronous discussion in the translation --- locale/zh_TW/gitlab.po | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/locale/zh_TW/gitlab.po b/locale/zh_TW/gitlab.po index 5ede5845bf7..f4847d7adbb 100644 --- a/locale/zh_TW/gitlab.po +++ b/locale/zh_TW/gitlab.po @@ -21,11 +21,11 @@ msgstr "" msgid "%d additional commit has been omitted to prevent performance issues." msgid_plural "" "%d additional commits have been omitted to prevent performance issues." -msgstr[0] "為提高頁面加載速度及性能,已省略了 %d 次送交 (commit)。" +msgstr[0] "因效能考量,不顯示 %d 個更動 (commit)。" msgid "%d commit" msgid_plural "%d commits" -msgstr[0] "%d 次送交 (commit)" +msgstr[0] "%d 個更動 (commit)" msgid "%{commit_author_link} committed %{commit_timeago}" msgstr "%{commit_author_link} 在 %{commit_timeago} 送交" @@ -76,7 +76,7 @@ msgstr "" "Yaml 模板,然後記得要送交 (commit) 您的編輯內容。%{link_to_autodeploy_doc}\n" msgid "BranchSwitcherPlaceholder|Search branches" -msgstr "搜索分支 (branches)" +msgstr "搜尋分支 (branches)" msgid "BranchSwitcherTitle|Switch branch" msgstr "切換分支 (branch)" @@ -200,7 +200,7 @@ msgid "Commits" msgstr "更動記錄 (commit) " msgid "Commits feed" -msgstr "送交動態" +msgstr "更動摘要 (commit feed)" msgid "Commits|History" msgstr "過去更動 (commit) " @@ -360,7 +360,7 @@ msgid "Files" msgstr "檔案" msgid "Filter by commit message" -msgstr "按送交消息過濾" +msgstr "以更動說明篩選" msgid "Find by path" msgstr "以路徑搜尋" @@ -1022,7 +1022,7 @@ msgid "Use your global notification setting" msgstr "使用全域通知設定" msgid "View open merge request" -msgstr "查看開啟的合並請求 (merge request)" +msgstr "查看此分支的合併請求 (merge request)" msgid "VisibilityLevel|Internal" msgstr "內部" -- cgit v1.2.3 From d9435d61218f677395f3b53976a41ac5f361f24b Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Thu, 6 Jul 2017 15:45:38 +0800 Subject: Backports for ee-2112 https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/2112 --- app/controllers/projects/variables_controller.rb | 15 ++++--- app/models/ci/build.rb | 21 ++++++---- app/models/project.rb | 3 +- doc/ci/variables/README.md | 11 ++--- doc/downgrade_ee_to_ce/README.md | 13 ++++++ lib/api/variables.rb | 8 +++- lib/gitlab/regex.rb | 6 ++- lib/gitlab/sql/glob.rb | 22 ++++++++++ spec/lib/gitlab/sql/glob_spec.rb | 53 ++++++++++++++++++++++++ spec/models/ci/build_spec.rb | 7 ++-- spec/models/ci/variable_spec.rb | 4 -- spec/models/project_spec.rb | 12 +++--- 12 files changed, 140 insertions(+), 35 deletions(-) create mode 100644 lib/gitlab/sql/glob.rb create mode 100644 spec/lib/gitlab/sql/glob_spec.rb diff --git a/app/controllers/projects/variables_controller.rb b/app/controllers/projects/variables_controller.rb index 573d1c05622..326d31ecec2 100644 --- a/app/controllers/projects/variables_controller.rb +++ b/app/controllers/projects/variables_controller.rb @@ -14,7 +14,7 @@ class Projects::VariablesController < Projects::ApplicationController def update @variable = @project.variables.find(params[:id]) - if @variable.update_attributes(project_params) + if @variable.update_attributes(variable_params) redirect_to project_variables_path(project), notice: 'Variable was successfully updated.' else render action: "show" @@ -22,9 +22,9 @@ class Projects::VariablesController < Projects::ApplicationController end def create - @variable = Ci::Variable.new(project_params) + @variable = @project.variables.new(variable_params) - if @variable.valid? && @project.variables << @variable + if @variable.save flash[:notice] = 'Variables were successfully updated.' redirect_to project_settings_ci_cd_path(project) else @@ -43,8 +43,11 @@ class Projects::VariablesController < Projects::ApplicationController private - def project_params - params.require(:variable) - .permit([:id, :key, :value, :protected, :_destroy]) + def variable_params + params.require(:variable).permit(*variable_params_attributes) + end + + def variable_params_attributes + %i[id key value protected _destroy] end end diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 2e7a80d308b..ef92f990032 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -186,6 +186,12 @@ module Ci # Variables whose value does not depend on environment def simple_variables + variables(environment: nil) + end + + # All variables, including those dependent on environment, which could + # contain unexpanded variables. + def variables(environment: persisted_environment) variables = predefined_variables variables += project.predefined_variables variables += pipeline.predefined_variables @@ -194,15 +200,11 @@ module Ci variables += project.deployment_variables if has_environment? variables += yaml_variables variables += user_variables - variables += project.secret_variables_for(ref).map(&:to_runner_variable) + variables += secret_variables(environment: environment) variables += trigger_request.user_variables if trigger_request - variables - end + variables += persisted_environment_variables if environment - # All variables, including those dependent on environment, which could - # contain unexpanded variables. - def variables - simple_variables.concat(persisted_environment_variables) + variables end def merge_request @@ -370,6 +372,11 @@ module Ci ] end + def secret_variables(environment: persisted_environment) + project.secret_variables_for(ref: ref, environment: environment) + .map(&:to_runner_variable) + end + def steps [Gitlab::Ci::Build::Step.from_commands(self), Gitlab::Ci::Build::Step.from_after_script(self)].compact diff --git a/app/models/project.rb b/app/models/project.rb index 3a5a01db518..6b2b4cf56cb 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1326,7 +1326,8 @@ class Project < ActiveRecord::Base variables end - def secret_variables_for(ref) + def secret_variables_for(ref:, environment: nil) + # EE would use the environment if protected_for?(ref) variables else diff --git a/doc/ci/variables/README.md b/doc/ci/variables/README.md index ee23ac0adbe..3501aae75ec 100644 --- a/doc/ci/variables/README.md +++ b/doc/ci/variables/README.md @@ -160,7 +160,7 @@ Secret variables can be added by going to your project's Once you set them, they will be available for all subsequent pipelines. -## Protected secret variables +### Protected secret variables >**Notes:** This feature requires GitLab 9.3 or higher. @@ -426,10 +426,11 @@ export CI_REGISTRY_PASSWORD="longalfanumstring" ``` [ce-13784]: https://gitlab.com/gitlab-org/gitlab-ce/issues/13784 -[runner]: https://docs.gitlab.com/runner/ -[triggered]: ../triggers/README.md -[triggers]: ../triggers/README.md#pass-job-variables-to-a-trigger +[eep]: https://about.gitlab.com/gitlab-ee/ "Available only in GitLab Enterprise Edition Premium" +[envs]: ../environments.md [protected branches]: ../../user/project/protected_branches.md [protected tags]: ../../user/project/protected_tags.md +[runner]: https://docs.gitlab.com/runner/ [shellexecutors]: https://docs.gitlab.com/runner/executors/ -[eep]: https://about.gitlab.com/gitlab-ee/ "Available only in GitLab Enterprise Edition Premium" +[triggered]: ../triggers/README.md +[triggers]: ../triggers/README.md#pass-job-variables-to-a-trigger diff --git a/doc/downgrade_ee_to_ce/README.md b/doc/downgrade_ee_to_ce/README.md index fe4b6d73771..75bae324585 100644 --- a/doc/downgrade_ee_to_ce/README.md +++ b/doc/downgrade_ee_to_ce/README.md @@ -46,6 +46,19 @@ $ sudo gitlab-rails runner "Service.where(type: ['JenkinsService', 'JenkinsDepre $ bundle exec rails runner "Service.where(type: ['JenkinsService', 'JenkinsDeprecatedService']).delete_all" production ``` +### Secret variables environment scopes + +If you're using this feature and there are variables sharing the same +key, but they have different scopes in a project, then you might want to +revisit the environment scope setting for those variables. + +In CE, environment scopes are completely ignored, therefore you could +accidentally get a variable which you're not expecting for a particular +environment. Make sure that you have the right variables in this case. + +Data is completely preserved, so you could always upgrade back to EE and +restore the behavior if you leave it alone. + ## Downgrade to CE After performing the above mentioned steps, you are now ready to downgrade your diff --git a/lib/api/variables.rb b/lib/api/variables.rb index 10374995497..7fa528fb2d3 100644 --- a/lib/api/variables.rb +++ b/lib/api/variables.rb @@ -45,7 +45,9 @@ module API optional :protected, type: String, desc: 'Whether the variable is protected' end post ':id/variables' do - variable = user_project.variables.create(declared_params(include_missing: false)) + variable_params = declared_params(include_missing: false) + + variable = user_project.variables.create(variable_params) if variable.valid? present variable, with: Entities::Variable @@ -67,7 +69,9 @@ module API return not_found!('Variable') unless variable - if variable.update(declared_params(include_missing: false).except(:key)) + variable_params = declared_params(include_missing: false).except(:key) + + if variable.update(variable_params) present variable, with: Entities::Variable else render_validation_error!(variable) diff --git a/lib/gitlab/regex.rb b/lib/gitlab/regex.rb index 057f32eaef7..c1ee20b6977 100644 --- a/lib/gitlab/regex.rb +++ b/lib/gitlab/regex.rb @@ -30,8 +30,12 @@ module Gitlab @container_repository_regex ||= %r{\A[a-z0-9]+(?:[-._/][a-z0-9]+)*\Z} end + def environment_name_regex_chars + 'a-zA-Z0-9_/\\$\\{\\}\\. -' + end + def environment_name_regex - @environment_name_regex ||= /\A[a-zA-Z0-9_\\\/\${}. -]+\z/.freeze + @environment_name_regex ||= /\A[#{environment_name_regex_chars}]+\z/.freeze end def environment_name_regex_message diff --git a/lib/gitlab/sql/glob.rb b/lib/gitlab/sql/glob.rb new file mode 100644 index 00000000000..5e89e12b2b1 --- /dev/null +++ b/lib/gitlab/sql/glob.rb @@ -0,0 +1,22 @@ +module Gitlab + module SQL + module Glob + extend self + + # Convert a simple glob pattern with wildcard (*) to SQL LIKE pattern + # with SQL expression + def to_like(pattern) + <<~SQL + REPLACE(REPLACE(REPLACE(#{pattern}, + #{q('%')}, #{q('\\%')}), + #{q('_')}, #{q('\\_')}), + #{q('*')}, #{q('%')}) + SQL + end + + def q(string) + ActiveRecord::Base.connection.quote(string) + end + end + end +end diff --git a/spec/lib/gitlab/sql/glob_spec.rb b/spec/lib/gitlab/sql/glob_spec.rb new file mode 100644 index 00000000000..451c583310d --- /dev/null +++ b/spec/lib/gitlab/sql/glob_spec.rb @@ -0,0 +1,53 @@ +require 'spec_helper' + +describe Gitlab::SQL::Glob, lib: true do + describe '.to_like' do + it 'matches * as %' do + expect(glob('apple', '*')).to be(true) + expect(glob('apple', 'app*')).to be(true) + expect(glob('apple', 'apple*')).to be(true) + expect(glob('apple', '*pple')).to be(true) + expect(glob('apple', 'ap*le')).to be(true) + + expect(glob('apple', '*a')).to be(false) + expect(glob('apple', 'app*a')).to be(false) + expect(glob('apple', 'ap*l')).to be(false) + end + + it 'matches % literally' do + expect(glob('100%', '100%')).to be(true) + + expect(glob('100%', '%')).to be(false) + end + + it 'matches _ literally' do + expect(glob('^_^', '^_^')).to be(true) + + expect(glob('^A^', '^_^')).to be(false) + end + end + + def glob(string, pattern) + match(string, subject.to_like(quote(pattern))) + end + + def match(string, pattern) + value = query("SELECT #{quote(string)} LIKE #{pattern}") + .rows.flatten.first + + case value + when 't', 1 + true + else + false + end + end + + def query(sql) + ActiveRecord::Base.connection.select_all(sql) + end + + def quote(string) + ActiveRecord::Base.connection.quote(string) + end +end diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index a7ba3a7c43e..2b10791ad6d 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -1496,9 +1496,10 @@ describe Ci::Build, :models do allow(pipeline).to receive(:predefined_variables) { [pipeline_pre_var] } allow(build).to receive(:yaml_variables) { [build_yaml_var] } - allow(project).to receive(:secret_variables_for).with(build.ref) do - [create(:ci_variable, key: 'secret', value: 'value')] - end + allow(project).to receive(:secret_variables_for) + .with(ref: 'master', environment: nil) do + [create(:ci_variable, key: 'secret', value: 'value')] + end end it do diff --git a/spec/models/ci/variable_spec.rb b/spec/models/ci/variable_spec.rb index 50f7c029af8..4ffbfa6c130 100644 --- a/spec/models/ci/variable_spec.rb +++ b/spec/models/ci/variable_spec.rb @@ -8,10 +8,6 @@ describe Ci::Variable, models: true do describe 'validations' do it { is_expected.to include_module(HasVariable) } it { is_expected.to validate_uniqueness_of(:key).scoped_to(:project_id, :environment_scope) } - it { is_expected.to validate_length_of(:key).is_at_most(255) } - it { is_expected.to allow_value('foo').for(:key) } - it { is_expected.not_to allow_value('foo bar').for(:key) } - it { is_expected.not_to allow_value('foo/bar').for(:key) } end describe '.unprotected' do diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index f50b4aea411..a9855cf343b 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -1875,7 +1875,12 @@ describe Project, models: true do create(:ci_variable, :protected, value: 'protected', project: project) end - subject { project.secret_variables_for('ref') } + subject { project.secret_variables_for(ref: 'ref') } + + before do + stub_application_setting( + default_branch_protection: Gitlab::Access::PROTECTION_NONE) + end shared_examples 'ref is protected' do it 'contains all the variables' do @@ -1884,11 +1889,6 @@ describe Project, models: true do end context 'when the ref is not protected' do - before do - stub_application_setting( - default_branch_protection: Gitlab::Access::PROTECTION_NONE) - end - it 'contains only the secret variables' do is_expected.to contain_exactly(secret_variable) end -- cgit v1.2.3 From c63e3221587daf9e7464f7d2079ca8ed3111f6ff Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Tue, 30 May 2017 13:54:59 +0200 Subject: Add many foreign keys to the projects table This removes the need for relying on Rails' "dependent" option for data removal, which is _incredibly_ slow (even when using :delete_all) when deleting large amounts of data. This also ensures data consistency is enforced on DB level and not on application level (something Rails is really bad at). This commit also includes various migrations to add foreign keys to tables that eventually point to "projects" to ensure no rows get orphaned upon removing a project. --- app/models/issue.rb | 2 +- app/models/merge_request.rb | 4 +- app/models/project.rb | 149 +++++++++------- .../unreleased/foreign-keys-for-project-model.yml | 4 + ..._project_foreign_keys_with_cascading_deletes.rb | 187 +++++++++++++++++++++ ...0029_correct_protected_branches_foreign_keys.rb | 40 +++++ ...2212_add_foreign_key_for_merge_request_diffs.rb | 30 ++++ db/schema.rb | 39 ++++- spec/models/concerns/issuable_spec.rb | 2 +- spec/models/forked_project_link_spec.rb | 6 +- spec/models/merge_request_spec.rb | 2 +- spec/models/project_spec.rb | 121 +++++++------ spec/presenters/ci/build_presenter_spec.rb | 4 +- .../expire_build_instance_artifacts_worker_spec.rb | 14 -- 14 files changed, 458 insertions(+), 146 deletions(-) create mode 100644 changelogs/unreleased/foreign-keys-for-project-model.yml create mode 100644 db/migrate/20170530130129_project_foreign_keys_with_cascading_deletes.rb create mode 100644 db/migrate/20170622130029_correct_protected_branches_foreign_keys.rb create mode 100644 db/migrate/20170622132212_add_foreign_key_for_merge_request_diffs.rb diff --git a/app/models/issue.rb b/app/models/issue.rb index a97e88f76f6..31e3504e535 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -25,7 +25,7 @@ class Issue < ActiveRecord::Base has_many :events, as: :target, dependent: :destroy - has_many :merge_requests_closing_issues, class_name: 'MergeRequestsClosingIssues', dependent: :delete_all + has_many :merge_requests_closing_issues, class_name: 'MergeRequestsClosingIssues' has_many :issue_assignees has_many :assignees, class_name: "User", through: :issue_assignees diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 808212c780c..d2534e9c858 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -12,7 +12,7 @@ class MergeRequest < ActiveRecord::Base belongs_to :source_project, class_name: "Project" belongs_to :merge_user, class_name: "User" - has_many :merge_request_diffs, dependent: :destroy + has_many :merge_request_diffs has_one :merge_request_diff, -> { order('merge_request_diffs.id DESC') } @@ -20,7 +20,7 @@ class MergeRequest < ActiveRecord::Base has_many :events, as: :target, dependent: :destroy - has_many :merge_requests_closing_issues, class_name: 'MergeRequestsClosingIssues', dependent: :delete_all + has_many :merge_requests_closing_issues, class_name: 'MergeRequestsClosingIssues' belongs_to :assignee, class_name: "User" diff --git a/app/models/project.rb b/app/models/project.rb index 3a5a01db518..181e9940ddb 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -59,6 +59,7 @@ class Project < ActiveRecord::Base update_column(:last_repository_updated_at, self.created_at) end + before_destroy :remove_private_deploy_keys after_destroy :remove_pages # update visibility_level of forks @@ -80,96 +81,108 @@ class Project < ActiveRecord::Base belongs_to :namespace has_one :last_event, -> {order 'events.created_at DESC'}, class_name: 'Event' - has_many :boards, before_add: :validate_board_limit, dependent: :destroy + has_many :boards, before_add: :validate_board_limit # Project services - has_one :campfire_service, dependent: :destroy - has_one :drone_ci_service, dependent: :destroy - has_one :emails_on_push_service, dependent: :destroy - has_one :pipelines_email_service, dependent: :destroy - has_one :irker_service, dependent: :destroy - has_one :pivotaltracker_service, dependent: :destroy - has_one :hipchat_service, dependent: :destroy - has_one :flowdock_service, dependent: :destroy - has_one :assembla_service, dependent: :destroy - has_one :asana_service, dependent: :destroy - has_one :gemnasium_service, dependent: :destroy - has_one :mattermost_slash_commands_service, dependent: :destroy - has_one :mattermost_service, dependent: :destroy - has_one :slack_slash_commands_service, dependent: :destroy - has_one :slack_service, dependent: :destroy - has_one :buildkite_service, dependent: :destroy - has_one :bamboo_service, dependent: :destroy - has_one :teamcity_service, dependent: :destroy - has_one :pushover_service, dependent: :destroy - has_one :jira_service, dependent: :destroy - has_one :redmine_service, dependent: :destroy - has_one :custom_issue_tracker_service, dependent: :destroy - has_one :bugzilla_service, dependent: :destroy - has_one :gitlab_issue_tracker_service, dependent: :destroy, inverse_of: :project - has_one :external_wiki_service, dependent: :destroy - has_one :kubernetes_service, dependent: :destroy, inverse_of: :project - has_one :prometheus_service, dependent: :destroy, inverse_of: :project - has_one :mock_ci_service, dependent: :destroy - has_one :mock_deployment_service, dependent: :destroy - has_one :mock_monitoring_service, dependent: :destroy - has_one :microsoft_teams_service, dependent: :destroy - - has_one :forked_project_link, dependent: :destroy, foreign_key: "forked_to_project_id" + has_one :campfire_service + has_one :drone_ci_service + has_one :emails_on_push_service + has_one :pipelines_email_service + has_one :irker_service + has_one :pivotaltracker_service + has_one :hipchat_service + has_one :flowdock_service + has_one :assembla_service + has_one :asana_service + has_one :gemnasium_service + has_one :mattermost_slash_commands_service + has_one :mattermost_service + has_one :slack_slash_commands_service + has_one :slack_service + has_one :buildkite_service + has_one :bamboo_service + has_one :teamcity_service + has_one :pushover_service + has_one :jira_service + has_one :redmine_service + has_one :custom_issue_tracker_service + has_one :bugzilla_service + has_one :gitlab_issue_tracker_service, inverse_of: :project + has_one :external_wiki_service + has_one :kubernetes_service, inverse_of: :project + has_one :prometheus_service, inverse_of: :project + has_one :mock_ci_service + has_one :mock_deployment_service + has_one :mock_monitoring_service + has_one :microsoft_teams_service + + has_one :forked_project_link, foreign_key: "forked_to_project_id" has_one :forked_from_project, through: :forked_project_link has_many :forked_project_links, foreign_key: "forked_from_project_id" has_many :forks, through: :forked_project_links, source: :forked_to_project # Merge Requests for target project should be removed with it - has_many :merge_requests, dependent: :destroy, foreign_key: 'target_project_id' - has_many :issues, dependent: :destroy - has_many :labels, dependent: :destroy, class_name: 'ProjectLabel' - has_many :services, dependent: :destroy - has_many :events, dependent: :destroy - has_many :milestones, dependent: :destroy - has_many :notes, dependent: :destroy - has_many :snippets, dependent: :destroy, class_name: 'ProjectSnippet' - has_many :hooks, dependent: :destroy, class_name: 'ProjectHook' - has_many :protected_branches, dependent: :destroy - has_many :protected_tags, dependent: :destroy + has_many :merge_requests, foreign_key: 'target_project_id' + has_many :issues + has_many :labels, class_name: 'ProjectLabel' + has_many :services + has_many :events + has_many :milestones + has_many :notes + has_many :snippets, class_name: 'ProjectSnippet' + has_many :hooks, class_name: 'ProjectHook' + has_many :protected_branches + has_many :protected_tags has_many :project_authorizations has_many :authorized_users, through: :project_authorizations, source: :user, class_name: 'User' - has_many :project_members, -> { where(requested_at: nil) }, dependent: :destroy, as: :source + has_many :project_members, -> { where(requested_at: nil) }, + as: :source, dependent: :delete_all + alias_method :members, :project_members has_many :users, through: :project_members - has_many :requesters, -> { where.not(requested_at: nil) }, dependent: :destroy, as: :source, class_name: 'ProjectMember' + has_many :requesters, -> { where.not(requested_at: nil) }, + as: :source, class_name: 'ProjectMember', dependent: :delete_all - has_many :deploy_keys_projects, dependent: :destroy + has_many :deploy_keys_projects has_many :deploy_keys, through: :deploy_keys_projects - has_many :users_star_projects, dependent: :destroy + has_many :users_star_projects has_many :starrers, through: :users_star_projects, source: :user - has_many :releases, dependent: :destroy + has_many :releases has_many :lfs_objects_projects, dependent: :destroy has_many :lfs_objects, through: :lfs_objects_projects - has_many :project_group_links, dependent: :destroy + has_many :project_group_links has_many :invited_groups, through: :project_group_links, source: :group - has_many :pages_domains, dependent: :destroy - has_many :todos, dependent: :destroy - has_many :notification_settings, dependent: :destroy, as: :source + has_many :pages_domains + has_many :todos + has_many :notification_settings, as: :source, dependent: :delete_all + + has_one :import_data, class_name: 'ProjectImportData' + has_one :project_feature + has_one :statistics, class_name: 'ProjectStatistics' - has_one :import_data, dependent: :delete, class_name: 'ProjectImportData' - has_one :project_feature, dependent: :destroy - has_one :statistics, class_name: 'ProjectStatistics', dependent: :delete + # Container repositories need to remove data from the container registry, + # which is not managed by the DB. Hence we're still using dependent: :destroy + # here. has_many :container_repositories, dependent: :destroy - has_many :commit_statuses, dependent: :destroy - has_many :pipelines, dependent: :destroy, class_name: 'Ci::Pipeline' - has_many :builds, class_name: 'Ci::Build' # the builds are created from the commit_statuses - has_many :runner_projects, dependent: :destroy, class_name: 'Ci::RunnerProject' + has_many :commit_statuses + has_many :pipelines, class_name: 'Ci::Pipeline' + + # Ci::Build objects store data on the file system such as artifact files and + # build traces. Currently there's no efficient way of removing this data in + # bulk that doesn't involve loading the rows into memory. As a result we're + # still using `dependent: :destroy` here. + has_many :builds, class_name: 'Ci::Build', dependent: :destroy + has_many :runner_projects, class_name: 'Ci::RunnerProject' has_many :runners, through: :runner_projects, source: :runner, class_name: 'Ci::Runner' has_many :variables, class_name: 'Ci::Variable' - has_many :triggers, dependent: :destroy, class_name: 'Ci::Trigger' - has_many :environments, dependent: :destroy - has_many :deployments, dependent: :destroy - has_many :pipeline_schedules, dependent: :destroy, class_name: 'Ci::PipelineSchedule' + has_many :triggers, class_name: 'Ci::Trigger' + has_many :environments + has_many :deployments + has_many :pipeline_schedules, class_name: 'Ci::PipelineSchedule' has_many :active_runners, -> { active }, through: :runner_projects, source: :runner, class_name: 'Ci::Runner' @@ -1240,7 +1253,13 @@ class Project < ActiveRecord::Base File.join(pages_path, 'public') end + def remove_private_deploy_keys + deploy_keys.where(public: false).delete_all + end + def remove_pages + ::Projects::UpdatePagesConfigurationService.new(self).execute + # 1. We rename pages to temporary directory # 2. We wait 5 minutes, due to NFS caching # 3. We asynchronously remove pages with force diff --git a/changelogs/unreleased/foreign-keys-for-project-model.yml b/changelogs/unreleased/foreign-keys-for-project-model.yml new file mode 100644 index 00000000000..3648b1c3735 --- /dev/null +++ b/changelogs/unreleased/foreign-keys-for-project-model.yml @@ -0,0 +1,4 @@ +--- +title: Speed up project removals by adding foreign keys with cascading deletes to various tables +merge_request: +author: diff --git a/db/migrate/20170530130129_project_foreign_keys_with_cascading_deletes.rb b/db/migrate/20170530130129_project_foreign_keys_with_cascading_deletes.rb new file mode 100644 index 00000000000..3eaafac321d --- /dev/null +++ b/db/migrate/20170530130129_project_foreign_keys_with_cascading_deletes.rb @@ -0,0 +1,187 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class ProjectForeignKeysWithCascadingDeletes < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + CONCURRENCY = 4 + + disable_ddl_transaction! + + # The tables/columns for which to remove orphans and add foreign keys. Order + # matters as some tables/columns should be processed before others. + TABLES = [ + [:boards, :projects, :project_id], + [:lists, :labels, :label_id], + [:lists, :boards, :board_id], + [:services, :projects, :project_id], + [:forked_project_links, :projects, :forked_to_project_id], + [:merge_requests, :projects, :target_project_id], + [:labels, :projects, :project_id], + [:issues, :projects, :project_id], + [:events, :projects, :project_id], + [:milestones, :projects, :project_id], + [:notes, :projects, :project_id], + [:snippets, :projects, :project_id], + [:web_hooks, :projects, :project_id], + [:protected_branch_merge_access_levels, :protected_branches, :protected_branch_id], + [:protected_branch_push_access_levels, :protected_branches, :protected_branch_id], + [:protected_branches, :projects, :project_id], + [:protected_tags, :projects, :project_id], + [:deploy_keys_projects, :projects, :project_id], + [:users_star_projects, :projects, :project_id], + [:releases, :projects, :project_id], + [:project_group_links, :projects, :project_id], + [:pages_domains, :projects, :project_id], + [:todos, :projects, :project_id], + [:project_import_data, :projects, :project_id], + [:project_features, :projects, :project_id], + [:ci_builds, :projects, :project_id], + [:ci_pipelines, :projects, :project_id], + [:ci_runner_projects, :projects, :project_id], + [:ci_triggers, :projects, :project_id], + [:environments, :projects, :project_id], + [:deployments, :projects, :project_id] + ] + + def up + # These existing foreign keys don't have an "ON DELETE CASCADE" clause. + remove_foreign_key_without_error(:boards, :project_id) + remove_foreign_key_without_error(:lists, :label_id) + remove_foreign_key_without_error(:lists, :board_id) + remove_foreign_key_without_error(:protected_branch_merge_access_levels, + :protected_branch_id) + + remove_foreign_key_without_error(:protected_branch_push_access_levels, + :protected_branch_id) + + remove_orphaned_rows + add_foreign_keys + + # These columns are not indexed yet, meaning a cascading delete would take + # forever. + add_concurrent_index(:project_group_links, :project_id) + add_concurrent_index(:pages_domains, :project_id) + end + + def down + TABLES.each do |(source, _, column)| + remove_foreign_key_without_error(source, column) + end + + add_concurrent_foreign_key(:boards, :projects, column: :project_id) + add_concurrent_foreign_key(:lists, :labels, column: :label_id) + add_concurrent_foreign_key(:lists, :boards, column: :board_id) + + add_concurrent_foreign_key(:protected_branch_merge_access_levels, + :protected_branches, + column: :protected_branch_id) + + add_concurrent_foreign_key(:protected_branch_push_access_levels, + :protected_branches, + column: :protected_branch_id) + + remove_index_without_error(:project_group_links, :project_id) + remove_index_without_error(:pages_domains, :project_id) + end + + def add_foreign_keys + TABLES.each do |(source, target, column)| + add_concurrent_foreign_key(source, target, column: column) + end + end + + # Removes orphans from various tables concurrently. + def remove_orphaned_rows + Gitlab::Database.with_connection_pool(CONCURRENCY) do |pool| + queues = queues_for_rows(TABLES) + + threads = queues.map do |queue| + Thread.new do + pool.with_connection do |connection| + Thread.current[:foreign_key_connection] = connection + + # Disables statement timeouts for the current connection. This is + # necessary as removing of orphaned data might otherwise exceed the + # statement timeout. + disable_statement_timeout + + remove_orphans(*queue.pop) until queue.empty? + + steal_from_queues(queues - [queue]) + end + end + end + + threads.each(&:join) + end + end + + def steal_from_queues(queues) + loop do + stolen = false + + queues.each do |queue| + # Stealing is racy so it's possible a pop might be called on an + # already-empty queue. + begin + remove_orphans(*queue.pop(true)) + stolen = true + rescue ThreadError + end + end + + break unless stolen + end + end + + def remove_orphans(source, target, column) + quoted_source = quote_table_name(source) + quoted_target = quote_table_name(target) + quoted_column = quote_column_name(column) + + execute <<-EOF.strip_heredoc + DELETE FROM #{quoted_source} + WHERE NOT EXISTS ( + SELECT true + FROM #{quoted_target} + WHERE #{quoted_target}.id = #{quoted_source}.#{quoted_column} + ) + AND #{quoted_source}.#{quoted_column} IS NOT NULL + EOF + end + + def remove_foreign_key_without_error(table, column) + remove_foreign_key(table, column: column) + rescue ArgumentError + end + + def remove_index_without_error(table, column) + remove_concurrent_index(table, column) + rescue ArgumentError + end + + def connection + # Rails memoizes connection objects, but this causes them to be shared + # amongst threads; we don't want that. + Thread.current[:foreign_key_connection] || ActiveRecord::Base.connection + end + + def queues_for_rows(rows) + queues = Array.new(CONCURRENCY) { Queue.new } + slice_size = rows.length / CONCURRENCY + + # Divide all the tuples as evenly as possible amongst the queues. + rows.each_slice(slice_size).each_with_index do |slice, index| + bucket = index % CONCURRENCY + + slice.each do |row| + queues[bucket] << row + end + end + + queues + end +end diff --git a/db/migrate/20170622130029_correct_protected_branches_foreign_keys.rb b/db/migrate/20170622130029_correct_protected_branches_foreign_keys.rb new file mode 100644 index 00000000000..46497775527 --- /dev/null +++ b/db/migrate/20170622130029_correct_protected_branches_foreign_keys.rb @@ -0,0 +1,40 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class CorrectProtectedBranchesForeignKeys < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + disable_ddl_transaction! + + def up + remove_foreign_key_without_error(:protected_branch_push_access_levels, + column: :protected_branch_id) + + execute <<-EOF + DELETE FROM protected_branch_push_access_levels + WHERE NOT EXISTS ( + SELECT true + FROM protected_branches + WHERE protected_branch_push_access_levels.protected_branch_id = protected_branches.id + ) + AND protected_branch_id IS NOT NULL + EOF + + add_concurrent_foreign_key(:protected_branch_push_access_levels, + :protected_branches, + column: :protected_branch_id) + end + + def down + # Previously there was a foreign key without a CASCADING DELETE, so we'll + # just leave the foreign key in place. + end + + def remove_foreign_key_without_error(*args) + remove_foreign_key(*args) + rescue ArgumentError + end +end diff --git a/db/migrate/20170622132212_add_foreign_key_for_merge_request_diffs.rb b/db/migrate/20170622132212_add_foreign_key_for_merge_request_diffs.rb new file mode 100644 index 00000000000..9f524fac8a7 --- /dev/null +++ b/db/migrate/20170622132212_add_foreign_key_for_merge_request_diffs.rb @@ -0,0 +1,30 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddForeignKeyForMergeRequestDiffs < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + disable_ddl_transaction! + + def up + execute <<-EOF + DELETE FROM merge_request_diffs + WHERE NOT EXISTS ( + SELECT true + FROM merge_requests + WHERE merge_requests.id = merge_request_diffs.merge_request_id + ) + EOF + + add_concurrent_foreign_key(:merge_request_diffs, + :merge_requests, + column: :merge_request_id) + end + + def down + remove_foreign_key(:merge_request_diffs, column: :merge_request_id) + end +end diff --git a/db/schema.rb b/db/schema.rb index 40f30a10a01..f12fdf903c5 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -971,6 +971,7 @@ ActiveRecord::Schema.define(version: 20170703102400) do end add_index "pages_domains", ["domain"], name: "index_pages_domains_on_domain", unique: true, using: :btree + add_index "pages_domains", ["project_id"], name: "index_pages_domains_on_project_id", using: :btree create_table "personal_access_tokens", force: :cascade do |t| t.integer "user_id", null: false @@ -1020,6 +1021,7 @@ ActiveRecord::Schema.define(version: 20170703102400) do end add_index "project_group_links", ["group_id"], name: "index_project_group_links_on_group_id", using: :btree + add_index "project_group_links", ["project_id"], name: "index_project_group_links_on_project_id", using: :btree create_table "project_import_data", force: :cascade do |t| t.integer "project_id" @@ -1528,48 +1530,75 @@ ActiveRecord::Schema.define(version: 20170703102400) do add_index "web_hooks", ["project_id"], name: "index_web_hooks_on_project_id", using: :btree add_index "web_hooks", ["type"], name: "index_web_hooks_on_type", using: :btree - add_foreign_key "boards", "projects" + add_foreign_key "boards", "projects", name: "fk_f15266b5f9", on_delete: :cascade add_foreign_key "chat_teams", "namespaces", on_delete: :cascade add_foreign_key "ci_builds", "ci_pipelines", column: "auto_canceled_by_id", name: "fk_a2141b1522", on_delete: :nullify add_foreign_key "ci_builds", "ci_stages", column: "stage_id", name: "fk_3a9eaa254d", on_delete: :cascade + add_foreign_key "ci_builds", "projects", name: "fk_befce0568a", on_delete: :cascade add_foreign_key "ci_pipeline_schedules", "projects", name: "fk_8ead60fcc4", on_delete: :cascade add_foreign_key "ci_pipeline_schedules", "users", column: "owner_id", name: "fk_9ea99f58d2", on_delete: :nullify add_foreign_key "ci_pipelines", "ci_pipeline_schedules", column: "pipeline_schedule_id", name: "fk_3d34ab2e06", on_delete: :nullify add_foreign_key "ci_pipelines", "ci_pipelines", column: "auto_canceled_by_id", name: "fk_262d4c2d19", on_delete: :nullify + add_foreign_key "ci_pipelines", "projects", name: "fk_86635dbd80", on_delete: :cascade + add_foreign_key "ci_runner_projects", "projects", name: "fk_4478a6f1e4", on_delete: :cascade add_foreign_key "ci_stages", "ci_pipelines", column: "pipeline_id", name: "fk_fb57e6cc56", on_delete: :cascade add_foreign_key "ci_stages", "projects", name: "fk_2360681d1d", on_delete: :cascade add_foreign_key "ci_trigger_requests", "ci_triggers", column: "trigger_id", name: "fk_b8ec8b7245", on_delete: :cascade + add_foreign_key "ci_triggers", "projects", name: "fk_e3e63f966e", on_delete: :cascade add_foreign_key "ci_triggers", "users", column: "owner_id", name: "fk_e8e10d1964", on_delete: :cascade add_foreign_key "ci_variables", "projects", name: "fk_ada5eb64b3", on_delete: :cascade add_foreign_key "container_repositories", "projects" + add_foreign_key "deploy_keys_projects", "projects", name: "fk_58a901ca7e", on_delete: :cascade + add_foreign_key "deployments", "projects", name: "fk_b9a3851b82", on_delete: :cascade + add_foreign_key "environments", "projects", name: "fk_d1c8c1da6a", on_delete: :cascade + add_foreign_key "events", "projects", name: "fk_0434b48643", on_delete: :cascade + add_foreign_key "forked_project_links", "projects", column: "forked_to_project_id", name: "fk_434510edb0", on_delete: :cascade add_foreign_key "issue_assignees", "issues", name: "fk_b7d881734a", on_delete: :cascade add_foreign_key "issue_assignees", "users", name: "fk_5e0c8d9154", on_delete: :cascade add_foreign_key "issue_metrics", "issues", on_delete: :cascade + add_foreign_key "issues", "projects", name: "fk_899c8f3231", on_delete: :cascade add_foreign_key "label_priorities", "labels", on_delete: :cascade add_foreign_key "label_priorities", "projects", on_delete: :cascade add_foreign_key "labels", "namespaces", column: "group_id", on_delete: :cascade - add_foreign_key "lists", "boards" - add_foreign_key "lists", "labels" + add_foreign_key "labels", "projects", name: "fk_7de4989a69", on_delete: :cascade + add_foreign_key "lists", "boards", name: "fk_0d3f677137", on_delete: :cascade + add_foreign_key "lists", "labels", name: "fk_7a5553d60f", on_delete: :cascade add_foreign_key "merge_request_diff_files", "merge_request_diffs", on_delete: :cascade + add_foreign_key "merge_request_diffs", "merge_requests", name: "fk_8483f3258f", on_delete: :cascade add_foreign_key "merge_request_metrics", "ci_pipelines", column: "pipeline_id", on_delete: :cascade add_foreign_key "merge_request_metrics", "merge_requests", on_delete: :cascade + add_foreign_key "merge_requests", "projects", column: "target_project_id", name: "fk_a6963e8447", on_delete: :cascade add_foreign_key "merge_requests_closing_issues", "issues", on_delete: :cascade add_foreign_key "merge_requests_closing_issues", "merge_requests", on_delete: :cascade + add_foreign_key "milestones", "projects", name: "fk_9bd0a0c791", on_delete: :cascade + add_foreign_key "notes", "projects", name: "fk_99e097b079", on_delete: :cascade add_foreign_key "oauth_openid_requests", "oauth_access_grants", column: "access_grant_id", name: "fk_oauth_openid_requests_oauth_access_grants_access_grant_id" + add_foreign_key "pages_domains", "projects", name: "fk_ea2f6dfc6f", on_delete: :cascade add_foreign_key "personal_access_tokens", "users" add_foreign_key "project_authorizations", "projects", on_delete: :cascade add_foreign_key "project_authorizations", "users", on_delete: :cascade + add_foreign_key "project_features", "projects", name: "fk_18513d9b92", on_delete: :cascade + add_foreign_key "project_group_links", "projects", name: "fk_daa8cee94c", on_delete: :cascade + add_foreign_key "project_import_data", "projects", name: "fk_ffb9ee3a10", on_delete: :cascade add_foreign_key "project_statistics", "projects", on_delete: :cascade - add_foreign_key "protected_branch_merge_access_levels", "protected_branches" - add_foreign_key "protected_branch_push_access_levels", "protected_branches" + add_foreign_key "protected_branch_merge_access_levels", "protected_branches", name: "fk_8a3072ccb3", on_delete: :cascade + add_foreign_key "protected_branch_push_access_levels", "protected_branches", name: "fk_9ffc86a3d9", on_delete: :cascade + add_foreign_key "protected_branches", "projects", name: "fk_7a9c6d93e7", on_delete: :cascade add_foreign_key "protected_tag_create_access_levels", "namespaces", column: "group_id" add_foreign_key "protected_tag_create_access_levels", "protected_tags" add_foreign_key "protected_tag_create_access_levels", "users" + add_foreign_key "protected_tags", "projects", name: "fk_8e4af87648", on_delete: :cascade + add_foreign_key "releases", "projects", name: "fk_47fe2a0596", on_delete: :cascade + add_foreign_key "services", "projects", name: "fk_71cce407f9", on_delete: :cascade + add_foreign_key "snippets", "projects", name: "fk_be41fd4bb7", on_delete: :cascade add_foreign_key "subscriptions", "projects", on_delete: :cascade add_foreign_key "system_note_metadata", "notes", name: "fk_d83a918cb1", on_delete: :cascade add_foreign_key "timelogs", "issues", name: "fk_timelogs_issues_issue_id", on_delete: :cascade add_foreign_key "timelogs", "merge_requests", name: "fk_timelogs_merge_requests_merge_request_id", on_delete: :cascade + add_foreign_key "todos", "projects", name: "fk_45054f9c45", on_delete: :cascade add_foreign_key "trending_projects", "projects", on_delete: :cascade add_foreign_key "u2f_registrations", "users" + add_foreign_key "users_star_projects", "projects", name: "fk_22cd27ddfc", on_delete: :cascade add_foreign_key "web_hook_logs", "web_hooks", on_delete: :cascade + add_foreign_key "web_hooks", "projects", name: "fk_0c8ca6d9d1", on_delete: :cascade end diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb index ac9303370ab..505039c9d88 100644 --- a/spec/models/concerns/issuable_spec.rb +++ b/spec/models/concerns/issuable_spec.rb @@ -155,7 +155,7 @@ describe Issuable do end describe "#sort" do - let(:project) { build_stubbed(:empty_project) } + let(:project) { create(:empty_project) } context "by milestone due date" do # Correct order is: diff --git a/spec/models/forked_project_link_spec.rb b/spec/models/forked_project_link_spec.rb index 5c13cf584f9..38fbdd2536a 100644 --- a/spec/models/forked_project_link_spec.rb +++ b/spec/models/forked_project_link_spec.rb @@ -42,7 +42,7 @@ describe ForkedProjectLink, "add link on fork" do describe '#forked?' do let(:project_to) { create(:project, forked_project_link: forked_project_link) } - let(:forked_project_link) { build(:forked_project_link) } + let(:forked_project_link) { create(:forked_project_link) } before do forked_project_link.forked_from_project = project_from @@ -59,9 +59,9 @@ describe ForkedProjectLink, "add link on fork" do end it "project_to.destroy destroys fork_link" do - expect(forked_project_link).to receive(:destroy) - project_to.destroy + + expect(ForkedProjectLink.exists?(id: forked_project_link.id)).to eq(false) end end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 587d4b83cb4..d91f1f1a11c 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -10,7 +10,7 @@ describe MergeRequest, models: true do it { is_expected.to belong_to(:source_project).class_name('Project') } it { is_expected.to belong_to(:merge_user).class_name("User") } it { is_expected.to belong_to(:assignee) } - it { is_expected.to have_many(:merge_request_diffs).dependent(:destroy) } + it { is_expected.to have_many(:merge_request_diffs) } end describe 'modules' do diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index f50b4aea411..75fa2d2eb46 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -7,50 +7,50 @@ describe Project, models: true do it { is_expected.to belong_to(:creator).class_name('User') } it { is_expected.to have_many(:users) } it { is_expected.to have_many(:services) } - it { is_expected.to have_many(:events).dependent(:destroy) } - it { is_expected.to have_many(:merge_requests).dependent(:destroy) } - it { is_expected.to have_many(:issues).dependent(:destroy) } - it { is_expected.to have_many(:milestones).dependent(:destroy) } - it { is_expected.to have_many(:project_members).dependent(:destroy) } + it { is_expected.to have_many(:events) } + it { is_expected.to have_many(:merge_requests) } + it { is_expected.to have_many(:issues) } + it { is_expected.to have_many(:milestones) } + it { is_expected.to have_many(:project_members).dependent(:delete_all) } it { is_expected.to have_many(:users).through(:project_members) } - it { is_expected.to have_many(:requesters).dependent(:destroy) } - it { is_expected.to have_many(:notes).dependent(:destroy) } - it { is_expected.to have_many(:snippets).class_name('ProjectSnippet').dependent(:destroy) } - it { is_expected.to have_many(:deploy_keys_projects).dependent(:destroy) } + it { is_expected.to have_many(:requesters).dependent(:delete_all) } + it { is_expected.to have_many(:notes) } + it { is_expected.to have_many(:snippets).class_name('ProjectSnippet') } + it { is_expected.to have_many(:deploy_keys_projects) } it { is_expected.to have_many(:deploy_keys) } - it { is_expected.to have_many(:hooks).dependent(:destroy) } - it { is_expected.to have_many(:protected_branches).dependent(:destroy) } - it { is_expected.to have_one(:forked_project_link).dependent(:destroy) } - it { is_expected.to have_one(:slack_service).dependent(:destroy) } - it { is_expected.to have_one(:microsoft_teams_service).dependent(:destroy) } - it { is_expected.to have_one(:mattermost_service).dependent(:destroy) } - it { is_expected.to have_one(:pushover_service).dependent(:destroy) } - it { is_expected.to have_one(:asana_service).dependent(:destroy) } - it { is_expected.to have_many(:boards).dependent(:destroy) } - it { is_expected.to have_one(:campfire_service).dependent(:destroy) } - it { is_expected.to have_one(:drone_ci_service).dependent(:destroy) } - it { is_expected.to have_one(:emails_on_push_service).dependent(:destroy) } - it { is_expected.to have_one(:pipelines_email_service).dependent(:destroy) } - it { is_expected.to have_one(:irker_service).dependent(:destroy) } - it { is_expected.to have_one(:pivotaltracker_service).dependent(:destroy) } - it { is_expected.to have_one(:hipchat_service).dependent(:destroy) } - it { is_expected.to have_one(:flowdock_service).dependent(:destroy) } - it { is_expected.to have_one(:assembla_service).dependent(:destroy) } - it { is_expected.to have_one(:slack_slash_commands_service).dependent(:destroy) } - it { is_expected.to have_one(:mattermost_slash_commands_service).dependent(:destroy) } - it { is_expected.to have_one(:gemnasium_service).dependent(:destroy) } - it { is_expected.to have_one(:buildkite_service).dependent(:destroy) } - it { is_expected.to have_one(:bamboo_service).dependent(:destroy) } - it { is_expected.to have_one(:teamcity_service).dependent(:destroy) } - it { is_expected.to have_one(:jira_service).dependent(:destroy) } - it { is_expected.to have_one(:redmine_service).dependent(:destroy) } - it { is_expected.to have_one(:custom_issue_tracker_service).dependent(:destroy) } - it { is_expected.to have_one(:bugzilla_service).dependent(:destroy) } - it { is_expected.to have_one(:gitlab_issue_tracker_service).dependent(:destroy) } - it { is_expected.to have_one(:external_wiki_service).dependent(:destroy) } - it { is_expected.to have_one(:project_feature).dependent(:destroy) } - it { is_expected.to have_one(:statistics).class_name('ProjectStatistics').dependent(:delete) } - it { is_expected.to have_one(:import_data).class_name('ProjectImportData').dependent(:delete) } + it { is_expected.to have_many(:hooks) } + it { is_expected.to have_many(:protected_branches) } + it { is_expected.to have_one(:forked_project_link) } + it { is_expected.to have_one(:slack_service) } + it { is_expected.to have_one(:microsoft_teams_service) } + it { is_expected.to have_one(:mattermost_service) } + it { is_expected.to have_one(:pushover_service) } + it { is_expected.to have_one(:asana_service) } + it { is_expected.to have_many(:boards) } + it { is_expected.to have_one(:campfire_service) } + it { is_expected.to have_one(:drone_ci_service) } + it { is_expected.to have_one(:emails_on_push_service) } + it { is_expected.to have_one(:pipelines_email_service) } + it { is_expected.to have_one(:irker_service) } + it { is_expected.to have_one(:pivotaltracker_service) } + it { is_expected.to have_one(:hipchat_service) } + it { is_expected.to have_one(:flowdock_service) } + it { is_expected.to have_one(:assembla_service) } + it { is_expected.to have_one(:slack_slash_commands_service) } + it { is_expected.to have_one(:mattermost_slash_commands_service) } + it { is_expected.to have_one(:gemnasium_service) } + it { is_expected.to have_one(:buildkite_service) } + it { is_expected.to have_one(:bamboo_service) } + it { is_expected.to have_one(:teamcity_service) } + it { is_expected.to have_one(:jira_service) } + it { is_expected.to have_one(:redmine_service) } + it { is_expected.to have_one(:custom_issue_tracker_service) } + it { is_expected.to have_one(:bugzilla_service) } + it { is_expected.to have_one(:gitlab_issue_tracker_service) } + it { is_expected.to have_one(:external_wiki_service) } + it { is_expected.to have_one(:project_feature) } + it { is_expected.to have_one(:statistics).class_name('ProjectStatistics') } + it { is_expected.to have_one(:import_data).class_name('ProjectImportData') } it { is_expected.to have_one(:last_event).class_name('Event') } it { is_expected.to have_one(:forked_from_project).through(:forked_project_link) } it { is_expected.to have_many(:commit_statuses) } @@ -62,18 +62,18 @@ describe Project, models: true do it { is_expected.to have_many(:variables) } it { is_expected.to have_many(:triggers) } it { is_expected.to have_many(:pages_domains) } - it { is_expected.to have_many(:labels).class_name('ProjectLabel').dependent(:destroy) } - it { is_expected.to have_many(:users_star_projects).dependent(:destroy) } - it { is_expected.to have_many(:environments).dependent(:destroy) } - it { is_expected.to have_many(:deployments).dependent(:destroy) } - it { is_expected.to have_many(:todos).dependent(:destroy) } - it { is_expected.to have_many(:releases).dependent(:destroy) } - it { is_expected.to have_many(:lfs_objects_projects).dependent(:destroy) } - it { is_expected.to have_many(:project_group_links).dependent(:destroy) } - it { is_expected.to have_many(:notification_settings).dependent(:destroy) } + it { is_expected.to have_many(:labels).class_name('ProjectLabel') } + it { is_expected.to have_many(:users_star_projects) } + it { is_expected.to have_many(:environments) } + it { is_expected.to have_many(:deployments) } + it { is_expected.to have_many(:todos) } + it { is_expected.to have_many(:releases) } + it { is_expected.to have_many(:lfs_objects_projects) } + it { is_expected.to have_many(:project_group_links) } + it { is_expected.to have_many(:notification_settings).dependent(:delete_all) } it { is_expected.to have_many(:forks).through(:forked_project_links) } it { is_expected.to have_many(:uploads).dependent(:destroy) } - it { is_expected.to have_many(:pipeline_schedules).dependent(:destroy) } + it { is_expected.to have_many(:pipeline_schedules) } context 'after initialized' do it "has a project_feature" do @@ -2199,4 +2199,21 @@ describe Project, models: true do end end end + + describe '#remove_private_deploy_keys' do + it 'removes the private deploy keys of a project' do + project = create(:empty_project) + + private_key = create(:deploy_key, public: false) + public_key = create(:deploy_key, public: true) + + create(:deploy_keys_project, deploy_key: private_key, project: project) + create(:deploy_keys_project, deploy_key: public_key, project: project) + + project.remove_private_deploy_keys + + expect(project.deploy_keys.where(public: false).any?).to eq(false) + expect(project.deploy_keys.where(public: true).any?).to eq(true) + end + end end diff --git a/spec/presenters/ci/build_presenter_spec.rb b/spec/presenters/ci/build_presenter_spec.rb index 518e97d17a1..f05d5c7fce5 100644 --- a/spec/presenters/ci/build_presenter_spec.rb +++ b/spec/presenters/ci/build_presenter_spec.rb @@ -85,7 +85,7 @@ describe Ci::BuildPresenter do describe 'quack like a Ci::Build permission-wise' do context 'user is not allowed' do - let(:project) { build_stubbed(:empty_project, public_builds: false) } + let(:project) { create(:empty_project, public_builds: false) } it 'returns false' do expect(presenter.can?(nil, :read_build)).to be_falsy @@ -93,7 +93,7 @@ describe Ci::BuildPresenter do end context 'user is allowed' do - let(:project) { build_stubbed(:empty_project, :public) } + let(:project) { create(:empty_project, :public) } it 'returns true' do expect(presenter.can?(nil, :read_build)).to be_truthy diff --git a/spec/workers/expire_build_instance_artifacts_worker_spec.rb b/spec/workers/expire_build_instance_artifacts_worker_spec.rb index 1d8da68883b..bed5c5e2ecb 100644 --- a/spec/workers/expire_build_instance_artifacts_worker_spec.rb +++ b/spec/workers/expire_build_instance_artifacts_worker_spec.rb @@ -30,20 +30,6 @@ describe ExpireBuildInstanceArtifactsWorker do expect(build.reload.artifacts_file_identifier).to be_nil end end - - context 'when associated project was removed' do - let(:build) do - create(:ci_build, :artifacts, artifacts_expiry) do |build| - build.project.pending_delete = true - end - end - - it 'does not remove artifacts' do - expect do - build.reload.artifacts_file - end.not_to raise_error - end - end end context 'with not yet expired artifacts' do -- cgit v1.2.3 From 8fbbf41e29f5e0f56b7eb9d37aadba856b68bcce Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Thu, 8 Jun 2017 17:16:27 +0200 Subject: Added Cop to blacklist the use of `dependent:` This is allowed for existing instances so we don't end up 76 offenses right away, but for new code one should _only_ use this if they _have_ to remove non database data. Even then it's usually better to do this in a service class as this gives you more control over how to remove the data (e.g. in bulk). --- app/models/appearance.rb | 2 +- app/models/board.rb | 2 +- app/models/ci/pipeline.rb | 2 +- app/models/ci/runner.rb | 2 +- app/models/concerns/awardable.rb | 2 +- app/models/concerns/issuable.rb | 6 +-- app/models/concerns/protected_ref.rb | 2 +- app/models/concerns/routable.rb | 4 +- app/models/concerns/spammable.rb | 2 +- app/models/concerns/subscribable.rb | 2 +- app/models/concerns/time_trackable.rb | 2 +- app/models/deploy_key.rb | 2 +- app/models/environment.rb | 2 +- app/models/group.rb | 10 ++-- app/models/hooks/web_hook.rb | 2 +- app/models/issue.rb | 9 +++- app/models/label.rb | 4 +- app/models/lfs_object.rb | 2 +- app/models/merge_request.rb | 6 ++- app/models/milestone.rb | 2 +- app/models/namespace.rb | 4 +- app/models/note.rb | 4 +- app/models/project.rb | 14 ++--- .../project_services/slash_commands_service.rb | 2 +- app/models/snippet.rb | 2 +- app/models/user.rb | 60 +++++++++++----------- rubocop/cop/active_record_dependent.rb | 26 ++++++++++ rubocop/rubocop.rb | 1 + spec/rubocop/cop/active_record_dependent_spec.rb | 33 ++++++++++++ 29 files changed, 140 insertions(+), 73 deletions(-) create mode 100644 rubocop/cop/active_record_dependent.rb create mode 100644 spec/rubocop/cop/active_record_dependent_spec.rb diff --git a/app/models/appearance.rb b/app/models/appearance.rb index c79326e8427..f9c48482be7 100644 --- a/app/models/appearance.rb +++ b/app/models/appearance.rb @@ -10,5 +10,5 @@ class Appearance < ActiveRecord::Base mount_uploader :logo, AttachmentUploader mount_uploader :header_logo, AttachmentUploader - has_many :uploads, as: :model, dependent: :destroy + has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent end diff --git a/app/models/board.rb b/app/models/board.rb index 18081a32157..97d0f550925 100644 --- a/app/models/board.rb +++ b/app/models/board.rb @@ -1,7 +1,7 @@ class Board < ActiveRecord::Base belongs_to :project - has_many :lists, -> { order(:list_type, :position) }, dependent: :delete_all + has_many :lists, -> { order(:list_type, :position) }, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent validates :project, presence: true diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index c5847dee7f7..b646b32fc64 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -14,7 +14,7 @@ module Ci has_many :stages has_many :statuses, class_name: 'CommitStatus', foreign_key: :commit_id has_many :builds, foreign_key: :commit_id - has_many :trigger_requests, dependent: :destroy, foreign_key: :commit_id + has_many :trigger_requests, dependent: :destroy, foreign_key: :commit_id # rubocop:disable Cop/ActiveRecordDependent # Merge requests for which the current pipeline is running against # the merge request's latest commit. diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index d12f96f3d0b..f5790f9744f 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -8,7 +8,7 @@ module Ci FORM_EDITABLE = %i[description tag_list active run_untagged locked].freeze has_many :builds - has_many :runner_projects, dependent: :destroy + has_many :runner_projects, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_many :projects, through: :runner_projects has_one :last_build, ->() { order('id DESC') }, class_name: 'Ci::Build' diff --git a/app/models/concerns/awardable.rb b/app/models/concerns/awardable.rb index a7fd0a15f0f..f4f9b037957 100644 --- a/app/models/concerns/awardable.rb +++ b/app/models/concerns/awardable.rb @@ -2,7 +2,7 @@ module Awardable extend ActiveSupport::Concern included do - has_many :award_emoji, -> { includes(:user).order(:id) }, as: :awardable, dependent: :destroy + has_many :award_emoji, -> { includes(:user).order(:id) }, as: :awardable, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent if self < Participable # By default we always load award_emoji user association diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 41c8b525273..23cb85600da 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -30,7 +30,7 @@ module Issuable belongs_to :updated_by, class_name: "User" belongs_to :last_edited_by, class_name: 'User' belongs_to :milestone - has_many :notes, as: :noteable, inverse_of: :noteable, dependent: :destroy do + has_many :notes, as: :noteable, inverse_of: :noteable, dependent: :destroy do # rubocop:disable Cop/ActiveRecordDependent def authors_loaded? # We check first if we're loaded to not load unnecessarily. loaded? && to_a.all? { |note| note.association(:author).loaded? } @@ -42,9 +42,9 @@ module Issuable end end - has_many :label_links, as: :target, dependent: :destroy + has_many :label_links, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_many :labels, through: :label_links - has_many :todos, as: :target, dependent: :destroy + has_many :todos, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_one :metrics diff --git a/app/models/concerns/protected_ref.rb b/app/models/concerns/protected_ref.rb index 47e71c58557..fc6b840f7a8 100644 --- a/app/models/concerns/protected_ref.rb +++ b/app/models/concerns/protected_ref.rb @@ -17,7 +17,7 @@ module ProtectedRef class_methods do def protected_ref_access_levels(*types) types.each do |type| - has_many :"#{type}_access_levels", dependent: :destroy + has_many :"#{type}_access_levels", dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent validates :"#{type}_access_levels", length: { is: 1, message: "are restricted to a single instance per #{self.model_name.human}." } diff --git a/app/models/concerns/routable.rb b/app/models/concerns/routable.rb index ee108f010a6..f5048d17d80 100644 --- a/app/models/concerns/routable.rb +++ b/app/models/concerns/routable.rb @@ -4,8 +4,8 @@ module Routable extend ActiveSupport::Concern included do - has_one :route, as: :source, autosave: true, dependent: :destroy - has_many :redirect_routes, as: :source, autosave: true, dependent: :destroy + has_one :route, as: :source, autosave: true, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent + has_many :redirect_routes, as: :source, autosave: true, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent validates_associated :route validates :route, presence: true diff --git a/app/models/concerns/spammable.rb b/app/models/concerns/spammable.rb index 647a6cad3d7..bd75f25a210 100644 --- a/app/models/concerns/spammable.rb +++ b/app/models/concerns/spammable.rb @@ -8,7 +8,7 @@ module Spammable end included do - has_one :user_agent_detail, as: :subject, dependent: :destroy + has_one :user_agent_detail, as: :subject, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent attr_accessor :spam attr_accessor :spam_log diff --git a/app/models/concerns/subscribable.rb b/app/models/concerns/subscribable.rb index f60a0f8f438..274b38a7708 100644 --- a/app/models/concerns/subscribable.rb +++ b/app/models/concerns/subscribable.rb @@ -9,7 +9,7 @@ module Subscribable extend ActiveSupport::Concern included do - has_many :subscriptions, dependent: :destroy, as: :subscribable + has_many :subscriptions, dependent: :destroy, as: :subscribable # rubocop:disable Cop/ActiveRecordDependent end def subscribed?(user, project = nil) diff --git a/app/models/concerns/time_trackable.rb b/app/models/concerns/time_trackable.rb index 9cf83440784..b517ddaebd7 100644 --- a/app/models/concerns/time_trackable.rb +++ b/app/models/concerns/time_trackable.rb @@ -18,7 +18,7 @@ module TimeTrackable validates :time_estimate, numericality: { message: 'has an invalid format' }, allow_nil: false validate :check_negative_time_spent - has_many :timelogs, dependent: :destroy + has_many :timelogs, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent end def spend_time(options) diff --git a/app/models/deploy_key.rb b/app/models/deploy_key.rb index 053f2a11aa0..51768dd96bc 100644 --- a/app/models/deploy_key.rb +++ b/app/models/deploy_key.rb @@ -1,5 +1,5 @@ class DeployKey < Key - has_many :deploy_keys_projects, dependent: :destroy + has_many :deploy_keys_projects, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_many :projects, through: :deploy_keys_projects scope :in_projects, ->(projects) { joins(:deploy_keys_projects).where('deploy_keys_projects.project_id in (?)', projects) } diff --git a/app/models/environment.rb b/app/models/environment.rb index eb24ff00ce3..e9ebf0637f3 100644 --- a/app/models/environment.rb +++ b/app/models/environment.rb @@ -6,7 +6,7 @@ class Environment < ActiveRecord::Base belongs_to :project, required: true, validate: true - has_many :deployments, dependent: :destroy + has_many :deployments, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_one :last_deployment, -> { order('deployments.id DESC') }, class_name: 'Deployment' before_validation :nullify_external_url diff --git a/app/models/group.rb b/app/models/group.rb index a6fdb30f84c..b93fce6100d 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -8,7 +8,7 @@ class Group < Namespace include Referable include SelectForProjectAuthorization - has_many :group_members, -> { where(requested_at: nil) }, dependent: :destroy, as: :source + has_many :group_members, -> { where(requested_at: nil) }, dependent: :destroy, as: :source # rubocop:disable Cop/ActiveRecordDependent alias_method :members, :group_members has_many :users, through: :group_members has_many :owners, @@ -16,11 +16,11 @@ class Group < Namespace through: :group_members, source: :user - has_many :requesters, -> { where.not(requested_at: nil) }, dependent: :destroy, as: :source, class_name: 'GroupMember' + has_many :requesters, -> { where.not(requested_at: nil) }, dependent: :destroy, as: :source, class_name: 'GroupMember' # rubocop:disable Cop/ActiveRecordDependent - has_many :project_group_links, dependent: :destroy + has_many :project_group_links, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_many :shared_projects, through: :project_group_links, source: :project - has_many :notification_settings, dependent: :destroy, as: :source + has_many :notification_settings, dependent: :destroy, as: :source # rubocop:disable Cop/ActiveRecordDependent has_many :labels, class_name: 'GroupLabel' validate :avatar_type, if: ->(user) { user.avatar.present? && user.avatar_changed? } @@ -31,7 +31,7 @@ class Group < Namespace validates :two_factor_grace_period, presence: true, numericality: { greater_than_or_equal_to: 0 } mount_uploader :avatar, AvatarUploader - has_many :uploads, as: :model, dependent: :destroy + has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent after_create :post_create_hook after_destroy :post_destroy_hook diff --git a/app/models/hooks/web_hook.rb b/app/models/hooks/web_hook.rb index 7503f3739c3..7a9f8997959 100644 --- a/app/models/hooks/web_hook.rb +++ b/app/models/hooks/web_hook.rb @@ -12,7 +12,7 @@ class WebHook < ActiveRecord::Base default_value_for :repository_update_events, false default_value_for :enable_ssl_verification, true - has_many :web_hook_logs, dependent: :destroy + has_many :web_hook_logs, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent scope :push_hooks, -> { where(push_events: true) } scope :tag_push_hooks, -> { where(tag_push_events: true) } diff --git a/app/models/issue.rb b/app/models/issue.rb index 31e3504e535..01f985823e1 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -23,9 +23,14 @@ class Issue < ActiveRecord::Base belongs_to :project belongs_to :moved_to, class_name: 'Issue' - has_many :events, as: :target, dependent: :destroy + has_many :events, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent - has_many :merge_requests_closing_issues, class_name: 'MergeRequestsClosingIssues' + has_many :merge_requests_closing_issues, + class_name: 'MergeRequestsClosingIssues', + dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent + + has_many :issue_assignees + has_many :assignees, class_name: "User", through: :issue_assignees has_many :issue_assignees has_many :assignees, class_name: "User", through: :issue_assignees diff --git a/app/models/label.rb b/app/models/label.rb index ed6a8411da9..674bb3f2720 100644 --- a/app/models/label.rb +++ b/app/models/label.rb @@ -15,9 +15,9 @@ class Label < ActiveRecord::Base default_value_for :color, DEFAULT_COLOR - has_many :lists, dependent: :destroy + has_many :lists, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_many :priorities, class_name: 'LabelPriority' - has_many :label_links, dependent: :destroy + has_many :label_links, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_many :issues, through: :label_links, source: :target, source_type: 'Issue' has_many :merge_requests, through: :label_links, source: :target, source_type: 'MergeRequest' diff --git a/app/models/lfs_object.rb b/app/models/lfs_object.rb index 7712d5783e0..b7cf96abe83 100644 --- a/app/models/lfs_object.rb +++ b/app/models/lfs_object.rb @@ -1,5 +1,5 @@ class LfsObject < ActiveRecord::Base - has_many :lfs_objects_projects, dependent: :destroy + has_many :lfs_objects_projects, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_many :projects, through: :lfs_objects_projects validates :oid, presence: true, uniqueness: true diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index d2534e9c858..fb69c3d2230 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -18,9 +18,11 @@ class MergeRequest < ActiveRecord::Base belongs_to :head_pipeline, foreign_key: "head_pipeline_id", class_name: "Ci::Pipeline" - has_many :events, as: :target, dependent: :destroy + has_many :events, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent - has_many :merge_requests_closing_issues, class_name: 'MergeRequestsClosingIssues' + has_many :merge_requests_closing_issues, + class_name: 'MergeRequestsClosingIssues', + dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent belongs_to :assignee, class_name: "User" diff --git a/app/models/milestone.rb b/app/models/milestone.rb index d2e2749f70d..c0ccbf8e27e 100644 --- a/app/models/milestone.rb +++ b/app/models/milestone.rb @@ -21,7 +21,7 @@ class Milestone < ActiveRecord::Base has_many :issues has_many :labels, -> { distinct.reorder('labels.title') }, through: :issues has_many :merge_requests - has_many :events, as: :target, dependent: :destroy + has_many :events, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent scope :active, -> { with_state(:active) } scope :closed, -> { with_state(:closed) } diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 672eab94c07..15713fc5f6d 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -15,13 +15,13 @@ class Namespace < ActiveRecord::Base cache_markdown_field :description, pipeline: :description - has_many :projects, dependent: :destroy + has_many :projects, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_many :project_statistics belongs_to :owner, class_name: "User" belongs_to :parent, class_name: "Namespace" has_many :children, class_name: "Namespace", foreign_key: :parent_id - has_one :chat_team, dependent: :destroy + has_one :chat_team, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent validates :owner, presence: true, unless: ->(n) { n.type == "Group" } validates :name, diff --git a/app/models/note.rb b/app/models/note.rb index dfd435bcdf6..3d39047d32f 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -46,8 +46,8 @@ class Note < ActiveRecord::Base belongs_to :updated_by, class_name: "User" belongs_to :last_edited_by, class_name: 'User' - has_many :todos, dependent: :destroy - has_many :events, as: :target, dependent: :destroy + has_many :todos, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent + has_many :events, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_one :system_note_metadata delegate :gfm_reference, :local_reference, to: :noteable diff --git a/app/models/project.rb b/app/models/project.rb index 181e9940ddb..5c673fdb7ba 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -138,26 +138,26 @@ class Project < ActiveRecord::Base has_many :project_authorizations has_many :authorized_users, through: :project_authorizations, source: :user, class_name: 'User' has_many :project_members, -> { where(requested_at: nil) }, - as: :source, dependent: :delete_all + as: :source, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent alias_method :members, :project_members has_many :users, through: :project_members has_many :requesters, -> { where.not(requested_at: nil) }, - as: :source, class_name: 'ProjectMember', dependent: :delete_all + as: :source, class_name: 'ProjectMember', dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent has_many :deploy_keys_projects has_many :deploy_keys, through: :deploy_keys_projects has_many :users_star_projects has_many :starrers, through: :users_star_projects, source: :user has_many :releases - has_many :lfs_objects_projects, dependent: :destroy + has_many :lfs_objects_projects, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_many :lfs_objects, through: :lfs_objects_projects has_many :project_group_links has_many :invited_groups, through: :project_group_links, source: :group has_many :pages_domains has_many :todos - has_many :notification_settings, as: :source, dependent: :delete_all + has_many :notification_settings, as: :source, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent has_one :import_data, class_name: 'ProjectImportData' has_one :project_feature @@ -166,7 +166,7 @@ class Project < ActiveRecord::Base # Container repositories need to remove data from the container registry, # which is not managed by the DB. Hence we're still using dependent: :destroy # here. - has_many :container_repositories, dependent: :destroy + has_many :container_repositories, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_many :commit_statuses has_many :pipelines, class_name: 'Ci::Pipeline' @@ -175,7 +175,7 @@ class Project < ActiveRecord::Base # build traces. Currently there's no efficient way of removing this data in # bulk that doesn't involve loading the rows into memory. As a result we're # still using `dependent: :destroy` here. - has_many :builds, class_name: 'Ci::Build', dependent: :destroy + has_many :builds, class_name: 'Ci::Build', dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_many :runner_projects, class_name: 'Ci::RunnerProject' has_many :runners, through: :runner_projects, source: :runner, class_name: 'Ci::Runner' has_many :variables, class_name: 'Ci::Variable' @@ -237,7 +237,7 @@ class Project < ActiveRecord::Base before_save :ensure_runners_token mount_uploader :avatar, AvatarUploader - has_many :uploads, as: :model, dependent: :destroy + has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent # Scopes scope :pending_delete, -> { where(pending_delete: true) } diff --git a/app/models/project_services/slash_commands_service.rb b/app/models/project_services/slash_commands_service.rb index 4592cb747a0..eb4da68bb7e 100644 --- a/app/models/project_services/slash_commands_service.rb +++ b/app/models/project_services/slash_commands_service.rb @@ -5,7 +5,7 @@ class SlashCommandsService < Service prop_accessor :token - has_many :chat_names, foreign_key: :service_id, dependent: :destroy + has_many :chat_names, foreign_key: :service_id, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent def valid_token?(token) self.respond_to?(:token) && diff --git a/app/models/snippet.rb b/app/models/snippet.rb index b3aa7bb986e..09d5ff46618 100644 --- a/app/models/snippet.rb +++ b/app/models/snippet.rb @@ -30,7 +30,7 @@ class Snippet < ActiveRecord::Base belongs_to :author, class_name: 'User' belongs_to :project - has_many :notes, as: :noteable, dependent: :destroy + has_many :notes, as: :noteable, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent delegate :name, :email, to: :author, prefix: true, allow_nil: true diff --git a/app/models/user.rb b/app/models/user.rb index 0febae84873..84dcc0fc26b 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -67,24 +67,24 @@ class User < ActiveRecord::Base # # Namespace for personal projects - has_one :namespace, -> { where type: nil }, dependent: :destroy, foreign_key: :owner_id, autosave: true + has_one :namespace, -> { where type: nil }, dependent: :destroy, foreign_key: :owner_id, autosave: true # rubocop:disable Cop/ActiveRecordDependent # Profile has_many :keys, -> do type = Key.arel_table[:type] where(type.not_eq('DeployKey').or(type.eq(nil))) - end, dependent: :destroy - has_many :deploy_keys, -> { where(type: 'DeployKey') }, dependent: :destroy + end, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent + has_many :deploy_keys, -> { where(type: 'DeployKey') }, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent - has_many :emails, dependent: :destroy - has_many :personal_access_tokens, dependent: :destroy - has_many :identities, dependent: :destroy, autosave: true - has_many :u2f_registrations, dependent: :destroy - has_many :chat_names, dependent: :destroy + has_many :emails, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent + has_many :personal_access_tokens, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent + has_many :identities, dependent: :destroy, autosave: true # rubocop:disable Cop/ActiveRecordDependent + has_many :u2f_registrations, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent + has_many :chat_names, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent # Groups - has_many :members, dependent: :destroy - has_many :group_members, -> { where(requested_at: nil) }, dependent: :destroy, source: 'GroupMember' + has_many :members, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent + has_many :group_members, -> { where(requested_at: nil) }, dependent: :destroy, source: 'GroupMember' # rubocop:disable Cop/ActiveRecordDependent has_many :groups, through: :group_members has_many :owned_groups, -> { where members: { access_level: Gitlab::Access::OWNER } }, through: :group_members, source: :group has_many :masters_groups, -> { where members: { access_level: Gitlab::Access::MASTER } }, through: :group_members, source: :group @@ -92,35 +92,35 @@ class User < ActiveRecord::Base # Projects has_many :groups_projects, through: :groups, source: :projects has_many :personal_projects, through: :namespace, source: :projects - has_many :project_members, -> { where(requested_at: nil) }, dependent: :destroy + has_many :project_members, -> { where(requested_at: nil) }, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_many :projects, through: :project_members has_many :created_projects, foreign_key: :creator_id, class_name: 'Project' - has_many :users_star_projects, dependent: :destroy + has_many :users_star_projects, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_many :starred_projects, through: :users_star_projects, source: :project has_many :project_authorizations has_many :authorized_projects, through: :project_authorizations, source: :project - has_many :snippets, dependent: :destroy, foreign_key: :author_id - has_many :notes, dependent: :destroy, foreign_key: :author_id - has_many :issues, dependent: :destroy, foreign_key: :author_id - has_many :merge_requests, dependent: :destroy, foreign_key: :author_id - has_many :events, dependent: :destroy, foreign_key: :author_id - has_many :subscriptions, dependent: :destroy + has_many :snippets, dependent: :destroy, foreign_key: :author_id # rubocop:disable Cop/ActiveRecordDependent + has_many :notes, dependent: :destroy, foreign_key: :author_id # rubocop:disable Cop/ActiveRecordDependent + has_many :issues, dependent: :destroy, foreign_key: :author_id # rubocop:disable Cop/ActiveRecordDependent + has_many :merge_requests, dependent: :destroy, foreign_key: :author_id # rubocop:disable Cop/ActiveRecordDependent + has_many :events, dependent: :destroy, foreign_key: :author_id # rubocop:disable Cop/ActiveRecordDependent + has_many :subscriptions, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_many :recent_events, -> { order "id DESC" }, foreign_key: :author_id, class_name: "Event" - has_many :oauth_applications, class_name: 'Doorkeeper::Application', as: :owner, dependent: :destroy - has_one :abuse_report, dependent: :destroy, foreign_key: :user_id - has_many :reported_abuse_reports, dependent: :destroy, foreign_key: :reporter_id, class_name: "AbuseReport" - has_many :spam_logs, dependent: :destroy - has_many :builds, dependent: :nullify, class_name: 'Ci::Build' - has_many :pipelines, dependent: :nullify, class_name: 'Ci::Pipeline' - has_many :todos, dependent: :destroy - has_many :notification_settings, dependent: :destroy - has_many :award_emoji, dependent: :destroy - has_many :triggers, dependent: :destroy, class_name: 'Ci::Trigger', foreign_key: :owner_id + has_many :oauth_applications, class_name: 'Doorkeeper::Application', as: :owner, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent + has_one :abuse_report, dependent: :destroy, foreign_key: :user_id # rubocop:disable Cop/ActiveRecordDependent + has_many :reported_abuse_reports, dependent: :destroy, foreign_key: :reporter_id, class_name: "AbuseReport" # rubocop:disable Cop/ActiveRecordDependent + has_many :spam_logs, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent + has_many :builds, dependent: :nullify, class_name: 'Ci::Build' # rubocop:disable Cop/ActiveRecordDependent + has_many :pipelines, dependent: :nullify, class_name: 'Ci::Pipeline' # rubocop:disable Cop/ActiveRecordDependent + has_many :todos, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent + has_many :notification_settings, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent + has_many :award_emoji, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent + has_many :triggers, dependent: :destroy, class_name: 'Ci::Trigger', foreign_key: :owner_id # rubocop:disable Cop/ActiveRecordDependent has_many :issue_assignees has_many :assigned_issues, class_name: "Issue", through: :issue_assignees, source: :issue - has_many :assigned_merge_requests, dependent: :nullify, foreign_key: :assignee_id, class_name: "MergeRequest" + has_many :assigned_merge_requests, dependent: :nullify, foreign_key: :assignee_id, class_name: "MergeRequest" # rubocop:disable Cop/ActiveRecordDependent # # Validations @@ -211,7 +211,7 @@ class User < ActiveRecord::Base end mount_uploader :avatar, AvatarUploader - has_many :uploads, as: :model, dependent: :destroy + has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent # Scopes scope :admins, -> { where(admin: true) } diff --git a/rubocop/cop/active_record_dependent.rb b/rubocop/cop/active_record_dependent.rb new file mode 100644 index 00000000000..8d15f150885 --- /dev/null +++ b/rubocop/cop/active_record_dependent.rb @@ -0,0 +1,26 @@ +require_relative '../model_helpers' + +module RuboCop + module Cop + # Cop that prevents the use of `dependent: ...` in ActiveRecord models. + class ActiveRecordDependent < RuboCop::Cop::Cop + include ModelHelpers + + MSG = 'Do not use `dependent: to remove associated data, ' \ + 'use foreign keys with cascading deletes instead'.freeze + + METHOD_NAMES = [:has_many, :has_one, :belongs_to].freeze + + def on_send(node) + return unless in_model?(node) + return unless METHOD_NAMES.include?(node.children[1]) + + node.children.last.each_node(:pair) do |pair| + key_name = pair.children[0].children[0] + + add_offense(pair, :expression) if key_name == :dependent + end + end + end + end +end diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb index 69b4b29507c..1a9bdd9325e 100644 --- a/rubocop/rubocop.rb +++ b/rubocop/rubocop.rb @@ -4,6 +4,7 @@ require_relative 'cop/activerecord_serialize' require_relative 'cop/redirect_with_status' require_relative 'cop/polymorphic_associations' require_relative 'cop/project_path_helper' +require_relative 'cop/active_record_dependent' require_relative 'cop/migration/add_column' require_relative 'cop/migration/add_column_with_default_to_large_table' require_relative 'cop/migration/add_concurrent_foreign_key' diff --git a/spec/rubocop/cop/active_record_dependent_spec.rb b/spec/rubocop/cop/active_record_dependent_spec.rb new file mode 100644 index 00000000000..599a032bfc5 --- /dev/null +++ b/spec/rubocop/cop/active_record_dependent_spec.rb @@ -0,0 +1,33 @@ +require 'spec_helper' +require 'rubocop' +require 'rubocop/rspec/support' +require_relative '../../../rubocop/cop/active_record_dependent' + +describe RuboCop::Cop::ActiveRecordDependent do + include CopHelper + + subject(:cop) { described_class.new } + + context 'inside the app/models directory' do + it 'registers an offense when dependent: is used' do + allow(cop).to receive(:in_model?).and_return(true) + + inspect_source(cop, 'belongs_to :foo, dependent: :destroy') + + aggregate_failures do + expect(cop.offenses.size).to eq(1) + expect(cop.offenses.map(&:line)).to eq([1]) + end + end + end + + context 'outside the app/models directory' do + it 'does nothing' do + allow(cop).to receive(:in_model?).and_return(false) + + inspect_source(cop, 'belongs_to :foo, dependent: :destroy') + + expect(cop.offenses).to be_empty + end + end +end -- cgit v1.2.3 From 6f1e00ea36bdcc39da955f7aa2add6a21432d190 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Fri, 9 Jun 2017 00:59:50 +0200 Subject: Update CI runner factories to not use invalid IDs These IDs point to non-existing rows, causing the foreign key constraints to fail. --- spec/factories/ci/runner_projects.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/factories/ci/runner_projects.rb b/spec/factories/ci/runner_projects.rb index 6712dd5d82e..33a17cf7ed5 100644 --- a/spec/factories/ci/runner_projects.rb +++ b/spec/factories/ci/runner_projects.rb @@ -1,6 +1,6 @@ FactoryGirl.define do factory :ci_runner_project, class: Ci::RunnerProject do - runner_id 1 - project_id 1 + runner factory: :ci_runner + project factory: :empty_project end end -- cgit v1.2.3 From e1a3bf30b6ea04f2c658729f65a0eb09847dd341 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Mon, 3 Jul 2017 16:01:41 +0200 Subject: Rename ActiverecordSerialize cop This cop has been renamed to ActiveRecordSerialize to match the way "ActiveRecord" is usually written. --- app/models/application_setting.rb | 14 +++++----- app/models/audit_event.rb | 2 +- app/models/ci/build.rb | 4 +-- app/models/ci/trigger_request.rb | 2 +- app/models/diff_note.rb | 6 ++--- app/models/event.rb | 2 +- app/models/hooks/web_hook_log.rb | 6 ++--- app/models/legacy_diff_note.rb | 2 +- app/models/merge_request.rb | 2 +- app/models/merge_request_diff.rb | 4 +-- app/models/personal_access_token.rb | 2 +- app/models/project_import_data.rb | 2 +- app/models/sent_notification.rb | 2 +- app/models/service.rb | 2 +- app/models/user.rb | 2 +- rubocop/cop/active_record_serialize.rb | 18 +++++++++++++ rubocop/cop/activerecord_serialize.rb | 18 ------------- rubocop/rubocop.rb | 2 +- spec/rubocop/cop/active_record_serialize_spec.rb | 33 ++++++++++++++++++++++++ spec/rubocop/cop/activerecord_serialize_spec.rb | 33 ------------------------ 20 files changed, 79 insertions(+), 79 deletions(-) create mode 100644 rubocop/cop/active_record_serialize.rb delete mode 100644 rubocop/cop/activerecord_serialize.rb create mode 100644 spec/rubocop/cop/active_record_serialize_spec.rb delete mode 100644 spec/rubocop/cop/activerecord_serialize_spec.rb diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 668caef0d2c..b0d7f7ef5f5 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -13,13 +13,13 @@ class ApplicationSetting < ActiveRecord::Base [\r\n] # any number of newline characters }x - serialize :restricted_visibility_levels # rubocop:disable Cop/ActiverecordSerialize - serialize :import_sources # rubocop:disable Cop/ActiverecordSerialize - serialize :disabled_oauth_sign_in_sources, Array # rubocop:disable Cop/ActiverecordSerialize - serialize :domain_whitelist, Array # rubocop:disable Cop/ActiverecordSerialize - serialize :domain_blacklist, Array # rubocop:disable Cop/ActiverecordSerialize - serialize :repository_storages # rubocop:disable Cop/ActiverecordSerialize - serialize :sidekiq_throttling_queues, Array # rubocop:disable Cop/ActiverecordSerialize + serialize :restricted_visibility_levels # rubocop:disable Cop/ActiveRecordSerialize + serialize :import_sources # rubocop:disable Cop/ActiveRecordSerialize + serialize :disabled_oauth_sign_in_sources, Array # rubocop:disable Cop/ActiveRecordSerialize + serialize :domain_whitelist, Array # rubocop:disable Cop/ActiveRecordSerialize + serialize :domain_blacklist, Array # rubocop:disable Cop/ActiveRecordSerialize + serialize :repository_storages # rubocop:disable Cop/ActiveRecordSerialize + serialize :sidekiq_throttling_queues, Array # rubocop:disable Cop/ActiveRecordSerialize cache_markdown_field :sign_in_text cache_markdown_field :help_page_text diff --git a/app/models/audit_event.rb b/app/models/audit_event.rb index 46d412fbd72..112a8778b4e 100644 --- a/app/models/audit_event.rb +++ b/app/models/audit_event.rb @@ -1,5 +1,5 @@ class AuditEvent < ActiveRecord::Base - serialize :details, Hash # rubocop:disable Cop/ActiverecordSerialize + serialize :details, Hash # rubocop:disable Cop/ActiveRecordSerialize belongs_to :user, foreign_key: :author_id diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 2e7a80d308b..5fb4c7e3cb3 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -19,8 +19,8 @@ module Ci ) end - serialize :options # rubocop:disable Cop/ActiverecordSerialize - serialize :yaml_variables, Gitlab::Serializer::Ci::Variables # rubocop:disable Cop/ActiverecordSerialize + serialize :options # rubocop:disable Cop/ActiveRecordSerialize + serialize :yaml_variables, Gitlab::Serializer::Ci::Variables # rubocop:disable Cop/ActiveRecordSerialize delegate :name, to: :project, prefix: true diff --git a/app/models/ci/trigger_request.rb b/app/models/ci/trigger_request.rb index 564334ad1ad..c58ce5c3717 100644 --- a/app/models/ci/trigger_request.rb +++ b/app/models/ci/trigger_request.rb @@ -6,7 +6,7 @@ module Ci belongs_to :pipeline, foreign_key: :commit_id has_many :builds - serialize :variables # rubocop:disable Cop/ActiverecordSerialize + serialize :variables # rubocop:disable Cop/ActiveRecordSerialize def user_variables return [] unless variables diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb index 20ef1378500..e9a60e6ce09 100644 --- a/app/models/diff_note.rb +++ b/app/models/diff_note.rb @@ -6,9 +6,9 @@ class DiffNote < Note NOTEABLE_TYPES = %w(MergeRequest Commit).freeze - serialize :original_position, Gitlab::Diff::Position # rubocop:disable Cop/ActiverecordSerialize - serialize :position, Gitlab::Diff::Position # rubocop:disable Cop/ActiverecordSerialize - serialize :change_position, Gitlab::Diff::Position # rubocop:disable Cop/ActiverecordSerialize + serialize :original_position, Gitlab::Diff::Position # rubocop:disable Cop/ActiveRecordSerialize + serialize :position, Gitlab::Diff::Position # rubocop:disable Cop/ActiveRecordSerialize + serialize :change_position, Gitlab::Diff::Position # rubocop:disable Cop/ActiveRecordSerialize validates :original_position, presence: true validates :position, presence: true diff --git a/app/models/event.rb b/app/models/event.rb index 29bc141c5cd..8d93a228494 100644 --- a/app/models/event.rb +++ b/app/models/event.rb @@ -50,7 +50,7 @@ class Event < ActiveRecord::Base belongs_to :target, polymorphic: true # rubocop:disable Cop/PolymorphicAssociations # For Hash only - serialize :data # rubocop:disable Cop/ActiverecordSerialize + serialize :data # rubocop:disable Cop/ActiveRecordSerialize # Callbacks after_create :reset_project_activity diff --git a/app/models/hooks/web_hook_log.rb b/app/models/hooks/web_hook_log.rb index d73cfcf630d..e72c125fb69 100644 --- a/app/models/hooks/web_hook_log.rb +++ b/app/models/hooks/web_hook_log.rb @@ -1,9 +1,9 @@ class WebHookLog < ActiveRecord::Base belongs_to :web_hook - serialize :request_headers, Hash # rubocop:disable Cop/ActiverecordSerialize - serialize :request_data, Hash # rubocop:disable Cop/ActiverecordSerialize - serialize :response_headers, Hash # rubocop:disable Cop/ActiverecordSerialize + serialize :request_headers, Hash # rubocop:disable Cop/ActiveRecordSerialize + serialize :request_data, Hash # rubocop:disable Cop/ActiveRecordSerialize + serialize :response_headers, Hash # rubocop:disable Cop/ActiveRecordSerialize validates :web_hook, presence: true diff --git a/app/models/legacy_diff_note.rb b/app/models/legacy_diff_note.rb index 2d5909ab25e..c36be956ff0 100644 --- a/app/models/legacy_diff_note.rb +++ b/app/models/legacy_diff_note.rb @@ -7,7 +7,7 @@ class LegacyDiffNote < Note include NoteOnDiff - serialize :st_diff # rubocop:disable Cop/ActiverecordSerialize + serialize :st_diff # rubocop:disable Cop/ActiveRecordSerialize validates :line_code, presence: true, line_code: true diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index fb69c3d2230..2752181ed79 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -26,7 +26,7 @@ class MergeRequest < ActiveRecord::Base belongs_to :assignee, class_name: "User" - serialize :merge_params, Hash # rubocop:disable Cop/ActiverecordSerialize + serialize :merge_params, Hash # rubocop:disable Cop/ActiveRecordSerialize after_create :ensure_merge_request_diff, unless: :importing? after_update :reload_diff_if_branch_changed diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index f1ee4d3f7a9..0cbf58a2325 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -12,8 +12,8 @@ class MergeRequestDiff < ActiveRecord::Base belongs_to :merge_request has_many :merge_request_diff_files, -> { order(:merge_request_diff_id, :relative_order) } - serialize :st_commits # rubocop:disable Cop/ActiverecordSerialize - serialize :st_diffs # rubocop:disable Cop/ActiverecordSerialize + serialize :st_commits # rubocop:disable Cop/ActiveRecordSerialize + serialize :st_diffs # rubocop:disable Cop/ActiveRecordSerialize state_machine :state, initial: :empty do state :collected diff --git a/app/models/personal_access_token.rb b/app/models/personal_access_token.rb index 6e13f9b2089..654be927ed8 100644 --- a/app/models/personal_access_token.rb +++ b/app/models/personal_access_token.rb @@ -3,7 +3,7 @@ class PersonalAccessToken < ActiveRecord::Base include TokenAuthenticatable add_authentication_token_field :token - serialize :scopes, Array # rubocop:disable Cop/ActiverecordSerialize + serialize :scopes, Array # rubocop:disable Cop/ActiveRecordSerialize belongs_to :user diff --git a/app/models/project_import_data.rb b/app/models/project_import_data.rb index e3cafd4d1c6..37730474324 100644 --- a/app/models/project_import_data.rb +++ b/app/models/project_import_data.rb @@ -10,7 +10,7 @@ class ProjectImportData < ActiveRecord::Base insecure_mode: true, algorithm: 'aes-256-cbc' - serialize :data, JSON # rubocop:disable Cop/ActiverecordSerialize + serialize :data, JSON # rubocop:disable Cop/ActiveRecordSerialize validates :project, presence: true diff --git a/app/models/sent_notification.rb b/app/models/sent_notification.rb index edde7bedbab..298569cb7a6 100644 --- a/app/models/sent_notification.rb +++ b/app/models/sent_notification.rb @@ -1,5 +1,5 @@ class SentNotification < ActiveRecord::Base - serialize :position, Gitlab::Diff::Position # rubocop:disable Cop/ActiverecordSerialize + serialize :position, Gitlab::Diff::Position # rubocop:disable Cop/ActiveRecordSerialize belongs_to :project belongs_to :noteable, polymorphic: true # rubocop:disable Cop/PolymorphicAssociations diff --git a/app/models/service.rb b/app/models/service.rb index 6a0b0a5c522..60fa81b18c4 100644 --- a/app/models/service.rb +++ b/app/models/service.rb @@ -2,7 +2,7 @@ # and implement a set of methods class Service < ActiveRecord::Base include Sortable - serialize :properties, JSON # rubocop:disable Cop/ActiverecordSerialize + serialize :properties, JSON # rubocop:disable Cop/ActiveRecordSerialize default_value_for :active, false default_value_for :push_events, true diff --git a/app/models/user.rb b/app/models/user.rb index 84dcc0fc26b..4411a06d429 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -41,7 +41,7 @@ class User < ActiveRecord::Base otp_secret_encryption_key: Gitlab::Application.secrets.otp_key_base devise :two_factor_backupable, otp_number_of_backup_codes: 10 - serialize :otp_backup_codes, JSON # rubocop:disable Cop/ActiverecordSerialize + serialize :otp_backup_codes, JSON # rubocop:disable Cop/ActiveRecordSerialize devise :lockable, :recoverable, :rememberable, :trackable, :validatable, :omniauthable, :confirmable, :registerable diff --git a/rubocop/cop/active_record_serialize.rb b/rubocop/cop/active_record_serialize.rb new file mode 100644 index 00000000000..204caf37f8b --- /dev/null +++ b/rubocop/cop/active_record_serialize.rb @@ -0,0 +1,18 @@ +require_relative '../model_helpers' + +module RuboCop + module Cop + # Cop that prevents the use of `serialize` in ActiveRecord models. + class ActiveRecordSerialize < RuboCop::Cop::Cop + include ModelHelpers + + MSG = 'Do not store serialized data in the database, use separate columns and/or tables instead'.freeze + + def on_send(node) + return unless in_model?(node) + + add_offense(node, :selector) if node.children[1] == :serialize + end + end + end +end diff --git a/rubocop/cop/activerecord_serialize.rb b/rubocop/cop/activerecord_serialize.rb deleted file mode 100644 index 9bdcc3b4c34..00000000000 --- a/rubocop/cop/activerecord_serialize.rb +++ /dev/null @@ -1,18 +0,0 @@ -require_relative '../model_helpers' - -module RuboCop - module Cop - # Cop that prevents the use of `serialize` in ActiveRecord models. - class ActiverecordSerialize < RuboCop::Cop::Cop - include ModelHelpers - - MSG = 'Do not store serialized data in the database, use separate columns and/or tables instead'.freeze - - def on_send(node) - return unless in_model?(node) - - add_offense(node, :selector) if node.children[1] == :serialize - end - end - end -end diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb index 1a9bdd9325e..1e70314598a 100644 --- a/rubocop/rubocop.rb +++ b/rubocop/rubocop.rb @@ -1,6 +1,6 @@ require_relative 'cop/custom_error_class' require_relative 'cop/gem_fetcher' -require_relative 'cop/activerecord_serialize' +require_relative 'cop/active_record_serialize' require_relative 'cop/redirect_with_status' require_relative 'cop/polymorphic_associations' require_relative 'cop/project_path_helper' diff --git a/spec/rubocop/cop/active_record_serialize_spec.rb b/spec/rubocop/cop/active_record_serialize_spec.rb new file mode 100644 index 00000000000..b94b25cecd0 --- /dev/null +++ b/spec/rubocop/cop/active_record_serialize_spec.rb @@ -0,0 +1,33 @@ +require 'spec_helper' +require 'rubocop' +require 'rubocop/rspec/support' +require_relative '../../../rubocop/cop/active_record_serialize' + +describe RuboCop::Cop::ActiveRecordSerialize do + include CopHelper + + subject(:cop) { described_class.new } + + context 'inside the app/models directory' do + it 'registers an offense when serialize is used' do + allow(cop).to receive(:in_model?).and_return(true) + + inspect_source(cop, 'serialize :foo') + + aggregate_failures do + expect(cop.offenses.size).to eq(1) + expect(cop.offenses.map(&:line)).to eq([1]) + end + end + end + + context 'outside the app/models directory' do + it 'does nothing' do + allow(cop).to receive(:in_model?).and_return(false) + + inspect_source(cop, 'serialize :foo') + + expect(cop.offenses).to be_empty + end + end +end diff --git a/spec/rubocop/cop/activerecord_serialize_spec.rb b/spec/rubocop/cop/activerecord_serialize_spec.rb deleted file mode 100644 index 5bd7e5fa926..00000000000 --- a/spec/rubocop/cop/activerecord_serialize_spec.rb +++ /dev/null @@ -1,33 +0,0 @@ -require 'spec_helper' -require 'rubocop' -require 'rubocop/rspec/support' -require_relative '../../../rubocop/cop/activerecord_serialize' - -describe RuboCop::Cop::ActiverecordSerialize do - include CopHelper - - subject(:cop) { described_class.new } - - context 'inside the app/models directory' do - it 'registers an offense when serialize is used' do - allow(cop).to receive(:in_model?).and_return(true) - - inspect_source(cop, 'serialize :foo') - - aggregate_failures do - expect(cop.offenses.size).to eq(1) - expect(cop.offenses.map(&:line)).to eq([1]) - end - end - end - - context 'outside the app/models directory' do - it 'does nothing' do - allow(cop).to receive(:in_model?).and_return(false) - - inspect_source(cop, 'serialize :foo') - - expect(cop.offenses).to be_empty - end - end -end -- cgit v1.2.3 From 9786e4fb0a35f65b48c0f13b66783fb8bb8f8bc8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= Date: Thu, 6 Jul 2017 10:45:24 +0000 Subject: Revert "Merge branch 'winh-mr-widget-no-pipeline' into 'master'" This reverts merge request !12127 --- .../components/mr_widget_pipeline.js | 9 +-- .../components/mr_widget_pipeline_spec.js | 66 ++++++---------------- 2 files changed, 19 insertions(+), 56 deletions(-) diff --git a/app/assets/javascripts/vue_merge_request_widget/components/mr_widget_pipeline.js b/app/assets/javascripts/vue_merge_request_widget/components/mr_widget_pipeline.js index e8b3cf2f729..c02e10128e2 100644 --- a/app/assets/javascripts/vue_merge_request_widget/components/mr_widget_pipeline.js +++ b/app/assets/javascripts/vue_merge_request_widget/components/mr_widget_pipeline.js @@ -17,9 +17,6 @@ export default { return hasCI && !ciStatus; }, - hasPipeline() { - return Object.keys(this.mr.pipeline || {}).length > 0; - }, svg() { return statusIconEntityMap.icon_status_failed; }, @@ -33,11 +30,7 @@ export default { template: `
- -