From 6e032d7ba0cd4ba7053f51dc4e14e0bd69d41217 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 2 Mar 2017 22:12:15 +0800
Subject: [PATCH] Set default cache key for jobs, detail:

* Replace Unspecified with a field so that it's less surprising
* Define inspect for Node for easy debugging (and avoid building
  a very huge string potentially from built-in inspect)
* Set default cache key to 'default'
---
 lib/gitlab/ci/config/entry/cache.rb           | 10 ++++++
 lib/gitlab/ci/config/entry/factory.rb         |  6 ++--
 lib/gitlab/ci/config/entry/key.rb             |  4 +++
 lib/gitlab/ci/config/entry/node.rb            | 13 +++++++-
 lib/gitlab/ci/config/entry/undefined.rb       |  4 +++
 lib/gitlab/ci/config/entry/unspecified.rb     | 19 -----------
 spec/lib/gitlab/ci/config/entry/cache_spec.rb | 15 +++++++++
 .../gitlab/ci/config/entry/factory_spec.rb    |  4 +--
 .../lib/gitlab/ci/config/entry/global_spec.rb |  2 +-
 spec/lib/gitlab/ci/config/entry/job_spec.rb   |  3 +-
 spec/lib/gitlab/ci/config/entry/jobs_spec.rb  |  6 ++--
 spec/lib/gitlab/ci/config/entry/key_spec.rb   |  6 ++++
 .../ci/config/entry/unspecified_spec.rb       | 32 -------------------
 13 files changed, 62 insertions(+), 62 deletions(-)
 delete mode 100644 lib/gitlab/ci/config/entry/unspecified.rb
 delete mode 100644 spec/lib/gitlab/ci/config/entry/unspecified_spec.rb

diff --git a/lib/gitlab/ci/config/entry/cache.rb b/lib/gitlab/ci/config/entry/cache.rb
index 066643ccfcc..4378768d0de 100644
--- a/lib/gitlab/ci/config/entry/cache.rb
+++ b/lib/gitlab/ci/config/entry/cache.rb
@@ -22,6 +22,16 @@ module Gitlab
 
           entry :paths, Entry::Paths,
             description: 'Specify which paths should be cached across builds.'
+
+          helpers :key
+
+          def self.default
+            { key: Entry::Key.default }
+          end
+
+          def value
+            super.merge(key: key_value)
+          end
         end
       end
     end
diff --git a/lib/gitlab/ci/config/entry/factory.rb b/lib/gitlab/ci/config/entry/factory.rb
index 6be8288748f..3b6d10a5378 100644
--- a/lib/gitlab/ci/config/entry/factory.rb
+++ b/lib/gitlab/ci/config/entry/factory.rb
@@ -37,9 +37,7 @@ module Gitlab
             # See issue #18775.
             #
             if @value.nil?
-              Entry::Unspecified.new(
-                fabricate_unspecified
-              )
+              fabricate_unspecified
             else
               fabricate(@entry, @value)
             end
@@ -56,7 +54,7 @@ module Gitlab
               fabricate(Entry::Undefined)
             else
               fabricate(@entry, @entry.default)
-            end
+            end.tap(&:unspecify)
           end
 
           def fabricate(entry, value = nil)
diff --git a/lib/gitlab/ci/config/entry/key.rb b/lib/gitlab/ci/config/entry/key.rb
index 0e4c9fe6edc..f27ad0a7759 100644
--- a/lib/gitlab/ci/config/entry/key.rb
+++ b/lib/gitlab/ci/config/entry/key.rb
@@ -11,6 +11,10 @@ module Gitlab
           validations do
             validates :config, key: true
           end
+
+          def self.default
+            'default'
+          end
         end
       end
     end
diff --git a/lib/gitlab/ci/config/entry/node.rb b/lib/gitlab/ci/config/entry/node.rb
index 55a5447ab51..5c745babcb9 100644
--- a/lib/gitlab/ci/config/entry/node.rb
+++ b/lib/gitlab/ci/config/entry/node.rb
@@ -15,6 +15,7 @@ module Gitlab
             @config = config
             @metadata = metadata
             @entries = {}
