Improve implementation of variables

7 files changed, 88 insertions, 70 deletions

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 6115ffd87c2..21c0e128b98 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -145,7 +145,15 @@ module Ci
     end
 
     def variables
-      predefined_variables + yaml_variables + project_variables + trigger_variables
+      variables = predefined_variables
+      variables += project.predefined_variables
+      variables += pipeline.predefined_variables
+      variables += runner.predefined_variables if runner
+      variables += project.container_registry_variables
+      variables += yaml_variables
+      variables += project.secret_variables
+      variables += trigger_request.user_variables if trigger_request
+      variables
     end
 
     def merge_request
@@ -430,63 +438,23 @@ module Ci
       self.update(erased_by: user, erased_at: Time.now, artifacts_expire_at: nil)
     end
 
-    def project_variables
-      project.variables.map do |variable|
-        { key: variable.key, value: variable.value, public: false }
-      end
-    end
-
-    def trigger_variables
-      if trigger_request && trigger_request.variables
-        trigger_request.variables.map do |key, value|
-          { key: key, value: value, public: false }
-        end
-      else
-        []
-      end
-    end
-
     def predefined_variables
-      variables = []
-      variables << { key: 'CI', value: 'true', public: true }
-      variables << { key: 'GITLAB_CI', value: 'true', public: true }
-
-      variables << { key: 'CI_BUILD_ID', value: id.to_s, public: true }
-      variables << { key: 'CI_BUILD_TOKEN', value: token, public: false }
-      variables << { key: 'CI_BUILD_REF', value: sha, public: true }
-      variables << { key: 'CI_BUILD_BEFORE_SHA', value: before_sha, public: true }
-      variables << { key: 'CI_BUILD_REF_NAME', value: ref, public: true }
+      variables = [
+        { key: 'CI', value: 'true', public: true },
+        { key: 'GITLAB_CI', value: 'true', public: true },
+        { key: 'CI_BUILD_ID', value: id.to_s, public: true },
+        { key: 'CI_BUILD_TOKEN', value: token, public: false },
+        { key: 'CI_BUILD_REF', value: sha, public: true },
+        { key: 'CI_BUILD_BEFORE_SHA', value: before_sha, public: true },
+        { key: 'CI_BUILD_REF_NAME', value: ref, public: true },
+        { key: 'CI_BUILD_NAME', value: name, public: true },
+        { key: 'CI_BUILD_STAGE', value: stage, 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 }
+      ]
       variables << { key: 'CI_BUILD_TAG', value: ref, public: true } if tag?
-      variables << { key: 'CI_BUILD_NAME', value: name, public: true }
-      variables << { key: 'CI_BUILD_STAGE', value: stage, public: true }
       variables << { key: 'CI_BUILD_TRIGGERED', value: 'true', public: true } if trigger_request
-
-      variables << { key: 'CI_PIPELINE_ID', value: pipeline.id.to_s, public: true }
-
-      variables << { key: 'CI_PROJECT_ID', value: project.id.to_s, public: true }
-      variables << { key: 'CI_PROJECT_NAME', value: project.path, public: true }
-      variables << { key: 'CI_PROJECT_PATH', value: project.path_with_namespace, public: true }
-      variables << { key: 'CI_PROJECT_NAMESPACE', value: project.namespace.path, public: true }
-      variables << { key: 'CI_PROJECT_URL', value: project.web_url, public: true }
-
-      if Gitlab.config.registry.enabled
-        variables << { key: 'CI_REGISTRY', value: Gitlab.config.registry.host_port, public: true }
-
-        if project.container_registry_enabled?
-          variables << { key: 'CI_REGISTRY_IMAGE', value: project.container_registry_repository_url, public: true }
-        end
-      end
-
-      variables << { key: 'CI_SERVER_NAME', value: 'GitLab', public: true }
-      variables << { key: 'CI_SERVER_VERSION', value: Gitlab::VERSION, public: true }
-      variables << { key: 'CI_SERVER_REVISION', value: Gitlab::REVISION, public: true }
-
-      if runner
-        variables << { key: 'CI_RUNNER_ID', value: runner.id.to_s, public: true }
-        variables << { key: 'CI_RUNNER_DESCRIPTION', value: runner.description, public: true }
-        variables << { key: 'CI_RUNNER_TAGS', value: runner.tag_list.to_s, public: true }
-      end
-
       variables
     end
 
diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index aca8607f4e8..ad117e3950d 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -198,6 +198,12 @@ module Ci
       Note.for_commit_id(sha)
     end
 
+    def predefined_variables
+      [
+        { key: 'CI_PIPELINE_ID', value: id.to_s, public: true }
+      ]
+    end
+
     private
 
     def build_builds_for_stages(stages, user, status, trigger_request)
diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb
index b64ec79ec2b..49f05f881a2 100644
--- a/app/models/ci/runner.rb
+++ b/app/models/ci/runner.rb
@@ -114,6 +114,14 @@ module Ci
       tag_list.any?
     end
 
+    def predefined_variables
+      [
+        { key: 'CI_RUNNER_ID', value: id.to_s, public: true },
+        { key: 'CI_RUNNER_DESCRIPTION', value: description, public: true },
+        { key: 'CI_RUNNER_TAGS', value: tag_list.to_s, public: true }
+      ]
+    end
+
     private
 
     def tag_constraints
diff --git a/app/models/ci/trigger_request.rb b/app/models/ci/trigger_request.rb
index fcf2b6dc5e2..fc674871743 100644
--- a/app/models/ci/trigger_request.rb
+++ b/app/models/ci/trigger_request.rb
@@ -7,5 +7,13 @@ module Ci
     has_many :builds, class_name: 'Ci::Build'
 
     serialize :variables
+
+    def user_variables
+      return [] unless variables
+
+      variables.map do |key, value|
+        { key: key, value: value, public: false }
+      end
+    end
   end
 end
diff --git a/app/models/project.rb b/app/models/project.rb
index a805f5d97bc..71e22087187 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -1164,4 +1164,31 @@ class Project < ActiveRecord::Base
   def ensure_dir_exist
     gitlab_shell.add_namespace(repository_storage_path, namespace.path)
   end
+
+  def predefined_variables
+    [
+      { key: 'CI_PROJECT_ID', value: id.to_s, public: true },
+      { key: 'CI_PROJECT_NAME', value: path, public: true },
+      { key: 'CI_PROJECT_PATH', value: path_with_namespace, public: true },
+      { key: 'CI_PROJECT_NAMESPACE', value: namespace.path, public: true },
+      { key: 'CI_PROJECT_URL', value: web_url, public: true }
+    ]
+  end
+
+  def container_registry_variables
+    return [] unless Gitlab.config.registry.enabled
+
+    variables = [
+      { key: 'CI_REGISTRY', value: Gitlab.config.registry.host_port, public: true }
+    ]
+
+    variables << { key: 'CI_REGISTRY_IMAGE', value: container_registry_repository_url, public: true } if container_registry_enabled?
+    variables
+  end
+
+  def secret_variables
+    variables.map do |variable|
+      { key: variable.key, value: variable.value, public: false }
+    end
+  end
 end
diff --git a/doc/ci/variables/README.md b/doc/ci/variables/README.md
index 420a3c831fa..c9eebec801d 100644
--- a/doc/ci/variables/README.md
+++ b/doc/ci/variables/README.md
@@ -37,15 +37,15 @@ The `API_TOKEN` will take the Secure Variable value: `SECURE`.
 | **CI_BUILD_TOKEN**      | all    | 1.2    | Token used for authenticating with the GitLab Container Registry |
 | **CI_PIPELINE_ID**      | 8.10   | 0.5    | The unique id of the current pipeline that GitLab CI uses internally |
 | **CI_PROJECT_ID**       | all    | all    | The unique id of the current project that GitLab CI uses internally |
-| **CI_PROJECT_NAME**     | 8.10   | 0.5    | The project name that is currently build |
-| **CI_PROJECT_NAMESPACE**| 8.10   | 0.5    | The project namespace that is currently build |
+| **CI_PROJECT_NAME**     | 8.10   | 0.5    | The project name that is currently being built |
+| **CI_PROJECT_NAMESPACE**| 8.10   | 0.5    | The project namespace that is currently being built |
 | **CI_PROJECT_PATH**     | 8.10   | 0.5    | The namespace with project name |
 | **CI_PROJECT_URL**      | 8.10   | 0.5    | The HTTP address to access project |
