Merge branch 'make-before-after-overridable' into 'master'

Make before_script and after_script overridable

This is makes it possible to overwrite the before_script and after_script at job level.

This is continuation of https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/3771


See merge request !3772

4 files changed, 109 insertions, 18 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 1aebff54dfc..aa6b97bf051 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -52,6 +52,7 @@ v 8.7.0 (unreleased)
   - Use rugged to change HEAD in Project#change_head (P.S.V.R)
   - API: Ability to filter milestones by state `active` and `closed` (Robert Schilling)
   - API: Fix milestone filtering by `iid` (Robert Schilling)
+  - Make before_script and after_script overridable on per-job (Kamil Trzciński)
   - API: Delete notes of issues, snippets, and merge requests (Robert Schilling)
   - Implement 'Groups View' as an option for dashboard preferences !3379 (Elias W.)
   - Better errors handling when creating milestones inside groups
diff --git a/doc/ci/yaml/README.md b/doc/ci/yaml/README.md
index bd3ec9e4688..7e9bced7616 100644
--- a/doc/ci/yaml/README.md
+++ b/doc/ci/yaml/README.md
@@ -31,6 +31,7 @@ If you want a quick introduction to GitLab CI, follow our
     - [artifacts](#artifacts)
         - [artifacts:name](#artifacts-name)
     - [dependencies](#dependencies)
+    - [before_script and after_script](#before_script-and-after_script)
 - [Hidden jobs](#hidden-jobs)
 - [Special YAML features](#special-yaml-features)
     - [Anchors](#anchors)
@@ -349,6 +350,8 @@ job_name:
 | dependencies  | no | Define other builds that a build depends on so that you can pass artifacts between them|
 | artifacts     | no | Define list build artifacts |
 | cache         | no | Define list of files that should be cached between subsequent runs |
+| before_script | no | Override a set of commands that are executed before build |
+| after_script  | no | Override a set of commands that are executed after build |
 
 ### script
 
@@ -705,6 +708,23 @@ deploy:
   script: make deploy
 ```
 
+### before_script and after_script
+
+It's possible to overwrite globally defined `before_script` and `after_script`:
+
+```yaml
+before_script
+- global before script
+
+job:
+  before_script:
+  - execute this instead of global before script
+  script:
+  - my command
+  after_script:
+  - execute this after my script
+```
+
 ## Hidden jobs
 
 >**Note:**
diff --git a/lib/ci/gitlab_ci_yaml_processor.rb b/lib/ci/gitlab_ci_yaml_processor.rb
index d8a7fcfe15e..ff9887cba1e 100644
--- a/lib/ci/gitlab_ci_yaml_processor.rb
+++ b/lib/ci/gitlab_ci_yaml_processor.rb
@@ -7,7 +7,7 @@ module Ci
     ALLOWED_YAML_KEYS = [:before_script, :after_script, :image, :services, :types, :stages, :variables, :cache]
     ALLOWED_JOB_KEYS = [:tags, :script, :only, :except, :type, :image, :services,
                         :allow_failure, :type, :stage, :when, :artifacts, :cache,
-                        :dependencies, :variables]
+                        :dependencies, :before_script, :after_script, :variables]
 
     attr_reader :before_script, :after_script, :image, :services, :path, :cache
 
@@ -84,7 +84,7 @@ module Ci
       {
         stage_idx: stages.index(job[:stage]),
         stage: job[:stage],
-        commands: "#{@before_script.join("\n")}\n#{normalize_script(job[:script])}",
+        commands: [job[:before_script] || @before_script, job[:script]].flatten.join("\n"),
         tag_list: job[:tags] || [],
         name: name,
         only: job[:only],
@@ -97,19 +97,11 @@ module Ci
           artifacts: job[:artifacts],
           cache: job[:cache] || @cache,
           dependencies: job[:dependencies],
-          after_script: @after_script,
+          after_script: job[:after_script] || @after_script,
         }.compact
       }
     end
 
-    def normalize_script(script)
-      if script.is_a? Array
-        script.join("\n")
-      else
-        script
-      end
-    end
-
     def validate!
       validate_global!
 
@@ -168,6 +160,7 @@ module Ci
       validate_job_name!(name)
       validate_job_keys!(name, job)
       validate_job_types!(name, job)
+      validate_job_script!(name, job)
 
       validate_job_stage!(name, job) if job[:stage]
       validate_job_variables!(name, job) if job[:variables]
@@ -191,10 +184,6 @@ module Ci
     end
 
     def validate_job_types!(name, job)
-      if !validate_string(job[:script]) && !validate_array_of_strings(job[:script])
-        raise ValidationError, "#{name} job: script should be a string or an array of a strings"
-      end
-
       if job[:image] && !validate_string(job[:image])
         raise ValidationError, "#{name} job: image should be a string"
       end
@@ -224,6 +213,20 @@ module Ci
       end
     end
 
+    def validate_job_script!(name, job)
+      if !validate_string(job[:script]) && !validate_array_of_strings(job[:script])
+        raise ValidationError, "#{name} job: script should be a string or an array of a strings"
+      end
+
+      if job[:before_script] && !validate_array_of_strings(job[:before_script])
+        raise ValidationError, "#{name} job: before_script should be an array of strings"
+      end
+
+      if job[:after_script] && !validate_array_of_strings(job[:after_script])
+        raise ValidationError, "#{name} job: after_script should be an array of strings"
+      end
+    end
+
     def validate_job_stage!(name, job)
       unless job[:stage].is_a?(String) && job[:stage].in?(stages)
         raise ValidationError, "#{name} job: stage parameter should be #{stages.join(", ")}"
diff --git a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
index d317870f496..643acf0303c 100644
--- a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
+++ b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
@@ -293,6 +293,46 @@ module Ci
       
       subject { config_processor.builds_for_stage_and_ref("test", "master").first }
       
+      describe "before_script" do
+        context "in global context" do
+          let(:config) do
+            {
+              before_script: ["global script"],
+              test: { script: ["script"] }
+            }
+          end
+          
+          it "return commands with scripts concencaced" do
+            expect(subject[:commands]).to eq("global script\nscript")
+          end
+        end
+ 
+        context "overwritten in local context" do
+          let(:config) do
+            {
+              before_script: ["global script"],
+              test: { before_script: ["local script"], script: ["script"] }
+            }
+          end
+
+          it "return commands with scripts concencaced" do
+            expect(subject[:commands]).to eq("local script\nscript")
+          end
+        end
+      end
+
+      describe "script" do
+        let(:config) do
+          {
+            test: { script: ["script"] }
+          }
+        end
+
+        it "return commands with scripts concencaced" do
+          expect(subject[:commands]).to eq("script")
+        end
+      end
+
       describe "after_script" do
         context "in global context" do
           let(:config) do
@@ -306,6 +346,19 @@ module Ci
             expect(subject[:options][:after_script]).to eq(["after_script"])
           end
         end
+
+        context "overwritten in local context" do
+          let(:config) do
+            {
+              after_script: ["local after_script"],
+              test: { after_script: ["local after_script"], script: ["script"] }
+            }
+          end
+
+          it "return after_script in options" do
+            expect(subject[:options][:after_script]).to eq(["local after_script"])
+          end
+        end
       end
     end
 
@@ -614,7 +667,7 @@ module Ci
           stage_idx: 1,
           name: :normal_job,
           only: nil,
-          commands: "\ntest",
+          commands: "test",
           tag_list: [],
           options: {},
           when: "on_success",
@@ -641,7 +694,7 @@ EOT
           stage_idx: 1,
           name: :job1,
           only: nil,
-          commands: "\nexecute-script-for-job",
+          commands: "execute-script-for-job",
           tag_list: [],
           options: {},
           when: "on_success",
@@ -653,7 +706,7 @@ EOT
           stage_idx: 1,
           name: :job2,
           only: nil,
-          commands: "\nexecute-script-for-job",
+          commands: "execute-script-for-job",
           tag_list: [],
           options: {},
           when: "on_success",
@@ -685,6 +738,13 @@ EOT
         end.to raise_error(GitlabCiYamlProcessor::ValidationError, "before_script should be an array of strings")
       end
 
+      it "returns errors if job before_script parameter is not an array of strings" do
+        config = YAML.dump({ rspec: { script: "test", before_script: [10, "test"] } })
+        expect do
+          GitlabCiYamlProcessor.new(config, path)
+        end.to raise_error(GitlabCiYamlProcessor::ValidationError, "rspec job: before_script should be an array of strings")
+      end
+
       it "returns errors if after_script parameter is invalid" do
         config = YAML.dump({ after_script: "bundle update", rspec: { script: "test" } })
         expect do
@@ -692,6 +752,13 @@ EOT
         end.to raise_error(GitlabCiYamlProcessor::ValidationError, "after_script should be an array of strings")
       end
 
+      it "returns errors if job after_script parameter is not an array of strings" do
+        config = YAML.dump({ rspec: { script: "test", after_script: [10, "test"] } })
+        expect do
+          GitlabCiYamlProcessor.new(config, path)
+        end.to raise_error(GitlabCiYamlProcessor::ValidationError, "rspec job: after_script should be an array of strings")
+      end
+
       it "returns errors if image parameter is invalid" do
         config = YAML.dump({ image: ["test"], rspec: { script: "test" } })
         expect do