Ticket/2.7.x/11802 puppet module list#333
Conversation
This was only being used in one place, and the method itself is fairly useless since it's passed an environment, and then just uses the environment to find the modulepath. It's better to just use the environment in the place the Module.modulepath was being called.
It looks like this was meant as a way to manage the list of module dependencies, but it wasn't being called and the dependencies weren't getting loaded from the metadata. Something like this will be added back when we get dependency info working.
These specs were creating environments when they didn't need to, and not testing what the description said. The test to show that a string was accepted passed an environment object.
A test environment was being setup repeatedly so I just moved it into a let block. I also used real modulepaths on the environments instead of stubbing the value since it's shorter and easier to read.
For the `module list` command we want to be able to tell about all the modules in multiple directories of an environment's modulepath. Currently, if we have a module with the same name in two different directories of the modulepath it's impossible to get information on it since Puppet::Module.new takes a name and environment will always assume the path for the module is in the first directory of the environment's modulepath. This change allows the module's path to be passed in so that you can instantiate a Puppet::Module for any module location.
There's already a modules method on environments that returns your modules, but only the first one in the module path. For example, if you have a modulepath with multiple directories, and a module is in both directories, you only ever info back about the module in the first directory in the modulepath. For the `module list` command we need to show details about all the modules split up by path.
This action will allow users to get more information about their installed modules, making managing them easier. For now this just lists the modules split out by path, with the paths able to be specified with modulepath or environment options and defaulting to Puppet's modulepath.
There was a problem hiding this comment.
It doesn't look like these get mutated, so they could probably be let methods, right?
There was a problem hiding this comment.
They could be, but I think it's easier to read to have them next to where they're used to form the composite modulepath, and it's not like they're slow to create. Otherwise you end up with a couple lets that all depend on each other and I found that harder to reason about - not much, but I still prefer keeping the related stuff in the before block.
|
That looks solid. I am going to wait on Kelsey rather than merging it myself. |
|
Daniel, why are you waiting on Kelsey? Aren't you supposed to merge things? |
|
In this case, I want to make sure he has a chance to look at this and catch any issues that are project specific rather than generic; I don't feel comfortable that I know enough about the semantics of the change to be merging it yet. If he wasn't available or whatever, then I could merge it, but it doesn't seem necessary right now. |
|
+1 on this commit, Got a chance to play with it, and everything looks good to me. |
…e_list Ticket/2.7.x/11802 puppet module list
Revert "Merge pull request puppetlabs#329 from mruzicka/PCP-206_update_module_result_structure"
3 participants
This series of commits adds the ability to list the modules that are installed with options to let you look at other environments and module paths.
This involves some cleanup and enhancements to Puppet::Module and Puppet::Node::Environment.