danielparks / puppet-rustup

Puppet module to manage Rust with rustup
https://forge.puppet.com/modules/dp/rustup/readme
Other
0 stars 0 forks source link

Switch entirely to require from require_relative (readability). #29

Closed danielparks closed 2 years ago

bastelfreak commented 2 years ago

@danielparks Hi! thanks for the awesomme module. Stupid question, could you revert this commit? With require_relative, environment isolation works. Puppet will ensure that the loaded lib comes from the same environment as the type and provider. With just using require, you potentially load the puppet_x lib from another code environment and cause some trouble. An example PR is https://github.com/puppetlabs/puppetlabs-stdlib/pull/1275. We (voxpupuli.org) and Puppet are currently checking for a rubocop rule to even enforce require_relative.

danielparks commented 2 years ago

Sure, thanks for the explanation. Just out of curiosity, how did you find this?

bastelfreak commented 2 years ago

I'm one of the Vox Pupuli maintainers and we maintain a lot of modules (https://github.com/voxpupuli?q=puppet-&type=all&language=&sort=) :D. We noticed this in a few modules. This is usually a problem when a module is updated in a new environment and production still uses an older version, where some methods from a class in puppet_x aren't yet implemented.

danielparks commented 2 years ago

Ah, sure. :) I meant, how did you find this PR I made on this repo — it seems pretty obscure. :)

I opened a PR #36 to switch to require_relative, but I note that the stdlib PR you mentioned actually changes a require_relative in a way that looks like it shouldn’t make a difference.

-require_relative '../stdlib'
+require_relative '../../puppet_x/stdlib'

Should I adjust my require_relatives to back further up the directory tree and include the full puppet_x path? I have a hard time imagining why that would be necessary, but it sounds like it is.

Thanks!