Merge branch 'artifacts-when' into 'master'

Added when to artifacts

The syntax for this will be:

```
job:
  artifacts:
    when: on_failure
```

cc @grzesiek @tmaczukin 

Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/4034


See merge request !4201

4 files changed, 78 insertions, 1 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 546aaaae0c9..7a6a14919da 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -37,6 +37,7 @@ v 8.9.0 (unreleased)
   - Links from a wiki page to other wiki pages should be rewritten as expected
   - Add option to project to only allow merge requests to be merged if the build succeeds (Rui Santos)
   - Fix issues filter when ordering by milestone
+  - Added artifacts:when to .gitlab-ci.yml - this requires GitLab Runner 1.3
   - Todos will display target state if issuable target is 'Closed' or 'Merged'
   - Fix bug when sorting issues by milestone due date and filtering by two or more labels
   - Add support for using Yubikeys (U2F) for two-factor authentication
diff --git a/doc/ci/yaml/README.md b/doc/ci/yaml/README.md
index a3481f58c6c..0707555e393 100644
--- a/doc/ci/yaml/README.md
+++ b/doc/ci/yaml/README.md
@@ -30,6 +30,7 @@ If you want a quick introduction to GitLab CI, follow our
     - [when](#when)
     - [artifacts](#artifacts)
         - [artifacts:name](#artifacts-name)
+        - [artifacts:when](#artifacts-when)
     - [dependencies](#dependencies)
     - [before_script and after_script](#before_script-and-after_script)
 - [Hidden jobs](#hidden-jobs)
@@ -651,6 +652,32 @@ job:
     untracked: true
 ```
 
+#### artifacts:when
+
+>**Note:**
+Introduced in GitLab 8.9 and GitLab Runner v1.3.0.
+
+`artifacts:when` is used to upload artifacts on build failure or despite the
+failure.
+
+`artifacts:when` can be set to one of the following values:
+
+1. `on_success` - upload artifacts only when build succeeds. This is the default
+1. `on_failure` - upload artifacts only when build fails
+1. `always` - upload artifacts despite the build status
+
+---
+
+**Example configurations**
+
+To upload artifacts only when build fails.
+
+```yaml
+job:
+  artifacts:
+    when: on_failure
+```
+
 ### dependencies
 
 >**Note:**
diff --git a/lib/ci/gitlab_ci_yaml_processor.rb b/lib/ci/gitlab_ci_yaml_processor.rb
index 130f5b0892e..40a5d180fd0 100644
--- a/lib/ci/gitlab_ci_yaml_processor.rb
+++ b/lib/ci/gitlab_ci_yaml_processor.rb
@@ -8,6 +8,8 @@ module Ci
     ALLOWED_JOB_KEYS = [:tags, :script, :only, :except, :type, :image, :services,
                         :allow_failure, :type, :stage, :when, :artifacts, :cache,
                         :dependencies, :before_script, :after_script, :variables]
+    ALLOWED_CACHE_KEYS = [:key, :untracked, :paths]
+    ALLOWED_ARTIFACTS_KEYS = [:name, :untracked, :paths, :when]
 
     attr_reader :before_script, :after_script, :image, :services, :path, :cache
 
@@ -135,6 +137,12 @@ module Ci
     end
 
     def validate_global_cache!
+      @cache.keys.each do |key|
+        unless ALLOWED_CACHE_KEYS.include? key
+          raise ValidationError, "#{name} cache unknown parameter #{key}"
+        end
+      end
+
       if @cache[:key] && !validate_string(@cache[:key])
         raise ValidationError, "cache:key parameter should be a string"
       end
@@ -200,7 +208,7 @@ module Ci
         raise ValidationError, "#{name} job: allow_failure parameter should be an boolean"
       end
 
-      if job[:when] && !job[:when].in?(%w(on_success on_failure always))
+      if job[:when] && !job[:when].in?(%w[on_success on_failure always])
         raise ValidationError, "#{name} job: when parameter should be on_success, on_failure or always"
       end
     end
@@ -233,6 +241,12 @@ module Ci
     end
 
     def validate_job_cache!(name, job)
+      job[:cache].keys.each do |key|
+        unless ALLOWED_CACHE_KEYS.include? key
+          raise ValidationError, "#{name} job: cache unknown parameter #{key}"
+        end
+      end
+
       if job[:cache][:key] && !validate_string(job[:cache][:key])
         raise ValidationError, "#{name} job: cache:key parameter should be a string"
       end
@@ -247,6 +261,12 @@ module Ci
     end
 
     def validate_job_artifacts!(name, job)
+      job[:artifacts].keys.each do |key|
+        unless ALLOWED_ARTIFACTS_KEYS.include? key
+          raise ValidationError, "#{name} job: artifacts unknown parameter #{key}"
+        end
+      end
+
       if job[:artifacts][:name] && !validate_string(job[:artifacts][:name])
         raise ValidationError, "#{name} job: artifacts:name parameter should be a string"
       end
@@ -258,6 +278,10 @@ module Ci
       if job[:artifacts][:paths] && !validate_array_of_strings(job[:artifacts][:paths])
         raise ValidationError, "#{name} job: artifacts:paths parameter should be an array of strings"
       end
+
+      if job[:artifacts][:when] && !job[:artifacts][:when].in?(%w[on_success on_failure always])
+        raise ValidationError, "#{name} job: artifacts:when parameter should be on_success, on_failure or always"
+      end
     end
 
     def validate_job_dependencies!(name, job)
diff --git a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
index 7375539cf17..304290d6608 100644
--- a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
+++ b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
@@ -501,6 +501,7 @@ module Ci
                              })
 
           config_processor = GitlabCiYamlProcessor.new(config, path)
+
           builds = config_processor.builds_for_stage_and_ref("test", "master")
           expect(builds.size).to eq(1)
           expect(builds.first[:when]).to eq(when_state)
@@ -601,6 +602,23 @@ module Ci
           allow_failure: false
         })
       end
+
+      %w[on_success on_failure always].each do |when_state|
+        it "returns artifacts for when #{when_state}  defined" do
+          config = YAML.dump({
+                               rspec: {
+                                 script: "rspec",
+                                 artifacts: { paths: ["logs/", "binaries/"], when: when_state }
+                               }
+                             })
+
+          config_processor = GitlabCiYamlProcessor.new(config, path)
+
+          builds = config_processor.builds_for_stage_and_ref("test", "master")
+          expect(builds.size).to eq(1)
+          expect(builds.first[:options][:artifacts][:when]).to eq(when_state)
+        end
+      end
     end
 
     describe "Dependencies" do
@@ -967,6 +985,13 @@ EOT
         end.to raise_error(GitlabCiYamlProcessor::ValidationError, "rspec job: artifacts:name parameter should be a string")
       end
 
+      it "returns errors if job artifacts:when is not an a predefined value" do
+        config = YAML.dump({ types: ["build", "test"], rspec: { script: "test", artifacts: { when: 1 } } })
+        expect do
+          GitlabCiYamlProcessor.new(config)
+        end.to raise_error(GitlabCiYamlProcessor::ValidationError, "rspec job: artifacts:when parameter should be on_success, on_failure or always")
+      end
+
       it "returns errors if job artifacts:untracked is not an array of strings" do
         config = YAML.dump({ types: ["build", "test"], rspec: { script: "test", artifacts: { untracked: "string" } } })
         expect do