Merge branch 'ce-to-ee-2018-05-21' into 'master'

CE upstream - 2018-05-21 15:29 UTC

See merge request gitlab-org/gitlab-ee!5789

app/models/ci/build.rb

@@ -603,6 +603,7 @@ module Ci
         break variables unless persisted?
 
         variables
+          .concat(pipeline.persisted_variables)
           .append(key: 'CI_JOB_ID', value: id.to_s)
           .append(key: 'CI_JOB_TOKEN', value: token, public: false)
           .append(key: 'CI_BUILD_ID', value: id.to_s)

app/models/ci/pipeline.rb

@@ -541,9 +541,14 @@ module Ci
       strong_memoize(:legacy_trigger) { trigger_requests.first }
     end
 
+    def persisted_variables
+      Gitlab::Ci::Variables::Collection.new.tap do |variables|
+        variables.append(key: 'CI_PIPELINE_ID', value: id.to_s) if persisted?
+      end
+    end
+
     def predefined_variables
       Gitlab::Ci::Variables::Collection.new
-        .append(key: 'CI_PIPELINE_ID', value: id.to_s)
         .append(key: 'CI_CONFIG_PATH', value: ci_yaml_file_path)
         .append(key: 'CI_PIPELINE_SOURCE', value: source.to_s)
         .append(key: 'CI_COMMIT_MESSAGE', value: git_commit_message)

app/models/ci/runner.rb

@@ -76,7 +76,7 @@ module Ci
       project_type: 3
     }
 
-    cached_attr_reader :version, :revision, :platform, :architecture, :contacted_at, :ip_address
+    cached_attr_reader :version, :revision, :platform, :architecture, :ip_address, :contacted_at
 
     chronic_duration_attr :maximum_timeout_human_readable, :maximum_timeout
 

app/models/concerns/redis_cacheable.rb

@@ -7,7 +7,11 @@ module RedisCacheable
   class_methods do
     def cached_attr_reader(*attributes)
       attributes.each do |attribute|
-        define_method("#{attribute}") do
+        define_method(attribute) do
+          unless self.has_attribute?(attribute)
+            raise ArgumentError, "`cached_attr_reader` requires the #{self.class.name}\##{attribute} attribute to have a database column"
+          end
+
           cached_attribute(attribute) || read_attribute(attribute)
         end
       end
@@ -15,13 +19,16 @@ module RedisCacheable
   end
 
   def cached_attribute(attribute)
-    (cached_attributes || {})[attribute]
+    cached_value = (cached_attributes || {})[attribute]
+    cast_value_from_cache(attribute, cached_value) if cached_value
   end
 
   def cache_attributes(values)
     Gitlab::Redis::SharedState.with do |redis|
       redis.set(cache_attribute_key, values.to_json, ex: CACHED_ATTRIBUTES_EXPIRY_TIME)
     end
+
+    clear_memoization(:cached_attributes)
   end
 
   private
@@ -38,4 +45,12 @@ module RedisCacheable
       end
     end
   end
+
+  def cast_value_from_cache(attribute, value)
+    if Gitlab.rails5?
+      self.class.type_for_attribute(attribute).cast(value)
+    else
+      self.class.column_for_attribute(attribute).type_cast_from_database(value)
+    end
+  end
 end

app/views/admin/broadcast_messages/_form.html.haml

@@ -27,11 +27,11 @@
     .col-sm-10
       = f.color_field :font, class: "form-control"
   .form-group
-    = f.label :starts_at, class: 'control-label'
+    = f.label :starts_at, _("Starts at (UTC)"), class: 'control-label'
     .col-sm-10.datetime-controls
       = f.datetime_select :starts_at, {}, class: 'form-control form-control-inline'
   .form-group
-    = f.label :ends_at, class: 'control-label'
+    = f.label :ends_at, _("Ends at (UTC)"), class: 'control-label'
     .col-sm-10.datetime-controls
       = f.datetime_select :ends_at, {}, class: 'form-control form-control-inline'
   .form-actions

app/views/admin/runners/_runner.html.haml

@@ -33,7 +33,7 @@
         = tag
   %td
     - if runner.contacted_at
-      #{time_ago_in_words(runner.contacted_at)} ago
+      = time_ago_with_tooltip runner.contacted_at
     - else
       Never
   %td.admin-runner-btn-group-cell

app/views/projects/registry/repositories/index.html.haml

@@ -30,7 +30,7 @@
           %br
           %p
             - deploy_token = link_to(_('deploy token'), help_page_path('user/project/deploy_tokens/index', anchor: 'read-container-registry-images'), target: '_blank')
