Revert changes to predefined variables hierarchy

app/models/ci/build.rb

@@ -35,7 +35,6 @@ module Ci
     serialize :yaml_variables, Gitlab::Serializer::Ci::Variables # rubocop:disable Cop/ActiveRecordSerialize
 
     delegate :name, to: :project, prefix: true
-    delegate :ref_slug, to: :pipeline
 
     validates :coverage, numericality: true, allow_blank: true
     validates :ref, presence: true
@@ -234,6 +233,17 @@ module Ci
       user == current_user
     end
 
+    # A slugified version of the build ref, suitable for inclusion in URLs and
+    # domain names. Rules:
+    #
+    #   * Lowercased
+    #   * Anything not matching [a-z0-9-] is replaced with a -
+    #   * Maximum length is 63 bytes
+    #   * First/Last Character is not a hyphen
+    def ref_slug
+      Gitlab::Utils.slugify(ref.to_s)
+    end
+
     # Variables whose value does not depend on environment
     def simple_variables
       variables(environment: nil)
@@ -243,18 +253,22 @@ module Ci
     # contain unexpanded variables.
     def variables(environment: persisted_environment)
       variables = predefined_variables
+      variables += project.predefined_variables
       variables += pipeline.predefined_variables
       variables += runner.predefined_variables if runner
+      variables += project.container_registry_variables
       variables += project.deployment_variables if has_environment?
+      variables += project.auto_devops_variables
       variables += yaml_variables
       variables += user_variables
       variables += project.group.secret_variables_for(ref, project).map(&:to_runner_variable) if project.group
       variables += secret_variables(environment: environment)
       variables += trigger_request.user_variables if trigger_request
+      variables += pipeline.variables.map(&:to_runner_variable)
+      variables += pipeline.pipeline_schedule.job_variables if pipeline.pipeline_schedule
       variables += persisted_environment_variables if environment
-      variables += pipeline.priority_variables
 
-      variables.reverse.uniq { |variable| variable.fetch(:key) }.reverse
+      variables
     end
 
     def features
@@ -527,15 +541,25 @@ module Ci
 
     def predefined_variables
       variables = [
+        { key: 'CI', value: 'true', public: true },
+        { key: 'GITLAB_CI', value: 'true', public: true },
+        { key: 'GITLAB_FEATURES', value: project.namespace.features.join(','), public: true },
+        { key: 'CI_SERVER_NAME', value: 'GitLab', public: true },
+        { key: 'CI_SERVER_VERSION', value: Gitlab::VERSION, public: true },
+        { key: 'CI_SERVER_REVISION', value: Gitlab::REVISION, public: true },
         { key: 'CI_JOB_ID', value: id.to_s, public: true },
         { key: 'CI_JOB_NAME', value: name, public: true },
         { key: 'CI_JOB_STAGE', value: stage, public: true },
         { key: 'CI_JOB_TOKEN', value: token, public: false },
+        { key: 'CI_COMMIT_SHA', value: sha, public: true },
+        { key: 'CI_COMMIT_REF_NAME', value: ref, public: true },
+        { key: 'CI_COMMIT_REF_SLUG', value: ref_slug, public: true },
         { key: 'CI_REGISTRY_USER', value: CI_REGISTRY_USER, public: true },
         { key: 'CI_REGISTRY_PASSWORD', value: token, public: false },
         { key: 'CI_REPOSITORY_URL', value: repo_url, public: false }
       ]
 
+      variables << { key: "CI_COMMIT_TAG", value: ref, public: true } if tag?
       variables << { key: "CI_PIPELINE_TRIGGERED", value: 'true', public: true } if trigger_request
       variables << { key: "CI_JOB_MANUAL", value: 'true', public: true } if action?
       variables.concat(legacy_variables)

app/models/ci/pipeline.rb

@@ -473,33 +473,11 @@ module Ci
     end
 
     def predefined_variables
