Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Users should be able to include external Puppetfiles in their own. #927

Closed
wants to merge 6 commits into from
Closed

Users should be able to include external Puppetfiles in their own. #927

wants to merge 6 commits into from

Conversation

mdwheele
Copy link

@mdwheele mdwheele commented May 10, 2019

Related: #885

Hello!

💁‍♀️ This PR adds a new incl syntax to the Puppetfile spec for including an external Puppetfile as if it were part of the environments dependencies.

Sample Puppetfile:

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

# Include shared modules.
incl '/usr/local/share/r10k/Puppetfile'

#
# Tenant-specific dependencies.
#
mod 'puppetlabs/stdlib'
mod 'puppetlabs-apache',
  :git => 'git@github.com:puppetlabs/puppetlabs-apache.git',
  :branch => 'master'

Sample External Puppetfile at /usr/local/share/r10k/Puppetfile:

#
# Shared dependencies.
#
mod 'puppetlabs/apt'

In our environment, we have multiple tenants separated into separate control repositories. Some of these tenants depend on shared modules (roles/profiles) we maintain as a community. This means that when we decide to update underlying component modules, we have to communicate those changes to individual tenants and it creates a release bottleneck.

One "hack" that we've been using in the last year was to have our r10k node write the "shared upstream Puppetfile" to disk and then in tenant Puppetfile, they do the following:

# "Include" shared Puppetfile
self.instance_eval(File.read('/usr/local/share/r10k/Puppetfile'))

# 
# Tenant-specific dependencies
#
mod 'camptocamp-kmod', '2.3.0'
mod 'crayfishx-firewalld', '3.4.0'

The fact that Puppetfile are eval()-d as Ruby currently allows us to solve the problem. I

In #885, an AST-based approach is proposed for hosts running Ruby 2.6+. If this were the standard behaviour, then our eval-based approach to sharing modules would break. My proposal is based on the implementation @binford2k provides in #885 and adds new syntax to officially support inclusion of an external Puppetfile.

Implementation Details / Credits

  • I plucked the AST-based parser implementation from POC hack to parse Puppetfile directly W/O eval #885 and credited @binford2k as the author in e177d55. Very awesome work.
  • Following, I implemented support for incl. The directive receives a single argument that can be a relative or absolute path. If a relative path is used, it is relative to the calling Puppetfile.
  • I did not impose any limit on the depth of includes that can be used. Arguably, as a matter of best practice, one might say "limit to a single depth of includes". I'm amenable to enforcing this, but left it out for now.
  • There is no cycle detection, but that seems like something someone would have to go out of their way to do here. This feature is intended to be a very simple include.
  • Includes are expected to function as if the contents of the file were in-lined into the calling Puppetfile.

Please let me know if you have any questions. I will be adding additional unit tests to cover this code, but wanted to get this PR submitted. I appreciate your time and consideration!

binford2k and others added 4 commits May 10, 2019 11:20
Ruby 2.6 instroduced the RubyVM::AbstractSyntaxTree, which will parse
arbitrary Ruby code into an AST. This means that instead of eval'ing
code, we can just parse it and take action on the three methods we care
about. In other words, the Puppetfile will no longer be arbitrary and
unchecked Ruby code.

See https://docs.ruby-lang.org/en/trunk/RubyVM/AbstractSyntaxTree.html
for more information.

Note: This implementation is *experimental* and will almost certainly
change. This should not be merged until upstream commits to a stable
API. But it's worth exploring and validating that we can indeed have
non-executable Puppetfiles.
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ binford2k
❌ Dustin Wheeler


Dustin Wheeler seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@binford2k
Copy link
Member

Hey, thanks for the idea @mdwheele! I'm going to close this now though, since we have a better way for you to include external Puppetfiles via your own policy. You'd want to write a short script that defined how to combine all the partials and then output a single YAML formatted list of modules to use in your environment. (You could even write your Puppetfiles in YAML, if you'd like)

See the exec source type for more information on how that would work.

@binford2k binford2k closed this Mar 17, 2021
@mdwheele
Copy link
Author

Hey, thanks for the idea @mdwheele! I'm going to close this now though, since we have a better way for you to include external Puppetfiles via your own policy. You'd want to write a short script that defined how to combine all the partials and then output a single YAML formatted list of modules to use in your environment. (You could even write your Puppetfiles in YAML, if you'd like)

No worries at all man! The exec source type looks like a much better (and safer) approach to this. We always knew that hack would eventually go away! 😀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants