From db2433c36da6410c803163139e41228f9ae3f26b Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Tue, 2 Jan 2018 20:24:12 +0100 Subject: wip --- app/models/clusters/applications/prometheus.rb | 12 +++++ app/models/environment.rb | 12 +++++ app/models/project_services/prometheus_service.rb | 63 +++++++++++++++++++--- .../queries/additional_metrics_deployment_query.rb | 2 +- lib/gitlab/prometheus/queries/deployment_query.rb | 2 +- lib/gitlab/prometheus_client.rb | 36 ++++++------- 6 files changed, 99 insertions(+), 28 deletions(-) diff --git a/app/models/clusters/applications/prometheus.rb b/app/models/clusters/applications/prometheus.rb index 9b0787ee6ca..72651a92e54 100644 --- a/app/models/clusters/applications/prometheus.rb +++ b/app/models/clusters/applications/prometheus.rb @@ -14,6 +14,18 @@ module Clusters 'stable/prometheus' end + def namespace + Gitlab::Kubernetes::Helm::NAMESPACE + end + + def service_name + 'prometheus-prometheus-server' + end + + def service_port + 80 + end + def chart_values_file "#{Rails.root}/vendor/#{name}/values.yaml" end diff --git a/app/models/environment.rb b/app/models/environment.rb index bf69b4c50f0..d895550784d 100644 --- a/app/models/environment.rb +++ b/app/models/environment.rb @@ -163,6 +163,18 @@ class Environment < ActiveRecord::Base end end + def enabled_clusters + slug = self.slug + result = project.clusters.enabled.select do |cluster| + scope = cluster.environment_scope || '*' + File.fnmatch(scope, slug) + end + + # sort results by descending order based on environment_scope being longer + # thus more closely matching environment slug + result.sort_by { |cluster| cluster.environment_scope.length }.reverse! + end + def slug super.presence || generate_slug end diff --git a/app/models/project_services/prometheus_service.rb b/app/models/project_services/prometheus_service.rb index fa7b3f2bcaf..bcb19eb8909 100644 --- a/app/models/project_services/prometheus_service.rb +++ b/app/models/project_services/prometheus_service.rb @@ -54,12 +54,16 @@ class PrometheusService < MonitoringService { success: false, result: err } end + def with_reactive_cache(cl, *args) + yield calculate_reactive_cache(cl, *args) + end + def environment_metrics(environment) with_reactive_cache(Gitlab::Prometheus::Queries::EnvironmentQuery.name, environment.id, &method(:rename_data_to_metrics)) end def deployment_metrics(deployment) - metrics = with_reactive_cache(Gitlab::Prometheus::Queries::DeploymentQuery.name, deployment.id, &method(:rename_data_to_metrics)) + metrics = with_reactive_cache(Gitlab::Prometheus::Queries::DeploymentQuery.name, deployment.environment.id, deployment.id, &method(:rename_data_to_metrics)) metrics&.merge(deployment_time: deployment.created_at.to_i) || {} end @@ -68,18 +72,24 @@ class PrometheusService < MonitoringService end def additional_deployment_metrics(deployment) - with_reactive_cache(Gitlab::Prometheus::Queries::AdditionalMetricsDeploymentQuery.name, deployment.id, &:itself) + with_reactive_cache(Gitlab::Prometheus::Queries::AdditionalMetricsDeploymentQuery.name, deployment.environment.id, deployment.id, &:itself) end def matched_metrics - with_reactive_cache(Gitlab::Prometheus::Queries::MatchedMetricsQuery.name, &:itself) + with_reactive_cache(Gitlab::Prometheus::Queries::MatchedMetricsQuery.name, nil, &:itself) + end + + def manual_mode? + false end # Cache metrics for specific environment - def calculate_reactive_cache(query_class_name, *args) + def calculate_reactive_cache(query_class_name, environment_id, *args) return unless active? && project && !project.pending_delete? + client = client_for_environment(environment_id) + - data = Kernel.const_get(query_class_name).new(client).query(*args) + data = Kernel.const_get(query_class_name).new(client).query(environment_id, *args) { success: true, data: data, @@ -89,12 +99,51 @@ class PrometheusService < MonitoringService { success: false, result: err.message } end - def client - @prometheus ||= Gitlab::PrometheusClient.new(api_url: api_url) + def client(environment_id) + if manual_mode? + Gitlab::PrometheusClient.new(api_url: api_url) + else + cluster(environment_id) + end + end + + def find_cluster_with_prometheus(environment_id) + clusters = if environment_id + ::Environment.find_by(id: environment_id).try(:enabled_clusters) || [] + else + project.clusters.enabled.select { |c| c.environment_scope == '*' || c.environment_scope == '' } + end + + clusters.detect { |cluster| cluster.application_prometheus.installed? } end private + def client_for_environment(environment_id) + cluster = find_cluster_with_prometheus(environment_id) + return unless cluster + + prometheus = cluster.application_prometheus + + client_through_kube_proxy(cluster.kubeclient, + 'service', + prometheus.service_name, + prometheus.service_port, + prometheus.namespace) if cluster.kubeclient + end + + def client_through_kube_proxy(kube_client, kind, name, port, namespace = '') + rest_client = kube_client.rest_client + base_url = rest_client.url + proxy_url = kube_client.proxy_url(kind, name, port, namespace) + + Rails.logger.warn rest_client[proxy_url.sub(base_url, '')] + Rails.logger.warn proxy_url.sub(base_url, '') + + Gitlab::PrometheusClient.new(api_url: api_url, rest_client: rest_client[proxy_url.sub(base_url, '')], headers: kube_client.headers) + end + + def rename_data_to_metrics(metrics) metrics[:metrics] = metrics.delete :data metrics diff --git a/lib/gitlab/prometheus/queries/additional_metrics_deployment_query.rb b/lib/gitlab/prometheus/queries/additional_metrics_deployment_query.rb index 69d055c901c..294a6ae34ca 100644 --- a/lib/gitlab/prometheus/queries/additional_metrics_deployment_query.rb +++ b/lib/gitlab/prometheus/queries/additional_metrics_deployment_query.rb @@ -4,7 +4,7 @@ module Gitlab class AdditionalMetricsDeploymentQuery < BaseQuery include QueryAdditionalMetrics - def query(deployment_id) + def query(environment_id, deployment_id) Deployment.find_by(id: deployment_id).try do |deployment| query_metrics( common_query_context( diff --git a/lib/gitlab/prometheus/queries/deployment_query.rb b/lib/gitlab/prometheus/queries/deployment_query.rb index 170f483540e..6e6da593178 100644 --- a/lib/gitlab/prometheus/queries/deployment_query.rb +++ b/lib/gitlab/prometheus/queries/deployment_query.rb @@ -2,7 +2,7 @@ module Gitlab module Prometheus module Queries class DeploymentQuery < BaseQuery - def query(deployment_id) + def query(environment_id, deployment_id) Deployment.find_by(id: deployment_id).try do |deployment| environment_slug = deployment.environment.slug diff --git a/lib/gitlab/prometheus_client.rb b/lib/gitlab/prometheus_client.rb index aa94614bf18..8ec4515fb12 100644 --- a/lib/gitlab/prometheus_client.rb +++ b/lib/gitlab/prometheus_client.rb @@ -3,10 +3,12 @@ module Gitlab # Helper methods to interact with Prometheus network services & resources class PrometheusClient - attr_reader :api_url + attr_reader :api_url, :rest_client, :headers - def initialize(api_url:) + def initialize(api_url:, rest_client: nil, headers: nil) @api_url = api_url + @rest_client = rest_client || RestClient::Resource.new(api_url) + @headers = headers || {} end def ping @@ -40,24 +42,15 @@ module Gitlab private def json_api_get(type, args = {}) - get(join_api_url(type, args)) + path = ['api', 'v1', type].join('/') + get(path, args) rescue Errno::ECONNREFUSED raise PrometheusError, 'Connection refused' end - def join_api_url(type, args = {}) - url = URI.parse(api_url) - rescue URI::Error - raise PrometheusError, "Invalid API URL: #{api_url}" - else - url.path = [url.path.sub(%r{/+\z}, ''), 'api', 'v1', type].join('/') - url.query = args.to_query - - url.to_s - end - - def get(url) - handle_response(HTTParty.get(url)) + def get(path, args) + response = rest_client[path].get(headers.merge(params: args)) + handle_response(response) rescue SocketError raise PrometheusError, "Can't connect to #{url}" rescue OpenSSL::SSL::SSLError @@ -67,15 +60,20 @@ module Gitlab end def handle_response(response) - if response.code == 200 && response['status'] == 'success' - response['data'] || {} + json_data = json_response(response) + if response.code == 200 && json_data['status'] == 'success' + json_data['data'] || {} elsif response.code == 400 - raise PrometheusError, response['error'] || 'Bad data received' + raise PrometheusError, json_data['error'] || 'Bad data received' else raise PrometheusError, "#{response.code} - #{response.body}" end end + def json_response(response) + JSON.parse(response.body) + end + def get_result(expected_type) data = yield data['result'] if data['resultType'] == expected_type -- cgit v1.2.1 From b38b5ceb8e039283e90dd323327e59c8f608c103 Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Tue, 2 Jan 2018 21:42:24 +0100 Subject: Move client creation to Prometheus Application, manufacture proper rest client --- app/models/clusters/applications/prometheus.rb | 15 ++++++--- app/models/project_services/prometheus_service.rb | 37 +++++------------------ lib/gitlab/prometheus_client.rb | 14 +++------ 3 files changed, 22 insertions(+), 44 deletions(-) diff --git a/app/models/clusters/applications/prometheus.rb b/app/models/clusters/applications/prometheus.rb index 72651a92e54..94cac9277a5 100644 --- a/app/models/clusters/applications/prometheus.rb +++ b/app/models/clusters/applications/prometheus.rb @@ -14,10 +14,6 @@ module Clusters 'stable/prometheus' end - def namespace - Gitlab::Kubernetes::Helm::NAMESPACE - end - def service_name 'prometheus-prometheus-server' end @@ -33,6 +29,17 @@ module Clusters def install_command Gitlab::Kubernetes::Helm::InstallCommand.new(name, chart: chart, chart_values_file: chart_values_file) end + + def proxy_client + return unless cluster.kubeclient + + kube_client = cluster.kubeclient + proxy_url = kube_client.proxy_url('service', service_name, service_port, Gitlab::Kubernetes::Helm::NAMESPACE) + + # ensures headers containing auth data are appended to original k8s client options + options = kube_client.rest_client.options.merge(headers: kube_client.headers) + RestClient::Resource.new(proxy_url, options) + end end end end diff --git a/app/models/project_services/prometheus_service.rb b/app/models/project_services/prometheus_service.rb index bcb19eb8909..5e5aa92fd1a 100644 --- a/app/models/project_services/prometheus_service.rb +++ b/app/models/project_services/prometheus_service.rb @@ -86,7 +86,7 @@ class PrometheusService < MonitoringService # Cache metrics for specific environment def calculate_reactive_cache(query_class_name, environment_id, *args) return unless active? && project && !project.pending_delete? - client = client_for_environment(environment_id) + client = client(environment_id) data = Kernel.const_get(query_class_name).new(client).query(environment_id, *args) @@ -101,12 +101,16 @@ class PrometheusService < MonitoringService def client(environment_id) if manual_mode? - Gitlab::PrometheusClient.new(api_url: api_url) + Gitlab::PrometheusClient.new(RestClient::Resource.new(api_url)) else - cluster(environment_id) + cluster = find_cluster_with_prometheus(environment_id) + + Gitlab::PrometheusClient.new(cluster.application_prometheus.proxy_client) if cluster end end + private + def find_cluster_with_prometheus(environment_id) clusters = if environment_id ::Environment.find_by(id: environment_id).try(:enabled_clusters) || [] @@ -117,33 +121,6 @@ class PrometheusService < MonitoringService clusters.detect { |cluster| cluster.application_prometheus.installed? } end - private - - def client_for_environment(environment_id) - cluster = find_cluster_with_prometheus(environment_id) - return unless cluster - - prometheus = cluster.application_prometheus - - client_through_kube_proxy(cluster.kubeclient, - 'service', - prometheus.service_name, - prometheus.service_port, - prometheus.namespace) if cluster.kubeclient - end - - def client_through_kube_proxy(kube_client, kind, name, port, namespace = '') - rest_client = kube_client.rest_client - base_url = rest_client.url - proxy_url = kube_client.proxy_url(kind, name, port, namespace) - - Rails.logger.warn rest_client[proxy_url.sub(base_url, '')] - Rails.logger.warn proxy_url.sub(base_url, '') - - Gitlab::PrometheusClient.new(api_url: api_url, rest_client: rest_client[proxy_url.sub(base_url, '')], headers: kube_client.headers) - end - - def rename_data_to_metrics(metrics) metrics[:metrics] = metrics.delete :data metrics diff --git a/lib/gitlab/prometheus_client.rb b/lib/gitlab/prometheus_client.rb index 8ec4515fb12..d1875dd1042 100644 --- a/lib/gitlab/prometheus_client.rb +++ b/lib/gitlab/prometheus_client.rb @@ -3,12 +3,10 @@ module Gitlab # Helper methods to interact with Prometheus network services & resources class PrometheusClient - attr_reader :api_url, :rest_client, :headers + attr_reader :rest_client, :headers - def initialize(api_url:, rest_client: nil, headers: nil) - @api_url = api_url + def initialize(rest_client) @rest_client = rest_client || RestClient::Resource.new(api_url) - @headers = headers || {} end def ping @@ -49,7 +47,7 @@ module Gitlab end def get(path, args) - response = rest_client[path].get(headers.merge(params: args)) + response = rest_client[path].get(params: args) handle_response(response) rescue SocketError raise PrometheusError, "Can't connect to #{url}" @@ -60,7 +58,7 @@ module Gitlab end def handle_response(response) - json_data = json_response(response) + json_data = JSON.parse(response.body) if response.code == 200 && json_data['status'] == 'success' json_data['data'] || {} elsif response.code == 400 @@ -70,10 +68,6 @@ module Gitlab end end - def json_response(response) - JSON.parse(response.body) - end - def get_result(expected_type) data = yield data['result'] if data['resultType'] == expected_type -- cgit v1.2.1 From 0c802f4fba67e4fe54f9c4442d280fc7465e6b3f Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Tue, 2 Jan 2018 22:40:03 +0100 Subject: Manual Configuration instead of Activation. Prometheus Service just got a bit weirder --- app/controllers/concerns/service_params.rb | 1 + app/models/project_services/prometheus_service.rb | 36 ++++++++++++++++++----- 2 files changed, 29 insertions(+), 8 deletions(-) diff --git a/app/controllers/concerns/service_params.rb b/app/controllers/concerns/service_params.rb index 3d61458c064..c1acb50b76c 100644 --- a/app/controllers/concerns/service_params.rb +++ b/app/controllers/concerns/service_params.rb @@ -32,6 +32,7 @@ module ServiceParams :issues_events, :issues_url, :jira_issue_transition_id, + :manual_configuration, :merge_requests_events, :mock_service_url, :namespace, diff --git a/app/models/project_services/prometheus_service.rb b/app/models/project_services/prometheus_service.rb index 5e5aa92fd1a..9644c27cae8 100644 --- a/app/models/project_services/prometheus_service.rb +++ b/app/models/project_services/prometheus_service.rb @@ -7,19 +7,27 @@ class PrometheusService < MonitoringService # Access to prometheus is directly through the API prop_accessor :api_url + boolean_accessor :manual_configuration - with_options presence: true, if: :activated? do + with_options presence: true, if: :manual_configuration? do validates :api_url, url: true end + before_save :synchronize_service_state! + after_save :clear_reactive_cache! + def initialize_properties if properties.nil? self.properties = {} end end + def show_active_box? + false + end + def title 'Prometheus' end @@ -34,6 +42,18 @@ class PrometheusService < MonitoringService def fields [ + { type: 'fieldset', + legend: 'Manual Configuration', + fields: [ + { + type: 'checkbox', + name: 'manual_configuration', + title: s_('PrometheusService|Active'), + required: true + } + ] + }, + { type: 'text', name: 'api_url', @@ -79,16 +99,11 @@ class PrometheusService < MonitoringService with_reactive_cache(Gitlab::Prometheus::Queries::MatchedMetricsQuery.name, nil, &:itself) end - def manual_mode? - false - end - # Cache metrics for specific environment def calculate_reactive_cache(query_class_name, environment_id, *args) return unless active? && project && !project.pending_delete? client = client(environment_id) - data = Kernel.const_get(query_class_name).new(client).query(environment_id, *args) { success: true, @@ -100,12 +115,13 @@ class PrometheusService < MonitoringService end def client(environment_id) - if manual_mode? + if manual_configuration? Gitlab::PrometheusClient.new(RestClient::Resource.new(api_url)) else cluster = find_cluster_with_prometheus(environment_id) + raise Gitlab::PrometheusError, "couldn't find cluster with Prometheus installed" unless cluster - Gitlab::PrometheusClient.new(cluster.application_prometheus.proxy_client) if cluster + Gitlab::PrometheusClient.new(cluster.application_prometheus.proxy_client) end end @@ -125,4 +141,8 @@ class PrometheusService < MonitoringService metrics[:metrics] = metrics.delete :data metrics end + + def synchronize_service_state! + self.active = manual_configuration + end end -- cgit v1.2.1 From 249c9a8cf63b5b36b86499720804a5180e640537 Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Tue, 2 Jan 2018 22:56:12 +0100 Subject: Auto enable prometheus service if Prometheus is Installed --- app/models/project_services/prometheus_service.rb | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/app/models/project_services/prometheus_service.rb b/app/models/project_services/prometheus_service.rb index 9644c27cae8..abd2be2e7fe 100644 --- a/app/models/project_services/prometheus_service.rb +++ b/app/models/project_services/prometheus_service.rb @@ -8,6 +8,7 @@ class PrometheusService < MonitoringService # Access to prometheus is directly through the API prop_accessor :api_url boolean_accessor :manual_configuration + boolean_accessor :prometheus_installed with_options presence: true, if: :manual_configuration? do validates :api_url, url: true @@ -20,7 +21,7 @@ class PrometheusService < MonitoringService def initialize_properties if properties.nil? - self.properties = {} + self.properties = { prometheus_installed: false } end end @@ -118,7 +119,7 @@ class PrometheusService < MonitoringService if manual_configuration? Gitlab::PrometheusClient.new(RestClient::Resource.new(api_url)) else - cluster = find_cluster_with_prometheus(environment_id) + cluster = cluster_with_prometheus(environment_id) raise Gitlab::PrometheusError, "couldn't find cluster with Prometheus installed" unless cluster Gitlab::PrometheusClient.new(cluster.application_prometheus.proxy_client) @@ -127,7 +128,7 @@ class PrometheusService < MonitoringService private - def find_cluster_with_prometheus(environment_id) + def cluster_with_prometheus(environment_id = nil) clusters = if environment_id ::Environment.find_by(id: environment_id).try(:enabled_clusters) || [] else @@ -143,6 +144,7 @@ class PrometheusService < MonitoringService end def synchronize_service_state! - self.active = manual_configuration + self.active = prometheus_installed? || manual_configuration? || cluster_with_prometheus.present? + self.prometheus_installed = !manual_configuration? && cluster_with_prometheus.present? end end -- cgit v1.2.1 From 80d4c0675f5715d724be20d47cafa372524e3ed1 Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Thu, 4 Jan 2018 02:45:57 +0100 Subject: Add test checking if prometheus integration is enabled after prometheus is installed --- app/models/clusters/applications/prometheus.rb | 9 ++++++++ app/models/project_services/prometheus_service.rb | 20 +++++++++++------ .../projects/services/prometheus/_help.html.haml | 10 +++++++++ lib/gitlab/prometheus/queries/environment_query.rb | 26 ++++++++++------------ .../prometheus/queries/matched_metrics_query.rb | 2 +- .../additional_metrics_deployment_query_spec.rb | 2 +- .../prometheus/queries/deployment_query_spec.rb | 2 +- .../clusters/applications/prometheus_spec.rb | 18 +++++++++++++++ 8 files changed, 65 insertions(+), 24 deletions(-) create mode 100644 app/views/projects/services/prometheus/_help.html.haml diff --git a/app/models/clusters/applications/prometheus.rb b/app/models/clusters/applications/prometheus.rb index 94cac9277a5..ac9ea707f62 100644 --- a/app/models/clusters/applications/prometheus.rb +++ b/app/models/clusters/applications/prometheus.rb @@ -10,6 +10,15 @@ module Clusters default_value_for :version, VERSION + state_machine :status do + after_transition any => [:installed] do |application| + application.cluster.projects.each do |project| + # raise "exe" + project.prometheus_service&.update(active: true) + end + end + end + def chart 'stable/prometheus' end diff --git a/app/models/project_services/prometheus_service.rb b/app/models/project_services/prometheus_service.rb index abd2be2e7fe..2fb94c1facb 100644 --- a/app/models/project_services/prometheus_service.rb +++ b/app/models/project_services/prometheus_service.rb @@ -1,3 +1,7 @@ +module Gitlab + PrometheusError = Class.new(StandardError) +end + class PrometheusService < MonitoringService include ReactiveService @@ -8,7 +12,6 @@ class PrometheusService < MonitoringService # Access to prometheus is directly through the API prop_accessor :api_url boolean_accessor :manual_configuration - boolean_accessor :prometheus_installed with_options presence: true, if: :manual_configuration? do validates :api_url, url: true @@ -18,10 +21,9 @@ class PrometheusService < MonitoringService after_save :clear_reactive_cache! - def initialize_properties if properties.nil? - self.properties = { prometheus_installed: false } + self.properties = { } end end @@ -54,7 +56,6 @@ class PrometheusService < MonitoringService } ] }, - { type: 'text', name: 'api_url', @@ -126,6 +127,10 @@ class PrometheusService < MonitoringService end end + def prometheus_installed? + cluster_with_prometheus.present? + end + private def cluster_with_prometheus(environment_id = nil) @@ -135,7 +140,7 @@ class PrometheusService < MonitoringService project.clusters.enabled.select { |c| c.environment_scope == '*' || c.environment_scope == '' } end - clusters.detect { |cluster| cluster.application_prometheus.installed? } + clusters.detect { |cluster| cluster.application_prometheus&.installed? } end def rename_data_to_metrics(metrics) @@ -144,7 +149,8 @@ class PrometheusService < MonitoringService end def synchronize_service_state! - self.active = prometheus_installed? || manual_configuration? || cluster_with_prometheus.present? - self.prometheus_installed = !manual_configuration? && cluster_with_prometheus.present? + self.active = prometheus_installed? || self.manual_configuration? + + true end end diff --git a/app/views/projects/services/prometheus/_help.html.haml b/app/views/projects/services/prometheus/_help.html.haml new file mode 100644 index 00000000000..96a194b212e --- /dev/null +++ b/app/views/projects/services/prometheus/_help.html.haml @@ -0,0 +1,10 @@ +.row.prepend-top-default.append-bottom-default + %p + - unless @service.manual_configuration? + - if @service.prometheus_installed? + = link_to 'Manage installed Prometheus', project_clusters_path(@project), class: 'btn btn-cancel' + - else + = link_to 'Install Prometheus', project_clusters_path(@project), class: 'btn btn-cancel' + - else + To automatically install prometheus disable manual configuration + diff --git a/lib/gitlab/prometheus/queries/environment_query.rb b/lib/gitlab/prometheus/queries/environment_query.rb index 1d17d3cfd56..8f1453f31bf 100644 --- a/lib/gitlab/prometheus/queries/environment_query.rb +++ b/lib/gitlab/prometheus/queries/environment_query.rb @@ -2,22 +2,20 @@ module Gitlab module Prometheus module Queries class EnvironmentQuery < BaseQuery - def query(environment_id) - ::Environment.find_by(id: environment_id).try do |environment| - environment_slug = environment.slug - timeframe_start = 8.hours.ago.to_f - timeframe_end = Time.now.to_f + def query + environment_slug = environment.slug + timeframe_start = 8.hours.ago.to_f + timeframe_end = Time.now.to_f - memory_query = raw_memory_usage_query(environment_slug) - cpu_query = raw_cpu_usage_query(environment_slug) + memory_query = raw_memory_usage_query(environment_slug) + cpu_query = raw_cpu_usage_query(environment_slug) - { - memory_values: client_query_range(memory_query, start: timeframe_start, stop: timeframe_end), - memory_current: client_query(memory_query, time: timeframe_end), - cpu_values: client_query_range(cpu_query, start: timeframe_start, stop: timeframe_end), - cpu_current: client_query(cpu_query, time: timeframe_end) - } - end + { + memory_values: client_query_range(memory_query, start: timeframe_start, stop: timeframe_end), + memory_current: client_query(memory_query, time: timeframe_end), + cpu_values: client_query_range(cpu_query, start: timeframe_start, stop: timeframe_end), + cpu_current: client_query(cpu_query, time: timeframe_end) + } end end end diff --git a/lib/gitlab/prometheus/queries/matched_metrics_query.rb b/lib/gitlab/prometheus/queries/matched_metrics_query.rb index 4c3edccc71a..d21f64a252b 100644 --- a/lib/gitlab/prometheus/queries/matched_metrics_query.rb +++ b/lib/gitlab/prometheus/queries/matched_metrics_query.rb @@ -4,7 +4,7 @@ module Gitlab class MatchedMetricsQuery < BaseQuery MAX_QUERY_ITEMS = 40.freeze - def query + def query(_ = nil) groups_data.map do |group, data| { group: group.name, diff --git a/spec/lib/gitlab/prometheus/queries/additional_metrics_deployment_query_spec.rb b/spec/lib/gitlab/prometheus/queries/additional_metrics_deployment_query_spec.rb index c7169717fc1..0697cb2def6 100644 --- a/spec/lib/gitlab/prometheus/queries/additional_metrics_deployment_query_spec.rb +++ b/spec/lib/gitlab/prometheus/queries/additional_metrics_deployment_query_spec.rb @@ -7,7 +7,7 @@ describe Gitlab::Prometheus::Queries::AdditionalMetricsDeploymentQuery do include_examples 'additional metrics query' do let(:deployment) { create(:deployment, environment: environment) } - let(:query_params) { [deployment.id] } + let(:query_params) { [environment.id, deployment.id] } it 'queries using specific time' do expect(client).to receive(:query_range).with(anything, diff --git a/spec/lib/gitlab/prometheus/queries/deployment_query_spec.rb b/spec/lib/gitlab/prometheus/queries/deployment_query_spec.rb index ffe3ad85baa..d166327b6d3 100644 --- a/spec/lib/gitlab/prometheus/queries/deployment_query_spec.rb +++ b/spec/lib/gitlab/prometheus/queries/deployment_query_spec.rb @@ -31,7 +31,7 @@ describe Gitlab::Prometheus::Queries::DeploymentQuery do expect(client).to receive(:query).with('avg(rate(container_cpu_usage_seconds_total{container_name!="POD",environment="environment-slug"}[30m])) * 100', time: stop_time) - expect(subject.query(deployment.id)).to eq(memory_values: nil, memory_before: nil, memory_after: nil, + expect(subject.query(environment.id, deployment.id)).to eq(memory_values: nil, memory_before: nil, memory_after: nil, cpu_values: nil, cpu_before: nil, cpu_after: nil) end end diff --git a/spec/models/clusters/applications/prometheus_spec.rb b/spec/models/clusters/applications/prometheus_spec.rb index 696099f7cf7..b9ce597364e 100644 --- a/spec/models/clusters/applications/prometheus_spec.rb +++ b/spec/models/clusters/applications/prometheus_spec.rb @@ -6,6 +6,24 @@ describe Clusters::Applications::Prometheus do include_examples 'cluster application specs', described_class + describe 'transition to installed' do + let(:project) { create(:project) } + let(:cluster) { create(:cluster, projects: [project]) } + let(:prometheus_service) { double('prometheus_service') } + + subject { create(:clusters_applications_prometheus, :installing, cluster: cluster) } + + before do + allow(project).to receive(:prometheus_service).and_return prometheus_service + end + + it 'ensures Prometheus service is activated' do + expect(prometheus_service).to receive(:update).with(active: true) + + subject.make_installed + end + end + describe "#chart_values_file" do subject { create(:clusters_applications_prometheus).chart_values_file } -- cgit v1.2.1 From 9c0b10da4147470050c1ae144fc75c2963eeb4ba Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Thu, 4 Jan 2018 03:13:54 +0100 Subject: Fix prometheus client tests --- lib/gitlab/prometheus_client.rb | 17 +++++++++++++---- spec/lib/gitlab/prometheus_client_spec.rb | 14 ++++++++------ 2 files changed, 21 insertions(+), 10 deletions(-) diff --git a/lib/gitlab/prometheus_client.rb b/lib/gitlab/prometheus_client.rb index d1875dd1042..79f66b42c21 100644 --- a/lib/gitlab/prometheus_client.rb +++ b/lib/gitlab/prometheus_client.rb @@ -50,10 +50,12 @@ module Gitlab response = rest_client[path].get(params: args) handle_response(response) rescue SocketError - raise PrometheusError, "Can't connect to #{url}" + raise PrometheusError, "Can't connect to #{rest_client.url}" rescue OpenSSL::SSL::SSLError - raise PrometheusError, "#{url} contains invalid SSL data" - rescue HTTParty::Error + raise PrometheusError, "#{rest_client.url} contains invalid SSL data" + rescue RestClient::ExceptionWithResponse => ex + handle_exception_response(ex.response) + rescue RestClient::Exception raise PrometheusError, "Network connection error" end @@ -61,7 +63,14 @@ module Gitlab json_data = JSON.parse(response.body) if response.code == 200 && json_data['status'] == 'success' json_data['data'] || {} - elsif response.code == 400 + else + raise PrometheusError, "#{response.code} - #{response.body}" + end + end + + def handle_exception_response(response) + if response.code == 400 + json_data = JSON.parse(response.body) raise PrometheusError, json_data['error'] || 'Bad data received' else raise PrometheusError, "#{response.code} - #{response.body}" diff --git a/spec/lib/gitlab/prometheus_client_spec.rb b/spec/lib/gitlab/prometheus_client_spec.rb index de625324092..575b7805e33 100644 --- a/spec/lib/gitlab/prometheus_client_spec.rb +++ b/spec/lib/gitlab/prometheus_client_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe Gitlab::PrometheusClient do include PrometheusHelpers - subject { described_class.new(api_url: 'https://prometheus.example.com') } + subject { described_class.new(RestClient::Resource.new('https://prometheus.example.com')) } describe '#ping' do it 'issues a "query" request to the API endpoint' do @@ -52,11 +52,13 @@ describe Gitlab::PrometheusClient do describe 'failure to reach a provided prometheus url' do let(:prometheus_url) {"https://prometheus.invalid.example.com"} + subject { described_class.new(RestClient::Resource.new(prometheus_url)) } + context 'exceptions are raised' do it 'raises a Gitlab::PrometheusError error when a SocketError is rescued' do req_stub = stub_prometheus_request_with_exception(prometheus_url, SocketError) - expect { subject.send(:get, prometheus_url) } + expect { subject.send(:get, '/', {}) } .to raise_error(Gitlab::PrometheusError, "Can't connect to #{prometheus_url}") expect(req_stub).to have_been_requested end @@ -64,15 +66,15 @@ describe Gitlab::PrometheusClient do it 'raises a Gitlab::PrometheusError error when a SSLError is rescued' do req_stub = stub_prometheus_request_with_exception(prometheus_url, OpenSSL::SSL::SSLError) - expect { subject.send(:get, prometheus_url) } + expect { subject.send(:get, '/', {}) } .to raise_error(Gitlab::PrometheusError, "#{prometheus_url} contains invalid SSL data") expect(req_stub).to have_been_requested end - it 'raises a Gitlab::PrometheusError error when a HTTParty::Error is rescued' do - req_stub = stub_prometheus_request_with_exception(prometheus_url, HTTParty::Error) + it 'raises a Gitlab::PrometheusError error when a RestClient::Exception is rescued' do + req_stub = stub_prometheus_request_with_exception(prometheus_url, RestClient::Exception) - expect { subject.send(:get, prometheus_url) } + expect { subject.send(:get, '/', {}) } .to raise_error(Gitlab::PrometheusError, "Network connection error") expect(req_stub).to have_been_requested end -- cgit v1.2.1 From e308bb0cd2226297093ffb488a4d97e3c02c4883 Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Thu, 4 Jan 2018 13:56:07 +0100 Subject: Cleanup implementation and add cluster finding tests --- app/models/clusters/cluster.rb | 3 + app/models/environment.rb | 12 -- app/models/project_services/prometheus_service.rb | 22 +++- lib/gitlab/prometheus_client.rb | 2 +- .../project_services/prometheus_service_spec.rb | 127 +++++++++++++++++++++ 5 files changed, 147 insertions(+), 19 deletions(-) diff --git a/app/models/clusters/cluster.rb b/app/models/clusters/cluster.rb index 5ecbd4cbceb..8678f70f78c 100644 --- a/app/models/clusters/cluster.rb +++ b/app/models/clusters/cluster.rb @@ -49,6 +49,9 @@ module Clusters scope :enabled, -> { where(enabled: true) } scope :disabled, -> { where(enabled: false) } + scope :for_environment, -> (env) { where(environment_scope: ['*', '', env.slug]) } + scope :for_all_environments, -> { where(environment_scope: ['*', '']) } + def status_name if provider provider.status_name diff --git a/app/models/environment.rb b/app/models/environment.rb index d895550784d..bf69b4c50f0 100644 --- a/app/models/environment.rb +++ b/app/models/environment.rb @@ -163,18 +163,6 @@ class Environment < ActiveRecord::Base end end - def enabled_clusters - slug = self.slug - result = project.clusters.enabled.select do |cluster| - scope = cluster.environment_scope || '*' - File.fnmatch(scope, slug) - end - - # sort results by descending order based on environment_scope being longer - # thus more closely matching environment slug - result.sort_by { |cluster| cluster.environment_scope.length }.reverse! - end - def slug super.presence || generate_slug end diff --git a/app/models/project_services/prometheus_service.rb b/app/models/project_services/prometheus_service.rb index 2fb94c1facb..580e2c01aaa 100644 --- a/app/models/project_services/prometheus_service.rb +++ b/app/models/project_services/prometheus_service.rb @@ -116,31 +116,41 @@ class PrometheusService < MonitoringService { success: false, result: err.message } end - def client(environment_id) + def client(environment_id = nil) if manual_configuration? Gitlab::PrometheusClient.new(RestClient::Resource.new(api_url)) else cluster = cluster_with_prometheus(environment_id) raise Gitlab::PrometheusError, "couldn't find cluster with Prometheus installed" unless cluster + rest_client = client_from_cluster(cluster) - Gitlab::PrometheusClient.new(cluster.application_prometheus.proxy_client) + raise Gitlab::PrometheusError, "couldn't create proxy Prometheus client" unless rest_client + Gitlab::PrometheusClient.new(rest_client) end end def prometheus_installed? - cluster_with_prometheus.present? + project.clusters.enabled.any? { |cluster| cluster.application_prometheus&.installed? } end private def cluster_with_prometheus(environment_id = nil) clusters = if environment_id - ::Environment.find_by(id: environment_id).try(:enabled_clusters) || [] + ::Environment.find_by(id: environment_id).try do |env| + # sort results by descending order based on environment_scope being longer + # thus more closely matching environment slug + project.clusters.enabled.for_environment(env).sort_by { |c| c.environment_scope&.length }.reverse! + end else - project.clusters.enabled.select { |c| c.environment_scope == '*' || c.environment_scope == '' } + project.clusters.enabled.for_all_environments end - clusters.detect { |cluster| cluster.application_prometheus&.installed? } + clusters&.detect { |cluster| cluster.application_prometheus&.installed? } + end + + def client_from_cluster(cluster) + cluster.application_prometheus.proxy_client end def rename_data_to_metrics(metrics) diff --git a/lib/gitlab/prometheus_client.rb b/lib/gitlab/prometheus_client.rb index 79f66b42c21..8264501b1ae 100644 --- a/lib/gitlab/prometheus_client.rb +++ b/lib/gitlab/prometheus_client.rb @@ -6,7 +6,7 @@ module Gitlab attr_reader :rest_client, :headers def initialize(rest_client) - @rest_client = rest_client || RestClient::Resource.new(api_url) + @rest_client = rest_client end def ping diff --git a/spec/models/project_services/prometheus_service_spec.rb b/spec/models/project_services/prometheus_service_spec.rb index bf39e8d7a39..3cbd0bb75f7 100644 --- a/spec/models/project_services/prometheus_service_spec.rb +++ b/spec/models/project_services/prometheus_service_spec.rb @@ -8,6 +8,8 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do let(:service) { project.prometheus_service } let(:environment_query) { Gitlab::Prometheus::Queries::EnvironmentQuery } + subject { project.prometheus_service } + describe "Associations" do it { is_expected.to belong_to :project } end @@ -132,4 +134,129 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do end end end + + describe '#client' do + context 'manual configuration is enabled' do + let(:api_url) { 'http://some_url' } + before do + subject.manual_configuration = true + subject.api_url = api_url + end + + it 'returns simple rest client from api_url' do + expect(subject.client).to be_instance_of(Gitlab::PrometheusClient) + expect(subject.client.rest_client.url).to eq(api_url) + end + end + + context 'manual configuration is disabled' do + let!(:cluster_for_all) { create(:cluster, environment_scope: '*', projects: [project]) } + let!(:cluster_for_dev) { create(:cluster, environment_scope: 'dev', projects: [project]) } + + let!(:prometheus_for_dev) { create(:clusters_applications_prometheus, :installed, cluster: cluster_for_dev) } + let(:proxy_client) { double('proxy_client') } + + before do + subject.manual_configuration = false + end + + context 'with cluster for all environments with prometheus installed' do + let!(:prometheus_for_all) { create(:clusters_applications_prometheus, :installed, cluster: cluster_for_all) } + + context 'without environment supplied' do + it 'returns client handling all environments' do + expect(subject).to receive(:client_from_cluster).with(cluster_for_all).and_return(proxy_client).twice + + expect(subject.client).to be_instance_of(Gitlab::PrometheusClient) + expect(subject.client.rest_client).to eq(proxy_client) + end + end + + context 'with dev environment supplied' do + let!(:environment) { create(:environment, project: project, name: 'dev') } + + it 'returns dev cluster client' do + expect(subject).to receive(:client_from_cluster).with(cluster_for_dev).and_return(proxy_client).twice + + expect(subject.client(environment.id)).to be_instance_of(Gitlab::PrometheusClient) + expect(subject.client(environment.id).rest_client).to eq(proxy_client) + end + end + + context 'with prod environment supplied' do + let!(:environment) { create(:environment, project: project, name: 'prod') } + + it 'returns dev cluster client' do + expect(subject).to receive(:client_from_cluster).with(cluster_for_all).and_return(proxy_client).twice + + expect(subject.client(environment.id)).to be_instance_of(Gitlab::PrometheusClient) + expect(subject.client(environment.id).rest_client).to eq(proxy_client) + end + end + end + + context 'with cluster for all environments without prometheus installed' do + context 'without environment supplied' do + it 'raises PrometheusError because cluster was not found' do + expect{subject.client}.to raise_error(Gitlab::PrometheusError, /couldn't find cluster with Prometheus installed/) + end + end + + context 'with dev environment supplied' do + let!(:environment) { create(:environment, project: project, name: 'dev') } + + it 'returns dev cluster client' do + expect(subject).to receive(:client_from_cluster).with(cluster_for_dev).and_return(proxy_client).twice + + expect(subject.client(environment.id)).to be_instance_of(Gitlab::PrometheusClient) + expect(subject.client(environment.id).rest_client).to eq(proxy_client) + end + end + + context 'with prod environment supplied' do + let!(:environment) { create(:environment, project: project, name: 'prod') } + + it 'raises PrometheusError because cluster was not found' do + expect{subject.client}.to raise_error(Gitlab::PrometheusError, /couldn't find cluster with Prometheus installed/) + end + end + end + end + end + + describe '#prometheus_installed?' do + subject { project.prometheus_service } + + context 'clusters with installed prometheus' do + let!(:cluster) { create(:cluster, projects: [project]) } + let!(:prometheus) { create(:clusters_applications_prometheus, :installed, cluster: cluster) } + + it 'returns true' do + expect(subject.prometheus_installed?).to be(true) + end + end + + context 'clusters without prometheus installed' do + let(:cluster) { create(:cluster, projects: [project]) } + let!(:prometheus) { create(:clusters_applications_prometheus, cluster: cluster) } + + it 'returns false' do + expect(subject.prometheus_installed?).to be(false) + end + end + + context 'clusters without prometheus' do + let(:cluster) { create(:cluster, projects: [project]) } + + it 'returns false' do + expect(subject.prometheus_installed?).to be(false) + end + end + + context 'no clusters' do + it 'returns false' do + expect(subject.prometheus_installed?).to be(false) + end + end + end end -- cgit v1.2.1 From 720032733ad8fdb6124f2d0eee89807196552ad3 Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Thu, 4 Jan 2018 15:04:14 +0100 Subject: Cleanup PrometheusService tests --- app/models/project_services/prometheus_service.rb | 4 -- lib/gitlab/prometheus/queries/environment_query.rb | 26 ++++---- .../project_services/prometheus_service_spec.rb | 71 ++++++++++++---------- 3 files changed, 52 insertions(+), 49 deletions(-) diff --git a/app/models/project_services/prometheus_service.rb b/app/models/project_services/prometheus_service.rb index 580e2c01aaa..8bb1eb28900 100644 --- a/app/models/project_services/prometheus_service.rb +++ b/app/models/project_services/prometheus_service.rb @@ -76,10 +76,6 @@ class PrometheusService < MonitoringService { success: false, result: err } end - def with_reactive_cache(cl, *args) - yield calculate_reactive_cache(cl, *args) - end - def environment_metrics(environment) with_reactive_cache(Gitlab::Prometheus::Queries::EnvironmentQuery.name, environment.id, &method(:rename_data_to_metrics)) end diff --git a/lib/gitlab/prometheus/queries/environment_query.rb b/lib/gitlab/prometheus/queries/environment_query.rb index 8f1453f31bf..1d17d3cfd56 100644 --- a/lib/gitlab/prometheus/queries/environment_query.rb +++ b/lib/gitlab/prometheus/queries/environment_query.rb @@ -2,20 +2,22 @@ module Gitlab module Prometheus module Queries class EnvironmentQuery < BaseQuery - def query - environment_slug = environment.slug - timeframe_start = 8.hours.ago.to_f - timeframe_end = Time.now.to_f + def query(environment_id) + ::Environment.find_by(id: environment_id).try do |environment| + environment_slug = environment.slug + timeframe_start = 8.hours.ago.to_f + timeframe_end = Time.now.to_f - memory_query = raw_memory_usage_query(environment_slug) - cpu_query = raw_cpu_usage_query(environment_slug) + memory_query = raw_memory_usage_query(environment_slug) + cpu_query = raw_cpu_usage_query(environment_slug) - { - memory_values: client_query_range(memory_query, start: timeframe_start, stop: timeframe_end), - memory_current: client_query(memory_query, time: timeframe_end), - cpu_values: client_query_range(cpu_query, start: timeframe_start, stop: timeframe_end), - cpu_current: client_query(cpu_query, time: timeframe_end) - } + { + memory_values: client_query_range(memory_query, start: timeframe_start, stop: timeframe_end), + memory_current: client_query(memory_query, time: timeframe_end), + cpu_values: client_query_range(cpu_query, start: timeframe_start, stop: timeframe_end), + cpu_current: client_query(cpu_query, time: timeframe_end) + } + end end end end diff --git a/spec/models/project_services/prometheus_service_spec.rb b/spec/models/project_services/prometheus_service_spec.rb index 3cbd0bb75f7..1ea162f7059 100644 --- a/spec/models/project_services/prometheus_service_spec.rb +++ b/spec/models/project_services/prometheus_service_spec.rb @@ -8,24 +8,22 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do let(:service) { project.prometheus_service } let(:environment_query) { Gitlab::Prometheus::Queries::EnvironmentQuery } - subject { project.prometheus_service } - describe "Associations" do it { is_expected.to belong_to :project } end describe 'Validations' do - context 'when service is active' do + context 'when manual_configuration is enabled' do before do - subject.active = true + subject.manual_configuration = true end it { is_expected.to validate_presence_of(:api_url) } end - context 'when service is inactive' do + context 'when manual configuration is disabled' do before do - subject.active = false + subject.manual_configuration = false end it { is_expected.not_to validate_presence_of(:api_url) } @@ -33,12 +31,17 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do end describe '#test' do + before do + service.manual_configuration = true + end + let!(:req_stub) { stub_prometheus_request(prometheus_query_url('1'), body: prometheus_value_body('vector')) } context 'success' do it 'reads the discovery endpoint' do + expect(service.test[:result]).to eq('Checked API endpoint') expect(service.test[:success]).to be_truthy - expect(req_stub).to have_been_requested + expect(req_stub).to have_been_requested.twice end end @@ -85,7 +88,7 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do let(:fake_deployment_time) { 10 } before do - stub_reactive_cache(service, prometheus_data, deployment_query, deployment.id) + stub_reactive_cache(service, prometheus_data, deployment_query, deployment.environment.id, deployment.id) end it 'returns reactive data' do @@ -98,13 +101,17 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do describe '#calculate_reactive_cache' do let(:environment) { create(:environment, slug: 'env-slug') } - - around do |example| - Timecop.freeze { example.run } + before do + service.manual_configuration = true + service.active = true end subject do - service.calculate_reactive_cache(environment_query.to_s, environment.id) + service.calculate_reactive_cache(environment_query.name, environment.id) + end + + around do |example| + Timecop.freeze { example.run } end context 'when service is inactive' do @@ -157,7 +164,7 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do let(:proxy_client) { double('proxy_client') } before do - subject.manual_configuration = false + service.manual_configuration = false end context 'with cluster for all environments with prometheus installed' do @@ -165,10 +172,10 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do context 'without environment supplied' do it 'returns client handling all environments' do - expect(subject).to receive(:client_from_cluster).with(cluster_for_all).and_return(proxy_client).twice + expect(service).to receive(:client_from_cluster).with(cluster_for_all).and_return(proxy_client).twice - expect(subject.client).to be_instance_of(Gitlab::PrometheusClient) - expect(subject.client.rest_client).to eq(proxy_client) + expect(service.client).to be_instance_of(Gitlab::PrometheusClient) + expect(service.client.rest_client).to eq(proxy_client) end end @@ -176,10 +183,10 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do let!(:environment) { create(:environment, project: project, name: 'dev') } it 'returns dev cluster client' do - expect(subject).to receive(:client_from_cluster).with(cluster_for_dev).and_return(proxy_client).twice + expect(service).to receive(:client_from_cluster).with(cluster_for_dev).and_return(proxy_client).twice - expect(subject.client(environment.id)).to be_instance_of(Gitlab::PrometheusClient) - expect(subject.client(environment.id).rest_client).to eq(proxy_client) + expect(service.client(environment.id)).to be_instance_of(Gitlab::PrometheusClient) + expect(service.client(environment.id).rest_client).to eq(proxy_client) end end @@ -187,10 +194,10 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do let!(:environment) { create(:environment, project: project, name: 'prod') } it 'returns dev cluster client' do - expect(subject).to receive(:client_from_cluster).with(cluster_for_all).and_return(proxy_client).twice + expect(service).to receive(:client_from_cluster).with(cluster_for_all).and_return(proxy_client).twice - expect(subject.client(environment.id)).to be_instance_of(Gitlab::PrometheusClient) - expect(subject.client(environment.id).rest_client).to eq(proxy_client) + expect(service.client(environment.id)).to be_instance_of(Gitlab::PrometheusClient) + expect(service.client(environment.id).rest_client).to eq(proxy_client) end end end @@ -198,7 +205,7 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do context 'with cluster for all environments without prometheus installed' do context 'without environment supplied' do it 'raises PrometheusError because cluster was not found' do - expect{subject.client}.to raise_error(Gitlab::PrometheusError, /couldn't find cluster with Prometheus installed/) + expect{service.client}.to raise_error(Gitlab::PrometheusError, /couldn't find cluster with Prometheus installed/) end end @@ -206,10 +213,10 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do let!(:environment) { create(:environment, project: project, name: 'dev') } it 'returns dev cluster client' do - expect(subject).to receive(:client_from_cluster).with(cluster_for_dev).and_return(proxy_client).twice + expect(service).to receive(:client_from_cluster).with(cluster_for_dev).and_return(proxy_client).twice - expect(subject.client(environment.id)).to be_instance_of(Gitlab::PrometheusClient) - expect(subject.client(environment.id).rest_client).to eq(proxy_client) + expect(service.client(environment.id)).to be_instance_of(Gitlab::PrometheusClient) + expect(service.client(environment.id).rest_client).to eq(proxy_client) end end @@ -217,7 +224,7 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do let!(:environment) { create(:environment, project: project, name: 'prod') } it 'raises PrometheusError because cluster was not found' do - expect{subject.client}.to raise_error(Gitlab::PrometheusError, /couldn't find cluster with Prometheus installed/) + expect{service.client}.to raise_error(Gitlab::PrometheusError, /couldn't find cluster with Prometheus installed/) end end end @@ -225,14 +232,12 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do end describe '#prometheus_installed?' do - subject { project.prometheus_service } - context 'clusters with installed prometheus' do let!(:cluster) { create(:cluster, projects: [project]) } let!(:prometheus) { create(:clusters_applications_prometheus, :installed, cluster: cluster) } it 'returns true' do - expect(subject.prometheus_installed?).to be(true) + expect(service.prometheus_installed?).to be(true) end end @@ -241,7 +246,7 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do let!(:prometheus) { create(:clusters_applications_prometheus, cluster: cluster) } it 'returns false' do - expect(subject.prometheus_installed?).to be(false) + expect(service.prometheus_installed?).to be(false) end end @@ -249,13 +254,13 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do let(:cluster) { create(:cluster, projects: [project]) } it 'returns false' do - expect(subject.prometheus_installed?).to be(false) + expect(service.prometheus_installed?).to be(false) end end context 'no clusters' do it 'returns false' do - expect(subject.prometheus_installed?).to be(false) + expect(service.prometheus_installed?).to be(false) end end end -- cgit v1.2.1 From ae9c8277d9c9fdc1ee9f190b610f756fbc999863 Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Thu, 4 Jan 2018 15:46:32 +0100 Subject: add tests for Manual configuration override and service activation synchronization --- .../project_services/prometheus_service_spec.rb | 66 ++++++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/spec/models/project_services/prometheus_service_spec.rb b/spec/models/project_services/prometheus_service_spec.rb index 1ea162f7059..aff03e28cc4 100644 --- a/spec/models/project_services/prometheus_service_spec.rb +++ b/spec/models/project_services/prometheus_service_spec.rb @@ -264,4 +264,70 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do end end end + + describe '#synchronize_service_state! before_save callback' do + context 'no clusters with prometheus are installed' do + context 'when service is inactive' do + before do + service.active = false + end + + it 'activates service when manual_configuration is enabled' do + expect { service.update!(manual_configuration: true) }.to change { service.active }.from(false).to(true) + end + + it 'keeps service inactive when manual_configuration is disabled' do + expect { service.update!(manual_configuration: false) }.not_to change { service.active }.from(false) + end + end + + context 'when service is active' do + before do + service.active = true + end + + it 'keeps the service active when manual_configuration is enabled' do + expect { service.update!(manual_configuration: true) }.not_to change { service.active }.from(true) + end + + it 'inactivates the service when manual_configuration is disabled' do + expect { service.update!(manual_configuration: false) }.to change { service.active }.from(true).to(false) + end + end + end + + context 'with prometheus installed in the cluster' do + before do + allow(service).to receive(:prometheus_installed?).and_return(true) + end + + context 'when service is inactive' do + before do + service.active = false + end + + it 'activates service when manual_configuration is enabled' do + expect { service.update!(manual_configuration: true) }.to change { service.active }.from(false).to(true) + end + + it 'activates service when manual_configuration is disabled' do + expect { service.update!(manual_configuration: false) }.to change { service.active }.from(false).to(true) + end + end + + context 'when service is active' do + before do + service.active = true + end + + it 'keeps service active when manual_configuration is enabled' do + expect { service.update!(manual_configuration: true) }.not_to change { service.active }.from(true) + end + + it 'keeps service active when manual_configuration is disabled' do + expect { service.update!(manual_configuration: false) }.not_to change { service.active }.from(true) + end + end + end + end end -- cgit v1.2.1 From 09473b192c70ada66148dace8c6196ccabfa1dd9 Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Thu, 4 Jan 2018 17:01:13 +0100 Subject: Test Prometheus proxy client generation --- app/models/clusters/applications/prometheus.rb | 9 +++- .../clusters/applications/prometheus_spec.rb | 51 ++++++++++++++++++++++ 2 files changed, 58 insertions(+), 2 deletions(-) diff --git a/app/models/clusters/applications/prometheus.rb b/app/models/clusters/applications/prometheus.rb index ac9ea707f62..8a135237b89 100644 --- a/app/models/clusters/applications/prometheus.rb +++ b/app/models/clusters/applications/prometheus.rb @@ -40,15 +40,20 @@ module Clusters end def proxy_client - return unless cluster.kubeclient + return unless kube_client - kube_client = cluster.kubeclient proxy_url = kube_client.proxy_url('service', service_name, service_port, Gitlab::Kubernetes::Helm::NAMESPACE) # ensures headers containing auth data are appended to original k8s client options options = kube_client.rest_client.options.merge(headers: kube_client.headers) RestClient::Resource.new(proxy_url, options) end + + private + + def kube_client + cluster&.kubeclient + end end end end diff --git a/spec/models/clusters/applications/prometheus_spec.rb b/spec/models/clusters/applications/prometheus_spec.rb index b9ce597364e..1fdfb262618 100644 --- a/spec/models/clusters/applications/prometheus_spec.rb +++ b/spec/models/clusters/applications/prometheus_spec.rb @@ -31,4 +31,55 @@ describe Clusters::Applications::Prometheus do expect(subject).to eq("#{Rails.root}/vendor/prometheus/values.yaml") end end + + describe '#proxy_client' do + context 'cluster is nil' do + it 'returns nil' do + expect(subject.cluster).to be_nil + expect(subject.proxy_client).to be_nil + end + end + + context "cluster doesn't have kubeclient" do + let(:cluster) { create(:cluster) } + subject { create(:clusters_applications_prometheus, cluster: cluster) } + + it 'returns nil' do + expect(subject.proxy_client).to be_nil + end + end + + context 'cluster has kubeclient' do + let(:kubernetes_url) { 'http://example.com' } + let(:k8s_discover_response) { { + resources: [ + { + name: 'service', + kind: 'Service' + }] + } } + + let(:kube_client) { Kubeclient::Client.new(kubernetes_url) } + + let(:cluster) { create(:cluster) } + subject { create(:clusters_applications_prometheus, cluster: cluster) } + + before do + allow(kube_client.rest_client).to receive(:get).and_return(k8s_discover_response.to_json) + allow(subject.cluster).to receive(:kubeclient).and_return(kube_client) + end + + it 'creates proxy prometheus rest client' do + expect(subject.proxy_client).to be_instance_of(RestClient::Resource) + end + + it 'creates proper url' do + expect(subject.proxy_client.url).to eq('http://example.com/api/v1/proxy/namespaces/gitlab-managed-apps/service/prometheus-prometheus-server:80') + end + + it 'copies options and headers from kube client to proxy client' do + expect(subject.proxy_client.options).to eq(kube_client.rest_client.options.merge(headers: kube_client.headers)) + end + end + end end -- cgit v1.2.1 From 57c1f7cae09fe60a4be8d99b64833779476f402d Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Thu, 4 Jan 2018 17:11:39 +0100 Subject: Fix rubocop warnings --- app/models/project_services/prometheus_service.rb | 9 ++++++--- .../gitlab/prometheus/queries/deployment_query_spec.rb | 2 +- spec/models/clusters/applications/prometheus_spec.rb | 17 ++++++++++------- spec/models/project_services/prometheus_service_spec.rb | 4 ++-- 4 files changed, 19 insertions(+), 13 deletions(-) diff --git a/app/models/project_services/prometheus_service.rb b/app/models/project_services/prometheus_service.rb index 8bb1eb28900..3bf44c1ab2b 100644 --- a/app/models/project_services/prometheus_service.rb +++ b/app/models/project_services/prometheus_service.rb @@ -23,7 +23,7 @@ class PrometheusService < MonitoringService def initialize_properties if properties.nil? - self.properties = { } + self.properties = {} end end @@ -45,7 +45,8 @@ class PrometheusService < MonitoringService def fields [ - { type: 'fieldset', + { + type: 'fieldset', legend: 'Manual Configuration', fields: [ { @@ -100,6 +101,7 @@ class PrometheusService < MonitoringService # Cache metrics for specific environment def calculate_reactive_cache(query_class_name, environment_id, *args) return unless active? && project && !project.pending_delete? + client = client(environment_id) data = Kernel.const_get(query_class_name).new(client).query(environment_id, *args) @@ -118,9 +120,10 @@ class PrometheusService < MonitoringService else cluster = cluster_with_prometheus(environment_id) raise Gitlab::PrometheusError, "couldn't find cluster with Prometheus installed" unless cluster - rest_client = client_from_cluster(cluster) + rest_client = client_from_cluster(cluster) raise Gitlab::PrometheusError, "couldn't create proxy Prometheus client" unless rest_client + Gitlab::PrometheusClient.new(rest_client) end end diff --git a/spec/lib/gitlab/prometheus/queries/deployment_query_spec.rb b/spec/lib/gitlab/prometheus/queries/deployment_query_spec.rb index d166327b6d3..84dc31d9732 100644 --- a/spec/lib/gitlab/prometheus/queries/deployment_query_spec.rb +++ b/spec/lib/gitlab/prometheus/queries/deployment_query_spec.rb @@ -32,6 +32,6 @@ describe Gitlab::Prometheus::Queries::DeploymentQuery do time: stop_time) expect(subject.query(environment.id, deployment.id)).to eq(memory_values: nil, memory_before: nil, memory_after: nil, - cpu_values: nil, cpu_before: nil, cpu_after: nil) + cpu_values: nil, cpu_before: nil, cpu_after: nil) end end diff --git a/spec/models/clusters/applications/prometheus_spec.rb b/spec/models/clusters/applications/prometheus_spec.rb index 1fdfb262618..1102249493c 100644 --- a/spec/models/clusters/applications/prometheus_spec.rb +++ b/spec/models/clusters/applications/prometheus_spec.rb @@ -51,13 +51,16 @@ describe Clusters::Applications::Prometheus do context 'cluster has kubeclient' do let(:kubernetes_url) { 'http://example.com' } - let(:k8s_discover_response) { { - resources: [ - { - name: 'service', - kind: 'Service' - }] - } } + let(:k8s_discover_response) do + { + resources: [ + { + name: 'service', + kind: 'Service' + } + ] + } + end let(:kube_client) { Kubeclient::Client.new(kubernetes_url) } diff --git a/spec/models/project_services/prometheus_service_spec.rb b/spec/models/project_services/prometheus_service_spec.rb index aff03e28cc4..789846250ad 100644 --- a/spec/models/project_services/prometheus_service_spec.rb +++ b/spec/models/project_services/prometheus_service_spec.rb @@ -205,7 +205,7 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do context 'with cluster for all environments without prometheus installed' do context 'without environment supplied' do it 'raises PrometheusError because cluster was not found' do - expect{service.client}.to raise_error(Gitlab::PrometheusError, /couldn't find cluster with Prometheus installed/) + expect { service.client }.to raise_error(Gitlab::PrometheusError, /couldn't find cluster with Prometheus installed/) end end @@ -224,7 +224,7 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do let!(:environment) { create(:environment, project: project, name: 'prod') } it 'raises PrometheusError because cluster was not found' do - expect{service.client}.to raise_error(Gitlab::PrometheusError, /couldn't find cluster with Prometheus installed/) + expect { service.client }.to raise_error(Gitlab::PrometheusError, /couldn't find cluster with Prometheus installed/) end end end -- cgit v1.2.1 From 1c7a61afd20b581167c76b69f0a303fcb25cd87d Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Thu, 4 Jan 2018 17:45:35 +0100 Subject: Add Changelog item --- .../unreleased/pawel-connect_to_prometheus_through_proxy-30480.yml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 changelogs/unreleased/pawel-connect_to_prometheus_through_proxy-30480.yml diff --git a/changelogs/unreleased/pawel-connect_to_prometheus_through_proxy-30480.yml b/changelogs/unreleased/pawel-connect_to_prometheus_through_proxy-30480.yml new file mode 100644 index 00000000000..b2bb173912a --- /dev/null +++ b/changelogs/unreleased/pawel-connect_to_prometheus_through_proxy-30480.yml @@ -0,0 +1,6 @@ +--- +title: Implement multi server support and use kube proxy to connect to Prometheus + servers inside K8S cluster +merge_request: 16182 +author: +type: added -- cgit v1.2.1 From 3727dfcd439f6224aa359e961fd6c0920e80718b Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Wed, 17 Jan 2018 12:06:34 +0100 Subject: add manual_configuration to prometheus_service factory to enable it by default --- spec/factories/services.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/factories/services.rb b/spec/factories/services.rb index 110ef33c6f7..faedcc38c8d 100644 --- a/spec/factories/services.rb +++ b/spec/factories/services.rb @@ -30,6 +30,7 @@ FactoryBot.define do project active true properties({ + manual_configuration: true, api_url: 'https://prometheus.example.com/' }) end -- cgit v1.2.1 From 4b1d42bbc583b0884498bd129653587acc69ab0f Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Wed, 17 Jan 2018 12:29:18 +0100 Subject: check if service is template --- app/models/project_services/prometheus_service.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/models/project_services/prometheus_service.rb b/app/models/project_services/prometheus_service.rb index 3bf44c1ab2b..e540a2c646a 100644 --- a/app/models/project_services/prometheus_service.rb +++ b/app/models/project_services/prometheus_service.rb @@ -129,6 +129,7 @@ class PrometheusService < MonitoringService end def prometheus_installed? + return false if template? project.clusters.enabled.any? { |cluster| cluster.application_prometheus&.installed? } end -- cgit v1.2.1 From 1c0437d95e6329035af49d13b26c4a60948b02e3 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Tue, 2 Jan 2018 09:23:39 +0100 Subject: Client implementation for WikiPageVerion Part of gitlab-org/gitaly#639 --- app/controllers/projects/wikis_controller.rb | 4 +- lib/gitlab/git/wiki.rb | 38 ++++++++++++++----- lib/gitlab/gitaly_client/wiki_service.rb | 56 +++++++++++++++++++++++++--- spec/models/wiki_page_spec.rb | 32 ++++++++++++---- 4 files changed, 105 insertions(+), 25 deletions(-) diff --git a/app/controllers/projects/wikis_controller.rb b/app/controllers/projects/wikis_controller.rb index 292e4158f8b..730f20bc086 100644 --- a/app/controllers/projects/wikis_controller.rb +++ b/app/controllers/projects/wikis_controller.rb @@ -76,9 +76,9 @@ class Projects::WikisController < Projects::ApplicationController @page = @project_wiki.find_page(params[:id]) if @page - @page_versions = Kaminari.paginate_array(@page.versions(page: params[:page]), + @page_versions = Kaminari.paginate_array(@page.versions(page: params[:page].to_i), total_count: @page.count_versions) - .page(params[:page]) + .page(params[:page]) else redirect_to( project_wiki_path(@project, :home), diff --git a/lib/gitlab/git/wiki.rb b/lib/gitlab/git/wiki.rb index d4a53d32c28..305bc9b66b0 100644 --- a/lib/gitlab/git/wiki.rb +++ b/lib/gitlab/git/wiki.rb @@ -93,11 +93,23 @@ module Gitlab # :per_page - The number of items per page. # :limit - Total number of items to return. def page_versions(page_path, options = {}) - current_page = gollum_page_by_path(page_path) + puts '-' * 80 + puts options + puts '-' * 80 + puts - commits_from_page(current_page, options).map do |gitlab_git_commit| - gollum_page = gollum_wiki.page(current_page.title, gitlab_git_commit.id) - Gitlab::Git::WikiPageVersion.new(gitlab_git_commit, gollum_page&.format) + byebug + @repository.gitaly_migrate(:wiki_page_versions) do |is_enabled| + if is_enabled + gitaly_wiki_client.page_versions(page_path, pagination_params(options)) + else + current_page = gollum_page_by_path(page_path) + + commits_from_page(current_page, options).map do |gitlab_git_commit| + gollum_page = gollum_wiki.page(current_page.title, gitlab_git_commit.id) + Gitlab::Git::WikiPageVersion.new(gitlab_git_commit, gollum_page&.format) + end + end end end @@ -124,15 +136,21 @@ module Gitlab # :per_page - The number of items per page. # :limit - Total number of items to return. def commits_from_page(gollum_page, options = {}) - unless options[:limit] - options[:offset] = ([1, options.delete(:page).to_i].max - 1) * Gollum::Page.per_page - options[:limit] = (options.delete(:per_page) || Gollum::Page.per_page).to_i - end + pagination_options = pagination_params(options) @repository.log(ref: gollum_page.last_version.id, path: gollum_page.path, - limit: options[:limit], - offset: options[:offset]) + limit: pagination_options[:limit], + offset: pagination_options[:offset]) + end + + def pagination_params(options) + return options if options[:limit] + + options = options.dup + options[:offset] = ([1, options.delete(:page).to_i].max - 1) * Gollum::Page.per_page + options[:limit] = (options.delete(:per_page) || Gollum::Page.per_page).to_i + options end def gollum_wiki diff --git a/lib/gitlab/gitaly_client/wiki_service.rb b/lib/gitlab/gitaly_client/wiki_service.rb index 5c5b170a3e0..fa7e416504d 100644 --- a/lib/gitlab/gitaly_client/wiki_service.rb +++ b/lib/gitlab/gitaly_client/wiki_service.rb @@ -101,6 +101,48 @@ module Gitlab pages end + # options: + # :page - The Integer page number. + # :per_page - The number of items per page. + # :limit - Total number of items to return. + def page_versions(page_path, options) + request = Gitaly::WikiGetPageVersionsRequest.new( + repository: @gitaly_repo, + page_path: encode_binary(page_path) + ) + + min_index = options[:offset].to_i + max_index = min_index + options[:limit].to_i + byebug + + stream = GitalyClient.call(@repository.storage, :wiki_service, :wiki_get_page_versions, request) + + version_index = 0 + versions = [] + + # Allow limit and offset to be send to Gitaly: TODO + stream.each do |message| + puts '§' * 80 + puts version_index + + message.versions.each do |version| + case version_index + when min_index...max_index + versions << new_wiki_page_version(version) + when max_index..Float::INFINITY + return versions + end + + version_index += 1 + puts version_index + end + end + + # when we're requesting page 99 but the stream doesn't go that far, whatever + # is fetched thus far + versions + end + def find_file(name, revision) request = Gitaly::WikiFindFileRequest.new( repository: @gitaly_repo, @@ -129,7 +171,7 @@ module Gitlab private - # If a block is given and the yielded value is true, iteration will be + # If a block is given and the yielded value is truthy, iteration will be # stopped early at that point; else the iterator is consumed entirely. # The iterator is traversed with `next` to allow resuming the iteration. def wiki_page_from_iterator(iterator) @@ -146,10 +188,7 @@ module Gitlab else wiki_page = GitalyClient::WikiPage.new(page.to_h) - version = Gitlab::Git::WikiPageVersion.new( - Gitlab::Git::Commit.decorate(@repository, page.version.commit), - page.version.format - ) + version = new_wiki_page_version(page.version) end end @@ -158,6 +197,13 @@ module Gitlab [wiki_page, version] end + def new_wiki_page_version(version) + Gitlab::Git::WikiPageVersion.new( + Gitlab::Git::Commit.decorate(@repository, version.commit), + version.format + ) + end + def gitaly_commit_details(commit_details) Gitaly::WikiCommitDetails.new( name: encode_binary(commit_details.name), diff --git a/spec/models/wiki_page_spec.rb b/spec/models/wiki_page_spec.rb index ea75434e399..835ac20c5d8 100644 --- a/spec/models/wiki_page_spec.rb +++ b/spec/models/wiki_page_spec.rb @@ -252,18 +252,34 @@ describe WikiPage do end describe "#versions" do - before do - create_page("Update", "content") - @page = wiki.find_page("Update") + shared_examples 'wiki page versions' do + let(:page) { wiki.find_page("Update") } + + before do + create_page("Update", "content") + end + + after do + destroy_page("Update") + end + + it "returns an array of all commits for the page" do + 3.times { |i| page.update(content: "content #{i}") } + + expect(page.versions.count).to eq(4) + end + + it 'returns instances of WikiPageVersion' do + expect(page.versions).to all( be_a(Gitlab::Git::WikiPageVersion) ) + end end - after do - destroy_page("Update") + context 'when Gitaly is enabled' do + it_behaves_like 'wiki page versions' end - it "returns an array of all commits for the page" do - 3.times { |i| @page.update(content: "content #{i}") } - expect(@page.versions.count).to eq(4) + context 'when Gitaly is disabled', :disable_gitaly do + it_behaves_like 'wiki page versions' end end -- cgit v1.2.1 From bd96f2d6e351229b888bf4f41f39a082d2ac16cd Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Tue, 16 Jan 2018 10:42:00 +0100 Subject: Upgrade Gitaly versions --- Gemfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile b/Gemfile index 6f82890d0d1..a4c3db4bf5f 100644 --- a/Gemfile +++ b/Gemfile @@ -406,7 +406,7 @@ group :ed25519 do end # Gitaly GRPC client -gem 'gitaly-proto', '~> 0.73.0', require: 'gitaly' +gem 'gitaly-proto', '~> 0.74.0', require: 'gitaly' gem 'toml-rb', '~> 0.3.15', require: false -- cgit v1.2.1 From a29f0c28fd07ba14f0d0e5fb9c878a2eb117e388 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Tue, 16 Jan 2018 11:12:08 +0100 Subject: Allow pagination for WikiVersions on Gitaly request --- Gemfile.lock | 4 ++-- lib/gitlab/git/wiki.rb | 32 +++++++++++++------------------- lib/gitlab/gitaly_client/wiki_service.rb | 26 ++++---------------------- 3 files changed, 19 insertions(+), 43 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 284e1e7654d..d69c532b309 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -285,7 +285,7 @@ GEM po_to_json (>= 1.0.0) rails (>= 3.2.0) gherkin-ruby (0.3.2) - gitaly-proto (0.73.0) + gitaly-proto (0.74.0) google-protobuf (~> 3.1) grpc (~> 1.0) github-linguist (4.7.6) @@ -1056,7 +1056,7 @@ DEPENDENCIES gettext (~> 3.2.2) gettext_i18n_rails (~> 1.8.0) gettext_i18n_rails_js (~> 1.2.0) - gitaly-proto (~> 0.73.0) + gitaly-proto (~> 0.74.0) github-linguist (~> 4.7.0) gitlab-flowdock-git-hook (~> 1.0.1) gitlab-markup (~> 1.6.2) diff --git a/lib/gitlab/git/wiki.rb b/lib/gitlab/git/wiki.rb index 305bc9b66b0..4ed78daa443 100644 --- a/lib/gitlab/git/wiki.rb +++ b/lib/gitlab/git/wiki.rb @@ -93,15 +93,15 @@ module Gitlab # :per_page - The number of items per page. # :limit - Total number of items to return. def page_versions(page_path, options = {}) - puts '-' * 80 - puts options - puts '-' * 80 - puts - - byebug @repository.gitaly_migrate(:wiki_page_versions) do |is_enabled| if is_enabled - gitaly_wiki_client.page_versions(page_path, pagination_params(options)) + versions = gitaly_wiki_client.page_versions(page_path, options) + + # Gitaly uses gollum-lib to get the versions. Gollum defaults to 20 + # per page, but also fetches 20 if `limit` or `per_page` < 20. + # Slicing returns an array with the expected number of items. + slice_bound = options[:limit] || options[:per_page] || Gollum::Page.per_page + versions[0..slice_bound] else current_page = gollum_page_by_path(page_path) @@ -136,21 +136,15 @@ module Gitlab # :per_page - The number of items per page. # :limit - Total number of items to return. def commits_from_page(gollum_page, options = {}) - pagination_options = pagination_params(options) + unless options[:limit] + options[:offset] = ([1, options.delete(:page).to_i].max - 1) * Gollum::Page.per_page + options[:limit] = (options.delete(:per_page) || Gollum::Page.per_page).to_i + end @repository.log(ref: gollum_page.last_version.id, path: gollum_page.path, - limit: pagination_options[:limit], - offset: pagination_options[:offset]) - end - - def pagination_params(options) - return options if options[:limit] - - options = options.dup - options[:offset] = ([1, options.delete(:page).to_i].max - 1) * Gollum::Page.per_page - options[:limit] = (options.delete(:per_page) || Gollum::Page.per_page).to_i - options + limit: options[:limit], + offset: options[:offset]) end def gollum_wiki diff --git a/lib/gitlab/gitaly_client/wiki_service.rb b/lib/gitlab/gitaly_client/wiki_service.rb index fa7e416504d..a98c3c0b160 100644 --- a/lib/gitlab/gitaly_client/wiki_service.rb +++ b/lib/gitlab/gitaly_client/wiki_service.rb @@ -108,38 +108,20 @@ module Gitlab def page_versions(page_path, options) request = Gitaly::WikiGetPageVersionsRequest.new( repository: @gitaly_repo, - page_path: encode_binary(page_path) + page_path: encode_binary(page_path), + page: options[:page] || 1, + per_page: options[:per_page] || Gollum::Page.per_page ) - min_index = options[:offset].to_i - max_index = min_index + options[:limit].to_i - byebug - stream = GitalyClient.call(@repository.storage, :wiki_service, :wiki_get_page_versions, request) - version_index = 0 versions = [] - - # Allow limit and offset to be send to Gitaly: TODO stream.each do |message| - puts '§' * 80 - puts version_index - message.versions.each do |version| - case version_index - when min_index...max_index - versions << new_wiki_page_version(version) - when max_index..Float::INFINITY - return versions - end - - version_index += 1 - puts version_index + versions << new_wiki_page_version(version) end end - # when we're requesting page 99 but the stream doesn't go that far, whatever - # is fetched thus far versions end -- cgit v1.2.1 From 28fd7b6c5df43b2f29557e813055deb438791c67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Mon, 22 Jan 2018 19:28:10 +0100 Subject: Add auto_devops_domain to ApplicationSettings model --- ...162010_add_auto_devops_domain_to_application_settings.rb | 13 +++++++++++++ db/schema.rb | 3 ++- 2 files changed, 15 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20180122162010_add_auto_devops_domain_to_application_settings.rb diff --git a/db/migrate/20180122162010_add_auto_devops_domain_to_application_settings.rb b/db/migrate/20180122162010_add_auto_devops_domain_to_application_settings.rb new file mode 100644 index 00000000000..7e16cb83087 --- /dev/null +++ b/db/migrate/20180122162010_add_auto_devops_domain_to_application_settings.rb @@ -0,0 +1,13 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddAutoDevopsDomainToApplicationSettings < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + def change + add_column :application_settings, :auto_devops_domain, :string + end +end diff --git a/db/schema.rb b/db/schema.rb index a0901833c3d..a683a60df45 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20180113220114) do +ActiveRecord::Schema.define(version: 20180122162010) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -155,6 +155,7 @@ ActiveRecord::Schema.define(version: 20180113220114) do t.integer "gitaly_timeout_medium", default: 30, null: false t.integer "gitaly_timeout_fast", default: 10, null: false t.boolean "authorized_keys_enabled", default: true, null: false + t.string "auto_devops_domain" end create_table "audit_events", force: :cascade do |t| -- cgit v1.2.1 From 147f0428c00e7a10e908438a99a1558c24be41f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Mon, 22 Jan 2018 19:29:15 +0100 Subject: Add validation for auto_devops_domain --- app/models/application_setting.rb | 5 +++++ spec/models/application_setting_spec.rb | 40 +++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 80bda7f22ff..0dee6df525d 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -117,6 +117,11 @@ class ApplicationSetting < ActiveRecord::Base validates :repository_storages, presence: true validate :check_repository_storages + validates :auto_devops_domain, + allow_blank: true, + hostname: { allow_numeric_hostname: true, require_valid_tld: true }, + if: :auto_devops_enabled? + validates :enabled_git_access_protocol, inclusion: { in: %w(ssh http), allow_blank: true, allow_nil: true } diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb index ef480e7a80a..0d7b98229a4 100644 --- a/spec/models/application_setting_spec.rb +++ b/spec/models/application_setting_spec.rb @@ -114,6 +114,46 @@ describe ApplicationSetting do it { expect(setting.repository_storages).to eq(['default']) } end + context 'auto_devops_domain setting' do + context 'when auto_devops_enabled? is true' do + before do + setting.update(auto_devops_enabled: true) + end + + context 'with a valid value' do + before do + setting.update(auto_devops_domain: 'domain.com') + end + + it 'is valid' do + expect(setting).to be_valid + end + end + + context 'with an invalid value' do + before do + setting.update(auto_devops_domain: 'definitelynotahostname') + end + + it 'is invalid' do + expect(setting).to be_invalid + end + end + end + + context 'when auto_devops_enabled? is false' do + before do + setting.update(auto_devops_enabled: false) + end + + it 'can be blank' do + setting.update(auto_devops_domain: '') + + expect(setting).to be_valid + end + end + end + context 'circuitbreaker settings' do [:circuitbreaker_failure_count_threshold, :circuitbreaker_check_interval, -- cgit v1.2.1 From 57060295031f1ad2d5f058889561a68ede04d2f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Mon, 22 Jan 2018 19:31:03 +0100 Subject: Expose auto_devops_domain in admin settings view --- app/helpers/application_settings_helper.rb | 1 + app/views/admin/application_settings/_form.html.haml | 7 ++++++- spec/features/admin/admin_settings_spec.rb | 10 ++++++++++ 3 files changed, 17 insertions(+), 1 deletion(-) diff --git a/app/helpers/application_settings_helper.rb b/app/helpers/application_settings_helper.rb index 8ef561d90e6..6f07428975d 100644 --- a/app/helpers/application_settings_helper.rb +++ b/app/helpers/application_settings_helper.rb @@ -148,6 +148,7 @@ module ApplicationSettingsHelper :akismet_enabled, :authorized_keys_enabled, :auto_devops_enabled, + :auto_devops_domain, :circuitbreaker_access_retries, :circuitbreaker_check_interval, :circuitbreaker_failure_count_threshold, diff --git a/app/views/admin/application_settings/_form.html.haml b/app/views/admin/application_settings/_form.html.haml index ba4ca88a8a9..d77d99180fe 100644 --- a/app/views/admin/application_settings/_form.html.haml +++ b/app/views/admin/application_settings/_form.html.haml @@ -249,7 +249,12 @@ .help-block It will automatically build, test, and deploy applications based on a predefined CI/CD configuration = link_to icon('question-circle'), help_page_path('topics/autodevops/index.md') - + .form-group + = f.label :auto_devops_domain, class: 'control-label col-sm-2' + .col-sm-10 + = f.text_field :auto_devops_domain, class: 'form-control', placeholder: 'domain.com' + .help-block + = s_("Specify a domain to use by default for every project's Auto Review Apps and Auto Deploy stages.") .form-group .col-sm-offset-2.col-sm-10 .checkbox diff --git a/spec/features/admin/admin_settings_spec.rb b/spec/features/admin/admin_settings_spec.rb index 1218ea52227..cfc6697c79a 100644 --- a/spec/features/admin/admin_settings_spec.rb +++ b/spec/features/admin/admin_settings_spec.rb @@ -47,6 +47,16 @@ feature 'Admin updates settings' do expect(page).to have_content "Application settings saved successfully" end + scenario 'Change AutoDevOps settings' do + check 'Enabled Auto DevOps (Beta) for projects by default' + fill_in 'Auto devops domain', with: 'domain.com' + click_button 'Save' + + expect(current_application_settings.auto_devops_enabled?).to be true + expect(current_application_settings.auto_devops_domain).to eq('domain.com') + expect(page).to have_content "Application settings saved successfully" + end + scenario 'Change Slack Notifications Service template settings' do first(:link, 'Service Templates').click click_link 'Slack notifications' -- cgit v1.2.1 From 8f0a14843a630fec2eec22920ce922d5548ddec6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Mon, 22 Jan 2018 19:31:35 +0100 Subject: Expose instance AutoDevOps domain in Project --- app/models/project.rb | 10 +++++++++- spec/models/project_spec.rb | 36 +++++++++++++++++++++++++++++++----- 2 files changed, 40 insertions(+), 6 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index c0f7b30ceb0..87616716c7c 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1604,7 +1604,15 @@ class Project < ActiveRecord::Base def auto_devops_variables return [] unless auto_devops_enabled? - auto_devops&.variables || [] + variables = [] + + if current_application_settings.auto_devops_domain.present? + variables << { key: 'AUTO_DEVOPS_DOMAIN', + value: current_application_settings.auto_devops_domain, + public: true } + end + + auto_devops&.variables || variables end def append_or_update_attribute(name, value) diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 987be8e8b46..dc0825aad73 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -2976,18 +2976,40 @@ describe Project do subject { project.auto_devops_variables } - context 'when enabled in settings' do + context 'when enabled in instance settings' do before do stub_application_setting(auto_devops_enabled: true) end + context 'when domain is empty' do + before do + stub_application_setting(auto_devops_domain: nil) + end + + it 'variables does not include AUTO_DEVOPS_DOMAIN' do + is_expected.not_to include(domain_variable) + end + end + + context 'when domain is configured' do + before do + stub_application_setting(auto_devops_domain: 'example.com') + end + + it 'variables includes AUTO_DEVOPS_DOMAIN' do + is_expected.to include(domain_variable) + end + end + end + + context 'when explicitely enabled' do context 'when domain is empty' do before do create(:project_auto_devops, project: project, domain: nil) end - it 'variables are empty' do - is_expected.to be_empty + it 'variables does not include AUTO_DEVOPS_DOMAIN' do + is_expected.not_to include(domain_variable) end end @@ -2996,11 +3018,15 @@ describe Project do create(:project_auto_devops, project: project, domain: 'example.com') end - it "variables are not empty" do - is_expected.not_to be_empty + it 'variables includes AUTO_DEVOPS_DOMAIN' do + is_expected.to include(domain_variable) end end end + + def domain_variable + { key: 'AUTO_DEVOPS_DOMAIN', value: 'example.com', public: true } + end end describe '#latest_successful_builds_for' do -- cgit v1.2.1 From f5591e4c901cea77f57e2b406e8e818d29919dfe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Mon, 22 Jan 2018 20:25:22 +0100 Subject: Add CHANGELOG entry --- .../38175-add-domain-field-to-auto-devops-application-setting.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/38175-add-domain-field-to-auto-devops-application-setting.yml diff --git a/changelogs/unreleased/38175-add-domain-field-to-auto-devops-application-setting.yml b/changelogs/unreleased/38175-add-domain-field-to-auto-devops-application-setting.yml new file mode 100644 index 00000000000..475e1dc12b5 --- /dev/null +++ b/changelogs/unreleased/38175-add-domain-field-to-auto-devops-application-setting.yml @@ -0,0 +1,5 @@ +--- +title: Add Auto DevOps Domain application setting +merge_request: 16604 +author: +type: changed -- cgit v1.2.1 From 6028f9eecfad1190e99c830b3367b7ccb9fb5c81 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Mon, 22 Jan 2018 20:37:02 +0100 Subject: Refactor auto_devops_domain setting specs --- spec/models/application_setting_spec.rb | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb index 0d7b98229a4..ae2d34750a7 100644 --- a/spec/models/application_setting_spec.rb +++ b/spec/models/application_setting_spec.rb @@ -120,6 +120,12 @@ describe ApplicationSetting do setting.update(auto_devops_enabled: true) end + it 'can be blank' do + setting.update(auto_devops_domain: '') + + expect(setting).to be_valid + end + context 'with a valid value' do before do setting.update(auto_devops_domain: 'domain.com') @@ -140,18 +146,6 @@ describe ApplicationSetting do end end end - - context 'when auto_devops_enabled? is false' do - before do - setting.update(auto_devops_enabled: false) - end - - it 'can be blank' do - setting.update(auto_devops_domain: '') - - expect(setting).to be_valid - end - end end context 'circuitbreaker settings' do -- cgit v1.2.1 From 62b7c3ba4aa1a354ac5380400c707a381ef0e5e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Mon, 22 Jan 2018 21:10:41 +0100 Subject: Short-circuit Project#auto_devops_variables This makes Project#auto_devops_variables more performant by evaluating the application settings only if necessary. --- app/models/project.rb | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index 87616716c7c..97f5e678021 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1604,15 +1604,13 @@ class Project < ActiveRecord::Base def auto_devops_variables return [] unless auto_devops_enabled? - variables = [] - - if current_application_settings.auto_devops_domain.present? - variables << { key: 'AUTO_DEVOPS_DOMAIN', - value: current_application_settings.auto_devops_domain, - public: true } - end - - auto_devops&.variables || variables + auto_devops&.variables || if current_application_settings.auto_devops_domain.present? + [{ key: 'AUTO_DEVOPS_DOMAIN', + value: current_application_settings.auto_devops_domain, + public: true }] + else + [] + end end def append_or_update_attribute(name, value) -- cgit v1.2.1 From 6714fbad8fc25d3cc6b295d26e3c220987c60fb0 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 19 Jan 2018 14:25:30 +0100 Subject: Remove redundant pipeline stages from the database --- ...80119121225_remove_redundant_pipeline_stages.rb | 26 +++++++++++++++ db/schema.rb | 2 +- .../remove_redundant_pipeline_stages_spec.rb | 39 ++++++++++++++++++++++ 3 files changed, 66 insertions(+), 1 deletion(-) create mode 100644 db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb create mode 100644 spec/migrations/remove_redundant_pipeline_stages_spec.rb diff --git a/db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb b/db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb new file mode 100644 index 00000000000..3868e0adae1 --- /dev/null +++ b/db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb @@ -0,0 +1,26 @@ +class RemoveRedundantPipelineStages < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def up + redundant_stages_ids = <<~SQL + SELECT id FROM ci_stages a WHERE ( + SELECT COUNT(*) FROM ci_stages b + WHERE a.pipeline_id = b.pipeline_id AND a.name = b.name + ) > 1 + SQL + + execute <<~SQL + UPDATE ci_builds SET stage_id = NULL WHERE ci_builds.stage_id IN (#{redundant_stages_ids}) + SQL + + execute <<~SQL + DELETE FROM ci_stages WHERE ci_stages.id IN (#{redundant_stages_ids}) + SQL + end + + def down + # noop + end +end diff --git a/db/schema.rb b/db/schema.rb index a0901833c3d..9becd794553 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20180113220114) do +ActiveRecord::Schema.define(version: 20180119121225) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" diff --git a/spec/migrations/remove_redundant_pipeline_stages_spec.rb b/spec/migrations/remove_redundant_pipeline_stages_spec.rb new file mode 100644 index 00000000000..3bd2a9a2146 --- /dev/null +++ b/spec/migrations/remove_redundant_pipeline_stages_spec.rb @@ -0,0 +1,39 @@ +require 'spec_helper' +require Rails.root.join('db', 'post_migrate', '20180119121225_remove_redundant_pipeline_stages.rb') + +describe RemoveRedundantPipelineStages, :migration do + let(:projects) { table(:projects) } + let(:pipelines) { table(:ci_pipelines) } + let(:stages) { table(:ci_stages) } + let(:builds) { table(:ci_builds) } + + before do + projects.create!(id: 123, name: 'gitlab', path: 'gitlab-ce') + pipelines.create!(id: 234, project_id: 123, ref: 'master', sha: 'adf43c3a') + + stages.create!(id: 6, project_id: 123, pipeline_id: 234, name: 'build') + stages.create!(id: 10, project_id: 123, pipeline_id: 234, name: 'build') + stages.create!(id: 21, project_id: 123, pipeline_id: 234, name: 'build') + stages.create!(id: 41, project_id: 123, pipeline_id: 234, name: 'test') + stages.create!(id: 62, project_id: 123, pipeline_id: 234, name: 'test') + stages.create!(id: 102, project_id: 123, pipeline_id: 234, name: 'deploy') + + builds.create!(id: 1, commit_id: 234, project_id: 123, stage_id: 10) + builds.create!(id: 2, commit_id: 234, project_id: 123, stage_id: 21) + builds.create!(id: 3, commit_id: 234, project_id: 123, stage_id: 21) + builds.create!(id: 4, commit_id: 234, project_id: 123, stage_id: 41) + builds.create!(id: 5, commit_id: 234, project_id: 123, stage_id: 62) + builds.create!(id: 6, commit_id: 234, project_id: 123, stage_id: 102) + end + + it 'removes ambiguous stages and preserves builds' do + expect(stages.all.count).to eq 6 + expect(builds.all.count).to eq 6 + + migrate! + + expect(stages.all.count).to eq 1 + expect(builds.all.count).to eq 6 + expect(builds.all.pluck(:stage_id).compact).to eq [102] + end +end -- cgit v1.2.1 From e73333242ac42a1020319f5d491daf0647af4c54 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 24 Jan 2018 12:56:11 +0100 Subject: Optimize SQL query that removes duplicated stages --- .../20180119121225_remove_redundant_pipeline_stages.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb b/db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb index 3868e0adae1..c1705cd8757 100644 --- a/db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb +++ b/db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb @@ -5,10 +5,10 @@ class RemoveRedundantPipelineStages < ActiveRecord::Migration def up redundant_stages_ids = <<~SQL - SELECT id FROM ci_stages a WHERE ( - SELECT COUNT(*) FROM ci_stages b - WHERE a.pipeline_id = b.pipeline_id AND a.name = b.name - ) > 1 + SELECT id FROM ci_stages WHERE (pipeline_id, name) IN ( + SELECT pipeline_id, name FROM ci_stages + GROUP BY pipeline_id, name HAVING COUNT(*) > 1 + ) SQL execute <<~SQL -- cgit v1.2.1 From aa290573aece6408ad8bb98c7a04257202be5ff8 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 24 Jan 2018 14:26:03 +0100 Subject: Add a new service for creating detached CI/CD jobs --- app/models/commit_status.rb | 4 +++- app/services/ci/create_job_service.rb | 12 ++++++++++++ app/services/projects/update_pages_service.rb | 18 ++++++++++-------- lib/api/commit_statuses.rb | 10 +++++++++- spec/services/ci/create_job_service_spec.rb | 20 ++++++++++++++++++++ 5 files changed, 54 insertions(+), 10 deletions(-) create mode 100644 app/services/ci/create_job_service.rb create mode 100644 spec/services/ci/create_job_service_spec.rb diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index c0263c0b4e2..f010b0ce9db 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -50,7 +50,9 @@ class CommitStatus < ActiveRecord::Base ## # We still create some CommitStatuses outside of CreatePipelineService. # - # These are pages deployments and external statuses. + # These are pages deployments and external statuses. We now handle these + # using CreateJobService, but we still need to ensure that a job has a + # stage assigned. TODO, In future releases we will add model validation. # before_create unless: :importing? do Ci::EnsureStageService.new(project, user).execute(self) do |stage| diff --git a/app/services/ci/create_job_service.rb b/app/services/ci/create_job_service.rb new file mode 100644 index 00000000000..683c3557cd0 --- /dev/null +++ b/app/services/ci/create_job_service.rb @@ -0,0 +1,12 @@ +module Ci + class CreateJobService < BaseService + def execute(subject = nil) + (subject || yield).tap do |subject| + Ci::EnsureStageService.new(project, current_user) + .execute(subject) + + subject.save! + end + end + end +end diff --git a/app/services/projects/update_pages_service.rb b/app/services/projects/update_pages_service.rb index a773222bf17..63d2b6c76e6 100644 --- a/app/services/projects/update_pages_service.rb +++ b/app/services/projects/update_pages_service.rb @@ -58,14 +58,16 @@ module Projects end def create_status - GenericCommitStatus.new( - project: project, - pipeline: build.pipeline, - user: build.user, - ref: build.ref, - stage: 'deploy', - name: 'pages:deploy' - ) + Ci::CreateJobService.new(project, build.user).execute do + GenericCommitStatus.new( + project: project, + pipeline: build.pipeline, + user: build.user, + ref: build.ref, + stage: 'deploy', + name: 'pages:deploy' + ) + end end def extract_archive!(temp_path) diff --git a/lib/api/commit_statuses.rb b/lib/api/commit_statuses.rb index 829eef18795..76cb9a8c808 100644 --- a/lib/api/commit_statuses.rb +++ b/lib/api/commit_statuses.rb @@ -69,6 +69,7 @@ module API name = params[:name] || params[:context] || 'default' pipeline = @project.pipeline_for(ref, commit.sha) + unless pipeline pipeline = @project.pipelines.create!( source: :external, @@ -90,7 +91,14 @@ module API optional_attributes = attributes_for_keys(%w[target_url description coverage]) - status.update(optional_attributes) if optional_attributes.any? + status.assign_attributes(optional_attributes) if optional_attributes.any? + + if status.new_record? + Ci::CreateJobService.new(@project, current_user).execute(status) + else + status.save! + end + render_validation_error!(status) if status.invalid? begin diff --git a/spec/services/ci/create_job_service_spec.rb b/spec/services/ci/create_job_service_spec.rb new file mode 100644 index 00000000000..a7a8032ba28 --- /dev/null +++ b/spec/services/ci/create_job_service_spec.rb @@ -0,0 +1,20 @@ +require 'spec_helper' + +describe Ci::CreateJobService, '#execute' do + set(:project) { create(:project, :repository) } + let(:user) { create(:admin) } + let(:status) { build(:ci_build) } + let(:service) { described_class.new(project, user) } + + it 'persists job object instantiated in the block' do + expect(service.execute { status }).to be_persisted + end + + it 'persists a job instance passed as an argument' do + expect(service.execute(status)).to be_persisted + end + + it 'ensures that a job has a stage assigned' do + expect(service.execute(status).stage_id).to be_present + end +end -- cgit v1.2.1 From e4aac7f365105cbc7547239066db075a44d69788 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 24 Jan 2018 14:40:11 +0100 Subject: Do not raise an error when trying to persist a job --- app/models/commit_status.rb | 4 ++-- app/services/ci/create_job_service.rb | 2 +- lib/api/commit_statuses.rb | 2 +- spec/services/ci/create_job_service_spec.rb | 6 ++++++ 4 files changed, 10 insertions(+), 4 deletions(-) diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index f010b0ce9db..47af20975ec 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -51,8 +51,8 @@ class CommitStatus < ActiveRecord::Base # We still create some CommitStatuses outside of CreatePipelineService. # # These are pages deployments and external statuses. We now handle these - # using CreateJobService, but we still need to ensure that a job has a - # stage assigned. TODO, In future releases we will add model validation. + # jobs using CreateJobService, but we still need to ensure that a job has + # a stage assigned. TODO, In future releases we will add model validation. # before_create unless: :importing? do Ci::EnsureStageService.new(project, user).execute(self) do |stage| diff --git a/app/services/ci/create_job_service.rb b/app/services/ci/create_job_service.rb index 683c3557cd0..7cb77edd690 100644 --- a/app/services/ci/create_job_service.rb +++ b/app/services/ci/create_job_service.rb @@ -5,7 +5,7 @@ module Ci Ci::EnsureStageService.new(project, current_user) .execute(subject) - subject.save! + subject.save end end end diff --git a/lib/api/commit_statuses.rb b/lib/api/commit_statuses.rb index 76cb9a8c808..3ec17c4d9f5 100644 --- a/lib/api/commit_statuses.rb +++ b/lib/api/commit_statuses.rb @@ -96,7 +96,7 @@ module API if status.new_record? Ci::CreateJobService.new(@project, current_user).execute(status) else - status.save! + status.save if status.changed? end render_validation_error!(status) if status.invalid? diff --git a/spec/services/ci/create_job_service_spec.rb b/spec/services/ci/create_job_service_spec.rb index a7a8032ba28..d9dd1a40325 100644 --- a/spec/services/ci/create_job_service_spec.rb +++ b/spec/services/ci/create_job_service_spec.rb @@ -17,4 +17,10 @@ describe Ci::CreateJobService, '#execute' do it 'ensures that a job has a stage assigned' do expect(service.execute(status).stage_id).to be_present end + + it 'does not raise error if status is invalid' do + status.name = nil + + expect(service.execute(status)).not_to be_persisted + end end -- cgit v1.2.1 From ce71b1ce072b024802e7e4ff07486e253c24d964 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 24 Jan 2018 15:21:20 +0100 Subject: Fix removing redundant pipeline stages on MySQL --- .../20180119121225_remove_redundant_pipeline_stages.rb | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb b/db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb index c1705cd8757..597f6c3ee48 100644 --- a/db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb +++ b/db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb @@ -12,12 +12,19 @@ class RemoveRedundantPipelineStages < ActiveRecord::Migration SQL execute <<~SQL - UPDATE ci_builds SET stage_id = NULL WHERE ci_builds.stage_id IN (#{redundant_stages_ids}) + UPDATE ci_builds SET stage_id = NULL WHERE stage_id IN (#{redundant_stages_ids}) SQL - execute <<~SQL - DELETE FROM ci_stages WHERE ci_stages.id IN (#{redundant_stages_ids}) - SQL + if Gitlab::Database.postgresql? + execute <<~SQL + DELETE FROM ci_stages WHERE id IN (#{redundant_stages_ids}) + SQL + else # We can't modify a table we are selecting from on MySQL + execute <<~SQL + DELETE a FROM ci_stages AS a, ci_stages AS b + WHERE a.pipeline_id = b.pipeline_id AND a.name = b.name + SQL + end end def down -- cgit v1.2.1 From b97b4d258552cadc55f408a28569c4a0d34b38a3 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 25 Jan 2018 12:55:11 +0100 Subject: Add an unique index on pipeline stage name --- ...180125111139_add_unique_pipeline_stage_name_index.rb | 17 +++++++++++++++++ db/schema.rb | 5 +++-- 2 files changed, 20 insertions(+), 2 deletions(-) create mode 100644 db/post_migrate/20180125111139_add_unique_pipeline_stage_name_index.rb diff --git a/db/post_migrate/20180125111139_add_unique_pipeline_stage_name_index.rb b/db/post_migrate/20180125111139_add_unique_pipeline_stage_name_index.rb new file mode 100644 index 00000000000..4fa148b2446 --- /dev/null +++ b/db/post_migrate/20180125111139_add_unique_pipeline_stage_name_index.rb @@ -0,0 +1,17 @@ +class AddUniquePipelineStageNameIndex < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + remove_concurrent_index :ci_stages, [:pipeline_id, :name] + add_concurrent_index :ci_stages, [:pipeline_id, :name], unique: true + end + + def down + remove_concurrent_index :ci_stages, [:pipeline_id, :name], unique: true + add_concurrent_index :ci_stages, [:pipeline_id, :name] + end +end diff --git a/db/schema.rb b/db/schema.rb index 9becd794553..d0802bed8f7 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20180119121225) do +ActiveRecord::Schema.define(version: 20180125111139) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -450,7 +450,8 @@ ActiveRecord::Schema.define(version: 20180119121225) do t.integer "lock_version" end - add_index "ci_stages", ["pipeline_id", "name"], name: "index_ci_stages_on_pipeline_id_and_name", using: :btree + add_index "ci_stages", ["pipeline_id", "name"], name: "index_ci_stages_on_pipeline_id_and_name", unique: true, using: :btree + add_index "ci_stages", ["pipeline_id", "name"], name: "index_ci_stages_on_pipeline_id_and_name_unique", unique: true, using: :btree add_index "ci_stages", ["pipeline_id"], name: "index_ci_stages_on_pipeline_id", using: :btree add_index "ci_stages", ["project_id"], name: "index_ci_stages_on_project_id", using: :btree -- cgit v1.2.1 From 50e9824115bbefeb59319b4f20622b162c22063f Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 25 Jan 2018 13:28:18 +0100 Subject: Fix migration removing duplicate stages on MySQL again --- db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb b/db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb index 597f6c3ee48..ce38026e7e2 100644 --- a/db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb +++ b/db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb @@ -23,6 +23,7 @@ class RemoveRedundantPipelineStages < ActiveRecord::Migration execute <<~SQL DELETE a FROM ci_stages AS a, ci_stages AS b WHERE a.pipeline_id = b.pipeline_id AND a.name = b.name + AND a.id <> b.id SQL end end -- cgit v1.2.1 From 9b73f14d07233af78512a93f9b8eadd696cd6fd1 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 25 Jan 2018 14:18:41 +0100 Subject: Fix indentation in migration removing duplicated stages --- db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb b/db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb index ce38026e7e2..89ed422ea1c 100644 --- a/db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb +++ b/db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb @@ -4,7 +4,7 @@ class RemoveRedundantPipelineStages < ActiveRecord::Migration DOWNTIME = false def up - redundant_stages_ids = <<~SQL + redundant_stages_ids = <<~SQL SELECT id FROM ci_stages WHERE (pipeline_id, name) IN ( SELECT pipeline_id, name FROM ci_stages GROUP BY pipeline_id, name HAVING COUNT(*) > 1 -- cgit v1.2.1 From 4e4ee368c9c02fcd52a1173df4e8ca882c644dba Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 25 Jan 2018 14:31:27 +0100 Subject: Remove unique index not added in a migration from schema --- db/schema.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/db/schema.rb b/db/schema.rb index d0802bed8f7..97cbbd111c8 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -451,7 +451,6 @@ ActiveRecord::Schema.define(version: 20180125111139) do end add_index "ci_stages", ["pipeline_id", "name"], name: "index_ci_stages_on_pipeline_id_and_name", unique: true, using: :btree - add_index "ci_stages", ["pipeline_id", "name"], name: "index_ci_stages_on_pipeline_id_and_name_unique", unique: true, using: :btree add_index "ci_stages", ["pipeline_id"], name: "index_ci_stages_on_pipeline_id", using: :btree add_index "ci_stages", ["project_id"], name: "index_ci_stages_on_project_id", using: :btree -- cgit v1.2.1 From 48b0bc330add0fbb3398890e20a7444ba69d5f9a Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 25 Jan 2018 15:15:51 +0100 Subject: Fix specs for retry build after making stages unique --- app/services/ci/retry_build_service.rb | 2 +- spec/services/ci/retry_build_service_spec.rb | 36 +++++++++++++++------------- 2 files changed, 21 insertions(+), 17 deletions(-) diff --git a/app/services/ci/retry_build_service.rb b/app/services/ci/retry_build_service.rb index c552193e66b..6128b2a8fbb 100644 --- a/app/services/ci/retry_build_service.rb +++ b/app/services/ci/retry_build_service.rb @@ -1,7 +1,7 @@ module Ci class RetryBuildService < ::BaseService CLONE_ACCESSORS = %i[pipeline project ref tag options commands name - allow_failure stage_id stage stage_idx trigger_request + allow_failure stage stage_id stage_idx trigger_request yaml_variables when environment coverage_regex description tag_list protected].freeze diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb index a06397a0782..cea5c8a6c05 100644 --- a/spec/services/ci/retry_build_service_spec.rb +++ b/spec/services/ci/retry_build_service_spec.rb @@ -5,7 +5,11 @@ describe Ci::RetryBuildService do set(:project) { create(:project) } set(:pipeline) { create(:ci_pipeline, project: project) } - let(:build) { create(:ci_build, pipeline: pipeline) } + let(:stage) do + Ci::Stage.create!(project: project, pipeline: pipeline, name: 'test') + end + + let(:build) { create(:ci_build, pipeline: pipeline, stage_id: stage.id) } let(:service) do described_class.new(project, user) @@ -26,29 +30,27 @@ describe Ci::RetryBuildService do user_id auto_canceled_by_id retried failure_reason].freeze shared_examples 'build duplication' do - let(:stage) do - # TODO, we still do not have factory for new stages, we will need to - # switch existing factory to persist stages, instead of using LegacyStage - # - Ci::Stage.create!(project: project, pipeline: pipeline, name: 'test') - end + let(:another_pipeline) { create(:ci_empty_pipeline, project: project) } let(:build) do create(:ci_build, :failed, :artifacts, :expired, :erased, :queued, :coverage, :tags, :allowed_to_fail, :on_tag, :triggered, :trace, :teardown_environment, - description: 'my-job', stage: 'test', pipeline: pipeline, - auto_canceled_by: create(:ci_empty_pipeline, project: project)) do |build| - ## - # TODO, workaround for FactoryBot limitation when having both - # stage (text) and stage_id (integer) columns in the table. - build.stage_id = stage.id - end + description: 'my-job', stage: 'test', stage_id: stage.id, + pipeline: pipeline, auto_canceled_by: another_pipeline) + end + + before do + # Make sure that build has both `stage_id` and `stage` because FactoryBot + # can reset one of the fields when assigning another. We plan to deprecate + # and remove legacy `stage` column in the future. + build.update_attributes(stage: 'test', stage_id: stage.id) end describe 'clone accessors' do CLONE_ACCESSORS.each do |attribute| it "clones #{attribute} build attribute" do + expect(build.send(attribute)).not_to be_nil expect(new_build.send(attribute)).not_to be_nil expect(new_build.send(attribute)).to eq build.send(attribute) end @@ -121,10 +123,12 @@ describe Ci::RetryBuildService do context 'when there are subsequent builds that are skipped' do let!(:subsequent_build) do - create(:ci_build, :skipped, stage_idx: 1, pipeline: pipeline) + create(:ci_build, :skipped, stage_idx: 2, + pipeline: pipeline, + stage: 'deploy') end - it 'resumes pipeline processing in subsequent stages' do + it 'resumes pipeline processing in a subsequent stage' do service.execute(build) expect(subsequent_build.reload).to be_created -- cgit v1.2.1 From 89ea6a9931bb232bd391ff4dace6e13deb6dee0c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Fri, 26 Jan 2018 17:06:52 +0100 Subject: Add Callout model --- app/models/callout.rb | 3 +++ app/models/user.rb | 1 + db/migrate/20180125214301_create_callouts.rb | 19 +++++++++++++++++++ 3 files changed, 23 insertions(+) create mode 100644 app/models/callout.rb create mode 100644 db/migrate/20180125214301_create_callouts.rb diff --git a/app/models/callout.rb b/app/models/callout.rb new file mode 100644 index 00000000000..b8131beb518 --- /dev/null +++ b/app/models/callout.rb @@ -0,0 +1,3 @@ +class Callout < ActiveRecord::Base + belongs_to :user +end diff --git a/app/models/user.rb b/app/models/user.rb index 9403da98268..b54d44fe80a 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -137,6 +137,7 @@ class User < ActiveRecord::Base has_many :assigned_merge_requests, dependent: :nullify, foreign_key: :assignee_id, class_name: "MergeRequest" # rubocop:disable Cop/ActiveRecordDependent has_many :custom_attributes, class_name: 'UserCustomAttribute' + has_many :callouts # # Validations diff --git a/db/migrate/20180125214301_create_callouts.rb b/db/migrate/20180125214301_create_callouts.rb new file mode 100644 index 00000000000..5dc9638a845 --- /dev/null +++ b/db/migrate/20180125214301_create_callouts.rb @@ -0,0 +1,19 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class CreateCallouts < ActiveRecord::Migration + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + def change + create_table :callouts do |t| + t.string :feature_name, null: false + t.boolean :dismissed_state, null: false + t.references :user, index: true, foreign_key: { on_delete: :cascade }, null: false + + t.timestamps_with_timezone null: false + end + + add_index :callouts, :feature_name, unique: true + end +end -- cgit v1.2.1 From 0dab083702ff4364bc25ea29812a4305c58e5a4f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Fri, 26 Jan 2018 17:07:35 +0100 Subject: Add Callout specs --- spec/factories/callouts.rb | 8 ++++++++ spec/models/callout_spec.rb | 9 +++++++++ 2 files changed, 17 insertions(+) create mode 100644 spec/factories/callouts.rb create mode 100644 spec/models/callout_spec.rb diff --git a/spec/factories/callouts.rb b/spec/factories/callouts.rb new file mode 100644 index 00000000000..b8ea879933e --- /dev/null +++ b/spec/factories/callouts.rb @@ -0,0 +1,8 @@ +FactoryBot.define do + factory :callout do + feature_name 'test_callout' + dismissed_state false + + user + end +end diff --git a/spec/models/callout_spec.rb b/spec/models/callout_spec.rb new file mode 100644 index 00000000000..8328ce06139 --- /dev/null +++ b/spec/models/callout_spec.rb @@ -0,0 +1,9 @@ +require 'rails_helper' + +describe Callout do + let(:callout) { create(:callout) } + + describe 'relationships' do + it { is_expected.to belong_to(:user) } + end +end -- cgit v1.2.1 From c4667f87037150a408da5c133e573714807e17b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Fri, 26 Jan 2018 22:13:54 +0100 Subject: Implement Callouts controller --- app/controllers/callouts_controller.rb | 27 +++++++++++++++++++++++++++ config/routes.rb | 5 +++++ 2 files changed, 32 insertions(+) create mode 100644 app/controllers/callouts_controller.rb diff --git a/app/controllers/callouts_controller.rb b/app/controllers/callouts_controller.rb new file mode 100644 index 00000000000..a7bccfcce78 --- /dev/null +++ b/app/controllers/callouts_controller.rb @@ -0,0 +1,27 @@ +class CalloutsController < ApplicationController + before_action :callout, only: [:dismiss] + + def dismiss + respond_to do |format| + format.json do + if @callout + @callout.update(dismissed_state: true) + else + Callout.create(feature_name: callout_param, dismissed_state: true, user: current_user) + end + + head :ok + end + end + end + + private + + def callout + @callout = Callout.find_by(user: current_user, feature_name: callout_param) + end + + def callout_param + params.require(:feature_name) + end +end diff --git a/config/routes.rb b/config/routes.rb index f162043dd5e..f768bcebc7e 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -90,6 +90,11 @@ Rails.application.routes.draw do # Notification settings resources :notification_settings, only: [:create, :update] + # Callouts + namespace :callouts do + post :dismiss + end + draw :google_api draw :import draw :uploads -- cgit v1.2.1 From bca10385539b44b39c28586d2d795e66a5e5e907 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Fri, 26 Jan 2018 22:49:20 +0100 Subject: Add CalloutsController specs --- spec/controllers/callouts_controller_spec.rb | 39 ++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) create mode 100644 spec/controllers/callouts_controller_spec.rb diff --git a/spec/controllers/callouts_controller_spec.rb b/spec/controllers/callouts_controller_spec.rb new file mode 100644 index 00000000000..bf2aae190f2 --- /dev/null +++ b/spec/controllers/callouts_controller_spec.rb @@ -0,0 +1,39 @@ +require 'spec_helper' + +describe CalloutsController do + let(:user) { create(:user) } + + before do + sign_in(user) + end + + describe "POST #dismiss" do + subject { post :dismiss, feature_name: 'feature_name', format: :json } + + context 'when callout entry does not exist' do + it 'should create a callout entry with dismissed state' do + expect { subject }.to change { Callout.count }.by(1) + end + + it 'should return success' do + subject + + expect(response).to have_gitlab_http_status(:ok) + end + end + + context 'when callout entry already exists' do + let!(:callout) { create(:callout, feature_name: 'feature_name', user: user) } + + it 'should update it with a dismissed state' do + expect { subject }.to change { callout.reload.dismissed_state }.from(false).to(true) + end + + it 'should return success' do + subject + + expect(response).to have_gitlab_http_status(:ok) + end + end + end +end -- cgit v1.2.1 From 860c7c4bd41ab6353834e8e765f5cd86d93a41c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Fri, 26 Jan 2018 23:05:28 +0100 Subject: Update database schema --- db/schema.rb | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/db/schema.rb b/db/schema.rb index 4e82a688725..fc12e5752b3 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20180115201419) do +ActiveRecord::Schema.define(version: 20180125214301) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -203,6 +203,17 @@ ActiveRecord::Schema.define(version: 20180115201419) do add_index "broadcast_messages", ["starts_at", "ends_at", "id"], name: "index_broadcast_messages_on_starts_at_and_ends_at_and_id", using: :btree + create_table "callouts", force: :cascade do |t| + t.string "feature_name", null: false + t.boolean "dismissed_state", null: false + t.integer "user_id", null: false + t.datetime_with_timezone "created_at", null: false + t.datetime_with_timezone "updated_at", null: false + end + + add_index "callouts", ["feature_name"], name: "index_callouts_on_feature_name", unique: true, using: :btree + add_index "callouts", ["user_id"], name: "index_callouts_on_user_id", using: :btree + create_table "chat_names", force: :cascade do |t| t.integer "user_id", null: false t.integer "service_id", null: false @@ -1924,6 +1935,7 @@ ActiveRecord::Schema.define(version: 20180115201419) do add_index "web_hooks", ["type"], name: "index_web_hooks_on_type", using: :btree add_foreign_key "boards", "projects", name: "fk_f15266b5f9", on_delete: :cascade + add_foreign_key "callouts", "users", on_delete: :cascade add_foreign_key "chat_teams", "namespaces", on_delete: :cascade add_foreign_key "ci_build_trace_section_names", "projects", on_delete: :cascade add_foreign_key "ci_build_trace_sections", "ci_build_trace_section_names", column: "section_name_id", name: "fk_264e112c66", on_delete: :cascade -- cgit v1.2.1 From 25e1345836d37a3162445720caf50808c2b3c025 Mon Sep 17 00:00:00 2001 From: Joshua Lambert Date: Fri, 26 Jan 2018 23:58:44 -0500 Subject: Fix values.yaml for Prometheus --- vendor/prometheus/values.yaml | 140 +++++++++++++++++++----------------------- 1 file changed, 64 insertions(+), 76 deletions(-) diff --git a/vendor/prometheus/values.yaml b/vendor/prometheus/values.yaml index 5249449c7f8..4c709aeac76 100644 --- a/vendor/prometheus/values.yaml +++ b/vendor/prometheus/values.yaml @@ -11,10 +11,10 @@ pushgateway: enabled: false serverFiles: - alerts: "" - rules: "" + alerts: {} + rules: {} - prometheus.yml: |- + prometheus.yml: rule_files: - /etc/config/rules - /etc/config/alerts @@ -26,92 +26,80 @@ serverFiles: - job_name: kubernetes-cadvisor scheme: https tls_config: - ca_file: "/var/run/secrets/kubernetes.io/serviceaccount/ca.crt" + ca_file: /var/run/secrets/kubernetes.io/serviceaccount/ca.crt insecure_skip_verify: true - bearer_token_file: "/var/run/secrets/kubernetes.io/serviceaccount/token" + bearer_token_file: /var/run/secrets/kubernetes.io/serviceaccount/token kubernetes_sd_configs: - - role: node - api_server: https://kubernetes.default.svc:443 - tls_config: - ca_file: "/var/run/secrets/kubernetes.io/serviceaccount/ca.crt" - bearer_token_file: "/var/run/secrets/kubernetes.io/serviceaccount/token" + - role: node relabel_configs: - - action: labelmap - regex: __meta_kubernetes_node_label_(.+) - - target_label: __address__ - replacement: kubernetes.default.svc:443 - - source_labels: - - __meta_kubernetes_node_name - regex: "(.+)" - target_label: __metrics_path__ - replacement: "/api/v1/nodes/${1}/proxy/metrics/cadvisor" + - action: labelmap + regex: __meta_kubernetes_node_label_(.+) + - target_label: __address__ + replacement: kubernetes.default.svc:443 + - source_labels: + - __meta_kubernetes_node_name + regex: "(.+)" + target_label: __metrics_path__ + replacement: "/api/v1/nodes/${1}/proxy/metrics/cadvisor" metric_relabel_configs: - - source_labels: - - pod_name - target_label: environment - regex: "(.+)-.+-.+" + - source_labels: + - pod_name + target_label: environment + regex: "(.+)-.+-.+" - job_name: kubernetes-nodes scheme: https tls_config: - ca_file: "/var/run/secrets/kubernetes.io/serviceaccount/ca.crt" + ca_file: /var/run/secrets/kubernetes.io/serviceaccount/ca.crt insecure_skip_verify: true - bearer_token_file: "/var/run/secrets/kubernetes.io/serviceaccount/token" + bearer_token_file: /var/run/secrets/kubernetes.io/serviceaccount/token kubernetes_sd_configs: - - role: node - api_server: https://kubernetes.default.svc:443 - tls_config: - ca_file: "/var/run/secrets/kubernetes.io/serviceaccount/ca.crt" - bearer_token_file: "/var/run/secrets/kubernetes.io/serviceaccount/token" + - role: node relabel_configs: - - action: labelmap - regex: __meta_kubernetes_node_label_(.+) - - target_label: __address__ - replacement: kubernetes.default.svc:443 - - source_labels: - - __meta_kubernetes_node_name - regex: "(.+)" - target_label: __metrics_path__ - replacement: "/api/v1/nodes/${1}/proxy/metrics" + - action: labelmap + regex: __meta_kubernetes_node_label_(.+) + - target_label: __address__ + replacement: kubernetes.default.svc:443 + - source_labels: + - __meta_kubernetes_node_name + regex: "(.+)" + target_label: __metrics_path__ + replacement: "/api/v1/nodes/${1}/proxy/metrics" metric_relabel_configs: - - source_labels: - - pod_name - target_label: environment - regex: "(.+)-.+-.+" + - source_labels: + - pod_name + target_label: environment + regex: "(.+)-.+-.+" - job_name: kubernetes-pods tls_config: - ca_file: "/var/run/secrets/kubernetes.io/serviceaccount/ca.crt" + ca_file: /var/run/secrets/kubernetes.io/serviceaccount/ca.crt insecure_skip_verify: true - bearer_token_file: "/var/run/secrets/kubernetes.io/serviceaccount/token" + bearer_token_file: /var/run/secrets/kubernetes.io/serviceaccount/token kubernetes_sd_configs: - - role: pod - api_server: https://kubernetes.default.svc:443 - tls_config: - ca_file: "/var/run/secrets/kubernetes.io/serviceaccount/ca.crt" - bearer_token_file: "/var/run/secrets/kubernetes.io/serviceaccount/token" + - role: pod relabel_configs: - - source_labels: - - __meta_kubernetes_pod_annotation_prometheus_io_scrape - action: keep - regex: 'true' - - source_labels: - - __meta_kubernetes_pod_annotation_prometheus_io_path - action: replace - target_label: __metrics_path__ - regex: "(.+)" - - source_labels: - - __address__ - - __meta_kubernetes_pod_annotation_prometheus_io_port - action: replace - regex: "([^:]+)(?::[0-9]+)?;([0-9]+)" - replacement: "$1:$2" - target_label: __address__ - - action: labelmap - regex: __meta_kubernetes_pod_label_(.+) - - source_labels: - - __meta_kubernetes_namespace - action: replace - target_label: kubernetes_namespace - - source_labels: - - __meta_kubernetes_pod_name - action: replace - target_label: kubernetes_pod_name + - source_labels: + - __meta_kubernetes_pod_annotation_prometheus_io_scrape + action: keep + regex: 'true' + - source_labels: + - __meta_kubernetes_pod_annotation_prometheus_io_path + action: replace + target_label: __metrics_path__ + regex: "(.+)" + - source_labels: + - __address__ + - __meta_kubernetes_pod_annotation_prometheus_io_port + action: replace + regex: "([^:]+)(?::[0-9]+)?;([0-9]+)" + replacement: "$1:$2" + target_label: __address__ + - action: labelmap + regex: __meta_kubernetes_pod_label_(.+) + - source_labels: + - __meta_kubernetes_namespace + action: replace + target_label: kubernetes_namespace + - source_labels: + - __meta_kubernetes_pod_name + action: replace + target_label: kubernetes_pod_name -- cgit v1.2.1 From ec6c5833b62df7ff1fe69d4b874762713b4068a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Sat, 27 Jan 2018 17:32:10 +0100 Subject: Implement CalloutsHelper --- app/helpers/callouts_helper.rb | 11 +++++++++ spec/helpers/callouts_helper_spec.rb | 45 ++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+) create mode 100644 app/helpers/callouts_helper.rb create mode 100644 spec/helpers/callouts_helper_spec.rb diff --git a/app/helpers/callouts_helper.rb b/app/helpers/callouts_helper.rb new file mode 100644 index 00000000000..e435879ee43 --- /dev/null +++ b/app/helpers/callouts_helper.rb @@ -0,0 +1,11 @@ +module CalloutsHelper + def show_gke_cluster_integration_callout?(kube_feature_name, project) + !user_dismissed?(kube_feature_name) && project.team.master?(current_user) + end + + private + + def user_dismissed?(feature_name) + Callout.find_by(user: current_user, feature_name: feature_name).dismissed_state? + end +end diff --git a/spec/helpers/callouts_helper_spec.rb b/spec/helpers/callouts_helper_spec.rb new file mode 100644 index 00000000000..feabbf33ef6 --- /dev/null +++ b/spec/helpers/callouts_helper_spec.rb @@ -0,0 +1,45 @@ +require "spec_helper" + +describe CalloutsHelper do + let(:user) { create(:user) } + + before do + allow(helper).to receive(:current_user).and_return(user) + end + + describe '.show_gke_cluster_integration_callout?' do + let(:project) { create(:project) } + + subject { helper.show_gke_cluster_integration_callout?('test_name', project) } + + context 'when user has not dismissed' do + before do + allow(helper).to receive(:user_dismissed?).and_return(false) + end + + context 'when user is master' do + before do + allow(project).to receive_message_chain(:team, :master?).and_return(true) + end + + it { is_expected.to be true } + end + + context 'when user is not master' do + before do + allow(project).to receive_message_chain(:team, :master?).and_return(false) + end + + it { is_expected.to be false } + end + end + + context 'when user dismissed' do + before do + allow(helper).to receive(:user_dismissed?).and_return(true) + end + + it { is_expected.to be false } + end + end +end -- cgit v1.2.1 From 8be4f3ecf480b64284bd0d2ed31f59f332df1bae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Sat, 27 Jan 2018 18:44:14 +0100 Subject: Move Callouts route to - path --- config/routes.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/config/routes.rb b/config/routes.rb index f768bcebc7e..abd626119ca 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -60,6 +60,11 @@ Rails.application.routes.draw do resources :issues, module: :boards, only: [:index, :update] end + + # Callouts + namespace :callouts do + post :dismiss + end end # Koding route @@ -90,11 +95,6 @@ Rails.application.routes.draw do # Notification settings resources :notification_settings, only: [:create, :update] - # Callouts - namespace :callouts do - post :dismiss - end - draw :google_api draw :import draw :uploads -- cgit v1.2.1 From b1c40590e7738a782d1295d743a6a3957723c4b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Mon, 29 Jan 2018 00:23:36 +0100 Subject: Extend Runner API helpers with cache info storage --- lib/api/helpers/runner.rb | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/lib/api/helpers/runner.rb b/lib/api/helpers/runner.rb index 2cae53dba53..ad8c1c4407c 100644 --- a/lib/api/helpers/runner.rb +++ b/lib/api/helpers/runner.rb @@ -27,6 +27,8 @@ module API end def update_runner_info + update_runner_info_cache + return unless update_runner? current_runner.contacted_at = Time.now @@ -35,13 +37,17 @@ module API end def update_runner? - # Use a random threshold to prevent beating DB updates. - # It generates a distribution between [40m, 80m]. - # - contacted_at_max_age = UPDATE_RUNNER_EVERY + Random.rand(UPDATE_RUNNER_EVERY) + # Use a 1h threshold to prevent beating DB updates. current_runner.contacted_at.nil? || - (Time.now - current_runner.contacted_at) >= contacted_at_max_age + (Time.now - current_runner.contacted_at) >= UPDATE_RUNNER_EVERY + end + + def update_runner_info_cache + Gitlab::Redis::SharedState.with do |redis| + redis_key = "#{current_runner.runner_info_key}:contacted_at" + redis.set(redis_key, Time.now) + end end def validate_job!(job) -- cgit v1.2.1 From 0be8b3cdf677f8b20be820ad3f6fdd4b9b08fc56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Mon, 29 Jan 2018 00:25:13 +0100 Subject: Check cache in Ci::Runner#online? --- app/models/ci/runner.rb | 9 +++++++- spec/models/ci/runner_spec.rb | 54 +++++++++++++++++++++++++++++++++---------- 2 files changed, 50 insertions(+), 13 deletions(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index dcbb397fb78..7e616ee9144 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -88,7 +88,10 @@ module Ci end def online? - contacted_at && contacted_at > self.class.contact_time_deadline + Gitlab::Redis::SharedState.with do |redis| + last_seen = redis.get("#{self.runner_info_key}:contacted_at") || contacted_at + last_seen && last_seen > self.class.contact_time_deadline + end end def status @@ -152,6 +155,10 @@ module Ci ensure_runner_queue_value == value if value.present? end + def runner_info_key + "runner:info:#{self.id}" + end + private def cleanup_runner_queue diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index b2b64e6ff48..fe9e5ec9436 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -95,28 +95,58 @@ describe Ci::Runner do subject { runner.online? } - context 'never contacted' do + context 'no cache value' do before do - runner.contacted_at = nil + stub_redis("#{runner.runner_info_key}:contacted_at", nil) end - it { is_expected.to be_falsey } - end + context 'never contacted' do + before do + runner.contacted_at = nil + end - context 'contacted long time ago time' do - before do - runner.contacted_at = 1.year.ago + it { is_expected.to be_falsey } + end + + context 'contacted long time ago time' do + before do + runner.contacted_at = 1.year.ago + end + + it { is_expected.to be_falsey } end - it { is_expected.to be_falsey } + context 'contacted 1s ago' do + before do + runner.contacted_at = 1.second.ago + end + + it { is_expected.to be_truthy } + end end - context 'contacted 1s ago' do - before do - runner.contacted_at = 1.second.ago + context 'with cache value' do + context 'contacted long time ago time' do + before do + stub_redis("#{runner.runner_info_key}:contacted_at", 1.year.ago.to_s) + end + + it { is_expected.to be_falsey } + end + + context 'contacted 1s ago' do + before do + stub_redis("#{runner.runner_info_key}:contacted_at", 1.second.ago.to_s) + end + + it { is_expected.to be_truthy } end + end - it { is_expected.to be_truthy } + def stub_redis(key, value) + redis = double + allow(Gitlab::Redis::SharedState).to receive(:with).and_yield(redis) + allow(redis).to receive(:get).with(key).and_return(value) end end -- cgit v1.2.1 From d90d141c24228b8df6333b03d26a1723480837ba Mon Sep 17 00:00:00 2001 From: Tony Rom Date: Wed, 20 Dec 2017 14:39:40 +0300 Subject: Add Colors to GitLab Flavored Markdown --- app/assets/javascripts/gfm_auto_complete.js | 2 +- app/assets/stylesheets/framework/gfm.scss | 30 ++++++++ changelogs/unreleased/24167__color_label.yml | 5 ++ doc/user/markdown.md | 41 ++++++++++- lib/banzai/color_parser.rb | 50 +++++++++++++ lib/banzai/filter/color_filter.rb | 31 ++++++++ lib/banzai/pipeline/broadcast_message_pipeline.rb | 1 + lib/banzai/pipeline/gfm_pipeline.rb | 1 + spec/features/markdown_spec.rb | 8 ++ spec/fixtures/markdown.md.erb | 12 +++ spec/javascripts/gfm_auto_complete_spec.js | 9 +++ spec/lib/banzai/color_parser_spec.rb | 90 +++++++++++++++++++++++ spec/lib/banzai/filter/color_filter_spec.rb | 61 +++++++++++++++ spec/support/matchers/markdown_matchers.rb | 21 ++++++ 14 files changed, 360 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/24167__color_label.yml create mode 100644 lib/banzai/color_parser.rb create mode 100644 lib/banzai/filter/color_filter.rb create mode 100644 spec/lib/banzai/color_parser_spec.rb create mode 100644 spec/lib/banzai/filter/color_filter_spec.rb diff --git a/app/assets/javascripts/gfm_auto_complete.js b/app/assets/javascripts/gfm_auto_complete.js index df20e1e9c88..57a1fa107e5 100644 --- a/app/assets/javascripts/gfm_auto_complete.js +++ b/app/assets/javascripts/gfm_auto_complete.js @@ -461,7 +461,7 @@ class GfmAutoComplete { const accentAChar = decodeURI('%C3%80'); const accentYChar = decodeURI('%C3%BF'); - const regexp = new RegExp(`^(?:\\B|[^a-zA-Z0-9_${atSymbolsWithoutBar}]|\\s)${resultantFlag}(?!${atSymbolsWithBar})((?:[A-Za-z${accentAChar}-${accentYChar}0-9_'.+-]|[^\\x00-\\x7a])*)$`, 'gi'); + const regexp = new RegExp(`^(?:\\B|[^a-zA-Z0-9_\`${atSymbolsWithoutBar}]|\\s)${resultantFlag}(?!${atSymbolsWithBar})((?:[A-Za-z${accentAChar}-${accentYChar}0-9_'.+-]|[^\\x00-\\x7a])*)$`, 'gi'); return regexp.exec(targetSubtext); } diff --git a/app/assets/stylesheets/framework/gfm.scss b/app/assets/stylesheets/framework/gfm.scss index 5621505996d..6eff57157cc 100644 --- a/app/assets/stylesheets/framework/gfm.scss +++ b/app/assets/stylesheets/framework/gfm.scss @@ -16,3 +16,33 @@ background-color: $user-mention-bg-hover; } } + +.gfm-color_chip { + display: inline-block; + margin-left: 4px; + margin-bottom: 2px; + vertical-align: middle; + border-radius: 3px; + + $side: 0.9em; + $bg-size: $side / 0.9; + $bg-pos: $bg-size / 2; + $bg-color: $gray-dark; + + width: $side; + height: $side; + background: $white-light; + background-image: linear-gradient(135deg, $bg-color 25%, transparent 0%, transparent 75%, $bg-color 0%), + linear-gradient(135deg, $bg-color 25%, transparent 0%, transparent 75%, $bg-color 0%); + background-size: $bg-size $bg-size; + background-position: 0 0, $bg-pos $bg-pos; + + > span { + display: inline-block; + width: 100%; + height: 100%; + margin-bottom: 2px; + border-radius: 3px; + border: 1px solid $black-transparent; + } +} diff --git a/changelogs/unreleased/24167__color_label.yml b/changelogs/unreleased/24167__color_label.yml new file mode 100644 index 00000000000..68c6c731163 --- /dev/null +++ b/changelogs/unreleased/24167__color_label.yml @@ -0,0 +1,5 @@ +--- +title: Add Colors to GitLab Flavored Markdown +merge_request: 16095 +author: Tony Rom +type: added diff --git a/doc/user/markdown.md b/doc/user/markdown.md index 552abac747b..b590dfa0d40 100644 --- a/doc/user/markdown.md +++ b/doc/user/markdown.md @@ -253,7 +253,7 @@ GFM will recognize the following: | `@user_name` | specific user | | `@group_name` | specific group | | `@all` | entire team | -| `#123` | issue | +| `#12345` | issue | | `!123` | merge request | | `$123` | snippet | | `~123` | label by ID | @@ -379,6 +379,45 @@ _Be advised that KaTeX only supports a [subset][katex-subset] of LaTeX._ >**Note:** This also works for the asciidoctor `:stem: latexmath`. For details see the [asciidoctor user manual][asciidoctor-manual]. +### Colors + +> If this is not rendered correctly, see +https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/user/markdown.md#colors + +It is possible to have color written in HEX, RGB or HSL format rendered with a color indicator. + +Color written inside backticks will be followed by a color "chip". + +Examples: + + `#F00` + `#F00A` + `#FF0000` + `#FF0000AA` + `RGB(0,255,0)` + `RGB(0%,100%,0%)` + `RGBA(0,255,0,0.7)` + `HSL(540,70%,50%)` + `HSLA(540,70%,50%,0.7)` + +Becomes: + +`#F00` +`#F00A` +`#FF0000` +`#FF0000AA` +`RGB(0,255,0)` +`RGB(0%,100%,0%)` +`RGBA(0,255,0,0.7)` +`HSL(540,70%,50%)` +`HSLA(540,70%,50%,0.7)` + +#### Supported formats: + +* HEX: `` `#RGB[A]` `` or `` `#RRGGBB[AA]` `` +* RGB: `` `RGB[A](R, G, B[, A])` `` +* HSL: `` `HSL[A](H, S, L[, A])` `` + ### Mermaid > [Introduced](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/15107) in diff --git a/lib/banzai/color_parser.rb b/lib/banzai/color_parser.rb new file mode 100644 index 00000000000..d96c0a1ed1f --- /dev/null +++ b/lib/banzai/color_parser.rb @@ -0,0 +1,50 @@ +module Banzai + module ColorParser + ALPHA = /0(?:\.\d+)?|\.\d+|1(?:\.0+)?/ # 0.0..1.0 + PERCENTS = /(?:\d{1,2}|100)%/ # 00%..100% + ALPHA_CHANNEL = /(?:,\s*(?:#{ALPHA}|#{PERCENTS}))?/ + BITS = /\d{1,2}|1\d\d|2(?:[0-4]\d|5[0-5])/ # 00..255 + DEGS = /-?\d+(?:deg)?/i # [-]digits[deg] + RADS = /-?(?:\d+(?:\.\d+)?|\.\d+)rad/i # [-](digits[.digits] OR .digits)rad + HEX_FORMAT = /\#(?:\h{3}|\h{4}|\h{6}|\h{8})/ + RGB_FORMAT = / + (?:rgba? + \( + (?: + (?:(?:#{BITS},\s*){2}#{BITS}) + | + (?:(?:#{PERCENTS},\s*){2}#{PERCENTS}) + ) + #{ALPHA_CHANNEL} + \) + ) + /xi + HSL_FORMAT = / + (?:hsla? + \( + (?:#{DEGS}|#{RADS}),\s*#{PERCENTS},\s*#{PERCENTS} + #{ALPHA_CHANNEL} + \) + ) + /xi + + FORMATS = [HEX_FORMAT, RGB_FORMAT, HSL_FORMAT].freeze + + class << self + # Public: Analyzes whether the String is a color code. + # + # text - The String to be parsed. + # + # Returns the recognized color String or nil if none was found. + def parse(text) + text if color_format =~ text + end + + private + + def color_format + @color_format ||= /\A(#{Regexp.union(FORMATS)})\z/ix + end + end + end +end diff --git a/lib/banzai/filter/color_filter.rb b/lib/banzai/filter/color_filter.rb new file mode 100644 index 00000000000..6ab29ac281f --- /dev/null +++ b/lib/banzai/filter/color_filter.rb @@ -0,0 +1,31 @@ +module Banzai + module Filter + # HTML filter that renders `color` followed by a color "chip". + # + class ColorFilter < HTML::Pipeline::Filter + COLOR_CHIP_CLASS = 'gfm-color_chip'.freeze + + def call + doc.css('code').each do |node| + color = ColorParser.parse(node.content) + node << color_chip(color) if color + end + + doc + end + + private + + def color_chip(color) + checkerboard = doc.document.create_element('span', class: COLOR_CHIP_CLASS) + chip = doc.document.create_element('span', style: inline_styles(color: color)) + + checkerboard << chip + end + + def inline_styles(color:) + "background-color: #{color};" + end + end + end +end diff --git a/lib/banzai/pipeline/broadcast_message_pipeline.rb b/lib/banzai/pipeline/broadcast_message_pipeline.rb index adc09c8afbd..5dd572de3a1 100644 --- a/lib/banzai/pipeline/broadcast_message_pipeline.rb +++ b/lib/banzai/pipeline/broadcast_message_pipeline.rb @@ -7,6 +7,7 @@ module Banzai Filter::SanitizationFilter, Filter::EmojiFilter, + Filter::ColorFilter, Filter::AutolinkFilter, Filter::ExternalLinkFilter ] diff --git a/lib/banzai/pipeline/gfm_pipeline.rb b/lib/banzai/pipeline/gfm_pipeline.rb index c746f6f64e9..4001b8a85e3 100644 --- a/lib/banzai/pipeline/gfm_pipeline.rb +++ b/lib/banzai/pipeline/gfm_pipeline.rb @@ -14,6 +14,7 @@ module Banzai Filter::SyntaxHighlightFilter, Filter::MathFilter, + Filter::ColorFilter, Filter::MermaidFilter, Filter::VideoLinkFilter, Filter::ImageLazyLoadFilter, diff --git a/spec/features/markdown_spec.rb b/spec/features/markdown_spec.rb index a2b78a5e021..f13d78d24e3 100644 --- a/spec/features/markdown_spec.rb +++ b/spec/features/markdown_spec.rb @@ -259,6 +259,10 @@ describe 'GitLab Markdown' do it 'includes VideoLinkFilter' do expect(doc).to parse_video_links end + + it 'includes ColorFilter' do + expect(doc).to parse_colors + end end context 'wiki pipeline' do @@ -320,6 +324,10 @@ describe 'GitLab Markdown' do it 'includes VideoLinkFilter' do expect(doc).to parse_video_links end + + it 'includes ColorFilter' do + expect(doc).to parse_colors + end end # Fake a `current_user` helper diff --git a/spec/fixtures/markdown.md.erb b/spec/fixtures/markdown.md.erb index 71abb6da607..da32a46675f 100644 --- a/spec/fixtures/markdown.md.erb +++ b/spec/fixtures/markdown.md.erb @@ -280,6 +280,18 @@ However the wrapping tags cannot be mixed as such: ![My Video](/assets/videos/gitlab-demo.mp4) +### Colors + +`#F00` +`#F00A` +`#FF0000` +`#FF0000AA` +`RGB(0,255,0)` +`RGB(0%,100%,0%)` +`RGBA(0,255,0,0.7)` +`HSL(540,70%,50%)` +`HSLA(540,70%,50%,0.7)` + ### Mermaid > If this is not rendered correctly, see diff --git a/spec/javascripts/gfm_auto_complete_spec.js b/spec/javascripts/gfm_auto_complete_spec.js index 6f357306ec7..182880d42e9 100644 --- a/spec/javascripts/gfm_auto_complete_spec.js +++ b/spec/javascripts/gfm_auto_complete_spec.js @@ -131,6 +131,7 @@ describe('GfmAutoComplete', function () { describe('should not match special sequences', () => { const ShouldNotBeFollowedBy = flags.concat(['\x00', '\x10', '\x3f', '\n', ' ']); + const ShouldNotBePrependedBy = ['`']; flagsUseDefaultMatcher.forEach((atSign) => { ShouldNotBeFollowedBy.forEach((followedSymbol) => { @@ -140,6 +141,14 @@ describe('GfmAutoComplete', function () { expect(defaultMatcher(atwhoInstance, atSign, seq)).toBe(null); }); }); + + ShouldNotBePrependedBy.forEach((prependedSymbol) => { + const seq = prependedSymbol + atSign; + + it(`should not match "${seq}"`, () => { + expect(defaultMatcher(atwhoInstance, atSign, seq)).toBe(null); + }); + }); }); }); }); diff --git a/spec/lib/banzai/color_parser_spec.rb b/spec/lib/banzai/color_parser_spec.rb new file mode 100644 index 00000000000..a1cb0c07b06 --- /dev/null +++ b/spec/lib/banzai/color_parser_spec.rb @@ -0,0 +1,90 @@ +require 'spec_helper' + +describe Banzai::ColorParser do + describe '.parse' do + context 'HEX format' do + [ + '#abc', '#ABC', + '#d2d2d2', '#D2D2D2', + '#123a', '#123A', + '#123456aa', '#123456AA' + ].each do |color| + it "parses the valid hex color #{color}" do + expect(subject.parse(color)).to eq(color) + end + end + + [ + '#', '#1', '#12', '#12g', '#12G', + '#12345', '#r2r2r2', '#R2R2R2', '#1234567', + '# 123', '# 1234', '# 123456', '# 12345678', + '#1 2 3', '#123 4', '#12 34 56', '#123456 78' + ].each do |color| + it "does not parse the invalid hex color #{color}" do + expect(subject.parse(color)).to be_nil + end + end + end + + context 'RGB format' do + [ + 'rgb(0,0,0)', 'rgb(255,255,255)', + 'rgb(0, 0, 0)', 'RGB(0,0,0)', + 'rgb(0,0,0,0)', 'rgb(0,0,0,0.0)', 'rgb(0,0,0,.0)', + 'rgb(0,0,0, 0)', 'rgb(0,0,0, 0.0)', 'rgb(0,0,0, .0)', + 'rgb(0,0,0,1)', 'rgb(0,0,0,1.0)', + 'rgba(0,0,0)', 'rgba(0,0,0,0)', 'RGBA(0,0,0)', + 'rgb(0%,0%,0%)', 'rgba(0%,0%,0%,0%)' + ].each do |color| + it "parses the valid rgb color #{color}" do + expect(subject.parse(color)).to eq(color) + end + end + + [ + 'FOOrgb(0,0,0)', 'rgb(0,0,0)BAR', + 'rgb(0,0,-1)', 'rgb(0,0,-0)', 'rgb(0,0,256)', + 'rgb(0,0,0,-0.1)', 'rgb(0,0,0,-0.0)', 'rgb(0,0,0,-.1)', + 'rgb(0,0,0,1.1)', 'rgb(0,0,0,2)', + 'rgba(0,0,0,)', 'rgba(0,0,0,0.)', 'rgba(0,0,0,1.)', + 'rgb(0,0,0%)', 'rgb(101%,0%,0%)' + ].each do |color| + it "does not parse the invalid rgb color #{color}" do + expect(subject.parse(color)).to be_nil + end + end + end + + context 'HSL format' do + [ + 'hsl(0,0%,0%)', 'hsl(0,100%,100%)', + 'hsl(540,0%,0%)', 'hsl(-720,0%,0%)', + 'hsl(0deg,0%,0%)', 'hsl(0DEG,0%,0%)', + 'hsl(0, 0%, 0%)', 'HSL(0,0%,0%)', + 'hsl(0,0%,0%,0)', 'hsl(0,0%,0%,0.0)', 'hsl(0,0%,0%,.0)', + 'hsl(0,0%,0%, 0)', 'hsl(0,0%,0%, 0.0)', 'hsl(0,0%,0%, .0)', + 'hsl(0,0%,0%,1)', 'hsl(0,0%,0%,1.0)', + 'hsla(0,0%,0%)', 'hsla(0,0%,0%,0)', 'HSLA(0,0%,0%)', + 'hsl(1rad,0%,0%)', 'hsl(1.1rad,0%,0%)', 'hsl(.1rad,0%,0%)', + 'hsl(-1rad,0%,0%)', 'hsl(1RAD,0%,0%)' + ].each do |color| + it "parses the valid hsl color #{color}" do + expect(subject.parse(color)).to eq(color) + end + end + + [ + 'hsl(+0,0%,0%)', 'hsl(0,0,0%)', 'hsl(0,0%,0)', 'hsl(0 deg,0%,0%)', + 'hsl(0,-0%,0%)', 'hsl(0,101%,0%)', 'hsl(0,-1%,0%)', + 'hsl(0,0%,0%,-0.1)', 'hsl(0,0%,0%,-.1)', + 'hsl(0,0%,0%,1.1)', 'hsl(0,0%,0%,2)', + 'hsl(0,0%,0%,)', 'hsl(0,0%,0%,0.)', 'hsl(0,0%,0%,1.)', + 'hsl(deg,0%,0%)', 'hsl(rad,0%,0%)' + ].each do |color| + it "does not parse the invalid hsl color #{color}" do + expect(subject.parse(color)).to be_nil + end + end + end + end +end diff --git a/spec/lib/banzai/filter/color_filter_spec.rb b/spec/lib/banzai/filter/color_filter_spec.rb new file mode 100644 index 00000000000..a098b037510 --- /dev/null +++ b/spec/lib/banzai/filter/color_filter_spec.rb @@ -0,0 +1,61 @@ +require 'spec_helper' + +describe Banzai::Filter::ColorFilter, lib: true do + include FilterSpecHelper + + let(:color) { '#F00' } + let(:color_chip_selector) { 'code > span.gfm-color_chip > span' } + + ['#123', '#1234', '#123456', '#12345678', + 'rgb(0,0,0)', 'RGB(0, 0, 0)', 'rgba(0,0,0,1)', 'RGBA(0,0,0,0.7)', + 'hsl(270,30%,50%)', 'HSLA(270, 30%, 50%, .7)'].each do |color| + it "inserts color chip for supported color format #{color}" do + content = code_tag(color) + doc = filter(content) + color_chip = doc.at_css(color_chip_selector) + + expect(color_chip.content).to be_empty + expect(color_chip.parent[:class]).to eq 'gfm-color_chip' + expect(color_chip[:style]).to eq "background-color: #{color};" + end + end + + it 'ignores valid color code without backticks(code tags)' do + doc = filter(color) + + expect(doc.css('span.gfm-color_chip').size).to be_zero + end + + it 'ignores valid color code with prepended space' do + content = code_tag(' ' + color) + doc = filter(content) + + expect(doc.css(color_chip_selector).size).to be_zero + end + + it 'ignores valid color code with appended space' do + content = code_tag(color + ' ') + doc = filter(content) + + expect(doc.css(color_chip_selector).size).to be_zero + end + + it 'ignores valid color code surrounded by spaces' do + content = code_tag(' ' + color + ' ') + doc = filter(content) + + expect(doc.css(color_chip_selector).size).to be_zero + end + + it 'ignores invalid color code' do + invalid_color = '#BAR' + content = code_tag(invalid_color) + doc = filter(content) + + expect(doc.css(color_chip_selector).size).to be_zero + end + + def code_tag(string) + "#{string}" + end +end diff --git a/spec/support/matchers/markdown_matchers.rb b/spec/support/matchers/markdown_matchers.rb index d12b2757427..ec4ec6f4038 100644 --- a/spec/support/matchers/markdown_matchers.rb +++ b/spec/support/matchers/markdown_matchers.rb @@ -190,6 +190,27 @@ module MarkdownMatchers expect(video['src']).to end_with('/assets/videos/gitlab-demo.mp4') end end + + # ColorFilter + matcher :parse_colors do + set_default_markdown_messages + + match do |actual| + color_chips = actual.css('code > span.gfm-color_chip > span') + + expect(color_chips.count).to eq(9) + + [ + '#F00', '#F00A', '#FF0000', '#FF0000AA', 'RGB(0,255,0)', + 'RGB(0%,100%,0%)', 'RGBA(0,255,0,0.7)', 'HSL(540,70%,50%)', + 'HSLA(540,70%,50%,0.7)' + ].each_with_index do |color, i| + parsed_color = Banzai::ColorParser.parse(color) + expect(color_chips[i]['style']).to match("background-color: #{parsed_color};") + expect(color_chips[i].parent.parent.content).to match(color) + end + end + end end # Monkeypatch the matcher DSL so that we can reduce some noisy duplication for -- cgit v1.2.1 From 397442a06140a8cf38bebe3f4519311b1448f23d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Mon, 29 Jan 2018 17:21:30 +0100 Subject: Update runner info on all authenticated requests --- lib/api/helpers/runner.rb | 2 ++ lib/api/runner.rb | 1 - 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/api/helpers/runner.rb b/lib/api/helpers/runner.rb index ad8c1c4407c..5ac9181f878 100644 --- a/lib/api/helpers/runner.rb +++ b/lib/api/helpers/runner.rb @@ -20,6 +20,8 @@ module API def authenticate_runner! forbidden! unless current_runner + + update_runner_info end def current_runner diff --git a/lib/api/runner.rb b/lib/api/runner.rb index 80feb629d54..50cb1df92ad 100644 --- a/lib/api/runner.rb +++ b/lib/api/runner.rb @@ -78,7 +78,6 @@ module API post '/request' do authenticate_runner! no_content! unless current_runner.active? - update_runner_info if current_runner.runner_queue_value_latest?(params[:last_update]) header 'X-GitLab-Last-Update', params[:last_update] -- cgit v1.2.1 From 92ac2b9ee68cad8c01a199e6475bbef272818da5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Mon, 29 Jan 2018 17:24:29 +0100 Subject: Add CHANGELOG entry --- ...ancels-stuck-builds-when-per-job-timeout-is-more-than-an-hour.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/38265-stuckcijobsworker-wrongly-detects-cancels-stuck-builds-when-per-job-timeout-is-more-than-an-hour.yml diff --git a/changelogs/unreleased/38265-stuckcijobsworker-wrongly-detects-cancels-stuck-builds-when-per-job-timeout-is-more-than-an-hour.yml b/changelogs/unreleased/38265-stuckcijobsworker-wrongly-detects-cancels-stuck-builds-when-per-job-timeout-is-more-than-an-hour.yml new file mode 100644 index 00000000000..4d8e6acfcb7 --- /dev/null +++ b/changelogs/unreleased/38265-stuckcijobsworker-wrongly-detects-cancels-stuck-builds-when-per-job-timeout-is-more-than-an-hour.yml @@ -0,0 +1,5 @@ +--- +title: Update runner info on all authenticated requests +merge_request: 16756 +author: +type: changed -- cgit v1.2.1 From bdd3e39b0bd4e8fcec5d6e2d97297840211dd921 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Mon, 29 Jan 2018 20:56:28 +0100 Subject: Move info update implementation to Ci::Runner model --- app/models/ci/runner.rb | 26 +++++++++++++++++++--- lib/api/helpers/runner.rb | 27 +--------------------- spec/models/ci/runner_spec.rb | 52 ++++++++++++++++++++++++++++++++++++++----- 3 files changed, 71 insertions(+), 34 deletions(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 7e616ee9144..44be247bcd3 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -5,6 +5,7 @@ module Ci RUNNER_QUEUE_EXPIRY_TIME = 60.minutes ONLINE_CONTACT_TIMEOUT = 1.hour + UPDATE_DB_RUNNER_INFO_EVERY = 1.hour AVAILABLE_SCOPES = %w[specific shared active paused online].freeze FORM_EDITABLE = %i[description tag_list active run_untagged locked access_level].freeze @@ -89,7 +90,7 @@ module Ci def online? Gitlab::Redis::SharedState.with do |redis| - last_seen = redis.get("#{self.runner_info_key}:contacted_at") || contacted_at + last_seen = redis.get("#{runner_info_redis_cache_key}:contacted_at") || contacted_at last_seen && last_seen > self.class.contact_time_deadline end end @@ -155,8 +156,16 @@ module Ci ensure_runner_queue_value == value if value.present? end - def runner_info_key - "runner:info:#{self.id}" + def update_runner_info(params) + update_runner_info_cache + + # Use a 1h threshold to prevent beating DB updates. + return unless self.contacted_at.nil? || + (Time.now - self.contacted_at) >= UPDATE_DB_RUNNER_INFO_EVERY + + self.contacted_at = Time.now + self.assign_attributes(params) + self.save if self.changed? end private @@ -171,6 +180,17 @@ module Ci "runner:build_queue:#{self.token}" end + def runner_info_redis_cache_key + "runner:info:#{self.id}" + end + + def update_runner_info_cache + Gitlab::Redis::SharedState.with do |redis| + redis_key = "#{runner_info_redis_cache_key}:contacted_at" + redis.set(redis_key, Time.now) + end + end + def tag_constraints unless has_tags? || run_untagged? errors.add(:tags_list, diff --git a/lib/api/helpers/runner.rb b/lib/api/helpers/runner.rb index 5ac9181f878..8f45cae0e60 100644 --- a/lib/api/helpers/runner.rb +++ b/lib/api/helpers/runner.rb @@ -5,7 +5,6 @@ module API JOB_TOKEN_HEADER = 'HTTP_JOB_TOKEN'.freeze JOB_TOKEN_PARAM = :token - UPDATE_RUNNER_EVERY = 10 * 60 def runner_registration_token_valid? ActiveSupport::SecurityUtils.variable_size_secure_compare(params[:token], @@ -21,37 +20,13 @@ module API def authenticate_runner! forbidden! unless current_runner - update_runner_info + current_runner.update_runner_info(get_runner_version_from_params) end def current_runner @runner ||= ::Ci::Runner.find_by_token(params[:token].to_s) end - def update_runner_info - update_runner_info_cache - - return unless update_runner? - - current_runner.contacted_at = Time.now - current_runner.assign_attributes(get_runner_version_from_params) - current_runner.save if current_runner.changed? - end - - def update_runner? - # Use a 1h threshold to prevent beating DB updates. - - current_runner.contacted_at.nil? || - (Time.now - current_runner.contacted_at) >= UPDATE_RUNNER_EVERY - end - - def update_runner_info_cache - Gitlab::Redis::SharedState.with do |redis| - redis_key = "#{current_runner.runner_info_key}:contacted_at" - redis.set(redis_key, Time.now) - end - end - def validate_job!(job) not_found! unless job diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index fe9e5ec9436..830f7cd68c5 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -97,7 +97,7 @@ describe Ci::Runner do context 'no cache value' do before do - stub_redis("#{runner.runner_info_key}:contacted_at", nil) + stub_redis_runner_contacted_at(nil) end context 'never contacted' do @@ -128,7 +128,7 @@ describe Ci::Runner do context 'with cache value' do context 'contacted long time ago time' do before do - stub_redis("#{runner.runner_info_key}:contacted_at", 1.year.ago.to_s) + stub_redis_runner_contacted_at(1.year.ago.to_s) end it { is_expected.to be_falsey } @@ -136,17 +136,17 @@ describe Ci::Runner do context 'contacted 1s ago' do before do - stub_redis("#{runner.runner_info_key}:contacted_at", 1.second.ago.to_s) + stub_redis_runner_contacted_at(1.second.ago.to_s) end it { is_expected.to be_truthy } end end - def stub_redis(key, value) + def stub_redis_runner_contacted_at(value) redis = double allow(Gitlab::Redis::SharedState).to receive(:with).and_yield(redis) - allow(redis).to receive(:get).with(key).and_return(value) + allow(redis).to receive(:get).with("#{runner.send(:runner_info_redis_cache_key)}:contacted_at").and_return(value) end end @@ -391,6 +391,48 @@ describe Ci::Runner do end end + describe '#update_runner_info' do + let(:runner) { create(:ci_runner) } + + subject { runner.update_runner_info(contacted_at: Time.now) } + + context 'when database was updated recently' do + before do + runner.update(contacted_at: Time.now) + end + + it 'updates cache' do + expect_redis_update + + subject + end + end + + context 'when database was not updated recently' do + before do + runner.update(contacted_at: 2.hours.ago) + end + + it 'updates database' do + expect_redis_update + + expect { subject }.to change { runner.reload.contacted_at } + end + + it 'updates cache' do + expect_redis_update + + subject + end + end + + def expect_redis_update + redis = double + expect(Gitlab::Redis::SharedState).to receive(:with).and_yield(redis) + expect(redis).to receive(:set).with("#{runner.send(:runner_info_redis_cache_key)}:contacted_at", anything) + end + end + describe '#destroy' do let(:runner) { create(:ci_runner) } -- cgit v1.2.1 From df2554558123fbfec007418601e2074cf251db46 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Mon, 29 Jan 2018 21:53:37 +0100 Subject: Generelized cached attribute usage in runner --- app/models/ci/runner.rb | 27 +++++++++++++++++++-------- spec/models/ci/runner_spec.rb | 17 +++++++++++------ 2 files changed, 30 insertions(+), 14 deletions(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 44be247bcd3..8fe20622723 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -68,6 +68,10 @@ module Ci ONLINE_CONTACT_TIMEOUT.ago end + def cached_contacted_at + runner_info_cache(:contacted_at) || self.contacted_at + end + def set_default_values self.token = SecureRandom.hex(15) if self.token.blank? end @@ -89,10 +93,7 @@ module Ci end def online? - Gitlab::Redis::SharedState.with do |redis| - last_seen = redis.get("#{runner_info_redis_cache_key}:contacted_at") || contacted_at - last_seen && last_seen > self.class.contact_time_deadline - end + cached_contacted_at && cached_contacted_at > self.class.contact_time_deadline end def status @@ -157,7 +158,7 @@ module Ci end def update_runner_info(params) - update_runner_info_cache + update_runner_info_cache(params) # Use a 1h threshold to prevent beating DB updates. return unless self.contacted_at.nil? || @@ -184,10 +185,20 @@ module Ci "runner:info:#{self.id}" end - def update_runner_info_cache + def update_runner_info_cache(params) + Gitlab::Redis::SharedState.with do |redis| + redis.set("#{runner_info_redis_cache_key}:contacted_at", Time.now) + + params.each do |key, value| + redis_key = "#{runner_info_redis_cache_key}:#{key}" + redis.set(redis_key, value) + end + end + end + + def runner_info_cache(attribute) Gitlab::Redis::SharedState.with do |redis| - redis_key = "#{runner_info_redis_cache_key}:contacted_at" - redis.set(redis_key, Time.now) + redis.get("#{runner_info_redis_cache_key}:#{attribute}") end end diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 830f7cd68c5..14747a23c82 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -394,7 +394,7 @@ describe Ci::Runner do describe '#update_runner_info' do let(:runner) { create(:ci_runner) } - subject { runner.update_runner_info(contacted_at: Time.now) } + subject { runner.update_runner_info(name: 'testing_runner') } context 'when database was updated recently' do before do @@ -402,7 +402,7 @@ describe Ci::Runner do end it 'updates cache' do - expect_redis_update + expect_redis_update(:contacted_at, :name) subject end @@ -414,22 +414,27 @@ describe Ci::Runner do end it 'updates database' do - expect_redis_update + expect_redis_update(:contacted_at, :name) expect { subject }.to change { runner.reload.contacted_at } + .and change { runner.reload.name } end it 'updates cache' do - expect_redis_update + expect_redis_update(:contacted_at, :name) subject end end - def expect_redis_update + def expect_redis_update(*params) redis = double expect(Gitlab::Redis::SharedState).to receive(:with).and_yield(redis) - expect(redis).to receive(:set).with("#{runner.send(:runner_info_redis_cache_key)}:contacted_at", anything) + + params.each do |param| + redis_key = "#{runner.send(:runner_info_redis_cache_key)}:#{param}" + expect(redis).to receive(:set).with(redis_key, anything) + end end end -- cgit v1.2.1 From b88103e4075678d032d7d7350caaece4a3091328 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Mon, 29 Jan 2018 22:38:09 +0100 Subject: Expose Ci::Runner#cached_contacted_at as Time --- app/models/ci/runner.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 8fe20622723..7cf36c0bfe0 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -69,7 +69,11 @@ module Ci end def cached_contacted_at - runner_info_cache(:contacted_at) || self.contacted_at + if runner_info_cache(:contacted_at) + Time.zone.parse(runner_info_cache(:contacted_at)) + else + self.contacted_at + end end def set_default_values -- cgit v1.2.1 From bb8490b018bda7327a82e8ed38d3f1d60c9b4c39 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Gray Date: Fri, 19 Jan 2018 14:40:18 -0700 Subject: Start redesign of group labels page --- app/assets/stylesheets/pages/labels.scss | 22 +++++++++++++++------ app/views/shared/_label.html.haml | 34 ++++++++++++-------------------- app/views/shared/_label_row.html.haml | 12 +++++++++++ 3 files changed, 41 insertions(+), 27 deletions(-) diff --git a/app/assets/stylesheets/pages/labels.scss b/app/assets/stylesheets/pages/labels.scss index e8cd8a4905c..91aa6648d41 100644 --- a/app/assets/stylesheets/pages/labels.scss +++ b/app/assets/stylesheets/pages/labels.scss @@ -64,7 +64,6 @@ .label { overflow: hidden; text-overflow: ellipsis; - vertical-align: middle; max-width: 100%; } } @@ -86,14 +85,17 @@ .label-description { display: block; margin-bottom: 10px; - margin-left: 50px; + + a { + color: $blue-600; + } @media (min-width: $screen-sm-min) { display: inline-block; - width: 30%; + max-width: 50%; margin-left: 10px; margin-bottom: 0; - vertical-align: middle; + vertical-align: top; } } @@ -116,6 +118,10 @@ } .manage-labels-list { + &.content-list li { + padding: $gl-padding 0; + } + > li:not(.empty-message):not(.is-not-draggable) { background-color: $white-light; cursor: move; @@ -133,8 +139,6 @@ } .btn-action { - color: $gl-text-color; - .fa { font-size: 18px; vertical-align: middle; @@ -155,6 +159,12 @@ float: right; } } + + @media (max-width: $screen-sm-max) { + .dropdown-menu { + min-width: 100%; + } + } } .draggable-handler { diff --git a/app/views/shared/_label.html.haml b/app/views/shared/_label.html.haml index 8e88cecaf9e..6a103f56706 100644 --- a/app/views/shared/_label.html.haml +++ b/app/views/shared/_label.html.haml @@ -8,7 +8,7 @@ %li{ id: label_css_id, data: { id: label.id } } = render "shared/label_row", label: label - .visible-xs.visible-sm-inline-block.visible-md-inline-block.dropdown + .visible-xs.visible-sm-inline-block.dropdown %button.btn.btn-default.label-options-toggle{ type: 'button', data: { toggle: "dropdown" } } Options = icon('caret-down') @@ -46,14 +46,18 @@ data: {confirm: 'Remove this label? Are you sure?'}, class: 'text-danger' - .pull-right.hidden-xs.hidden-sm.hidden-md - - if show_label_merge_requests_link - = link_to_label(label, subject: subject, type: :merge_request, css_class: 'btn btn-transparent btn-action btn-link') do - view merge requests - - if show_label_issues_link - = link_to_label(label, subject: subject, css_class: 'btn btn-transparent btn-action btn-link') do - view open issues - + .pull-right.hidden-xs.hidden-sm + - if label.is_a?(ProjectLabel) && label.project.group && can?(current_user, :admin_label, label.project.group) + = link_to promote_project_label_path(label.project, label), title: "Promote to Group Label", class: 'btn btn-transparent btn-action', data: {confirm: "You are about to promote #{label.title} to a group level. This will make this milestone available to all projects inside #{label.project.group.name}. The existing project label will be merged into the group level. This action cannot be reversed.", toggle: "tooltip"}, method: :post do + %span.sr-only Promote to Group + = icon('level-up') + - if can?(current_user, :admin_label, label) + = link_to edit_label_path(label), title: "Edit", class: 'btn btn-transparent btn-action', data: {toggle: "tooltip"} do + %span.sr-only Edit + = sprite_icon('pencil') + = link_to destroy_label_path(label), title: "Delete", class: 'btn btn-transparent btn-action remove-row', method: :delete, data: {confirm: label_deletion_confirm_text(label), toggle: "tooltip"} do + %span.sr-only Delete + = sprite_icon('remove') - if current_user .label-subscription.inline - if can_subscribe_to_label_in_different_levels?(label) @@ -75,15 +79,3 @@ %button.js-subscribe-button.label-subscribe-button.btn.btn-default{ type: 'button', data: { status: status, url: toggle_subscription_path } } %span= label_subscription_toggle_button_text(label, @project) = icon('spinner spin', class: 'label-subscribe-button-loading') - - - if label.is_a?(ProjectLabel) && label.project.group && can?(current_user, :admin_label, label.project.group) - = link_to promote_project_label_path(label.project, label), title: "Promote to Group Label", class: 'btn btn-transparent btn-action', data: {confirm: "You are about to promote #{label.title} to a group level. This will make this milestone available to all projects inside #{label.project.group.name}. The existing project label will be merged into the group level. This action cannot be reversed.", toggle: "tooltip"}, method: :post do - %span.sr-only Promote to Group - = icon('level-up') - - if can?(current_user, :admin_label, label) - = link_to edit_label_path(label), title: "Edit", class: 'btn btn-transparent btn-action', data: {toggle: "tooltip"} do - %span.sr-only Edit - = icon('pencil-square-o') - = link_to destroy_label_path(label), title: "Delete", class: 'btn btn-transparent btn-action remove-row', method: :delete, data: {confirm: label_deletion_confirm_text(label), toggle: "tooltip"} do - %span.sr-only Delete - = icon('trash-o') diff --git a/app/views/shared/_label_row.html.haml b/app/views/shared/_label_row.html.haml index 7f58298c60f..34da13ca52b 100644 --- a/app/views/shared/_label_row.html.haml +++ b/app/views/shared/_label_row.html.haml @@ -1,3 +1,7 @@ +- subject = local_assigns[:subject] +- show_label_issues_link = show_label_issuables_link?(label, :issues, project: @project) +- show_label_merge_requests_link = show_label_issuables_link?(label, :merge_requests, project: @project) + %span.label-row - if can?(current_user, :admin_label, @project) .draggable-handler @@ -16,3 +20,11 @@ - if label.description %span.label-description = markdown_field(label, :description) + .hidden-xs.hidden-sm + - if show_label_issues_link + = link_to_label(label, subject: subject) do + Issues + - if show_label_merge_requests_link + · + = link_to_label(label, subject: subject, type: :merge_request) do + Merge requests -- cgit v1.2.1 From af6aaf39f996e888ecfc536a42f25610623f6e02 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Gray Date: Mon, 22 Jan 2018 14:20:34 -0700 Subject: Change delete notice; update mobile styles --- app/assets/stylesheets/pages/labels.scss | 24 +++++++++++++++++++----- app/controllers/groups/labels_controller.rb | 2 +- app/views/shared/_label_row.html.haml | 22 +++++++++++----------- 3 files changed, 31 insertions(+), 17 deletions(-) diff --git a/app/assets/stylesheets/pages/labels.scss b/app/assets/stylesheets/pages/labels.scss index 91aa6648d41..9c4f51fc0d9 100644 --- a/app/assets/stylesheets/pages/labels.scss +++ b/app/assets/stylesheets/pages/labels.scss @@ -58,6 +58,7 @@ @media (min-width: $screen-sm-min) { width: 200px; + margin-left: $gl-padding * 2; margin-bottom: 0; } @@ -78,7 +79,7 @@ width: 100px; margin-left: 10px; margin-bottom: 0; - vertical-align: middle; + vertical-align: top; } } @@ -86,6 +87,10 @@ display: block; margin-bottom: 10px; + .description-text { + margin-bottom: $gl-padding; + } + a { color: $blue-600; } @@ -118,8 +123,10 @@ } .manage-labels-list { - &.content-list li { - padding: $gl-padding 0; + @media(min-width: $screen-md-min) { + &.content-list li { + padding: $gl-padding 0; + } } > li:not(.empty-message):not(.is-not-draggable) { @@ -160,7 +167,7 @@ } } - @media (max-width: $screen-sm-max) { + @media (max-width: $screen-xs-max) { .dropdown-menu { min-width: 100%; } @@ -169,6 +176,8 @@ .draggable-handler { display: inline-block; + vertical-align: top; + margin: 5px 0; opacity: 0; transition: opacity .3s; color: $gray-darkest; @@ -198,7 +207,7 @@ .toggle-priority { display: inline-block; - vertical-align: middle; + vertical-align: top; button { border-color: transparent; @@ -265,6 +274,11 @@ } .label-subscribe-button { + @media(min-width: $screen-md-min) { + min-width: 105px; + margin-left: $gl-padding; + } + .label-subscribe-button-icon { &[disabled] { opacity: 0.5; diff --git a/app/controllers/groups/labels_controller.rb b/app/controllers/groups/labels_controller.rb index dda59262483..f3a9e591c3e 100644 --- a/app/controllers/groups/labels_controller.rb +++ b/app/controllers/groups/labels_controller.rb @@ -54,7 +54,7 @@ class Groups::LabelsController < Groups::ApplicationController respond_to do |format| format.html do - redirect_to group_labels_path(@group), status: 302, notice: 'Label was removed' + redirect_to group_labels_path(@group), status: 302, notice: "#{@label.name} deleted permanently" end format.js end diff --git a/app/views/shared/_label_row.html.haml b/app/views/shared/_label_row.html.haml index 34da13ca52b..bd4f191502e 100644 --- a/app/views/shared/_label_row.html.haml +++ b/app/views/shared/_label_row.html.haml @@ -17,14 +17,14 @@ - if defined?(@project) && @project.group.present? %span.label-type = label.model_name.human.titleize - - if label.description - %span.label-description - = markdown_field(label, :description) - .hidden-xs.hidden-sm - - if show_label_issues_link - = link_to_label(label, subject: subject) do - Issues - - if show_label_merge_requests_link - · - = link_to_label(label, subject: subject, type: :merge_request) do - Merge requests + + %span.label-description + - if label.description.present? + .description-text + = markdown_field(label, :description) + .hidden-xs.hidden-sm + - if show_label_issues_link + = link_to_label(label, subject: subject) { 'Issues' } + - if show_label_merge_requests_link + · + = link_to_label(label, subject: subject, type: :merge_request) { 'Merge requests' } -- cgit v1.2.1 From 0ef1a582b625f7f6f52e377fc62dc526da34cfb6 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Gray Date: Fri, 26 Jan 2018 15:42:42 -0700 Subject: Add modal for label deletion --- app/assets/stylesheets/framework/modal.scss | 4 ++++ app/views/shared/_delete_label_modal.html.haml | 20 ++++++++++++++++++++ app/views/shared/_label.html.haml | 9 ++++++--- 3 files changed, 30 insertions(+), 3 deletions(-) create mode 100644 app/views/shared/_delete_label_modal.html.haml diff --git a/app/assets/stylesheets/framework/modal.scss b/app/assets/stylesheets/framework/modal.scss index 32b9894ae04..5438a4fa207 100644 --- a/app/assets/stylesheets/framework/modal.scss +++ b/app/assets/stylesheets/framework/modal.scss @@ -4,6 +4,10 @@ .page-title { margin-top: 0; + + .label { + font-size: initial; + } } } diff --git a/app/views/shared/_delete_label_modal.html.haml b/app/views/shared/_delete_label_modal.html.haml new file mode 100644 index 00000000000..f29c8f00d52 --- /dev/null +++ b/app/views/shared/_delete_label_modal.html.haml @@ -0,0 +1,20 @@ +.modal{ id: "modal-delete-label-#{label.id}", tabindex: -1 } + .modal-dialog + .modal-content + .modal-header + %button.close{ data: {dismiss: 'modal' } } × + %h3.page-title Delete #{render_colored_label(label, tooltip: false)} ? + + .modal-body + %p + %strong #{label.name} + will be permanently deleted from #{label.group.name}. This cannot be undone. + + .modal-footer + %a{ href: '#', data: { dismiss: 'modal' }, class: 'btn btn-default' } Cancel + + = link_to 'Delete label', + destroy_label_path(label), + title: 'Delete', + method: :delete, + class: 'btn btn-remove' diff --git a/app/views/shared/_label.html.haml b/app/views/shared/_label.html.haml index 6a103f56706..dd9b4b4db44 100644 --- a/app/views/shared/_label.html.haml +++ b/app/views/shared/_label.html.haml @@ -55,9 +55,10 @@ = link_to edit_label_path(label), title: "Edit", class: 'btn btn-transparent btn-action', data: {toggle: "tooltip"} do %span.sr-only Edit = sprite_icon('pencil') - = link_to destroy_label_path(label), title: "Delete", class: 'btn btn-transparent btn-action remove-row', method: :delete, data: {confirm: label_deletion_confirm_text(label), toggle: "tooltip"} do - %span.sr-only Delete - = sprite_icon('remove') + %span{ data: { toggle: 'modal', target: "#modal-delete-label-#{label.id}" } } + = link_to "#", title: "Delete", class: 'btn btn-transparent btn-action remove-row', data: { toggle: "tooltip" } do + %span.sr-only Delete + = sprite_icon('remove') - if current_user .label-subscription.inline - if can_subscribe_to_label_in_different_levels?(label) @@ -79,3 +80,5 @@ %button.js-subscribe-button.label-subscribe-button.btn.btn-default{ type: 'button', data: { status: status, url: toggle_subscription_path } } %span= label_subscription_toggle_button_text(label, @project) = icon('spinner spin', class: 'label-subscribe-button-loading') + += render 'shared/delete_label_modal', label: label -- cgit v1.2.1 From 3be6f68a33d163922a78c51928980efc57a3efb5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Mon, 29 Jan 2018 23:00:39 +0100 Subject: Make Ci::Runner#online? slightly more performant This is a small refactor to avoid querying Redis when we know there's nothing in it. --- app/models/ci/runner.rb | 2 +- spec/models/ci/runner_spec.rb | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 7cf36c0bfe0..f6d51fabd69 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -97,7 +97,7 @@ module Ci end def online? - cached_contacted_at && cached_contacted_at > self.class.contact_time_deadline + contacted_at && cached_contacted_at > self.class.contact_time_deadline end def status diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 14747a23c82..99b4a82da88 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -128,6 +128,7 @@ describe Ci::Runner do context 'with cache value' do context 'contacted long time ago time' do before do + runner.contacted_at = 1.year.ago stub_redis_runner_contacted_at(1.year.ago.to_s) end @@ -136,6 +137,7 @@ describe Ci::Runner do context 'contacted 1s ago' do before do + runner.contacted_at = 50.minutes.ago stub_redis_runner_contacted_at(1.second.ago.to_s) end -- cgit v1.2.1 From 63ecb57f1b956bb7901e27f26f3bc2f3f62bcaa8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Mon, 29 Jan 2018 23:25:56 +0100 Subject: Handle updating only contacted_at runner cache --- app/models/ci/runner.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index f6d51fabd69..fb766cb3793 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -193,7 +193,7 @@ module Ci Gitlab::Redis::SharedState.with do |redis| redis.set("#{runner_info_redis_cache_key}:contacted_at", Time.now) - params.each do |key, value| + params && params.each do |key, value| redis_key = "#{runner_info_redis_cache_key}:#{key}" redis.set(redis_key, value) end -- cgit v1.2.1 From 126b6bbc7fdeb1afd5b6d29c86051c48a09c4857 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Mon, 29 Jan 2018 23:27:03 +0100 Subject: Use faster model updates in #update_runner_info spec --- spec/models/ci/runner_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 99b4a82da88..ab931e5d43c 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -400,7 +400,7 @@ describe Ci::Runner do context 'when database was updated recently' do before do - runner.update(contacted_at: Time.now) + runner.contacted_at = Time.now end it 'updates cache' do @@ -412,7 +412,7 @@ describe Ci::Runner do context 'when database was not updated recently' do before do - runner.update(contacted_at: 2.hours.ago) + runner.contacted_at = 2.hours.ago end it 'updates database' do -- cgit v1.2.1 From 28fd49c1d23f18692b74e4f4e1f21c24c45da3e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Mon, 29 Jan 2018 23:31:34 +0100 Subject: Fix Redis leakage in Runner API specs --- spec/requests/api/runner_spec.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index cb66d23b77c..25d1ac73b9e 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -8,6 +8,7 @@ describe API::Runner do before do stub_gitlab_calls stub_application_setting(runners_registration_token: registration_token) + allow_any_instance_of(Ci::Runner).to receive(:update_runner_info_cache) end describe '/api/v4/runners' do -- cgit v1.2.1 From 977996d2b396c1892a0bcdb386d63e4f78675239 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Tue, 30 Jan 2018 00:05:48 +0100 Subject: Add check for guest user --- app/helpers/callouts_helper.rb | 2 +- spec/helpers/callouts_helper_spec.rb | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/app/helpers/callouts_helper.rb b/app/helpers/callouts_helper.rb index e435879ee43..a27533f30c6 100644 --- a/app/helpers/callouts_helper.rb +++ b/app/helpers/callouts_helper.rb @@ -1,6 +1,6 @@ module CalloutsHelper def show_gke_cluster_integration_callout?(kube_feature_name, project) - !user_dismissed?(kube_feature_name) && project.team.master?(current_user) + current_user && !user_dismissed?(kube_feature_name) && project.team.master?(current_user) end private diff --git a/spec/helpers/callouts_helper_spec.rb b/spec/helpers/callouts_helper_spec.rb index feabbf33ef6..f160aafbd7b 100644 --- a/spec/helpers/callouts_helper_spec.rb +++ b/spec/helpers/callouts_helper_spec.rb @@ -41,5 +41,13 @@ describe CalloutsHelper do it { is_expected.to be false } end + + context 'when the user is not logged in' do + before do + allow(helper).to receive(:current_user).and_return(nil) + end + + it { is_expected.to be_falsey } + end end end -- cgit v1.2.1 From 2cd71eb5f5fc4ae2b1e6385977ec7ef298dc77db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Tue, 30 Jan 2018 00:08:27 +0100 Subject: Add safe navigation for users without callout state --- app/helpers/callouts_helper.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/helpers/callouts_helper.rb b/app/helpers/callouts_helper.rb index a27533f30c6..199652b1175 100644 --- a/app/helpers/callouts_helper.rb +++ b/app/helpers/callouts_helper.rb @@ -6,6 +6,6 @@ module CalloutsHelper private def user_dismissed?(feature_name) - Callout.find_by(user: current_user, feature_name: feature_name).dismissed_state? + Callout.find_by(user: current_user, feature_name: feature_name)&.dismissed_state? end end -- cgit v1.2.1 From 983d03313517a948ceb41cd94ffedf4c8d9d3957 Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Tue, 30 Jan 2018 11:28:20 +0100 Subject: fix service generic tests --- app/models/project_services/prometheus_service.rb | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/app/models/project_services/prometheus_service.rb b/app/models/project_services/prometheus_service.rb index e540a2c646a..676cc32b2d9 100644 --- a/app/models/project_services/prometheus_service.rb +++ b/app/models/project_services/prometheus_service.rb @@ -1,7 +1,3 @@ -module Gitlab - PrometheusError = Class.new(StandardError) -end - class PrometheusService < MonitoringService include ReactiveService @@ -47,6 +43,7 @@ class PrometheusService < MonitoringService [ { type: 'fieldset', + name: 'manual_configuration', legend: 'Manual Configuration', fields: [ { -- cgit v1.2.1 From 89eb6222738f215e81df1c54f82e44050c2b0fe4 Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Tue, 30 Jan 2018 11:32:47 +0100 Subject: enable manual configuration property for all test prometheus services --- spec/factories/projects.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/spec/factories/projects.rb b/spec/factories/projects.rb index d0f3911f730..1dec4eb6c04 100644 --- a/spec/factories/projects.rb +++ b/spec/factories/projects.rb @@ -243,7 +243,8 @@ FactoryBot.define do project.create_prometheus_service( active: true, properties: { - api_url: 'https://prometheus.example.com' + api_url: 'https://prometheus.example.com', + manual_configuration: true } ) end -- cgit v1.2.1 From 529edc9e85493eb1a59b243d4a735f1ce363c13b Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Tue, 30 Jan 2018 11:35:30 +0100 Subject: Fix rubocop --- app/models/project_services/prometheus_service.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/models/project_services/prometheus_service.rb b/app/models/project_services/prometheus_service.rb index 676cc32b2d9..ae68fd9c3da 100644 --- a/app/models/project_services/prometheus_service.rb +++ b/app/models/project_services/prometheus_service.rb @@ -127,6 +127,7 @@ class PrometheusService < MonitoringService def prometheus_installed? return false if template? + project.clusters.enabled.any? { |cluster| cluster.application_prometheus&.installed? } end -- cgit v1.2.1 From f2253d48a109a12f00b87f5aaaa42299860315f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Tue, 30 Jan 2018 15:17:15 +0100 Subject: Show GKE cluster callout for project owner as well --- app/helpers/callouts_helper.rb | 4 +++- spec/helpers/callouts_helper_spec.rb | 16 +++++++++++++--- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/app/helpers/callouts_helper.rb b/app/helpers/callouts_helper.rb index 199652b1175..e65daa572a8 100644 --- a/app/helpers/callouts_helper.rb +++ b/app/helpers/callouts_helper.rb @@ -1,6 +1,8 @@ module CalloutsHelper def show_gke_cluster_integration_callout?(kube_feature_name, project) - current_user && !user_dismissed?(kube_feature_name) && project.team.master?(current_user) + current_user && !user_dismissed?(kube_feature_name) && + (project.team.master?(current_user) || + current_user == project.owner) end private diff --git a/spec/helpers/callouts_helper_spec.rb b/spec/helpers/callouts_helper_spec.rb index f160aafbd7b..8dd97e22477 100644 --- a/spec/helpers/callouts_helper_spec.rb +++ b/spec/helpers/callouts_helper_spec.rb @@ -26,11 +26,21 @@ describe CalloutsHelper do end context 'when user is not master' do - before do - allow(project).to receive_message_chain(:team, :master?).and_return(false) + context 'when the user is owner' do + before do + allow(project).to receive(:owner).and_return(user) + end + + it { is_expected.to be true } end - it { is_expected.to be false } + context 'when the user is not owner' do + before do + allow(project).to receive_message_chain(:team, :master?).and_return(false) + end + + it { is_expected.to be false } + end end end -- cgit v1.2.1 From 4157cad23b5215c5b878f120099b4632ed04ce04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Tue, 30 Jan 2018 15:30:49 +0100 Subject: Extract feature name into constant --- app/helpers/callouts_helper.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/app/helpers/callouts_helper.rb b/app/helpers/callouts_helper.rb index e65daa572a8..153797450de 100644 --- a/app/helpers/callouts_helper.rb +++ b/app/helpers/callouts_helper.rb @@ -1,6 +1,8 @@ module CalloutsHelper - def show_gke_cluster_integration_callout?(kube_feature_name, project) - current_user && !user_dismissed?(kube_feature_name) && + GKE_CLUSTER_INTEGRATION = 'gke_cluster_integration'.freeze + + def show_gke_cluster_integration_callout?(project) + current_user && !user_dismissed?(GKE_CLUSTER_INTEGRATION) && (project.team.master?(current_user) || current_user == project.owner) end -- cgit v1.2.1 From 7c659e5b533d4b2ee73a3ff0a013cac9366f6ace Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Tue, 30 Jan 2018 14:53:28 +0000 Subject: Converted issue.js to axios --- app/assets/javascripts/issue.js | 40 +++++++++----------- spec/javascripts/issue_spec.js | 82 +++++++++++++++++++++++++++++------------ 2 files changed, 76 insertions(+), 46 deletions(-) diff --git a/app/assets/javascripts/issue.js b/app/assets/javascripts/issue.js index 411c820cc43..ff65ea99e9a 100644 --- a/app/assets/javascripts/issue.js +++ b/app/assets/javascripts/issue.js @@ -1,7 +1,8 @@ /* eslint-disable func-names, space-before-function-paren, no-var, prefer-rest-params, wrap-iife, one-var, no-underscore-dangle, one-var-declaration-per-line, object-shorthand, no-unused-vars, no-new, comma-dangle, consistent-return, quotes, dot-notation, quote-props, prefer-arrow-callback, max-len */ import 'vendor/jquery.waitforimages'; +import axios from './lib/utils/axios_utils'; import { addDelimiter } from './lib/utils/text_utility'; -import Flash from './flash'; +import flash from './flash'; import TaskList from './task_list'; import CreateMergeRequestDropdown from './create_merge_request_dropdown'; import IssuablesHelper from './helpers/issuables_helper'; @@ -42,12 +43,8 @@ export default class Issue { this.disableCloseReopenButton($button); url = $button.attr('href'); - return $.ajax({ - type: 'PUT', - url: url - }) - .fail(() => new Flash(issueFailMessage)) - .done((data) => { + return axios.put(url) + .then(({ data }) => { const isClosedBadge = $('div.status-box-issue-closed'); const isOpenBadge = $('div.status-box-open'); const projectIssuesCounter = $('.issue_counter'); @@ -74,9 +71,10 @@ export default class Issue { } } } else { - new Flash(issueFailMessage); + flash(issueFailMessage); } }) + .catch(() => flash(issueFailMessage)) .then(() => { this.disableCloseReopenButton($button, false); }); @@ -115,24 +113,22 @@ export default class Issue { static initMergeRequests() { var $container; $container = $('#merge-requests'); - return $.getJSON($container.data('url')).fail(function() { - return new Flash('Failed to load referenced merge requests'); - }).done(function(data) { - if ('html' in data) { - return $container.html(data.html); - } - }); + return axios.get($container.data('url')) + .then(({ data }) => { + if ('html' in data) { + $container.html(data.html); + } + }).catch(() => flash('Failed to load referenced merge requests')); } static initRelatedBranches() { var $container; $container = $('#related-branches'); - return $.getJSON($container.data('url')).fail(function() { - return new Flash('Failed to load related branches'); - }).done(function(data) { - if ('html' in data) { - return $container.html(data.html); - } - }); + return axios.get($container.data('url')) + .then(({ data }) => { + if ('html' in data) { + $container.html(data.html); + } + }).catch(() => flash('Failed to load related branches')); } } diff --git a/spec/javascripts/issue_spec.js b/spec/javascripts/issue_spec.js index 2cd2e63b15d..2da7cede510 100644 --- a/spec/javascripts/issue_spec.js +++ b/spec/javascripts/issue_spec.js @@ -1,4 +1,6 @@ /* eslint-disable space-before-function-paren, one-var, one-var-declaration-per-line, no-use-before-define, comma-dangle, max-len */ +import MockAdaptor from 'axios-mock-adapter'; +import axios from '~/lib/utils/axios_utils'; import Issue from '~/issue'; import '~/lib/utils/text_utility'; @@ -88,6 +90,7 @@ describe('Issue', function() { [true, false].forEach((isIssueInitiallyOpen) => { describe(`with ${isIssueInitiallyOpen ? 'open' : 'closed'} issue`, function() { const action = isIssueInitiallyOpen ? 'close' : 'reopen'; + let mock; function ajaxSpy(req) { if (req.url === this.$triggeredButton.attr('href')) { @@ -104,6 +107,18 @@ describe('Issue', function() { return null; } + function mockCloseButtonResponseSuccess(url, response) { + mock.onPut(url).reply(() => { + expectNewBranchButtonState(true, false); + + return [200, response]; + }); + } + + function mockCloseButtonResponseError(url) { + mock.onPut(url).networkError(); + } + beforeEach(function() { if (isIssueInitiallyOpen) { loadFixtures('issues/open-issue.html.raw'); @@ -123,68 +138,87 @@ describe('Issue', function() { this.issueStateDeferred = new jQuery.Deferred(); this.canCreateBranchDeferred = new jQuery.Deferred(); + mock = new MockAdaptor(axios); + spyOn(jQuery, 'ajax').and.callFake(ajaxSpy.bind(this)); }); - it(`${action}s the issue`, function() { - this.$triggeredButton.trigger('click'); - this.issueStateDeferred.resolve({ + afterEach(() => { + mock.restore(); + }); + + it(`${action}s the issue`, function(done) { + mockCloseButtonResponseSuccess(this.$triggeredButton.attr('href'), { id: 34 }); + + this.$triggeredButton.trigger('click'); this.canCreateBranchDeferred.resolve({ can_create_branch: !isIssueInitiallyOpen }); - expectIssueState(!isIssueInitiallyOpen); - expect(this.$triggeredButton.get(0).getAttribute('disabled')).toBeNull(); - expect(this.$projectIssuesCounter.text()).toBe(isIssueInitiallyOpen ? '1,000' : '1,002'); - expectNewBranchButtonState(false, !isIssueInitiallyOpen); + setTimeout(() => { + expectIssueState(!isIssueInitiallyOpen); + expect(this.$triggeredButton.get(0).getAttribute('disabled')).toBeNull(); + expect(this.$projectIssuesCounter.text()).toBe(isIssueInitiallyOpen ? '1,000' : '1,002'); + expectNewBranchButtonState(false, !isIssueInitiallyOpen); + + done(); + }); }); - it(`fails to ${action} the issue if saved:false`, function() { - this.$triggeredButton.trigger('click'); - this.issueStateDeferred.resolve({ + it(`fails to ${action} the issue if saved:false`, function(done) { + mockCloseButtonResponseSuccess(this.$triggeredButton.attr('href'), { saved: false }); + this.$triggeredButton.trigger('click'); this.canCreateBranchDeferred.resolve({ can_create_branch: isIssueInitiallyOpen }); - expectIssueState(isIssueInitiallyOpen); - expect(this.$triggeredButton.get(0).getAttribute('disabled')).toBeNull(); - expectErrorMessage(); - expect(this.$projectIssuesCounter.text()).toBe('1,001'); - expectNewBranchButtonState(false, isIssueInitiallyOpen); + setTimeout(() => { + expectIssueState(isIssueInitiallyOpen); + expect(this.$triggeredButton.get(0).getAttribute('disabled')).toBeNull(); + expectErrorMessage(); + expect(this.$projectIssuesCounter.text()).toBe('1,001'); + expectNewBranchButtonState(false, isIssueInitiallyOpen); + + done(); + }); }); - it(`fails to ${action} the issue if HTTP error occurs`, function() { + it(`fails to ${action} the issue if HTTP error occurs`, function(done) { + mockCloseButtonResponseError(this.$triggeredButton.attr('href')); this.$triggeredButton.trigger('click'); - this.issueStateDeferred.reject(); this.canCreateBranchDeferred.resolve({ can_create_branch: isIssueInitiallyOpen }); - expectIssueState(isIssueInitiallyOpen); - expect(this.$triggeredButton.get(0).getAttribute('disabled')).toBeNull(); - expectErrorMessage(); - expect(this.$projectIssuesCounter.text()).toBe('1,001'); - expectNewBranchButtonState(false, isIssueInitiallyOpen); + setTimeout(() => { + expectIssueState(isIssueInitiallyOpen); + expect(this.$triggeredButton.get(0).getAttribute('disabled')).toBeNull(); + expectErrorMessage(); + expect(this.$projectIssuesCounter.text()).toBe('1,001'); + expectNewBranchButtonState(false, isIssueInitiallyOpen); + + done(); + }); }); it('disables the new branch button if Ajax call fails', function() { + mockCloseButtonResponseError(this.$triggeredButton.attr('href')); this.$triggeredButton.trigger('click'); - this.issueStateDeferred.reject(); this.canCreateBranchDeferred.reject(); expectNewBranchButtonState(false, false); }); it('does not trigger Ajax call if new branch button is missing', function() { + mockCloseButtonResponseError(this.$triggeredButton.attr('href')); Issue.$btnNewBranch = $(); this.canCreateBranchDeferred = null; this.$triggeredButton.trigger('click'); - this.issueStateDeferred.reject(); }); }); }); -- cgit v1.2.1 From 4cfe66ae5f2c1f83944999c640c175085008105b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Tue, 30 Jan 2018 16:59:10 +0100 Subject: Fix CalloutsHelper spec subject --- spec/helpers/callouts_helper_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/helpers/callouts_helper_spec.rb b/spec/helpers/callouts_helper_spec.rb index 8dd97e22477..7ec8ca8eb0c 100644 --- a/spec/helpers/callouts_helper_spec.rb +++ b/spec/helpers/callouts_helper_spec.rb @@ -10,7 +10,7 @@ describe CalloutsHelper do describe '.show_gke_cluster_integration_callout?' do let(:project) { create(:project) } - subject { helper.show_gke_cluster_integration_callout?('test_name', project) } + subject { helper.show_gke_cluster_integration_callout?(project) } context 'when user has not dismissed' do before do -- cgit v1.2.1 From d504118402668872d4acc67c10a40cd03d8f8e99 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Tue, 30 Jan 2018 16:33:23 +0000 Subject: Converted job.js to axios --- app/assets/javascripts/job.js | 12 +- spec/javascripts/job_spec.js | 263 ++++++++++++++++++++++-------------------- 2 files changed, 144 insertions(+), 131 deletions(-) diff --git a/app/assets/javascripts/job.js b/app/assets/javascripts/job.js index 9b5092c5e3f..d2cbc9b82b3 100644 --- a/app/assets/javascripts/job.js +++ b/app/assets/javascripts/job.js @@ -1,4 +1,5 @@ import _ from 'underscore'; +import axios from './lib/utils/axios_utils'; import { visitUrl } from './lib/utils/url_utility'; import bp from './breakpoints'; import { numberToHumanSize } from './lib/utils/number_utils'; @@ -171,11 +172,12 @@ export default class Job { } getBuildTrace() { - return $.ajax({ - url: `${this.pagePath}/trace.json`, - data: { state: this.state }, + return axios.get(`${this.pagePath}/trace.json`, { + params: { state: this.state }, }) - .done((log) => { + .then((res) => { + const log = res.data; + setCiStatusFavicon(`${this.pagePath}/status.json`); if (log.state) { @@ -217,7 +219,7 @@ export default class Job { visitUrl(this.pagePath); } }) - .fail(() => { + .catch(() => { this.$buildRefreshAnimation.remove(); }) .then(() => { diff --git a/spec/javascripts/job_spec.js b/spec/javascripts/job_spec.js index feb341d22e6..c3d8d821ac4 100644 --- a/spec/javascripts/job_spec.js +++ b/spec/javascripts/job_spec.js @@ -1,3 +1,5 @@ +import MockAdaptor from 'axios-mock-adapter'; +import axios from '~/lib/utils/axios_utils'; import { numberToHumanSize } from '~/lib/utils/number_utils'; import * as urlUtils from '~/lib/utils/url_utility'; import '~/lib/utils/datetime_utility'; @@ -6,11 +8,31 @@ import '~/breakpoints'; describe('Job', () => { const JOB_URL = `${gl.TEST_HOST}/frontend-fixtures/builds-project/-/jobs/1`; + let mock; + let response; + + function waitForPromise() { + return new Promise(resolve => requestAnimationFrame(resolve)); + } preloadFixtures('builds/build-with-artifacts.html.raw'); beforeEach(() => { loadFixtures('builds/build-with-artifacts.html.raw'); + + spyOn(urlUtils, 'visitUrl'); + + mock = new MockAdaptor(axios); + + mock.onGet(new RegExp(`${JOB_URL}/trace.json?(.*)`)).reply(() => { + return [200, response]; + }); + }); + + afterEach(() => { + mock.restore(); + + response = {}; }); describe('class constructor', () => { @@ -55,170 +77,159 @@ describe('Job', () => { }); describe('running build', () => { - it('updates the build trace on an interval', function () { - const deferred1 = $.Deferred(); - const deferred2 = $.Deferred(); - const deferred3 = $.Deferred(); - spyOn($, 'ajax').and.returnValues(deferred1.promise(), deferred2.promise(), deferred3.promise()); - spyOn(urlUtils, 'visitUrl'); - - deferred1.resolve({ + it('updates the build trace on an interval', function (done) { + response = { html: 'Update', status: 'running', state: 'newstate', append: true, complete: false, - }); - - deferred2.resolve(); - - deferred3.resolve({ - html: 'More', - status: 'running', - state: 'finalstate', - append: true, - complete: true, - }); + }; this.job = new Job(); - expect($('#build-trace .js-build-output').text()).toMatch(/Update/); - expect(this.job.state).toBe('newstate'); - - jasmine.clock().tick(4001); - - expect($('#build-trace .js-build-output').text()).toMatch(/UpdateMore/); - expect(this.job.state).toBe('finalstate'); + waitForPromise() + .then(() => { + expect($('#build-trace .js-build-output').text()).toMatch(/Update/); + expect(this.job.state).toBe('newstate'); + + response = { + html: 'More', + status: 'running', + state: 'finalstate', + append: true, + complete: true, + }; + }) + .then(() => jasmine.clock().tick(4001)) + .then(waitForPromise) + .then(() => { + expect($('#build-trace .js-build-output').text()).toMatch(/UpdateMore/); + expect(this.job.state).toBe('finalstate'); + }) + .then(done) + .catch(done.fail); }); - it('replaces the entire build trace', () => { - const deferred1 = $.Deferred(); - const deferred2 = $.Deferred(); - const deferred3 = $.Deferred(); - - spyOn($, 'ajax').and.returnValues(deferred1.promise(), deferred2.promise(), deferred3.promise()); - - spyOn(urlUtils, 'visitUrl'); - - deferred1.resolve({ + it('replaces the entire build trace', (done) => { + response = { html: 'Update', status: 'running', append: false, complete: false, - }); - - deferred2.resolve(); - - deferred3.resolve({ - html: 'Different', - status: 'running', - append: false, - }); + }; this.job = new Job(); - expect($('#build-trace .js-build-output').text()).toMatch(/Update/); - - jasmine.clock().tick(4001); - - expect($('#build-trace .js-build-output').text()).not.toMatch(/Update/); - expect($('#build-trace .js-build-output').text()).toMatch(/Different/); + waitForPromise() + .then(() => { + expect($('#build-trace .js-build-output').text()).toMatch(/Update/); + + response = { + html: 'Different', + status: 'running', + append: false, + }; + }) + .then(() => jasmine.clock().tick(4001)) + .then(waitForPromise) + .then(() => { + expect($('#build-trace .js-build-output').text()).not.toMatch(/Update/); + expect($('#build-trace .js-build-output').text()).toMatch(/Different/); + }) + .then(done) + .catch(done.fail); }); }); describe('truncated information', () => { describe('when size is less than total', () => { - it('shows information about truncated log', () => { - spyOn(urlUtils, 'visitUrl'); - const deferred = $.Deferred(); - spyOn($, 'ajax').and.returnValue(deferred.promise()); - - deferred.resolve({ + it('shows information about truncated log', (done) => { + response = { html: 'Update', status: 'success', append: false, size: 50, total: 100, - }); + }; this.job = new Job(); - expect(document.querySelector('.js-truncated-info').classList).not.toContain('hidden'); + waitForPromise() + .then(() => { + expect(document.querySelector('.js-truncated-info').classList).not.toContain('hidden'); + }) + .then(done) + .catch(done.fail); }); - it('shows the size in KiB', () => { + it('shows the size in KiB', (done) => { const size = 50; - spyOn(urlUtils, 'visitUrl'); - const deferred = $.Deferred(); - spyOn($, 'ajax').and.returnValue(deferred.promise()); - deferred.resolve({ + response = { html: 'Update', status: 'success', append: false, size, total: 100, - }); + }; this.job = new Job(); - expect( - document.querySelector('.js-truncated-info-size').textContent.trim(), - ).toEqual(`${numberToHumanSize(size)}`); + waitForPromise() + .then(() => { + expect( + document.querySelector('.js-truncated-info-size').textContent.trim(), + ).toEqual(`${numberToHumanSize(size)}`); + }) + .then(done) + .catch(done.fail); }); - it('shows incremented size', () => { - const deferred1 = $.Deferred(); - const deferred2 = $.Deferred(); - const deferred3 = $.Deferred(); - - spyOn($, 'ajax').and.returnValues(deferred1.promise(), deferred2.promise(), deferred3.promise()); - - spyOn(urlUtils, 'visitUrl'); - - deferred1.resolve({ + it('shows incremented size', (done) => { + response = { html: 'Update', status: 'success', append: false, size: 50, total: 100, - }); - - deferred2.resolve(); + }; this.job = new Job(); - expect( - document.querySelector('.js-truncated-info-size').textContent.trim(), - ).toEqual(`${numberToHumanSize(50)}`); - - jasmine.clock().tick(4001); - - deferred3.resolve({ - html: 'Update', - status: 'success', - append: true, - size: 10, - total: 100, - }); - - expect( - document.querySelector('.js-truncated-info-size').textContent.trim(), - ).toEqual(`${numberToHumanSize(60)}`); + waitForPromise() + .then(() => { + expect( + document.querySelector('.js-truncated-info-size').textContent.trim(), + ).toEqual(`${numberToHumanSize(50)}`); + + response = { + html: 'Update', + status: 'success', + append: true, + size: 10, + total: 100, + }; + }) + .then(() => jasmine.clock().tick(4001)) + .then(waitForPromise) + .then(() => { + expect( + document.querySelector('.js-truncated-info-size').textContent.trim(), + ).toEqual(`${numberToHumanSize(60)}`); + }) + .then(done) + .catch(done.fail); }); it('renders the raw link', () => { - const deferred = $.Deferred(); - spyOn(urlUtils, 'visitUrl'); - - spyOn($, 'ajax').and.returnValue(deferred.promise()); - deferred.resolve({ + response = { html: 'Update', status: 'success', append: false, size: 50, total: 100, - }); + }; this.job = new Job(); @@ -229,50 +240,50 @@ describe('Job', () => { }); describe('when size is equal than total', () => { - it('does not show the trunctated information', () => { - const deferred = $.Deferred(); - spyOn(urlUtils, 'visitUrl'); - - spyOn($, 'ajax').and.returnValue(deferred.promise()); - deferred.resolve({ + it('does not show the trunctated information', (done) => { + response = { html: 'Update', status: 'success', append: false, size: 100, total: 100, - }); + }; this.job = new Job(); - expect(document.querySelector('.js-truncated-info').classList).toContain('hidden'); + waitForPromise() + .then(() => { + expect(document.querySelector('.js-truncated-info').classList).toContain('hidden'); + }) + .then(done) + .catch(done.fail); }); }); }); describe('output trace', () => { - beforeEach(() => { - const deferred = $.Deferred(); - spyOn(urlUtils, 'visitUrl'); - - spyOn($, 'ajax').and.returnValue(deferred.promise()); - deferred.resolve({ + beforeEach((done) => { + response = { html: 'Update', status: 'success', append: false, size: 50, total: 100, - }); + }; this.job = new Job(); + + waitForPromise() + .then(done) + .catch(done.fail); }); it('should render trace controls', () => { const controllers = document.querySelector('.controllers'); - expect(controllers.querySelector('.js-raw-link-controller')).toBeDefined(); - expect(controllers.querySelector('.js-erase-link')).toBeDefined(); - expect(controllers.querySelector('.js-scroll-up')).toBeDefined(); - expect(controllers.querySelector('.js-scroll-down')).toBeDefined(); + expect(controllers.querySelector('.js-raw-link-controller')).not.toBeNull(); + expect(controllers.querySelector('.js-scroll-up')).not.toBeNull(); + expect(controllers.querySelector('.js-scroll-down')).not.toBeNull(); }); it('should render received output', () => { @@ -285,13 +296,13 @@ describe('Job', () => { describe('getBuildTrace', () => { it('should request build trace with state parameter', (done) => { - spyOn(jQuery, 'ajax').and.callThrough(); + spyOn(axios, 'get').and.callThrough(); // eslint-disable-next-line no-new new Job(); setTimeout(() => { - expect(jQuery.ajax).toHaveBeenCalledWith( - { url: `${JOB_URL}/trace.json`, data: { state: '' } }, + expect(axios.get).toHaveBeenCalledWith( + `${JOB_URL}/trace.json`, { params: { state: '' } }, ); done(); }, 0); -- cgit v1.2.1 From 05a3479c9efe81c1ced7bba37ec4da37e4a9fcd6 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Tue, 30 Jan 2018 16:40:47 +0000 Subject: Converted labels_select.js to axios --- app/assets/javascripts/labels_select.js | 170 ++++++++++++++++---------------- 1 file changed, 85 insertions(+), 85 deletions(-) diff --git a/app/assets/javascripts/labels_select.js b/app/assets/javascripts/labels_select.js index 664e793fc8e..06b8333db27 100644 --- a/app/assets/javascripts/labels_select.js +++ b/app/assets/javascripts/labels_select.js @@ -2,9 +2,12 @@ /* global Issuable */ /* global ListLabel */ import _ from 'underscore'; +import { __ } from './locale'; +import axios from './lib/utils/axios_utils'; import IssuableBulkUpdateActions from './issuable_bulk_update_actions'; import DropdownUtils from './filtered_search/dropdown_utils'; import CreateLabelDropdown from './create_label'; +import flash from './flash'; export default class LabelsSelect { constructor(els, options = {}) { @@ -82,99 +85,96 @@ export default class LabelsSelect { } $loading.removeClass('hidden').fadeIn(); $dropdown.trigger('loading.gl.dropdown'); - return $.ajax({ - type: 'PUT', - url: issueUpdateURL, - dataType: 'JSON', - data: data - }).done(function(data) { - var labelCount, template, labelTooltipTitle, labelTitles; - $loading.fadeOut(); - $dropdown.trigger('loaded.gl.dropdown'); - $selectbox.hide(); - data.issueURLSplit = issueURLSplit; - labelCount = 0; - if (data.labels.length) { - template = labelHTMLTemplate(data); - labelCount = data.labels.length; - } - else { - template = labelNoneHTMLTemplate; - } - $value.removeAttr('style').html(template); - $sidebarCollapsedValue.text(labelCount); - - if (data.labels.length) { - labelTitles = data.labels.map(function(label) { - return label.title; - }); - - if (labelTitles.length > 5) { - labelTitles = labelTitles.slice(0, 5); - labelTitles.push('and ' + (data.labels.length - 5) + ' more'); + axios.put(issueUpdateURL, data) + .then(({ data }) => { + var labelCount, template, labelTooltipTitle, labelTitles; + $loading.fadeOut(); + $dropdown.trigger('loaded.gl.dropdown'); + $selectbox.hide(); + data.issueURLSplit = issueURLSplit; + labelCount = 0; + if (data.labels.length) { + template = labelHTMLTemplate(data); + labelCount = data.labels.length; } - - labelTooltipTitle = labelTitles.join(', '); - } - else { - labelTooltipTitle = ''; - $sidebarLabelTooltip.tooltip('destroy'); - } - - $sidebarLabelTooltip - .attr('title', labelTooltipTitle) - .tooltip('fixTitle'); - - $('.has-tooltip', $value).tooltip({ - container: 'body' - }); - }); + else { + template = labelNoneHTMLTemplate; + } + $value.removeAttr('style').html(template); + $sidebarCollapsedValue.text(labelCount); + + if (data.labels.length) { + labelTitles = data.labels.map(function(label) { + return label.title; + }); + + if (labelTitles.length > 5) { + labelTitles = labelTitles.slice(0, 5); + labelTitles.push('and ' + (data.labels.length - 5) + ' more'); + } + + labelTooltipTitle = labelTitles.join(', '); + } + else { + labelTooltipTitle = ''; + $sidebarLabelTooltip.tooltip('destroy'); + } + + $sidebarLabelTooltip + .attr('title', labelTooltipTitle) + .tooltip('fixTitle'); + + $('.has-tooltip', $value).tooltip({ + container: 'body' + }); + }) + .catch(() => flash(__('Error saving label update.'))); }; $dropdown.glDropdown({ showMenuAbove: showMenuAbove, data: function(term, callback) { - return $.ajax({ - url: labelUrl - }).done(function(data) { - data = _.chain(data).groupBy(function(label) { - return label.title; - }).map(function(label) { - var color; - color = _.map(label, function(dup) { - return dup.color; - }); - return { - id: label[0].id, - title: label[0].title, - color: color, - duplicate: color.length > 1 - }; - }).value(); - if ($dropdown.hasClass('js-extra-options')) { - var extraData = []; - if (showNo) { - extraData.unshift({ - id: 0, - title: 'No Label' - }); - } - if (showAny) { - extraData.unshift({ - isAny: true, - title: 'Any Label' + axios.get(labelUrl) + .then((res) => { + let data = _.chain(res.data).groupBy(function(label) { + return label.title; + }).map(function(label) { + var color; + color = _.map(label, function(dup) { + return dup.color; }); + return { + id: label[0].id, + title: label[0].title, + color: color, + duplicate: color.length > 1 + }; + }).value(); + if ($dropdown.hasClass('js-extra-options')) { + var extraData = []; + if (showNo) { + extraData.unshift({ + id: 0, + title: 'No Label' + }); + } + if (showAny) { + extraData.unshift({ + isAny: true, + title: 'Any Label' + }); + } + if (extraData.length) { + extraData.push('divider'); + data = extraData.concat(data); + } } - if (extraData.length) { - extraData.push('divider'); - data = extraData.concat(data); + + callback(data); + if (showMenuAbove) { + $dropdown.data('glDropdown').positionMenuAbove(); } - } - - callback(data); - if (showMenuAbove) { - $dropdown.data('glDropdown').positionMenuAbove(); - } - }); + }) + .catch(() => flash(__('Error fetching labels.'))); }, renderRow: function(label, instance) { var $a, $li, color, colorEl, indeterminate, removesAll, selectedClass, spacing, i, marked, dropdownName, dropdownValue; -- cgit v1.2.1 From 8750507b5db62e7821380d075d46d1a1c4eaca9d Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Tue, 30 Jan 2018 16:47:19 +0000 Subject: Convert merge_conflict_service.js to axios --- .../merge_conflicts/merge_conflict_service.js | 14 +++----------- .../merge_conflicts/merge_conflicts_bundle.js | 18 ++++++++---------- 2 files changed, 11 insertions(+), 21 deletions(-) diff --git a/app/assets/javascripts/merge_conflicts/merge_conflict_service.js b/app/assets/javascripts/merge_conflicts/merge_conflict_service.js index c012b77e0bf..c68b47c9348 100644 --- a/app/assets/javascripts/merge_conflicts/merge_conflict_service.js +++ b/app/assets/javascripts/merge_conflicts/merge_conflict_service.js @@ -1,4 +1,5 @@ /* eslint-disable no-param-reassign, comma-dangle */ +import axios from '../lib/utils/axios_utils'; ((global) => { global.mergeConflicts = global.mergeConflicts || {}; @@ -10,20 +11,11 @@ } fetchConflictsData() { - return $.ajax({ - dataType: 'json', - url: this.conflictsPath - }); + return axios.get(this.conflictsPath); } submitResolveConflicts(data) { - return $.ajax({ - url: this.resolveConflictsPath, - data: JSON.stringify(data), - contentType: 'application/json', - dataType: 'json', - method: 'POST' - }); + return axios.post(this.resolveConflictsPath, data); } } diff --git a/app/assets/javascripts/merge_conflicts/merge_conflicts_bundle.js b/app/assets/javascripts/merge_conflicts/merge_conflicts_bundle.js index 792b7523889..bc805047f73 100644 --- a/app/assets/javascripts/merge_conflicts/merge_conflicts_bundle.js +++ b/app/assets/javascripts/merge_conflicts/merge_conflicts_bundle.js @@ -38,24 +38,22 @@ $(() => { showDiffViewTypeSwitcher() { return mergeConflictsStore.fileTextTypePresent(); } }, created() { - mergeConflictsService - .fetchConflictsData() - .done((data) => { + mergeConflictsService.fetchConflictsData() + .then(({ data }) => { if (data.type === 'error') { mergeConflictsStore.setFailedRequest(data.message); } else { mergeConflictsStore.setConflictsData(data); } - }) - .error(() => { - mergeConflictsStore.setFailedRequest(); - }) - .always(() => { + mergeConflictsStore.setLoadingState(false); this.$nextTick(() => { syntaxHighlight($('.js-syntax-highlight')); }); + }) + .catch(() => { + mergeConflictsStore.setFailedRequest(); }); }, methods: { @@ -82,10 +80,10 @@ $(() => { mergeConflictsService .submitResolveConflicts(mergeConflictsStore.getCommitData()) - .done((data) => { + .then(({ data }) => { window.location.href = data.redirect_to; }) - .error(() => { + .catch(() => { mergeConflictsStore.setSubmitState(false); new Flash('Failed to save merge conflicts resolutions. Please try again!'); }); -- cgit v1.2.1 From 58eb3c55c9d15bd604b926ffeae9401f0b70c53c Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Tue, 30 Jan 2018 17:23:27 +0000 Subject: Converted merge_request_tabs.js to axios --- app/assets/javascripts/merge_request_tabs.js | 39 ++++++----- spec/javascripts/merge_request_tabs_spec.js | 96 +++++++++++++++++++--------- 2 files changed, 84 insertions(+), 51 deletions(-) diff --git a/app/assets/javascripts/merge_request_tabs.js b/app/assets/javascripts/merge_request_tabs.js index acfc62fe5cb..f69506a0471 100644 --- a/app/assets/javascripts/merge_request_tabs.js +++ b/app/assets/javascripts/merge_request_tabs.js @@ -1,6 +1,7 @@ /* eslint-disable no-new, class-methods-use-this */ import Cookies from 'js-cookie'; +import axios from './lib/utils/axios_utils'; import Flash from './flash'; import BlobForkSuggestion from './blob/blob_fork_suggestion'; import initChangesDropdown from './init_changes_dropdown'; @@ -244,15 +245,19 @@ export default class MergeRequestTabs { if (this.commitsLoaded) { return; } - this.ajaxGet({ - url: `${source}.json`, - success: (data) => { + + this.toggleLoading(true) + + axios.get(`${source}.json`) + .then(({ data }) => { document.querySelector('div#commits').innerHTML = data.html; localTimeAgo($('.js-timeago', 'div#commits')); this.commitsLoaded = true; this.scrollToElement('#commits'); - }, - }); + + this.toggleLoading(false); + }) + .catch(() => new Flash('An error occurred while fetching this tab.', 'alert')); } mountPipelinesView() { @@ -283,9 +288,10 @@ export default class MergeRequestTabs { // some pages like MergeRequestsController#new has query parameters on that anchor const urlPathname = parseUrlPathname(source); - this.ajaxGet({ - url: `${urlPathname}.json${location.search}`, - success: (data) => { + this.toggleLoading(true); + + axios.get(`${urlPathname}.json${location.search}`) + .then(({ data }) => { const $container = $('#diffs'); $container.html(data.html); @@ -335,8 +341,10 @@ export default class MergeRequestTabs { // (discussion and diff tabs) and `:target` only applies to the first anchor.addClass('target'); } - }, - }); + + this.toggleLoading(false); + }) + .catch(() => Flash('An error occurred while fetching this tab.', 'alert')); } // Show or hide the loading spinner @@ -346,17 +354,6 @@ export default class MergeRequestTabs { $('.mr-loading-status .loading').toggle(status); } - ajaxGet(options) { - const defaults = { - beforeSend: () => this.toggleLoading(true), - error: () => new Flash('An error occurred while fetching this tab.', 'alert'), - complete: () => this.toggleLoading(false), - dataType: 'json', - type: 'GET', - }; - $.ajax($.extend({}, defaults, options)); - } - diffViewType() { return $('.inline-parallel-buttons a.active').data('view-type'); } diff --git a/spec/javascripts/merge_request_tabs_spec.js b/spec/javascripts/merge_request_tabs_spec.js index a6be474805b..116c4e8f0e4 100644 --- a/spec/javascripts/merge_request_tabs_spec.js +++ b/spec/javascripts/merge_request_tabs_spec.js @@ -1,5 +1,6 @@ /* eslint-disable no-var, comma-dangle, object-shorthand */ - +import MockAdaptor from 'axios-mock-adapter'; +import axios from '~/lib/utils/axios_utils'; import * as urlUtils from '~/lib/utils/url_utility'; import MergeRequestTabs from '~/merge_request_tabs'; import '~/commit/pipelines/pipelines_bundle'; @@ -46,7 +47,7 @@ import 'vendor/jquery.scrollTo'; describe('activateTab', function () { beforeEach(function () { - spyOn($, 'ajax').and.callFake(function () {}); + spyOn(axios, 'get').and.returnValue(Promise.resolve({ data: {} })); loadFixtures('merge_requests/merge_request_with_task_list.html.raw'); this.subject = this.class.activateTab; }); @@ -148,7 +149,7 @@ import 'vendor/jquery.scrollTo'; describe('setCurrentAction', function () { beforeEach(function () { - spyOn($, 'ajax').and.callFake(function () {}); + spyOn(axios, 'get').and.returnValue(Promise.resolve({ data: {} })); this.subject = this.class.setCurrentAction; }); @@ -214,13 +215,21 @@ import 'vendor/jquery.scrollTo'; }); describe('tabShown', () => { + let mock; + beforeEach(function () { - spyOn($, 'ajax').and.callFake(function (options) { - options.success({ html: '' }); + mock = new MockAdaptor(axios); + mock.onGet(/(.*)\/diffs\.json/).reply(200, { + data: { html: '' }, }); + loadFixtures('merge_requests/merge_request_with_task_list.html.raw'); }); + afterEach(() => { + mock.restore(); + }); + describe('with "Side-by-side"/parallel diff view', () => { beforeEach(function () { this.class.diffViewType = () => 'parallel'; @@ -292,16 +301,20 @@ import 'vendor/jquery.scrollTo'; it('triggers Ajax request to JSON endpoint', function (done) { const url = '/foo/bar/merge_requests/1/diffs'; - spyOn(this.class, 'ajaxGet').and.callFake((options) => { - expect(options.url).toEqual(`${url}.json`); + + spyOn(axios, 'get').and.callFake((reqUrl) => { + expect(reqUrl).toBe(`${url}.json`); + done(); + + return Promise.resolve({ data: {} }); }); this.class.loadDiff(url); }); it('triggers scroll event when diff already loaded', function (done) { - spyOn(this.class, 'ajaxGet').and.callFake(() => done.fail()); + spyOn(axios, 'get').and.callFake(done.fail); spyOn(document, 'dispatchEvent'); this.class.diffsLoaded = true; @@ -316,6 +329,7 @@ import 'vendor/jquery.scrollTo'; describe('with inline diff', () => { let noteId; let noteLineNumId; + let mock; beforeEach(() => { const diffsResponse = getJSONFixture(inlineChangesTabJsonFixture); @@ -330,29 +344,40 @@ import 'vendor/jquery.scrollTo'; .attr('href') .replace('#', ''); - spyOn($, 'ajax').and.callFake(function (options) { - options.success(diffsResponse); - }); + mock = new MockAdaptor(axios); + mock.onGet(/(.*)\/diffs\.json/).reply(200, diffsResponse); + }); + + afterEach(() => { + mock.restore(); }); describe('with note fragment hash', () => { - it('should expand and scroll to linked fragment hash #note_xxx', function () { + it('should expand and scroll to linked fragment hash #note_xxx', function (done) { spyOn(urlUtils, 'getLocationHash').and.returnValue(noteId); this.class.loadDiff('/foo/bar/merge_requests/1/diffs'); - expect(noteId.length).toBeGreaterThan(0); - expect(Notes.instance.toggleDiffNote).toHaveBeenCalledWith({ - target: jasmine.any(Object), - lineType: 'old', - forceShow: true, + setTimeout(() => { + expect(noteId.length).toBeGreaterThan(0); + expect(Notes.instance.toggleDiffNote).toHaveBeenCalledWith({ + target: jasmine.any(Object), + lineType: 'old', + forceShow: true, + }); + + done(); }); }); - it('should gracefully ignore non-existant fragment hash', function () { + it('should gracefully ignore non-existant fragment hash', function (done) { spyOn(urlUtils, 'getLocationHash').and.returnValue('note_something-that-does-not-exist'); this.class.loadDiff('/foo/bar/merge_requests/1/diffs'); - expect(Notes.instance.toggleDiffNote).not.toHaveBeenCalled(); + setTimeout(() => { + expect(Notes.instance.toggleDiffNote).not.toHaveBeenCalled(); + + done(); + }); }); }); @@ -370,6 +395,7 @@ import 'vendor/jquery.scrollTo'; describe('with parallel diff', () => { let noteId; let noteLineNumId; + let mock; beforeEach(() => { const diffsResponse = getJSONFixture(parallelChangesTabJsonFixture); @@ -384,30 +410,40 @@ import 'vendor/jquery.scrollTo'; .attr('href') .replace('#', ''); - spyOn($, 'ajax').and.callFake(function (options) { - options.success(diffsResponse); - }); + mock = new MockAdaptor(axios); + mock.onGet(/(.*)\/diffs\.json/).reply(200, diffsResponse); + }); + + afterEach(() => { + mock.restore(); }); describe('with note fragment hash', () => { - it('should expand and scroll to linked fragment hash #note_xxx', function () { + it('should expand and scroll to linked fragment hash #note_xxx', function (done) { spyOn(urlUtils, 'getLocationHash').and.returnValue(noteId); this.class.loadDiff('/foo/bar/merge_requests/1/diffs'); - expect(noteId.length).toBeGreaterThan(0); - expect(Notes.instance.toggleDiffNote).toHaveBeenCalledWith({ - target: jasmine.any(Object), - lineType: 'new', - forceShow: true, + setTimeout(() => { + expect(noteId.length).toBeGreaterThan(0); + expect(Notes.instance.toggleDiffNote).toHaveBeenCalledWith({ + target: jasmine.any(Object), + lineType: 'new', + forceShow: true, + }); + + done(); }); }); - it('should gracefully ignore non-existant fragment hash', function () { + it('should gracefully ignore non-existant fragment hash', function (done) { spyOn(urlUtils, 'getLocationHash').and.returnValue('note_something-that-does-not-exist'); this.class.loadDiff('/foo/bar/merge_requests/1/diffs'); - expect(Notes.instance.toggleDiffNote).not.toHaveBeenCalled(); + setTimeout(() => { + expect(Notes.instance.toggleDiffNote).not.toHaveBeenCalled(); + done(); + }); }); }); -- cgit v1.2.1 From 659e6f32d3d8e25f473c8a7413e5c6a0d19e41f6 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Gray Date: Mon, 29 Jan 2018 08:37:56 -0700 Subject: Fix labels specs --- app/views/shared/_delete_label_modal.html.haml | 4 ++-- app/views/shared/_label.html.haml | 2 +- features/steps/project/issues/labels.rb | 5 +++-- spec/features/projects/labels/update_prioritization_spec.rb | 2 +- 4 files changed, 7 insertions(+), 6 deletions(-) diff --git a/app/views/shared/_delete_label_modal.html.haml b/app/views/shared/_delete_label_modal.html.haml index f29c8f00d52..01effefc34d 100644 --- a/app/views/shared/_delete_label_modal.html.haml +++ b/app/views/shared/_delete_label_modal.html.haml @@ -7,8 +7,8 @@ .modal-body %p - %strong #{label.name} - will be permanently deleted from #{label.group.name}. This cannot be undone. + %strong= label.name + %span will be permanently deleted from #{label.is_a?(ProjectLabel)? label.project.name : label.group.name}. This cannot be undone. .modal-footer %a{ href: '#', data: { dismiss: 'modal' }, class: 'btn btn-default' } Cancel diff --git a/app/views/shared/_label.html.haml b/app/views/shared/_label.html.haml index dd9b4b4db44..9e140cd3f13 100644 --- a/app/views/shared/_label.html.haml +++ b/app/views/shared/_label.html.haml @@ -5,7 +5,7 @@ - show_label_merge_requests_link = show_label_issuables_link?(label, :merge_requests, project: @project) - show_label_issues_link = show_label_issuables_link?(label, :issues, project: @project) -%li{ id: label_css_id, data: { id: label.id } } +%li.label-list-item{ id: label_css_id, data: { id: label.id } } = render "shared/label_row", label: label .visible-xs.visible-sm-inline-block.dropdown diff --git a/features/steps/project/issues/labels.rb b/features/steps/project/issues/labels.rb index 196e0fff63a..4df96e081f9 100644 --- a/features/steps/project/issues/labels.rb +++ b/features/steps/project/issues/labels.rb @@ -15,8 +15,9 @@ class Spinach::Features::ProjectIssuesLabels < Spinach::FeatureSteps step 'I delete all labels' do page.within '.labels' do - page.all('.remove-row').each do - accept_confirm { first('.remove-row').click } + page.all('.label-list-item').each do + first('.remove-row').click + first(:link, 'Delete label').click end end end diff --git a/spec/features/projects/labels/update_prioritization_spec.rb b/spec/features/projects/labels/update_prioritization_spec.rb index 85bd776932b..ae8b1364ec7 100644 --- a/spec/features/projects/labels/update_prioritization_spec.rb +++ b/spec/features/projects/labels/update_prioritization_spec.rb @@ -99,7 +99,7 @@ feature 'Prioritize labels' do expect(page).to have_content 'wontfix' # Sort labels - drag_to(selector: '.js-prioritized-labels', from_index: 1, to_index: 2) + drag_to(selector: '.label-list-item', from_index: 1, to_index: 2) page.within('.prioritized-labels') do expect(first('li')).to have_content('feature') -- cgit v1.2.1 From f610a0cc17d1f9cd607810d5aabd7bf097f645d0 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Gray Date: Tue, 30 Jan 2018 13:36:27 -0700 Subject: Fix padding & font size on labels --- app/assets/stylesheets/framework/modal.scss | 5 +++-- app/assets/stylesheets/pages/labels.scss | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/app/assets/stylesheets/framework/modal.scss b/app/assets/stylesheets/framework/modal.scss index 5438a4fa207..1b785341246 100644 --- a/app/assets/stylesheets/framework/modal.scss +++ b/app/assets/stylesheets/framework/modal.scss @@ -5,8 +5,9 @@ .page-title { margin-top: 0; - .label { - font-size: initial; + .color-label { + font-size: 14px; + padding: 6px 10px; } } } diff --git a/app/assets/stylesheets/pages/labels.scss b/app/assets/stylesheets/pages/labels.scss index 9c4f51fc0d9..a72e654824e 100644 --- a/app/assets/stylesheets/pages/labels.scss +++ b/app/assets/stylesheets/pages/labels.scss @@ -105,7 +105,7 @@ } .label { - padding: 8px 9px 9px; + padding: 8px 12px; font-size: 14px; } } -- cgit v1.2.1 From 5126b1c5d3ba71a3ddad7e59142e46344b4c0eda Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Tue, 30 Jan 2018 22:02:44 +0000 Subject: Update auto_devops_domain help block with i18n --- app/views/admin/application_settings/_form.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/admin/application_settings/_form.html.haml b/app/views/admin/application_settings/_form.html.haml index d77d99180fe..e49fbafa61d 100644 --- a/app/views/admin/application_settings/_form.html.haml +++ b/app/views/admin/application_settings/_form.html.haml @@ -254,7 +254,7 @@ .col-sm-10 = f.text_field :auto_devops_domain, class: 'form-control', placeholder: 'domain.com' .help-block - = s_("Specify a domain to use by default for every project's Auto Review Apps and Auto Deploy stages.") + = s_("AdminSettings|Specify a domain to use by default for every project's Auto Review Apps and Auto Deploy stages.") .form-group .col-sm-offset-2.col-sm-10 .checkbox -- cgit v1.2.1 From d2688b2855c9a039335da0ad2484aa02a44c1b2b Mon Sep 17 00:00:00 2001 From: Mike Greiling Date: Tue, 30 Jan 2018 17:13:54 -0600 Subject: add manage button to application rows in cluster integration --- .../clusters/components/application_row.vue | 18 +++++++++++++++++- .../javascripts/clusters/components/applications.vue | 15 +++++++++++---- 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/app/assets/javascripts/clusters/components/application_row.vue b/app/assets/javascripts/clusters/components/application_row.vue index c13bbcee863..0aa3aead30d 100644 --- a/app/assets/javascripts/clusters/components/application_row.vue +++ b/app/assets/javascripts/clusters/components/application_row.vue @@ -32,6 +32,10 @@ type: String, required: false, }, + manageLink: { + type: String, + required: false, + }, description: { type: String, required: true, @@ -141,9 +145,21 @@