-| **CI_PROJECT_DIR**      | all    | all    | The full path where the repository is cloned and where the build is ran |
-| **CI_REGISTRY**         | 8.10   | 0.5    | If the Container Registry is enabled it returns address of GitLab's Container Registry |
-| **CI_REGISTRY_IMAGE**   | 8.10   | 0.5    | If the Container Registry is for project it returns the address of registry tied to specific project |
+| **CI_PROJECT_DIR**      | all    | all    | The full path where the repository is cloned and where the build is run |
+| **CI_REGISTRY**         | 8.10   | 0.5    | If the Container Registry is enabled it returns the address of GitLab's Container Registry |
+| **CI_REGISTRY_IMAGE**   | 8.10   | 0.5    | If the Container Registry is enabled for the project it returnes the address of the registry tied to the specific project |
 | **CI_RUNNER_ID**        | 8.10   | 0.5    | The unique id of the used runner |
-| **CI_RUNNER_DESCRIPTION** | 8.10 | 0.5    | The description of runners as saved in GitLab |
+| **CI_RUNNER_DESCRIPTION** | 8.10 | 0.5    | The description of the runner as saved in GitLab |
 | **CI_RUNNER_TAGS**      | 8.10   | 0.5    | The defined runner tags |
 
 **Some of the variables are only available when using runner with at least defined version.**
@@ -68,7 +68,7 @@ export CI_PROJECT_DIR="/builds/gitlab-org/gitlab-ce"
 export CI_PROJECT_NAME="gitlab-ce"
 export CI_PROJECT_NAMESPACE="gitlab-org"
 export CI_PROJECT_PATH="gitlab-org/gitlab-ce"
-export CI_PROJECT_URL="https://gitlab.com/gitlab-org/gitlab-ce.git"
+export CI_PROJECT_URL="https://gitlab.com/gitlab-org/gitlab-ce"
 export CI_REGISTRY="registry.gitlab.com"
 export CI_REGISTRY_IMAGE="registry.gitlab.com/gitlab-org/gitlab-ce"
 export CI_RUNNER_ID="10"
diff --git a/spec/models/build_spec.rb b/spec/models/build_spec.rb
index bff46d8cbab..f9c9725a23d 100644
--- a/spec/models/build_spec.rb
+++ b/spec/models/build_spec.rb
@@ -203,15 +203,15 @@ describe Ci::Build, models: true do
         { key: 'CI_BUILD_REF_NAME', value: 'master', public: true },
         { key: 'CI_BUILD_NAME', value: 'test', public: true },
         { key: 'CI_BUILD_STAGE', value: 'test', public: true },
-        { key: 'CI_PIPELINE_ID', value: pipeline.id.to_s, 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_PROJECT_ID', value: project.id.to_s, public: true },
         { key: 'CI_PROJECT_NAME', value: project.path, public: true },
         { key: 'CI_PROJECT_PATH', value: project.path_with_namespace, public: true },
         { key: 'CI_PROJECT_NAMESPACE', value: project.namespace.path, public: true },
         { key: 'CI_PROJECT_URL', value: project.web_url, 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: pipeline.id.to_s, public: true }
       ]
     end
 
@@ -362,12 +362,13 @@ describe Ci::Build, models: true do
     context 'returns variables in valid order' do
       before do
         allow(build).to receive(:predefined_variables) { ['predefined'] }
-        allow(build).to receive(:yaml_variables) { ['yaml variables'] }
-        allow(build).to receive(:project_variables) { ['secure variables'] }
-        allow(build).to receive(:trigger_variables) { ['trigger variables'] }
+        allow(project).to receive(:predefined_variables) { ['project'] }
+        allow(pipeline).to receive(:predefined_variables) { ['pipeline'] }
+        allow(build).to receive(:yaml_variables) { ['yaml'] }
+        allow(project).to receive(:secret_variables) { ['secret'] }
       end
 
-      it { is_expected.to eq(['predefined', 'yaml variables', 'secure variables', 'trigger variables']) }
+      it { is_expected.to eq(%w[predefined project pipeline yaml secret]) }
     end
   end