+            @specified = true
 
             @validator = self.class.validator.new(self)
             @validator.validate(:new)
@@ -62,14 +63,24 @@ module Gitlab
             end
           end
 
+          def unspecify
+            @specified = false
+          end
+
           def specified?
-            true
+            @specified
           end
 
           def relevant?
             true
           end
 
+          def inspect
+            val = if leaf? then config else descendants end
+            unspecified = if specified? then '' else '(unspecified) ' end
+            "#<#{self.class.name} #{unspecified}{#{key}: #{val.inspect}}>"
+          end
+
           def self.default
           end
 
diff --git a/lib/gitlab/ci/config/entry/undefined.rb b/lib/gitlab/ci/config/entry/undefined.rb
index b33b8238230..1171ac10f22 100644
--- a/lib/gitlab/ci/config/entry/undefined.rb
+++ b/lib/gitlab/ci/config/entry/undefined.rb
@@ -29,6 +29,10 @@ module Gitlab
           def relevant?
             false
           end
+
+          def inspect
+            "#<#{self.class.name}>"
+          end
         end
       end
     end
diff --git a/lib/gitlab/ci/config/entry/unspecified.rb b/lib/gitlab/ci/config/entry/unspecified.rb
deleted file mode 100644
index fbb2551e870..00000000000
--- a/lib/gitlab/ci/config/entry/unspecified.rb
+++ /dev/null
@@ -1,19 +0,0 @@
-module Gitlab
-  module Ci
-    class Config
-      module Entry
-        ##
-        # This class represents an unspecified entry.
-        #
-        # It decorates original entry adding method that indicates it is
-        # unspecified.
-        #
-        class Unspecified < SimpleDelegator
-          def specified?
-            false
-          end
-        end
-      end
-    end
-  end
-end
diff --git a/spec/lib/gitlab/ci/config/entry/cache_spec.rb b/spec/lib/gitlab/ci/config/entry/cache_spec.rb
index 70a327c5183..37abbc8a498 100644
--- a/spec/lib/gitlab/ci/config/entry/cache_spec.rb
+++ b/spec/lib/gitlab/ci/config/entry/cache_spec.rb
@@ -24,6 +24,21 @@ describe Gitlab::Ci::Config::Entry::Cache do
           expect(entry).to be_valid
         end
       end
+
+      context 'when key is missing' do
+        let(:config) do
+          { untracked: true,
+            paths: ['some/path/'] }
+        end
+
+        describe '#value' do
+          it 'sets key with the default' do
+            value = config.merge(key: Gitlab::Ci::Config::Entry::Key.default)
+
+            expect(entry.value).to eq(value)
+          end
+        end
+      end
     end
 
     context 'when entry value is not correct' do
diff --git a/spec/lib/gitlab/ci/config/entry/factory_spec.rb b/spec/lib/gitlab/ci/config/entry/factory_spec.rb
index 3395b3c645b..8dd48e4efae 100644
--- a/spec/lib/gitlab/ci/config/entry/factory_spec.rb
+++ b/spec/lib/gitlab/ci/config/entry/factory_spec.rb
@@ -60,13 +60,13 @@ describe Gitlab::Ci::Config::Entry::Factory do
     end
 
     context 'when creating entry with nil value' do
-      it 'creates an undefined entry' do
+      it 'creates an unspecified entry' do
         entry = factory
           .value(nil)
           .create!
 
         expect(entry)
-          .to be_an_instance_of Gitlab::Ci::Config::Entry::Unspecified
+          .not_to be_specified
       end
     end
 
diff --git a/spec/lib/gitlab/ci/config/entry/global_spec.rb b/spec/lib/gitlab/ci/config/entry/global_spec.rb
index ebd80ac5e1d..f9f43cae5e0 100644
--- a/spec/lib/gitlab/ci/config/entry/global_spec.rb
+++ b/spec/lib/gitlab/ci/config/entry/global_spec.rb
@@ -186,7 +186,7 @@ describe Gitlab::Ci::Config::Entry::Global do
 
         it 'contains unspecified nodes' do
           expect(global.descendants.first)
-            .to be_an_instance_of Gitlab::Ci::Config::Entry::Unspecified
+            .not_to be_specified
         end
       end
 
diff --git a/spec/lib/gitlab/ci/config/entry/job_spec.rb b/spec/lib/gitlab/ci/config/entry/job_spec.rb
index d20f4ec207d..4df119cf00e 100644
--- a/spec/lib/gitlab/ci/config/entry/job_spec.rb
+++ b/spec/lib/gitlab/ci/config/entry/job_spec.rb
@@ -144,7 +144,8 @@ describe Gitlab::Ci::Config::Entry::Job do
                    script: %w[rspec],
                    commands: "ls\npwd\nrspec",
                    stage: 'test',
-                   after_script: %w[cleanup])
+                   after_script: %w[cleanup],
+                   cache: { key: 'default' })
         end
       end
     end
diff --git a/spec/lib/gitlab/ci/config/entry/jobs_spec.rb b/spec/lib/gitlab/ci/config/entry/jobs_spec.rb
index aaebf783962..f0d12211483 100644
--- a/spec/lib/gitlab/ci/config/entry/jobs_spec.rb
+++ b/spec/lib/gitlab/ci/config/entry/jobs_spec.rb
@@ -62,11 +62,13 @@ describe Gitlab::Ci::Config::Entry::Jobs do
           rspec: { name: :rspec,
                    script: %w[rspec],
                    commands: 'rspec',
-                   stage: 'test' },
+                   stage: 'test',
+                   cache: { key: 'default' } },
           spinach: { name: :spinach,
                      script: %w[spinach],
                      commands: 'spinach',
-                     stage: 'test' })
+                     stage: 'test',
+                     cache: { key: 'default' } })
       end
     end
 
diff --git a/spec/lib/gitlab/ci/config/entry/key_spec.rb b/spec/lib/gitlab/ci/config/entry/key_spec.rb
index 0dd36fe1f44..5d4de60bc8a 100644
--- a/spec/lib/gitlab/ci/config/entry/key_spec.rb
+++ b/spec/lib/gitlab/ci/config/entry/key_spec.rb
@@ -31,4 +31,10 @@ describe Gitlab::Ci::Config::Entry::Key do
       end
     end
   end
+
+  describe '.default' do
+    it 'returns default key' do
+      expect(described_class.default).to eq 'default'
+    end
+  end
 end
diff --git a/spec/lib/gitlab/ci/config/entry/unspecified_spec.rb b/spec/lib/gitlab/ci/config/entry/unspecified_spec.rb
deleted file mode 100644
index 66f88fa35b6..00000000000
--- a/spec/lib/gitlab/ci/config/entry/unspecified_spec.rb
+++ /dev/null
@@ -1,32 +0,0 @@
-require 'spec_helper'
-
-describe Gitlab::Ci::Config::Entry::Unspecified do
-  let(:unspecified) { described_class.new(entry) }
-  let(:entry) { spy('Entry') }
-
-  describe '#valid?' do
-    it 'delegates method to entry' do
-      expect(unspecified.valid?).to eq entry
-    end
-  end
-
-  describe '#errors' do
-    it 'delegates method to entry' do
-      expect(unspecified.errors).to eq entry
-    end
-  end
-
-  describe '#value' do
-    it 'delegates method to entry' do
-      expect(unspecified.value).to eq entry
-    end
-  end
-
-  describe '#specified?' do
-    it 'is always false' do
-      allow(entry).to receive(:specified?).and_return(true)
-
-      expect(unspecified.specified?).to be false
-    end
-  end
-end