Merge branch '18969-allow-optional-caching-in-failed-builds' into 'master'

Allow optional caching in failed builds

See merge request gitlab-org/gitlab!41822

changelogs/unreleased/18969-allow-optional-caching-in-failed-builds.yml

+---
+title: Add cache:when keyword for ci yml config
+merge_request: 41822
+author:
+type: added

doc/ci/yaml/README.md

@@ -104,12 +104,12 @@ The following table lists available parameters for jobs:
 | [`script`](#script)                                | Shell script that is executed by a runner.                                                                                                                                           |
 | [`after_script`](#before_script-and-after_script)  | Override a set of commands that are executed after job.                                                                                                                             |
 | [`allow_failure`](#allow_failure)                  | Allow job to fail. Failed job does not contribute to commit status.                                                                                                                 |
-| [`artifacts`](#artifacts)                          | List of files and directories to attach to a job on success. Also available: `artifacts:paths`, `artifacts:exclude`, `artifacts:expose_as`, `artifacts:name`, `artifacts:untracked`, `artifacts:when`, `artifacts:expire_in`, `artifacts:reports`. |
+| [`artifacts`](#artifacts)                          | List of files and directories to attach to a job on success. Also available: `artifacts:paths`, `artifacts:exclude`, `artifacts:expose_as`, `artifacts:name`, `artifacts:untracked`, `artifacts:when`, `artifacts:expire_in`, and `artifacts:reports`. |
 | [`before_script`](#before_script-and-after_script) | Override a set of commands that are executed before job.                                                                                                                            |
-| [`cache`](#cache)                                  | List of files that should be cached between subsequent runs. Also available: `cache:paths`, `cache:key`, `cache:untracked`, and `cache:policy`.                                     |
+| [`cache`](#cache)                                  | List of files that should be cached between subsequent runs. Also available: `cache:paths`, `cache:key`, `cache:untracked`, `cache:when`, and `cache:policy`.                                     |
 | [`coverage`](#coverage)                            | Code coverage settings for a given job.                                                                                                                                             |
 | [`dependencies`](#dependencies)                    | Restrict which artifacts are passed to a specific job by providing a list of jobs to fetch artifacts from.                                                                          |
-| [`environment`](#environment)                      | Name of an environment to which the job deploys. Also available: `environment:name`, `environment:url`, `environment:on_stop`, `environment:auto_stop_in` and `environment:action`. |
+| [`environment`](#environment)                      | Name of an environment to which the job deploys. Also available: `environment:name`, `environment:url`, `environment:on_stop`, `environment:auto_stop_in`, and `environment:action`. |
 | [`except`](#onlyexcept-basic)                      | Limit when jobs are not created. Also available: [`except:refs`, `except:kubernetes`, `except:variables`, and `except:changes`](#onlyexcept-advanced).                              |
 | [`extends`](#extends)                              | Configuration entries that this job inherits from.                                                                                                                       |
 | [`image`](#image)                                  | Use Docker images. Also available: `image:name` and `image:entrypoint`.                                                                                                             |
@@ -2914,6 +2914,28 @@ rspec:
       - binaries/
 ```
 
+#### `cache:when`
+
+> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/18969) in GitLab 13.5 and GitLab Runner v13.5.0.
+
+`cache:when` defines when to save the cache, based on the status of the job. You can
+set `cache:when` to:
+
+- `on_success` - save the cache only when the job succeeds. This is the default.
+- `on_failure` - save the cache only when the job fails.
+- `always` - save the cache regardless of the job status.
+
+For example, to store a cache whether or not the job fails or succeeds:
+
+```yaml
+rspec:
+  script: rspec
+  cache:
+    paths:
+      - rspec/
+    when: 'always'
+```
+
 #### `cache:policy`
 
 > Introduced in GitLab 9.4.
@@ -3236,7 +3258,7 @@ failure.
 1. `on_failure` - upload artifacts only when the job fails.
 1. `always` - upload artifacts regardless of the job status.
 
-To upload artifacts only when job fails:
+For example, to upload artifacts only when a job fails:
 
 ```yaml
 job:

lib/api/entities/job_request/cache.rb

@@ -4,7 +4,7 @@ module API
   module Entities
     module JobRequest
       class Cache < Grape::Entity
-        expose :key, :untracked, :paths, :policy
+        expose :key, :untracked, :paths, :policy, :when
       end
     end
   end

lib/gitlab/ci/config/entry/cache.rb

@@ -9,14 +9,28 @@ module Gitlab
         #
         class Cache < ::Gitlab::Config::Entry::Node
           include ::Gitlab::Config::Entry::Configurable
+          include ::Gitlab::Config::Entry::Validatable
           include ::Gitlab::Config::Entry::Attributable
 
-          ALLOWED_KEYS = %i[key untracked paths policy].freeze
+          ALLOWED_KEYS = %i[key untracked paths when policy].freeze
+          ALLOWED_POLICY = %w[pull-push push pull].freeze
           DEFAULT_POLICY = 'pull-push'
+          ALLOWED_WHEN = %w[on_success on_failure always].freeze
+          DEFAULT_WHEN = 'on_success'
 
           validations do
-            validates :config, allowed_keys: ALLOWED_KEYS
-            validates :policy, inclusion: { in: %w[pull-push push pull], message: 'should be pull-push, push, or pull' }, allow_blank: true
+            validates :config, type: Hash, allowed_keys: ALLOWED_KEYS
+            validates :policy,
+              inclusion: { in: ALLOWED_POLICY, message: 'should be pull-push, push, or pull' },
+              allow_blank: true
+
+            with_options allow_nil: true do
+              validates :when,
+                  inclusion: {
+                    in: ALLOWED_WHEN,
+                    message: 'should be on_success, on_failure or always'
+                  }
+            end
           end
 
           entry :key, Entry::Key,
@@ -28,13 +42,15 @@ module Gitlab
           entry :paths, Entry::Paths,
             description: 'Specify which paths should be cached across builds.'
 
-          attributes :policy
+          attributes :policy, :when
 
           def value
             result = super
 
             result[:key] = key_value
             result[:policy] = policy || DEFAULT_POLICY
+            # Use self.when to avoid conflict with reserved word
+            result[:when] = self.when || DEFAULT_WHEN
 
             result
           end

lib/gitlab/ci/pipeline/seed/build/cache.rb

@@ -13,6 +13,7 @@ module Gitlab
               @paths = local_cache.delete(:paths)
               @policy = local_cache.delete(:policy)
               @untracked = local_cache.delete(:untracked)
+              @when = local_cache.delete(:when)
 
               raise ArgumentError, "unknown cache keys: #{local_cache.keys}" if local_cache.any?
             end
@@ -24,7 +25,8 @@ module Gitlab
                     key: key_string,
                     paths: @paths,
                     policy: @policy,
-                    untracked: @untracked
+                    untracked: @untracked,
+                    when: @when
                   }.compact.presence
                 }.compact
               }

spec/factories/ci/builds.rb

@@ -384,7 +384,8 @@ FactoryBot.define do
             key: 'cache_key',
             untracked: false,
             paths: ['vendor/*'],
-            policy: 'pull-push'
+            policy: 'pull-push',
+            when: 'on_success'
           }
         }
       end

spec/lib/gitlab/ci/config/entry/cache_spec.rb

@@ -13,18 +13,23 @@ RSpec.describe Gitlab::Ci::Config::Entry::Cache do
     context 'when entry config value is correct' do
       let(:policy) { nil }
       let(:key) { 'some key' }
+      let(:when_config) { nil }
 
       let(:config) do
-        { key: key,
+        {
+          key: key,
           untracked: true,
-          paths: ['some/path/'],
-          policy: policy }
+          paths: ['some/path/']
+        }.tap do |config|
+          config[:policy] = policy if policy
+          config[:when] = when_config if when_config
+        end
       end
 
       describe '#value' do
         shared_examples 'hash key value' do
           it 'returns hash value' do
-            expect(entry.value).to eq(key: key, untracked: true, paths: ['some/path/'], policy: 'pull-push')
+            expect(entry.value).to eq(key: key, untracked: true, paths: ['some/path/'], policy: 'pull-push', when: 'on_success')
           end
         end
 
@@ -49,6 +54,48 @@ RSpec.describe Gitlab::Ci::Config::Entry::Cache do
             expect(entry.value).to match(a_hash_including(key: nil))
           end
         end
+
+        context 'with `policy`' do
+          using RSpec::Parameterized::TableSyntax
+
+          where(:policy, :result) do
+            'pull-push' | 'pull-push'
+            'push'      | 'push'
+            'pull'      | 'pull'
+            'unknown'   | 'unknown' # invalid
+          end
+
+          with_them do
+            it { expect(entry.value).to include(policy: result) }
+          end
+        end
+
+        context 'without `policy`' do
+          it 'assigns policy to default' do
+            expect(entry.value).to include(policy: 'pull-push')
+          end
+        end
+
+        context 'with `when`' do
+          using RSpec::Parameterized::TableSyntax
+
+          where(:when_config, :result) do
+            'on_success' | 'on_success'
+            'on_failure' | 'on_failure'
+            'always'     | 'always'
+            'unknown'    | 'unknown' # invalid
+          end
+
+          with_them do
+            it { expect(entry.value).to include(when: result) }
+          end
+        end
+
+        context 'without `when`' do
+          it 'assigns when to default' do
+            expect(entry.value).to include(when: 'on_success')
+          end
+        end
       end
 
       describe '#valid?' do
@@ -61,28 +108,41 @@ RSpec.describe Gitlab::Ci::Config::Entry::Cache do
         end
       end
 
-      context 'policy is pull-push' do
-        let(:policy) { 'pull-push' }
+      context 'with `policy`' do
+        using RSpec::Parameterized::TableSyntax
 
-        it { is_expected.to be_valid }
-        it { expect(entry.value).to include(policy: 'pull-push') }
+        where(:policy, :valid) do
+          'pull-push' | true
+          'push'      | true
+          'pull'      | true
+          'unknown'   | false
         end
 
-      context 'policy is push' do
-        let(:policy) { 'push' }
-
-        it { is_expected.to be_valid }
-        it { expect(entry.value).to include(policy: 'push') }
+        with_them do
+          it 'returns expected validity' do
+            expect(entry.valid?).to eq(valid)
+          end
+        end
       end
 
-      context 'policy is pull' do
-        let(:policy) { 'pull' }
+      context 'with `when`' do
+        using RSpec::Parameterized::TableSyntax
 
-        it { is_expected.to be_valid }
-        it { expect(entry.value).to include(policy: 'pull') }
+        where(:when_config, :valid) do
+          'on_success' | true
+          'on_failure' | true
+          'always'     | true
+          'unknown'    | false
         end
 
-      context 'when key is missing' do
+        with_them do
+          it 'returns expected validity' do
+            expect(entry.valid?).to eq(valid)
+          end
+        end
+      end
+
+      context 'with key missing' do
         let(:config) do
           { untracked: true,
             paths: ['some/path/'] }
@@ -110,13 +170,21 @@ RSpec.describe Gitlab::Ci::Config::Entry::Cache do
         end
 
         context 'when policy is unknown' do
-          let(:config) { { policy: "unknown" } }
+          let(:config) { { policy: 'unknown' } }
 
           it 'reports error' do
             is_expected.to include('cache policy should be pull-push, push, or pull')
           end
         end
 
+        context 'when `when` is unknown' do
+          let(:config) { { when: 'unknown' } }
+
+          it 'reports error' do
+            is_expected.to include('cache when should be on_success, on_failure or always')
+          end
+        end
+
         context 'when descendants are invalid' do
           context 'with invalid keys' do
             let(:config) { { key: 1 } }

spec/lib/gitlab/ci/config/entry/job_spec.rb

@@ -537,7 +537,7 @@ RSpec.describe Gitlab::Ci::Config::Entry::Job do
 
       it 'overrides default config' do
         expect(entry[:image].value).to eq(name: 'some_image')
-        expect(entry[:cache].value).to eq(key: 'test', policy: 'pull-push')
+        expect(entry[:cache].value).to eq(key: 'test', policy: 'pull-push', when: 'on_success')
       end
     end
 
@@ -552,7 +552,7 @@ RSpec.describe Gitlab::Ci::Config::Entry::Job do
 
       it 'uses config from default entry' do
         expect(entry[:image].value).to eq 'specified'
-        expect(entry[:cache].value).to eq(key: 'test', policy: 'pull-push')
+        expect(entry[:cache].value).to eq(key: 'test', policy: 'pull-push', when: 'on_success')
       end
     end
 

spec/lib/gitlab/ci/config/entry/root_spec.rb

@@ -127,7 +127,7 @@ RSpec.describe Gitlab::Ci::Config::Entry::Root do
                        image: { name: 'ruby:2.7' },
                        services: [{ name: 'postgres:9.1' }, { name: 'mysql:5.5' }],
                        stage: 'test',
-                       cache: { key: 'k', untracked: true, paths: ['public/'], policy: 'pull-push' },
+                       cache: { key: 'k', untracked: true, paths: ['public/'], policy: 'pull-push', when: 'on_success' },
                        variables: { 'VAR' => 'root' },
                        ignore: false,
                        after_script: ['make clean'],
@@ -141,7 +141,7 @@ RSpec.describe Gitlab::Ci::Config::Entry::Root do
                          image: { name: 'ruby:2.7' },
                          services: [{ name: 'postgres:9.1' }, { name: 'mysql:5.5' }],
                          stage: 'test',
-                         cache: { key: 'k', untracked: true, paths: ['public/'], policy: 'pull-push' },
+                         cache: { key: 'k', untracked: true, paths: ['public/'], policy: 'pull-push', when: 'on_success' },
                          variables: { 'VAR' => 'root' },
                          ignore: false,
                          after_script: ['make clean'],
@@ -156,7 +156,7 @@ RSpec.describe Gitlab::Ci::Config::Entry::Root do
                          release: { name: "Release $CI_TAG_NAME", tag_name: 'v0.06', description: "./release_changelog.txt" },
                          image: { name: "ruby:2.7" },
                          services: [{ name: "postgres:9.1" }, { name: "mysql:5.5" }],
-                         cache: { key: "k", untracked: true, paths: ["public/"], policy: "pull-push" },
+                         cache: { key: "k", untracked: true, paths: ["public/"], policy: "pull-push", when: 'on_success' },
                          only: { refs: %w(branches tags) },
                          variables: { 'VAR' => 'job' },
                          after_script: [],
@@ -203,7 +203,7 @@ RSpec.describe Gitlab::Ci::Config::Entry::Root do
                        image: { name: 'ruby:2.7' },
                        services: [{ name: 'postgres:9.1' }, { name: 'mysql:5.5' }],
                        stage: 'test',
-                       cache: { key: 'k', untracked: true, paths: ['public/'], policy: "pull-push" },
+                       cache: { key: 'k', untracked: true, paths: ['public/'], policy: 'pull-push', when: 'on_success' },
                        variables: { 'VAR' => 'root' },
                        ignore: false,
                        after_script: ['make clean'],
@@ -215,7 +215,7 @@ RSpec.describe Gitlab::Ci::Config::Entry::Root do
                          image: { name: 'ruby:2.7' },
                          services: [{ name: 'postgres:9.1' }, { name: 'mysql:5.5' }],
                          stage: 'test',
-                         cache: { key: 'k', untracked: true, paths: ['public/'], policy: "pull-push" },
+                         cache: { key: 'k', untracked: true, paths: ['public/'], policy: 'pull-push', when: 'on_success' },
                          variables: { 'VAR' => 'job' },
                          ignore: false,
                          after_script: ['make clean'],
@@ -261,7 +261,7 @@ RSpec.describe Gitlab::Ci::Config::Entry::Root do
 
       describe '#cache_value' do
         it 'returns correct cache definition' do
-          expect(root.cache_value).to eq(key: 'a', policy: 'pull-push')
+          expect(root.cache_value).to eq(key: 'a', policy: 'pull-push', when: 'on_success')
         end
       end
     end

spec/lib/gitlab/ci/pipeline/seed/build/cache_spec.rb

@@ -224,7 +224,8 @@ RSpec.describe Gitlab::Ci::Pipeline::Seed::Build::Cache do
           key: 'a-key',
           paths: ['vendor/ruby'],
           untracked: true,
-          policy: 'push'
+          policy: 'push',
+          when: 'on_success'
         }
       end
 

spec/lib/gitlab/ci/yaml_processor_spec.rb

@@ -1361,7 +1361,8 @@ module Gitlab
             paths: ["logs/", "binaries/"],
             untracked: true,
             key: 'key',
-            policy: 'pull-push'
+            policy: 'pull-push',
+            when: 'on_success'
           )
         end
 
@@ -1383,7 +1384,8 @@ module Gitlab
             paths: ["logs/", "binaries/"],
             untracked: true,
             key: { files: ['file'] },
-            policy: 'pull-push'
+            policy: 'pull-push',
+            when: 'on_success'
           )
         end
 
@@ -1402,7 +1404,8 @@ module Gitlab
             paths: ['logs/', 'binaries/'],
             untracked: true,
             key: 'key',
-            policy: 'pull-push'
+            policy: 'pull-push',
+            when: 'on_success'
           )
         end
 
@@ -1425,7 +1428,8 @@ module Gitlab
             paths: ['logs/', 'binaries/'],
             untracked: true,
             key: { files: ['file'] },
-            policy: 'pull-push'
+            policy: 'pull-push',
+            when: 'on_success'
           )
         end
 
@@ -1448,7 +1452,8 @@ module Gitlab
             paths: ['logs/', 'binaries/'],
             untracked: true,
             key: { files: ['file'], prefix: 'prefix' },
-            policy: 'pull-push'
+            policy: 'pull-push',
+            when: 'on_success'
           )
         end
 
@@ -1468,7 +1473,8 @@ module Gitlab
             paths: ["test/"],
             untracked: false,
             key: 'local',
-            policy: 'pull-push'
+            policy: 'pull-push',
+            when: 'on_success'
           )
         end
       end

spec/requests/api/ci/runner/jobs_request_post_spec.rb

@@ -194,7 +194,8 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
             [{ 'key' => 'cache_key',
                'untracked' => false,
                'paths' => ['vendor/*'],
-               'policy' => 'pull-push' }]
+               'policy' => 'pull-push',
+               'when' => 'on_success' }]
           end
 
           let(:expected_features) { { 'trace_sections' => true } }

spec/services/ci/create_pipeline_service/cache_spec.rb

@@ -36,7 +36,8 @@ RSpec.describe Ci::CreatePipelineService do
           'key'       => 'a-key',
           'paths'     => ['logs/', 'binaries/'],
           'policy'    => 'pull-push',
-          'untracked' => true
+          'untracked' => true,
+          'when'      => 'on_success'
         }
 
         expect(pipeline).to be_persisted
