(PUP-4242) Correctly validate named definitions in plans

This bit of code validates that the name of a definition in a Puppet
manifest conforms to the naming convention of
<module>::<dir>::<filename>. However, it determines the expected module
name by searching up the path until it finds a known directory name that
might contain a .pp file. Previously, it was only looking for
'manifests', 'functions', and 'types'. This meant that plans couldn't be
loaded, since it couldn't find the expected module name.

Author: nicklewis

lib/puppet/pops/validation/checker4_0.rb

@@ -595,10 +595,11 @@ def initial_manifest?(path)
     full_path == manifest_setting || full_path.start_with?(manifest_setting)
   end
 
-  # Returns module root directory index in path for files under 'manifests', 'functions' and 'types'
+  # Returns module root directory index in path for files under 'manifests',
+  # 'functions', 'types' and 'plans'
   def find_module_definition_dir(path)
     reverse_index = path.reverse_each.find_index do |dir|
-      dir == 'manifests' || dir == 'functions' || dir == 'types'
+      dir == 'manifests' || dir == 'functions' || dir == 'types' || dir == 'plans'
     end
     return reverse_index.nil? ? nil : (path.length - 1) - reverse_index
   end

spec/unit/pops/validator/validator_spec.rb

@@ -34,7 +34,7 @@ def validate(factory)
     expect(validate(parse('function ::aaa() {}'))).to have_issue(Puppet::Pops::Issues::ILLEGAL_DEFINITION_NAME)
   end
 
-  it 'should raise error for illegal class locations' do
+  it 'should raise error for illegal definition locations' do
     expect(validate(parse('function aaa::ccc() {}', 'aaa/manifests/bbb.pp'))).to have_issue(Puppet::Pops::Issues::ILLEGAL_DEFINITION_LOCATION)
     expect(validate(parse('class bbb() {}', 'aaa/manifests/init.pp'))).to have_issue(Puppet::Pops::Issues::ILLEGAL_DEFINITION_LOCATION)
     expect(validate(parse('define aaa::bbb::ccc::eee() {}', 'aaa/manifests/bbb/ddd.pp'))).to have_issue(Puppet::Pops::Issues::ILLEGAL_DEFINITION_LOCATION)
@@ -225,6 +225,18 @@ def validate(factory)
   context 'with --tasks set' do
     before(:each) { Puppet[:tasks] = true }
 
+    it 'raises an error for illegal plan names' do
+      expect(validate(parse('plan aaa::ccc::eee() {}', 'aaa/plans/bbb/ccc/eee.pp'))).to have_issue(Puppet::Pops::Issues::ILLEGAL_DEFINITION_LOCATION)
+      expect(validate(parse('plan aaa() {}', 'aaa/plans/aaa.pp'))).to have_issue(Puppet::Pops::Issues::ILLEGAL_DEFINITION_LOCATION)
+      expect(validate(parse('plan aaa::bbb() {}', 'aaa/plans/bbb/bbb.pp'))).to have_issue(Puppet::Pops::Issues::ILLEGAL_DEFINITION_LOCATION)
+    end
+
+    it 'accepts legal plan names' do
+      expect(validate(parse('plan aaa::ccc::eee() {}', 'aaa/plans/ccc/eee.pp'))).not_to have_issue(Puppet::Pops::Issues::ILLEGAL_DEFINITION_LOCATION)
+      expect(validate(parse('plan aaa() {}', 'aaa/plans/init.pp'))).not_to have_issue(Puppet::Pops::Issues::ILLEGAL_DEFINITION_LOCATION)
+      expect(validate(parse('plan aaa::bbb() {}', 'aaa/plans/bbb.pp'))).not_to have_issue(Puppet::Pops::Issues::ILLEGAL_DEFINITION_LOCATION)
+    end
+
     it 'produces an error for application' do
       acceptor = validate(parse('application test {}'))
       expect(acceptor.error_count).to eql(1)