-            = s_('ContainerRegistry|You can also %{deploy_token} for read-only access to the registry images.').html_safe % { deploy_token: deploy_token }
+            = s_('ContainerRegistry|You can also use a %{deploy_token} for read-only access to the registry images.').html_safe % { deploy_token: deploy_token }
           %br
           %p
             = s_('ContainerRegistry|Once you log in, you&rsquo;re free to create and upload a container image using the common %{build} and %{push} commands').html_safe % { build: "<code>build</code>".html_safe, push: "<code>push</code>".html_safe }

app/views/shared/runners/show.html.haml

@@ -66,6 +66,6 @@
       %td Last contact
       %td
         - if @runner.contacted_at
-          #{time_ago_in_words(@runner.contacted_at)} ago
+          = time_ago_with_tooltip @runner.contacted_at
         - else
           Never

changelogs/unreleased/46082-runner-contacted_at-is-not-always-a-time-type.yml

+---
+title: Fix Runner contacted at tooltip cache.
+merge_request: 18810
+author:
+type: fixed

changelogs/unreleased/create-live-trace-only-if-job-is-complete.yml

+---
+title: Forbid to patch traces for finished jobs
+merge_request: 18969
+author:
+type: fixed

changelogs/unreleased/fix-gb-exclude-persisted-variables-from-environment-name.yml

+---
+title: Exclude CI_PIPELINE_ID from variables supported in dynamic environment name
+merge_request: 19032
+author:
+type: fixed

config/initializers/flipper.rb

-require 'flipper/adapters/active_record'
-require 'flipper/adapters/active_support_cache_store'
-
-Flipper.configure do |config|
-  config.default do
-    adapter = Flipper::Adapters::ActiveRecord.new(
-      feature_class: Feature::FlipperFeature, gate_class: Feature::FlipperGate)
-    cached_adapter = Flipper::Adapters::ActiveSupportCacheStore.new(
-      adapter,
-      Rails.cache,
-      expires_in: 1.hour)
-
-    Flipper.new(cached_adapter)
-  end
-end
-
 Feature.register_feature_groups
-
-unless Rails.env.test?
-  require 'flipper/middleware/memoizer'
-  Rails.application.config.middleware.use Flipper::Middleware::Memoizer
-end

doc/ci/environments.md