@@ -67,7 +68,8 @@ RSpec.describe Ci::CreatePipelineService do
           expected = {
             'key'       => /[a-f0-9]{40}/,
             'paths'     => ['logs/'],
-            'policy'    => 'pull-push'
+            'policy'    => 'pull-push',
+            'when'      => 'on_success'
           }
 
           expect(pipeline).to be_persisted
@@ -82,7 +84,8 @@ RSpec.describe Ci::CreatePipelineService do
           expected = {
             'key'       => /default/,
             'paths'     => ['logs/'],
-            'policy'    => 'pull-push'
+            'policy'    => 'pull-push',
+            'when'      => 'on_success'
           }
 
           expect(pipeline).to be_persisted
@@ -114,7 +117,8 @@ RSpec.describe Ci::CreatePipelineService do
           expected = {
             'key'       => /\$ENV_VAR-[a-f0-9]{40}/,
             'paths'     => ['logs/'],
-            'policy'    => 'pull-push'
+            'policy'    => 'pull-push',
+            'when'      => 'on_success'
           }
 
           expect(pipeline).to be_persisted
@@ -129,7 +133,8 @@ RSpec.describe Ci::CreatePipelineService do
           expected = {
             'key'       => /\$ENV_VAR-default/,
             'paths'     => ['logs/'],
-            'policy'    => 'pull-push'
+            'policy'    => 'pull-push',
+            'when'      => 'on_success'
           }
 
           expect(pipeline).to be_persisted