(RK-390) Remove hardcoded default for git ref

CHANGELOG.mkd

@@ -4,12 +4,12 @@ CHANGELOG
 Unreleased
 ----------
 
+- (RK-390) Remove default ref for deploying git modules [#1275](http://github.com/puppetlabs/r10k/pull/1275)
 - (RK-391) Change `exclude_spec` default to true for module spec dir deletion [#1264](https://github.com/puppetlabs/r10k/pull/1261)
 - (maint) Add Ruby 3.0 to rspec CI matrix [#1261](https://github.com/puppetlabs/r10k/pull/1261)
 - (RK-383) Remove deprecated `basedir` method from Puppetfile DSL. Users should use `environment_name` instead. [#1254](https://github.com/puppetlabs/r10k/pull/1254)
 - (RK-386) Remove deprecated `bare` environment type. [#1235](https://github.com/puppetlabs/r10k/issues/1235)
 - Drop EoL Ruby 2.3/2.4 support [#1280](https://github.com/puppetlabs/r10k/pull/1208)
-- (CODEMGMT-1295) Change default branch for git modules to `main`. [#1258](https://github.com/puppetlabs/r10k/pull/1258)
 
 3.14.1
 ------

doc/dynamic-environments/configuration.mkd

@@ -96,6 +96,18 @@ git:
 See the [git provider documentation](../git/providers.mkd) for more information
 regarding Git providers.
 
+#### default_ref
+
+r10k is unable to deploy a git module if no `ref` is specified. A `default_ref` can be
+set in the r10k config that will become the ref a module uses if not otherwise specified. This
+is the lowest priority setting for a module's `ref`. Read the [Puppetfile documentation](../puppetfile.mkd#git)
+for higher priority settings to determine a module's ref.
+
+```yaml
+git:
+  default_ref: main
+```
+
 #### proxy
 
 The 'proxy' setting allows you to set or override the global proxy setting specifically

doc/puppetfile.mkd

@@ -135,6 +135,12 @@ operations when updating the repo, which can speed up install times. When
 Module versions can also be specified using `:branch` to track a specific
 branch reference.
 
+In r10k 3.x the default branch was hardcoded to `master`; in 4.x that was
+removed. A `default_ref` can be specified in the r10k config to
+to mimic that old behavior, but it is recommended to set the ref on a
+per-module basis in the Puppetfile. Read [here](dynamic-environments/configuration.mkd#default_ref) for more info
+on the `default_ref` setting.
+
 #### Examples
 
 ```ruby

lib/r10k/action/deploy/environment.rb

@@ -45,6 +45,7 @@ def initialize(opts, argv, settings = {})
                 incremental: @incremental
               },
               modules: {
+                default_ref: settings.dig(:git, :default_ref),
                 exclude_spec: settings.dig(:deploy, :exclude_spec),
                 requested_modules: [],
                 deploy_modules: @modules,

lib/r10k/action/deploy/module.rb

@@ -39,6 +39,7 @@ def initialize(opts, argv, settings = {})
                 generate_types: @generate_types
               },
               modules: {
+                default_ref: settings.dig(:git, :default_ref),
                 exclude_spec: settings.dig(:deploy, :exclude_spec),
                 pool_size: @settings[:pool_size] || 4,
                 requested_modules: @argv.map.to_a,

lib/r10k/action/puppetfile/check.rb

@@ -9,12 +9,19 @@ class Check < R10K::Action::Base
 
         def call
           options = { basedir: @root }
+          options[:overrides] = {}
+          options[:overrides][:modules] = { default_ref: @settings.dig(:git, :default_ref) }
           options[:moduledir] = @moduledir if @moduledir
           options[:puppetfile] = @puppetfile if @puppetfile
 
           loader = R10K::ModuleLoader::Puppetfile.new(**options)
           begin
             loader.load!
+            loader.modules.each do |mod|
+              if mod.instance_of?(R10K::Module::Git)
+                mod.validate_ref_defined
+              end
+            end
             $stderr.puts _("Syntax OK")
             true
           rescue => e

lib/r10k/action/puppetfile/install.rb

@@ -12,6 +12,7 @@ class Install < R10K::Action::Base
         def call
           begin
             options = { basedir: @root, overrides: { force: @force || false } }
+            options[:overrides][:modules] = { default_ref: @settings.dig(:git, :default_ref) }
             options[:moduledir]  = @moduledir  if @moduledir
             options[:puppetfile] = @puppetfile if @puppetfile
 

lib/r10k/module/git.rb

@@ -63,15 +63,14 @@ def initialize(title, dirname, opts, environment=nil)
       :commit                  => :desired_ref,
       :ref                     => :desired_ref,
       :git                     => :remote,
-      :default_branch          => :default_ref,
+      :default_branch          => :default_branch,
       :default_branch_override => :default_override_ref,
     }, :raise_on_unhandled => false)
 
+    @default_ref = @default_branch.nil? ? @overrides.dig(:modules, :default_ref) : @default_branch
     force = @overrides[:force]
     @force = force == false ? false : true
 
-    @desired_ref ||= 'main'
-
     if @desired_ref == :control_branch
       if @environment && @environment.respond_to?(:ref)
         @desired_ref = @environment.ref
@@ -116,6 +115,14 @@ def cachedir
     @repo.cache.sanitized_dirname
   end
 
+  def validate_ref_defined
+    if @desired_ref.nil? && @default_ref.nil? && @default_override_ref.nil?
+      msg = "No ref defined for module #{@name}. Add a ref to the module definition "
+      msg << "or set git:default_ref in the r10k.yaml config to configure a global default ref."
+      raise ArgumentError, msg
+    end
+  end
+
   private
 
   def validate_ref(desired, default, default_override)
@@ -147,7 +154,9 @@ def validate_ref(desired, default, default_override)
         msg << "or resolve default ref '%{default}'"
         vars[:default] = default
       else
-        msg << "and no default provided"
+        msg << "and no default provided. r10k no longer hardcodes 'master' as the default ref."
+        msg << "Consider setting a ref per module in the Puppetfile or setting git:default_ref"
+        msg << "in your r10k config."
       end
 
       raise ArgumentError, _(msg.join(' ')) % vars

lib/r10k/settings.rb

@@ -19,6 +19,10 @@ class << self
     def self.git_settings
       R10K::Settings::Collection.new(:git, [
 
+        Definition.new(:default_ref, {
+          :desc => "User-defined default ref from which to deploy modules when not otherwise specified; nil unless configured via the r10k.yaml config.",
+          :default => nil}),
+
         EnumDefinition.new(:provider, {
           :desc => "The Git provider to use. Valid values: 'shellgit', 'rugged'",
           :normalize => lambda { |input| input.to_sym },

spec/unit/action/deploy/module_spec.rb

@@ -253,9 +253,9 @@
         loader = environment.loader
         allow(loader).to receive(:puppetfile_content).and_return('')
         expect(loader).to receive(:load) do
-          loader.add_module('mod1', { git: 'git://remote' })
-          loader.add_module('mod2', { git: 'git://remote' })
-          loader.add_module('mod3', { git: 'git://remote' })
+          loader.add_module('mod1', { git: 'git://remote', default_branch: 'main'})
+          loader.add_module('mod2', { git: 'git://remote', default_branch: 'main'})
+          loader.add_module('mod3', { git: 'git://remote', default_branch: 'main'})
 
           loaded_content = loader.load!
           loaded_content[:modules].each do |mod|
@@ -315,8 +315,8 @@
           # it so it will create the correct loaded_content.
           allow(loader).to receive(:puppetfile_content).and_return('')
           expect(loader).to receive(:load) do
-            loader.add_module('mod1', { git: 'git://remote' })
-            loader.add_module('mod2', { git: 'git://remote' })
+            loader.add_module('mod1', { git: 'git://remote', default_branch: 'main'})
+            loader.add_module('mod2', { git: 'git://remote', default_branch: 'main'})
 
             loaded_content = loader.load!
             loaded_content[:modules].each do |mod|

spec/unit/action/puppetfile/check_spec.rb

@@ -3,7 +3,14 @@
 
 describe R10K::Action::Puppetfile::Check do
   let(:default_opts) { {root: "/some/nonexistent/path"} }
-  let(:loader) { instance_double('R10K::ModuleLoader::Puppetfile', :load! => {}) }
+  let(:modules) do
+    [R10K::Module::Git.new("author/modname",
+                           "/some/nonexistent/path/modname",
+                           {git: 'https://my/git/remote', branch: 'main'})]
+  end
+
+
+  let(:loader) { instance_double('R10K::ModuleLoader::Puppetfile', :load! => {}, :modules => modules) }
 
   def checker(opts = {}, argv = [], settings = {})
     opts = default_opts.merge(opts)
@@ -15,11 +22,39 @@ def checker(opts = {}, argv = [], settings = {})
       to receive(:new).
       with({
         basedir: "/some/nonexistent/path",
+        overrides: {modules: {default_ref: nil}}
       }).and_return(loader)
   end
 
   it_behaves_like "a puppetfile action"
 
+  describe 'when no ref is defined' do
+    let(:modules) do
+      [R10K::Module::Git.new("author/modname",
+                             "/some/nonexistent/path/modname",
+                             {git: 'https://my/git/remote'})]
+    end
+
+    it 'returns an error message' do
+      expect($stderr).to receive(:puts).with(/no ref defined/i)
+      checker.call
+    end
+  end
+
+  describe 'when a default_ref is defined' do
+    let(:modules) do
+      [R10K::Module::Git.new("author/modname",
+                             "/some/nonexistent/path/modname",
+                             {git: 'https://my/git/remote',
+                              overrides: {modules: {default_ref: 'main'}}})]
+    end
+
+    it 'is valid syntax' do
+      expect($stderr).to receive(:puts).with(/Syntax OK/i)
+      checker.call
+    end
+  end
+
   it "prints 'Syntax OK' when the Puppetfile syntax could be validated" do
     expect($stderr).to receive(:puts).with("Syntax OK")
 
@@ -45,6 +80,7 @@ def checker(opts = {}, argv = [], settings = {})
       to receive(:new).
       with({
         basedir: "/some/nonexistent/path",
+        overrides: {modules: {default_ref: nil}},
         puppetfile: "/custom/puppetfile/path"
       }).and_return(loader)
 

spec/unit/action/puppetfile/install_spec.rb

@@ -18,7 +18,7 @@ def installer(opts = {}, argv = [], settings = {})
     allow(loader).to receive(:load!).and_return({})
     allow(R10K::ModuleLoader::Puppetfile).to receive(:new).
       with({basedir: "/some/nonexistent/path",
-            overrides: {force: false}}).
+            overrides: {force: false, modules: {default_ref: nil}}}).
       and_return(loader)
   end
 
@@ -54,6 +54,16 @@ def installer(opts = {}, argv = [], settings = {})
 
       expect(installer.call).to eq false
     end
+
+    it "reads in the default for git refs" do
+      modules.each { |m| expect(m).to receive(:sync) }
+      expect(R10K::ModuleLoader::Puppetfile).to receive(:new).
+        with({basedir: "/some/nonexistent/path",
+              overrides: {force: false, modules: {default_ref: 'main'}}}).
+        and_return(loader)
+
+      installer({}, [], {git: {default_ref: 'main'}}).call
+    end
   end
 
   describe "purging" do
@@ -80,15 +90,15 @@ def installer(opts = {}, argv = [], settings = {})
     it "can use a custom moduledir path" do
       expect(R10K::ModuleLoader::Puppetfile).to receive(:new).
         with({basedir: "/some/nonexistent/path",
-              overrides: { force: false },
+              overrides: {force: false, modules: {default_ref: nil}},
               puppetfile: "/some/other/path/Puppetfile"}).
         and_return(loader)
 
       installer({puppetfile: "/some/other/path/Puppetfile"}).call
 
       expect(R10K::ModuleLoader::Puppetfile).to receive(:new).
         with({basedir: "/some/nonexistent/path",
-              overrides: { force: false },
+              overrides: {force: false, modules: {default_ref: nil}},
               moduledir: "/some/other/path/site-modules"}).
         and_return(loader)
 
@@ -103,7 +113,7 @@ def installer(opts = {}, argv = [], settings = {})
       subject = described_class.new({root: "/some/nonexistent/path", force: true}, [], {})
       expect(R10K::ModuleLoader::Puppetfile).to receive(:new).
         with({basedir: "/some/nonexistent/path",
-              overrides: { force: true }}).
+              overrides: {force: true, modules: {default_ref: nil}}}).
         and_return(loader)
       subject.call
     end

spec/unit/module/git_spec.rb

@@ -86,7 +86,8 @@
 
   describe "properties" do
     subject do
-      described_class.new('boolean', '/moduledir', {:git => 'git://git.example.com/adrienthebo/puppet-boolean'})
+      described_class.new('boolean', '/moduledir', {:git => 'git://git.example.com/adrienthebo/puppet-boolean',
+                                                    overrides: {modules: {default_ref: "main"}}})
     end
 
     before(:each) do
@@ -119,7 +120,7 @@
     let(:title) { "#{module_org}-#{module_name}" }
     let(:dirname) { Pathname.new(Dir.mktmpdir) }
     let(:spec_path) { dirname + module_name + 'spec' }
-    subject { described_class.new(title, dirname, {}) }
+    subject { described_class.new(title, dirname, {overrides: {modules: {default_ref: "main"}}}) }
 
     before(:each) do
       allow(mock_repo).to receive(:resolve).with('main').and_return('abc123')
@@ -177,15 +178,29 @@ def test_module(extra_opts, env=nil)
       allow(mock_repo).to receive(:head).and_return('abc123')
     end
 
-    describe "desired ref" do
-      context "when no desired ref is given" do
-        it "defaults to main" do
-          expect(mock_repo).to receive(:resolve).with('main').and_return('abc123')
+    it "raises an argument error when no refs are supplied" do
+      expect{test_module({}).properties}.to raise_error(ArgumentError, /unable.*desired ref.*no default/i)
+    end
 
-          expect(test_module({}).properties).to include(expected: 'main')
+    describe 'the overrides->modules->default_ref' do
+      context 'specifying a default_ref only' do
+        let(:opts) { {overrides: {modules: {default_ref: 'cranberry'}}} }
+        it "sets the expected ref to default_ref" do
+          expect(mock_repo).to receive(:resolve).with('cranberry').and_return('def456')
+          expect(test_module(opts).properties).to include(expected: 'cranberry')
         end
       end
 
+      context 'specifying a default_ref and a default_branch' do
+        let(:opts) { {default_branch: 'orange',  overrides: {modules: {default_ref: 'cranberry'}}}}
+        it "sets the expected ref to the default_branch" do
+          expect(mock_repo).to receive(:resolve).with('orange').and_return('def456')
+          expect(test_module(opts).properties).to include(expected: 'orange')
+        end
+      end
+    end
+
+    describe "desired ref" do
       context "specifying a static desired branch" do
         let(:opts) { { branch: 'banana' } }