@@ -252,6 +252,7 @@ including predefined, secure variables and `.gitlab-ci.yml`
 [`variables`](yaml/README.md#variables). You however cannot use variables
 defined under `script` or on the Runner's side. There are other variables that
 are unsupported in environment name context:
+- `CI_PIPELINE_ID`
 - `CI_JOB_ID`
 - `CI_JOB_TOKEN`
 - `CI_BUILD_ID`

doc/ci/variables/README.md

@@ -574,6 +574,7 @@ We do not support variables containing tokens because of security reasons.
 
 You can find a full list of unsupported variables below:
 
+- `CI_PIPELINE_ID`
 - `CI_JOB_ID`
 - `CI_JOB_TOKEN`
 - `CI_BUILD_ID`

lib/api/runner.rb

@@ -149,6 +149,7 @@ module API
       end
       patch '/:id/trace' do
         job = authenticate_job!
+        forbidden!('Job is not running') unless job.running?
 
         error!('400 Missing header Content-Range', 400) unless request.headers.key?('Content-Range')
         content_range = request.headers['Content-Range']

lib/feature.rb

+require 'flipper/adapters/active_record'
+require 'flipper/adapters/active_support_cache_store'
+
 class Feature
   # Classes to override flipper table names
   class FlipperFeature < Flipper::Adapters::ActiveRecord::Feature
@@ -60,7 +63,8 @@ class Feature
     end
 
     def flipper
-      @flipper ||= Flipper.instance
+      Thread.current[:flipper] ||=
+        Flipper.new(flipper_adapter).tap { |flip| flip.memoize = true }
     end
 
     # This method is called from config/initializers/flipper.rb and can be used
@@ -68,5 +72,16 @@ class Feature
     # See https://docs.gitlab.com/ee/development/feature_flags.html#feature-groups
     def register_feature_groups
     end
+
+    def flipper_adapter
+      active_record_adapter = Flipper::Adapters::ActiveRecord.new(
+        feature_class: FlipperFeature,
+        gate_class: FlipperGate)
+
+      Flipper::Adapters::ActiveSupportCacheStore.new(
+        active_record_adapter,
+        Rails.cache,
+        expires_in: 1.hour)
+    end
   end
 end

locale/gitlab.pot

@@ -8,8 +8,8 @@ msgid ""
 msgstr ""
 "Project-Id-Version: gitlab 1.0.0\n"
 "Report-Msgid-Bugs-To: \n"
-"POT-Creation-Date: 2018-05-16 14:41+0200\n"
-"PO-Revision-Date: 2018-05-16 14:41+0200\n"
+"POT-Creation-Date: 2018-05-22 07:03+0000\n"
+"PO-Revision-Date: 2018-05-22 07:03+0000\n"
 "Last-Translator: FULL NAME <EMAIL@ADDRESS>\n"
 "Language-Team: LANGUAGE <LL@li.org>\n"
 "Language: \n"
@@ -1668,7 +1668,7 @@ msgstr ""
 msgid "ContainerRegistry|With the Docker Container Registry integrated into GitLab, every project can have its own space to store its Docker images."
 msgstr ""
 
-msgid "ContainerRegistry|You can also %{deploy_token} for read-only access to the registry images."
+msgid "ContainerRegistry|You can also use a %{deploy_token} for read-only access to the registry images."
 msgstr ""
 
 msgid "Continuous Integration and Deployment"
@@ -2156,6 +2156,9 @@ msgstr ""
 msgid "Enabled"
 msgstr ""
 
+msgid "Ends at (UTC)"
+msgstr ""
+
 msgid "Environments"
 msgstr ""
 
@@ -4693,6 +4696,9 @@ msgstr ""
 msgid "Started"
 msgstr ""
 
+msgid "Starts at (UTC)"
+msgstr ""
+
 msgid "State your message to activate"
 msgstr ""
 
@@ -5257,6 +5263,9 @@ msgstr ""
 msgid "Toggle Sidebar"
 msgstr ""
 
+msgid "Toggle discussion"
+msgstr ""
+
 msgid "Toggle sidebar"
 msgstr ""
 
@@ -5996,9 +6005,6 @@ msgstr ""
 msgid "mrWidget|An error occured while removing your approval."
 msgstr ""
 
-msgid "mrWidget|An error occured while retrieving approval data for this merge request."
-msgstr ""
-
 msgid "mrWidget|An error occured while submitting your approval."
 msgstr ""
 

spec/models/ci/build_spec.rb

@@ -648,6 +648,14 @@ describe Ci::Build do
 
         it { is_expected.to eq('review/host') }
       end
+
+      context 'when using persisted variables' do
+        let(:build) do
+          create(:ci_build, environment: 'review/x$CI_BUILD_ID')
+        end
+
+        it { is_expected.to eq('review/x') }
+      end
     end
 
     describe '#starts_environment?' do
@@ -1545,6 +1553,7 @@ describe Ci::Build do
     let(:container_registry_enabled) { false }
     let(:predefined_variables) do
       [
+        { key: 'CI_PIPELINE_ID', value: pipeline.id.to_s, public: true },
         { key: 'CI_JOB_ID', value: build.id.to_s, public: true },
         { key: 'CI_JOB_TOKEN', value: build.token, public: false },
         { key: 'CI_BUILD_ID', value: build.id.to_s, public: true },
@@ -1576,7 +1585,6 @@ describe Ci::Build do
         { key: 'CI_PROJECT_NAMESPACE', value: project.namespace.full_path, public: true },
         { key: 'CI_PROJECT_URL', value: project.web_url, public: true },
         { key: 'CI_PROJECT_VISIBILITY', value: 'private', public: true },
-        { key: 'CI_PIPELINE_ID', value: pipeline.id.to_s, public: true },
         { key: 'CI_CONFIG_PATH', value: pipeline.ci_yaml_file_path, public: true },
         { key: 'CI_PIPELINE_SOURCE', value: pipeline.source, public: true },
         { key: 'CI_COMMIT_MESSAGE', value: pipeline.git_commit_message, public: true },

spec/models/ci/pipeline_spec.rb

@@ -171,13 +171,39 @@ describe Ci::Pipeline, :mailer do
     end
   end
 
+  describe '#persisted_variables' do
+    context 'when pipeline is not persisted yet' do
+      subject { build(:ci_pipeline).persisted_variables }
+
+      it 'does not contain some variables' do
+        keys = subject.map { |variable| variable[:key] }
+
+        expect(keys).not_to include 'CI_PIPELINE_ID'
+      end
+    end
+
+    context 'when pipeline is persisted' do
+      subject { build_stubbed(:ci_pipeline).persisted_variables }
+
+      it 'does contains persisted variables' do
+        keys = subject.map { |variable| variable[:key] }
+
+        expect(keys).to eq %w[CI_PIPELINE_ID]
+      end
+    end
+  end
+
   describe '#predefined_variables' do
     subject { pipeline.predefined_variables }
 
     it 'includes all predefined variables in a valid order' do
       keys = subject.map { |variable| variable[:key] }
 
-      expect(keys).to eq %w[CI_PIPELINE_ID CI_CONFIG_PATH CI_PIPELINE_SOURCE CI_COMMIT_MESSAGE CI_COMMIT_TITLE CI_COMMIT_DESCRIPTION]
+      expect(keys).to eq %w[CI_CONFIG_PATH
+                            CI_PIPELINE_SOURCE
+                            CI_COMMIT_MESSAGE
+                            CI_COMMIT_TITLE
+                            CI_COMMIT_DESCRIPTION]
     end
   end
 

spec/models/concerns/redis_cacheable_spec.rb

 require 'spec_helper'
 
 describe RedisCacheable do
-  let(:model) { double }
+  let(:model) do
+    Struct.new(:id, :attributes) do
+      def read_attribute(attribute)
+        attributes[attribute]
+      end
+
+      def cast_value_from_cache(attribute, cached_value)
+        cached_value
+      end
+
+      def has_attribute?(attribute)
+        attributes.has_key?(attribute)
+      end
+    end
+  end
+
+  let(:payload) { { name: 'value', time: Time.zone.now } }
+  let(:instance) { model.new(1, payload) }
+  let(:cache_key) { instance.__send__(:cache_attribute_key) }
 
   before do
-    model.extend(described_class)
-    allow(model).to receive(:cache_attribute_key).and_return('key')
+    model.include(described_class)
   end
 
   describe '#cached_attribute' do
-    let(:payload) { { attribute: 'value' } }
-
-    subject { model.cached_attribute(payload.keys.first) }
+    subject { instance.cached_attribute(payload.keys.first) }
 
     it 'gets the cache attribute' do
       Gitlab::Redis::SharedState.with do |redis|
-        expect(redis).to receive(:get).with('key')
+        expect(redis).to receive(:get).with(cache_key)
           .and_return(payload.to_json)
       end
 
@@ -24,16 +39,62 @@ describe RedisCacheable do
   end
 
   describe '#cache_attributes' do
-    let(:values) { { name: 'new_name' } }
-
-    subject { model.cache_attributes(values) }
+    subject { instance.cache_attributes(payload) }
 
     it 'sets the cache attributes' do
       Gitlab::Redis::SharedState.with do |redis|
-        expect(redis).to receive(:set).with('key', values.to_json, anything)
+        expect(redis).to receive(:set).with(cache_key, payload.to_json, anything)
       end
 
       subject
     end
   end
+
+  describe '#cached_attr_reader', :clean_gitlab_redis_shared_state do
+    subject { instance.name }
+
+    before do
+      model.cached_attr_reader(:name)
+    end
+
+    context 'when there is no cached value' do
+      it 'reads the attribute' do
+        expect(instance).to receive(:read_attribute).and_call_original
+
+        expect(subject).to eq(payload[:name])
+      end
+    end
+
+    context 'when there is a cached value' do
+      it 'reads the cached value' do
+        expect(instance).not_to receive(:read_attribute)
+
+        instance.cache_attributes(payload)
+
+        expect(subject).to eq(payload[:name])
+      end
+    end
+
+    it 'always returns the latest values' do
+      expect(instance.name).to eq(payload[:name])
+
+      instance.cache_attributes(name: 'new_value')
+
+      expect(instance.name).to eq('new_value')
+    end
+  end
+
+  describe '#cast_value_from_cache' do
+    subject { instance.__send__(:cast_value_from_cache, attribute, value) }
+
+    context 'with runner contacted_at' do
+      let(:instance) { Ci::Runner.new }
+      let(:attribute) { :contacted_at }
+      let(:value) { '2018-05-07 13:53:08 UTC' }
+
+      it 'converts cache string to appropriate type' do
+        expect(subject).to be_an_instance_of(ActiveSupport::TimeWithZone)
+      end
+    end
+  end
 end

spec/requests/api/runner_spec.rb

@@ -920,6 +920,22 @@ describe API::Runner, :clean_gitlab_redis_shared_state do
             expect(job.reload.trace.raw).to eq 'BUILD TRACE appended appended'
           end
 
+          context 'when job is cancelled' do
+            before do
+              job.cancel
+            end
+
+            context 'when trace is patched' do
+              before do
+                patch_the_trace
+              end
+
+              it 'returns Forbidden ' do
+                expect(response.status).to eq(403)
+              end
+            end
+          end
+
           context 'when redis data are flushed' do
             before do
               redis_shared_state_cleanup!

spec/services/ci/create_pipeline_service_spec.rb

@@ -395,7 +395,27 @@ describe Ci::CreatePipelineService do
         result = execute_service
 
         expect(result).to be_persisted
-        expect(Environment.find_by(name: "review/master")).not_to be_nil
+        expect(Environment.find_by(name: "review/master")).to be_present
+      end
+    end
+
+    context 'with environment name including persisted variables' do
+      before do
+        config = YAML.dump(
+          deploy: {
+            environment: { name: "review/id1$CI_PIPELINE_ID/id2$CI_BUILD_ID" },
+            script: 'ls'
+          }
+        )
+
+        stub_ci_pipeline_yaml_file(config)
+      end
+
+      it 'skipps persisted variables in environment name' do
+        result = execute_service
+
+        expect(result).to be_persisted
+        expect(Environment.find_by(name: "review/id1/id2")).to be_present
       end
     end