rnelson0 / puppet-generate-puppetfile

Generate or update a Puppetfile for use with r10k or librarian-puppet. Optionally, create a fixtures file for rspec-puppet.
92 stars 22 forks source link

generate-puppetfile ignores Git modules whose name contains a slash #56

Closed bitfield closed 6 years ago

bitfield commented 6 years ago

It seems that if a non-Forge module contains a slash in its name, generate-puppetfile assumes it must be a Forge module, fails to find it on the Forge, removes it from the output, and consequently generates a broken Puppetfile.

Input Puppetfile:

forge 'http://forge.puppetlabs.com'

mod 'puppetlabs/stdlib', '4.17.1'
mod 'pbg/ntp',
  :git => 'https://github.com/bitfield/pbg-ntp.git',
  :tag => '0.1.1'

Output:

/opt/puppetlabs/puppet/bin/generate-puppetfile -p /etc/puppetlabs/code/environments/pbg/Puppetfile.test

Installing modules. This may take a few minutes.

There was a problem with the module name 'pbg/ntp'.
  Check that modules exist as under the listed name, and/or your connectivity to the puppet forge.

Here is the PARTIAL Puppetfile that would have been generated.

Your Puppetfile has been generated. Copy and paste between the markers:

=======================================================================
forge 'http://forge.puppetlabs.com'

# Modules discovered by generate-puppetfile
mod 'puppetlabs/stdlib', '4.17.1'
# Discovered elements from existing Puppetfile
  :git => 'https://github.com/bitfield/pbg-ntp.git',
  :tag => '0.1.1'
=======================================================================

As you can see, the line mod 'pbg/ntp', has been omitted from the output, resulting in an invalid Puppetfile:

/opt/puppetlabs/puppet/bin/r10k puppetfile check
Failed to evaluate /etc/puppetlabs/code/environments/pbg/Puppetfile
Original exception:
/etc/puppetlabs/code/environments/pbg/Puppetfile:6: syntax error, unexpected =>, expecting end-of-input
:git => 'https://github.com/bitfield/
       ^

I am guessing that the slash is the culprit; if I change it to a hyphen, the problem does not occur.

I suggest that if a module has a :git attribute, it is not a Forge module, and generate-puppetfile should not attempt to look for it there. Alternatively, if a module is not found, rather than dropping it from the output, perhaps generate-puppetfile should simply pass it through untouched as a 'discovered element'.

rnelson0 commented 6 years ago

@bitfield Slashes and hyphens are both prohibited in module names per https://docs.puppet.com/puppet/5.0/lang_reserved.html#modules. The slashes specifically would interfere with the autoloader since it would change the path construction. So, if logic to deal with the git tag following a module name is inserted (probably more accurately, trailing commas followed by a non-version number), other problems may persist afterward.

bitfield commented 6 years ago

I appreciate that. Interestingly, r10k has no problem installing this module, and indeed you can include it; the name becomes simply ntp.

I guess my question is really about what generate-puppetfile should do on encountering a module like this. I feel it should simply pass it through to the output along with all other unrecognised matter. The current behaviour actually breaks the Puppetfile, because it eats the mod 'pbg/ntp' line but not the connected attributes. It would be nice if generate-puppetfile's output was always at least a valid Puppetfile.

Detecting when modules are git modules is really a separate issue, and if you think it's worth pursuing, I'll open one.

rnelson0 commented 6 years ago

I am happy to "pass it on" so that r10k or librarian-puppet can figure out how to handle it, I just wanted to point out the standard.

We should definitely not eat the line and disconnect it from its continuance, which I think is the same practical effect of detecting when modules are non-forge (git, svn, etc) modules. Does this seem like the correct decision tree?

While the multi-line capture isn't hugely important, since currently that's just pasted as a blob at the end, it would allow future transformations on a per-module basis vs a per-line basis, so might be good to work in at the same time.