-      predefined = [
-        { key: 'CI', value: 'true', public: true },
-        { key: 'GITLAB_CI', value: 'true', public: true },
-        { key: 'CI_SERVER_NAME', value: 'GitLab', public: true },
-        { key: 'CI_SERVER_VERSION', value: Gitlab::VERSION, public: true },
-        { key: 'CI_SERVER_REVISION', value: Gitlab::REVISION, public: true },
+      [
         { key: 'CI_PIPELINE_ID', value: id.to_s, public: true },
         { key: 'CI_CONFIG_PATH', value: ci_yaml_file_path, public: true },
-        { key: 'CI_PIPELINE_SOURCE', value: source.to_s, public: true },
-        { key: 'CI_COMMIT_SHA', value: sha, public: true },
-        { key: 'CI_COMMIT_REF_NAME', value: ref, public: true },
-        { key: 'CI_COMMIT_REF_SLUG', value: ref_slug, public: true }
+        { key: 'CI_PIPELINE_SOURCE', value: source.to_s, public: true }
       ]
-
-      predefined.push(key: 'CI_COMMIT_TAG', value: ref, public: true) if tag?
-      predefined.push(key: 'CI_PIPELINE_TRIGGERED', value: 'true', public: true) if variables.any?
-
-      Array(project.predefined_variables) + predefined
-    end
-
-    def priority_variables
-      Array(pipeline_schedule&.job_variables) +
-        self.variables.map(&:to_runner_variable)
-    end
-
-    def runtime_variables
-      predefined_variables + priority_variables
     end
 
     def queued_duration
@@ -539,18 +517,6 @@ module Ci
       @latest_builds_with_artifacts ||= builds.latest.with_artifacts.to_a
     end
 
-    # A slugified version of the ref, suitable for inclusion in URLs and
-    # domain names. Rules:
-    #
-    #   * Lowercased
-    #   * Anything not matching [a-z0-9-] is replaced with a -
-    #   * Maximum length is 63 bytes
-    #   * First/Last Character is not a hyphen
-    #
-    def ref_slug
-      Gitlab::Utils.slugify(ref.to_s)
-    end
-
     private
 
     def ci_yaml_from_repo

app/models/project.rb

@@ -1582,21 +1582,15 @@ class Project < ActiveRecord::Base
   end
 
   def predefined_variables
-    variables = [
+    [
       { key: 'CI_PROJECT_ID', value: id.to_s, public: true },
       { key: 'CI_PROJECT_NAME', value: path, public: true },
       { key: 'CI_PROJECT_PATH', value: full_path, public: true },
       { key: 'CI_PROJECT_PATH_SLUG', value: full_path_slug, public: true },
       { key: 'CI_PROJECT_NAMESPACE', value: namespace.full_path, public: true },
       { key: 'CI_PROJECT_URL', value: web_url, public: true },
-      { key: 'CI_PROJECT_VISIBILITY', value: Gitlab::VisibilityLevel.string_level(visibility_level), public: true },
-      { key: 'GITLAB_FEATURES', value: namespace.features.join(','), public: true }
+      { key: 'CI_PROJECT_VISIBILITY', value: Gitlab::VisibilityLevel.string_level(visibility_level), public: true }
     ]
-
-    variables += container_registry_variables
-    variables += auto_devops_variables
-
-    variables
   end
 
   def container_registry_variables

spec/models/ci/build_spec.rb

@@ -1245,8 +1245,24 @@ describe Ci::Build do
   end
 
   describe '#ref_slug' do
-    it 'delegates ref_slug method to the pipeline' do
-      expect(build).to delegate_method(:ref_slug).to(:pipeline)
+    {
+      'master'                => 'master',
+      '1-foo'                 => '1-foo',
+      'fix/1-foo'             => 'fix-1-foo',
+      'fix-1-foo'             => 'fix-1-foo',
+      'a' * 63                => 'a' * 63,
+      'a' * 64                => 'a' * 63,
+      'FOO'                   => 'foo',
+      '-' + 'a' * 61 + '-'    => 'a' * 61,
+      '-' + 'a' * 62 + '-'    => 'a' * 62,
+      '-' + 'a' * 63 + '-'    => 'a' * 62,
+      'a' * 62 + ' '          => 'a' * 62
+    }.each do |ref, slug|
+      it "transforms #{ref} to #{slug}" do
+        build.ref = ref
+
+        expect(build.ref_slug).to eq(slug)
+      end
     end
   end
 
@@ -1838,8 +1854,7 @@ describe Ci::Build do
         end
 
         allow_any_instance_of(Ci::Pipeline)
-          .to receive(:predefined_variables)
-          .and_return([project_pre_var] + [pipeline_pre_var])
+          .to receive(:predefined_variables) { [pipeline_pre_var] }
       end
 
       it do
@@ -1875,32 +1890,6 @@ describe Ci::Build do
         end
       end
     end
-
-    context 'when there are duplicated variables present ' do
-      context 'when there are duplicated YAML variables' do
-        before do
-          build.yaml_variables = [{ key: 'MYVAR', value: 'first', public: true },
-                                  { key: 'MYVAR', value: 'second', public: true }]
-        end
-
-        it 'keeps the last occurence of a variable by given key' do
-          expect(subject).not_to include(key: 'MYVAR', value: 'first', public: true)
-          expect(subject).to include(key: 'MYVAR', value: 'second', public: true)
-        end
-      end
-
-      context 'when pipeline trigger variable overrides YAML variables' do
-        before do
-          build.yaml_variables = [{ key: 'MYVAR', value: 'myvar', public: true }]
-          pipeline.variables.build(key: 'MYVAR', value: 'pipeline value')
-        end
-
-        it 'overrides YAML variable with a pipeline trigger variable' do
-          expect(subject).not_to include(key: 'MYVAR', value: 'myvar', public: true)
-          expect(subject).to include(key: 'MYVAR', value: 'pipeline value', public: false)
-        end
-      end
-    end
   end
 
   describe 'state transition: any => [:pending]' do

spec/models/ci/pipeline_spec.rb

@@ -167,47 +167,16 @@ describe Ci::Pipeline, :mailer do
     end
   end
 
-  describe 'pipeline variables' do
   describe '#predefined_variables' do
     subject { pipeline.predefined_variables }
 
     it { is_expected.to be_an(Array) }
 
     it 'includes the defined keys' do
-        keys = subject.map { |v| v.fetch(:key) }
+      keys = subject.map { |v| v[:key] }
 
       expect(keys).to include('CI_PIPELINE_ID', 'CI_CONFIG_PATH', 'CI_PIPELINE_SOURCE')
     end
-
-      it 'includes project-level predefined variables' do
-        keys = subject.map { |v| v.fetch(:key) }
-
-        expect(keys).to include('CI_PROJECT_NAME')
-      end
-    end
-
-    describe '#priority_variables' do
-      before do
-        pipeline.variables.build(key: 'MY_VAR', value: 'my var')
-      end
-
-      it 'returns trigger variables' do
-        expect(pipeline.priority_variables)
-          .to include(key: 'MY_VAR', value: 'my var', public: false)
-      end
-    end
-
-    describe '#runtime_variables' do
-      before do
-        pipeline.variables.build(key: 'MY_VAR', value: 'my var')
-      end
-
-      it 'includes predefined and priority variables' do
-        variables = pipeline.runtime_variables.map { |v| v.fetch(:key) }
-
-        expect(variables).to include('MY_VAR', 'CI_PIPELINE_ID', 'CI_PROJECT_ID')
-      end
-    end
   end
 
   describe '#auto_canceled?' do
@@ -1280,28 +1249,6 @@ describe Ci::Pipeline, :mailer do
     end
   end
 
-  describe '#ref_slug' do
-    {
-      'master'                => 'master',
-      '1-foo'                 => '1-foo',
-      'fix/1-foo'             => 'fix-1-foo',
-      'fix-1-foo'             => 'fix-1-foo',
-      'a' * 63                => 'a' * 63,
-      'a' * 64                => 'a' * 63,
-      'FOO'                   => 'foo',
-      '-' + 'a' * 61 + '-'    => 'a' * 61,
-      '-' + 'a' * 62 + '-'    => 'a' * 62,
-      '-' + 'a' * 63 + '-'    => 'a' * 62,
-      'a' * 62 + ' '          => 'a' * 62
-    }.each do |ref, slug|
-      it "transforms #{ref} to #{slug}" do
-        pipeline.ref = ref
-
-        expect(pipeline.ref_slug).to eq(slug)
-      end
-    end
-  end
-
   describe '#execute_hooks' do
     let!(:build_a) { create_build('a', 0) }
     let!(:build_b) { create_build('b', 0) }