(#7316) Append module plugin directories to $LOAD_PATH#1319
Merged
zaphod42 merged 6 commits intopuppetlabs:masterfrom Dec 12, 2012
Merged
Conversation
Several places use an Autoloader to list files that could be loaded
relative to some prefix, e.g. puppet/application. But then the same
prefix is specified again when requiring a specific instance, e.g.
require File.join('puppet', 'application', name)
This commit adds an Autoload instance method to expand a path, so the
above becomes:
require @loader.expand(name)
lib/puppet/interface.rb
Outdated
Contributor
There was a problem hiding this comment.
This should just be changed to a deprecation_warning and documented with @deprecated.
Contributor
|
There is a recurring pattern in this where the only interesting parts of |
Previously, the logic for listing and requiring applications was in two places, CommandLine and Application, respectively. The CommandLine contained logic, basically duplicated from the Autoloader, to walk a search path and generate a list unique applications relative to 'puppet/application'. The Application contained logic for requiring the application from 'puppet/application', duplicating the logic from CommandLine. This commit moves the logic for listing available applications to the Application class, which delegates to its autoloader. Note the autoloader already takes gem and module paths into account, so no change in functionality there. This commit also deprecates the CommandLine available_subcomands instance and class methods.
Previously, calling Puppet::Interface.faces generated a different list of face applications than those included in Puppet::Application.available_application_names, since the former only considered the $LOAD_PATH, and the latter used an Autoloader, which includes gem directories and the current environment's modulepath. From what I can tell, the method is never used, as puppet itself accesses faces via Puppet::Interface[name, version] method. However, if someone had called `faces` it would have excluded faces that could be loaded via require.
Previously, face applications delivered as a module could not be loaded. This is because FaceCollection has problems loading code. For one, it require's faces and actions within, but load's actions that reside in different files. So it conceivable that a reloaded face application could run with older faces, but newer actions. Second, if we load the face, it will call Puppet::Interface.define, which will check if the face has already been loaded by calling FaceCollection[name, version]. But that will attempt to load the face that we're currently loading, getting into an infinite loop. Third, the help face application gets a list of available application names from Application. This list includes applications from gem and module paths. However, the help face application fails to load faces which reside in the modulepath, since it's using require and not going through the Autoloader. While faces could be modified to use an Autoloader, the longer term plan is to stop pretending we can reload code in ruby, i.e. always require based on the current $LOAD_PATH. This commit adds a `each_plugin_directory` method that yields each module plugin directories based on the current environment. It also modifies the CommandLine to append those directories, unless we are loading the master or agent applications. In the case of an agent, the libdir is always in the $LOAD_PATH, and it will load pluginsync'ed files from there. This aspect of puppetlabs#4248 has been fixed for sometime. In the case of master, we compile catalogs in the context of the client's environment, not necessarily the environment the master itself is configured to use. As such, we only want to autoload custom functions from those modules, not require them. In other words, we don't want those modules in the $LOAD_PATH to start with. This aspect of puppetlabs#4248 (the ability to load utility module code from a custom function) is not fixed, and can't really be fixed until we move to one process per environment compilation. In the case of apply, we compile catalogs, but always in its current environment, so we can safely append to the $LOAD_PATH. And same for face applications. As a result, the help face can load other faces from modules.
In commit 6f38feb for #12763, we no longer prevent actions from being reloaded, e.g. during pluginsync. But this only applies for "normal" actions, not script actions. This commit makes a similar change for script actions so they're consistent. With that said, the whole reloading of actions is dubious. If the action is declared within the body of the face, then the action will be required along with the face, and neither can be reloaded. Only if the action is in a separate file will it be loaded via the Autoloader, and therefore be reloadable. Also, it doesn't appear we can reload default actions, though we can reload script actions (if they were originally loaded).
This acceptance tests verifies that we can load a face application from an environment-specific modulepath. It ensures the face is an available application, that we can get help for the face, and that we can execute a simple action, and an action that requires utility code to perform the action.
Contributor
Author
|
@zaphod42 I've updated the PR, except for the comments about expand, the extension list, and the acceptance test. For the first two, I'm thinking of another set of commits to refactor all of the places where we call autoloader.files_to_load. It turns out we always do the same thing, File.basename(dir, '.rb') or chomp('.rb'). As for the acceptance test, I tried to re-write it using the other util methods and it was coming out a lot more verbose, because you can't use a HERE doc with the create_test_file, at least it didn't seem to work that way. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
These changes add modulepath lib directories to the $LOAD_PATH for applications other than agent or master. In the case of agent, modules are pluginsynced to the libdir, so it can already load faces, e.g. if you had a provider that needed to execute a face application. In the case of a master, we already use the autoloader to load code, e.g. functions and reports, from environment specific modulepaths.
These commits do not solve issue http://projects.puppetlabs.com/issues/4248, which is the ability for master-side code to require utility code from modules. Additional changes will be required to support that.
There is also still an issue with cloud provisioner, since the node_aws face uses the --tags option that conflicts with puppet's global option of the same name. This is issue http://projects.puppetlabs.com/issues/16651.