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

`Vagrant.has_plugin?` should use the gem name #2617

Merged
merged 1 commit into from Dec 20, 2013

Conversation

Projects
None yet
6 participants
@tmatilai
Copy link
Collaborator

tmatilai commented Dec 16, 2013

As discussed in the comments of #2189, for consistency Vagrant.has_plugin? should probably use gem names instead of the name attribute of the plugins. The gem name is already used for example in vagrant plugin install/list and Vagrant.require_plugin.

Also it requires extra effort to search the name attribute. And the plugin authors might even change it between releases.

@mmgaggle

This comment has been minimized.

Copy link

mmgaggle commented Dec 9, 2013

+1 FWIW, this behavior confused me when I tried to do this:

https://gist.github.com/mmgaggle/7882220

@tralamazza

This comment has been minimized.

Copy link

tralamazza commented Dec 10, 2013

While it's true that name should probably be just the gem's name we cannot ignore that Vagrant is deployed. I would phase it out slowly, i.e. have has_plugin? recognize both current name and the gem name initially, then slowly deprecate it (with a warning possibly if the old name way is used).

@tmatilai

This comment has been minimized.

Copy link
Collaborator

tmatilai commented Dec 10, 2013

@tralamazza agreed, good idea.

@mitchellh

This comment has been minimized.

Copy link
Member

mitchellh commented Dec 10, 2013

I agree as well. I've tagged this as a bug and would be happy to see this in 1.4.x. Should be pretty easy.

@tmatilai

This comment has been minimized.

Copy link
Collaborator

tmatilai commented Dec 15, 2013

I was taking a closer look on this, and unfortunately it doesn't seem trivial. Hopefully I'm missing something.

It's easy to check if a gem with the specified name is found, but how to map the gem name to registered plugins?
One solution could maybe be to parse the plugin's path from caller list in Plugin.name and store it to a class variable (for the inheriting class). Then in has_plugin? we could check if that is a subdirectory of a matched gem.

Does anyone come up with a simpler solution? I'll hopefully have some hacking time later tonight so I can see if that works.

@rogeriopradoj

This comment has been minimized.

Copy link
Contributor

rogeriopradoj commented Dec 15, 2013

For now, I'm updating (slowly) the list on https://github.com/mitchellh/vagrant/wiki/Available-Vagrant-Plugins, informing for each one of plugins the rubygem name beside internal plugin name.

I've included this information there:

(*) There are two distinct names for each one of the plugins:

  • rubygem name (used for installing plugin, via $ vagrant plugin install rubygem-plugin-name)
  • internal name (used in Vagrantfile, via Vagrant.has_plugin?('Internal_Plugin_Name') )

The following list has both names in this order: rubygem name /internal name/

👍 for Vagrant.has_plugin? using rubygem name, as @tmatilai said!

@tmatilai

This comment has been minimized.

Copy link
Collaborator

tmatilai commented Dec 16, 2013

Well, at least it seems to work. I converted this issue to a PR to help discussion. 🐺

@fgrehm

This comment has been minimized.

Copy link
Collaborator

fgrehm commented Dec 16, 2013

I'm not sure this has been tried before but cant we just keep track of plugin names from successfull Vagrant.require_plugin calls into a Set object? It sounds a bit too simplistic but require_plugin is used by Vagrant internally when parsing the plugins.json file and it might make things easier :)

@tmatilai

This comment has been minimized.

Copy link
Collaborator

tmatilai commented Dec 16, 2013

@fgrehm good call.

The issue with require_plugin is that it's just a wrapper around Kernel#require with error handling. So it guarantees that the gem is loaded, but there's no check if the gem is actually a plugin, or that it registers itself with #name call. I think it would still be enough for us.

Better yet we could also double check that it is listed in plugin.json (i.e. installed explicitly with plugin install). Then the behavior would be in align with plugin list etc. But I don't know how to get handle to env in has_plugin?.

@fgrehm

This comment has been minimized.

Copy link
Collaborator

fgrehm commented Dec 17, 2013

@tmatilai I believe that keeping track of plugins loaded with require_plugin should be enough. But in case you want to build up the path to the plugins.json file to create a VagrantPlugins::CommandPlugin::StateFile to do your stuff (I'm assuming that's what you need an env for) you can have a look at this and this for inspiration. I think we won't be able to get hold of an Environment outside of "action stack" since there is no global instance of that class over here and has_plugin? is a class method :-)

I hope that helps!

@tmatilai

This comment has been minimized.

Copy link
Collaborator

tmatilai commented Dec 17, 2013

Thanks Fábio! Yeah, I had already looked at the code. We would also need this. So a lot of fragile duplication, or refactoring. That's why I was missing the env instance. =)

I can make a competing implementation using the require_plugin (only). Let's see how that looks. But can't promise when I get that done.

@fgrehm

This comment has been minimized.

Copy link
Collaborator

fgrehm commented Dec 18, 2013

Ok, just to be clear, I'm not advocating copy & pasting that stuff around 😜 I just wanted to point out the code you might be interested on looking at in case you wanted to go that direction 😃

But! Refactoring it out to a hypothetical tiny Vagrant::HomePath class or Vagrant::Util::HomePath module that encapsulates this logic shouldn't be that hard.

What do you guys think?

@tmatilai

This comment has been minimized.

Copy link
Collaborator

tmatilai commented Dec 18, 2013

Bueno, here is a new branch that uses Vagrant.require_plugin to store the gem name. I think the plugin manager is a good place for that list too.

What say you?

@tmatilai

This comment has been minimized.

Copy link
Collaborator

tmatilai commented Dec 20, 2013

👂 Quiet here... =)

Well, I swapped now the "fgrehm" branch in this PR. I feel it's enough even without plugin.json confirmation. We can add that later if needed. (The first implementation can still be found here.)

IMHO this is ready for reviewing.

@@ -164,10 +166,20 @@ def register(plugin)
end
end

# This registers a required plugin. This should _NEVER_ be called by
# the public and should only be called from within Vagrant.
def require_plugin(gem_name)

This comment has been minimized.

@fgrehm

fgrehm Dec 20, 2013

Collaborator

@tmatilai I think this method should be called something like plugin_required / register_plugin to better express its purpose :)
WDYT?

This comment has been minimized.

@tmatilai

tmatilai Dec 20, 2013

Collaborator

#register already exists (it's called from Plugin#name), so all *register* patterns are imho bad.

I think I chose #require_plugin as it's the same as in Vagrant from where it's called. Plain #require would be in align with register, but it shadows Kernel#require. And feels a bit confusing here.

Dunno, I'm totally fine with #plugin_required too.

This comment has been minimized.

@fgrehm

fgrehm Dec 20, 2013

Collaborator

Dunno, I'd probably change it to plugin_required because the method is not requiring the plugin code itself, it has already been done before getting here :-)
</nitpick>

This comment has been minimized.

@tmatilai

tmatilai Dec 20, 2013

Collaborator

That's a solid argument. And I don't find this nitpicking. Naming is really important. And difficult. So thanks! =)

@fgrehm

This comment has been minimized.

Copy link
Collaborator

fgrehm commented Dec 20, 2013

@tmatilai thanks for working on it :-) Apart from my silly nitpick, this LGTM but it would be nice to have a second 👍 before merging

/cc @mitchellh @phinze @gildegoma

@mitchellh

This comment has been minimized.

Copy link
Member

mitchellh commented Dec 20, 2013

I think this is good for now. Good job. I think eventually though I want a higher level plugin manager class because plugins will probably always be gems so I don't think it needs to necessarily go into the versioned manager. But that would be a much bigger change I wouldn't be comfortable making at the moment.

core: Vagrant.has_plugin? tries to match gem name first
Search primary from the list of gem names which have been loaded by
`Vagrant.require_plugin`. Fall back to matching registered plugin names.

tmatilai added a commit that referenced this pull request Dec 20, 2013

Merge pull request #2617 from tmatilai/has_plugin_by_gem_name
`Vagrant.has_plugin?` should use the gem name

@tmatilai tmatilai merged commit d9c804e into hashicorp:master Dec 20, 2013

1 check passed

default The Travis CI build passed
Details

@tmatilai tmatilai deleted the tmatilai:has_plugin_by_gem_name branch Dec 20, 2013

@rogeriopradoj

This comment has been minimized.

Copy link
Contributor

rogeriopradoj commented Dec 21, 2013

Excited to test it in Vagrant 1.4.2!!!

@rogeriopradoj

This comment has been minimized.

Copy link
Contributor

rogeriopradoj commented Dec 30, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment