Merge pull request #1240 from b4ldr/fix_strip_component

use correct reference when using strip_component

CHANGELOG.mkd

@@ -4,6 +4,8 @@ CHANGELOG
 Unreleased
 ----------
 
+
+- Record unprocessed environment name, so that `strip_component` does not cause truncated environment names to be used as git branches, resulting in errors or incorrect deploys. [#1240](https://github.com/puppetlabs/r10k/pull/1240)
 - (CODEMGMT-1294) Resync repos with unresolvable refs [#1239](https://github.com/puppetlabs/r10k/pull/1239)
 - (RK-378) Restore access to the environment name from the Puppetfile [#1241](https://github.com/puppetlabs/r10k/pull/1241)
 - (CODEMGMT-1300) Ensure the remote url in rugged cache directories is current [#1245](https://github.com/puppetlabs/r10k/pull/1245)

lib/r10k/environment/name.rb

@@ -6,17 +6,22 @@ module Environment
     class Name
 
       # @!attribute [r] name
-      #   @return [String] The unmodified name of the environment
+      #   @return [String] The functional name of the environment derived from inputs and options.
       attr_reader :name
 
+      # @!attribute [r] original_name
+      #   @return [String] The unmodified name originally given to create the object.
+      attr_reader :original_name
+
       INVALID_CHARACTERS = %r[\W]
 
-      def initialize(name, opts)
+      def initialize(original_name, opts)
         @source  = opts[:source]
         @prefix  = opts[:prefix]
         @invalid = opts[:invalid]
 
-        @name = derive_name(name, opts[:strip_component])
+        @name = derive_name(original_name, opts[:strip_component])
+        @original_name = original_name
         @opts = opts
 
         case @invalid
@@ -71,8 +76,8 @@ def dirname
 
       private
 
-      def derive_name(name, strip_component)
-        return name unless strip_component
+      def derive_name(original_name, strip_component)
+        return original_name unless strip_component
 
         unless strip_component.is_a?(String)
           raise _('Improper configuration value given for strip_component setting in %{src} source. ' \
@@ -82,11 +87,11 @@ def derive_name(name, strip_component)
 
         if %r{^/.*/$}.match(strip_component)
           regex = Regexp.new(strip_component[1..-2])
-          name.gsub(regex, '')
-        elsif name.start_with?(strip_component)
-          name[strip_component.size..-1]
+          original_name.gsub(regex, '')
+        elsif original_name.start_with?(strip_component)
+          original_name[strip_component.size..-1]
         else
-          name
+          original_name
         end
       end
 

lib/r10k/source/git.rb

@@ -100,26 +100,26 @@ def reload!
 
   def generate_environments
     envs = []
-    branch_names.each do |bn|
-      if bn.valid?
-        envs << R10K::Environment::Git.new(bn.name,
+    environment_names.each do |en|
+      if en.valid?
+        envs << R10K::Environment::Git.new(en.name,
                                            @basedir,
-                                           bn.dirname,
+                                           en.dirname,
                                            {remote: remote,
-                                            ref: bn.name,
+                                            ref: en.original_name,
                                             puppetfile_name: puppetfile_name,
                                             overrides: @options[:overrides]})
-      elsif bn.correct?
-       logger.warn _("Environment %{env_name} contained non-word characters, correcting name to %{corrected_env_name}") % {env_name: bn.name.inspect, corrected_env_name: bn.dirname}
-        envs << R10K::Environment::Git.new(bn.name,
+      elsif en.correct?
+       logger.warn _("Environment %{env_name} contained non-word characters, correcting name to %{corrected_env_name}") % {env_name: en.name.inspect, corrected_env_name: en.dirname}
+        envs << R10K::Environment::Git.new(en.name,
                                            @basedir,
-                                           bn.dirname,
+                                           en.dirname,
                                            {remote: remote,
-                                            ref: bn.name,
+                                            ref: en.original_name,
                                             puppetfile_name: puppetfile_name,
                                             overrides: @options[:overrides]})
-      elsif bn.validate?
-       logger.error _("Environment %{env_name} contained non-word characters, ignoring it.") % {env_name: bn.name.inspect}
+      elsif en.validate?
+       logger.error _("Environment %{env_name} contained non-word characters, ignoring it.") % {env_name: en.name.inspect}
       end
     end
 
@@ -157,22 +157,22 @@ def filter_branches_by_command(branches, command)
 
   private
 
-  def branch_names
+  def environment_names
     opts = {prefix: @prefix,
             invalid: @invalid_branches,
             source: @name,
             strip_component: @strip_component}
-    branches = @cache.branches
+    branch_names = @cache.branches
     if @ignore_branch_prefixes && !@ignore_branch_prefixes.empty?
-      branches = filter_branches_by_regexp(branches, @ignore_branch_prefixes)
+      branch_names = filter_branches_by_regexp(branch_names, @ignore_branch_prefixes)
     end
 
     if @filter_command && !@filter_command.empty?
-      branches = filter_branches_by_command(branches, @filter_command)
+      branch_names = filter_branches_by_command(branch_names, @filter_command)
     end
 
-    branches.map do |branch|
-      R10K::Environment::Name.new(branch, opts)
+    branch_names.map do |branch_name|
+      R10K::Environment::Name.new(branch_name, opts)
     end
   end
 end

spec/unit/action/deploy/module_spec.rb

@@ -241,7 +241,7 @@
 
       allow(R10K::Git::StatefulRepository).to receive(:new).and_return(repo)
       allow(R10K::Git).to receive_message_chain(:cache, :generate).and_return(cache)
-      allow_any_instance_of(R10K::Source::Git).to receive(:branch_names).and_return([R10K::Environment::Name.new('first', {})])
+      allow_any_instance_of(R10K::Source::Git).to receive(:environment_names).and_return([R10K::Environment::Name.new('first', {})])
 
       expect(subject).to receive(:visit_environment).and_wrap_original do |original, environment, &block|
         # For this test we want to have realistic Modules and access to
@@ -300,7 +300,7 @@
 
       allow(R10K::Git::StatefulRepository).to receive(:new).and_return(repo)
       allow(R10K::Git).to receive_message_chain(:cache, :generate).and_return(cache)
-      allow_any_instance_of(R10K::Source::Git).to receive(:branch_names).and_return([R10K::Environment::Name.new('first', {}),
+      allow_any_instance_of(R10K::Source::Git).to receive(:environment_names).and_return([R10K::Environment::Name.new('first', {}),
                                                                                      R10K::Environment::Name.new('second', {})])
 
       expect(subject).to receive(:visit_environment).and_wrap_original do |original, environment, &block|

spec/unit/environment/name_spec.rb

@@ -6,21 +6,29 @@
     it "does not modify the given name when no strip_component is given" do
       bn = described_class.new('myenv', source: 'source', prefix: false)
       expect(bn.dirname).to eq 'myenv'
+      expect(bn.name).to eq 'myenv'
+      expect(bn.original_name).to eq 'myenv'
     end
 
     it "removes the first occurance of a regex match when a regex is given" do
       bn = described_class.new('myenv', source: 'source', prefix: false, strip_component: '/env/')
       expect(bn.dirname).to eq 'my'
+      expect(bn.name).to eq 'my'
+      expect(bn.original_name).to eq 'myenv'
     end
 
     it "does not modify the given name when there is no regex match" do
       bn = described_class.new('myenv', source: 'source', prefix: false, strip_component: '/bar/')
       expect(bn.dirname).to eq 'myenv'
+      expect(bn.name).to eq 'myenv'
+      expect(bn.original_name).to eq 'myenv'
     end
 
     it "removes the given name's prefix when it matches strip_component" do
       bn = described_class.new('env/prod', source: 'source', prefix: false, strip_component: 'env/')
       expect(bn.dirname).to eq 'prod'
+      expect(bn.name).to eq 'prod'
+      expect(bn.original_name).to eq 'env/prod'
     end
 
     it "raises an error when given an integer" do
@@ -34,21 +42,29 @@
     it "uses the branch name as the dirname when prefixing is off" do
       bn = described_class.new('mybranch', :source => 'source', :prefix => false)
       expect(bn.dirname).to eq 'mybranch'
+      expect(bn.name).to eq 'mybranch'
+      expect(bn.original_name).to eq 'mybranch'
     end
 
     it "prepends the source name when prefixing is on" do
       bn = described_class.new('mybranch', :source => 'source', :prefix => true)
       expect(bn.dirname).to eq 'source_mybranch'
+      expect(bn.name).to eq 'mybranch'
+      expect(bn.original_name).to eq 'mybranch'
     end
 
     it "prepends the prefix name when prefixing is overridden" do
       bn = described_class.new('mybranch', {:prefix => "bar", :sourcename => 'foo'})
       expect(bn.dirname).to eq 'bar_mybranch'
+      expect(bn.name).to eq 'mybranch'
+      expect(bn.original_name).to eq 'mybranch'
     end
 
     it "uses the branch name as the dirname when prefixing is nil" do
       bn = described_class.new('mybranch', {:prefix => nil, :sourcename => 'foo'})
       expect(bn.dirname).to eq 'mybranch'
+      expect(bn.name).to eq 'mybranch'
+      expect(bn.original_name).to eq 'mybranch'
     end
   end
 
@@ -149,6 +165,8 @@
         it "replaces invalid characters in #{branch} with underscores" do
           bn = described_class.new(branch.dup, {:correct => true})
           expect(bn.dirname).to eq branch.gsub(/\W/, '_')
+          expect(bn.name).to eq branch
+          expect(bn.original_name).to eq branch
         end
       end