Add autorequire for Package['git']

[reidmv]

If the git package is being managed, it stands to reason that the git package should be installed before trying to potentially manage git repositories using vcsrepo resources.

This commit adds an autorequire to the vcsrepo type that reflects the above premise.

[jhoblitt]

I agree that this is makes sense but I believe the conclusion of the last debate on this topic was not to do autorequire. If this is to be merged, it would be nice if it included a test.

[sodabrew]

Right - the major issues were the name of the git package on various systems (e.g. git-core is a Debian-ism), and that it doesn't work if git is not represented by a package.

[jhoblitt]

There's also the issue of requiring a package resource instead of a class. It depends on the intended usage but I have setup ssh keys before most of usage of the vcsrepo type.

Still, I don't think autorequire really hurts that much since it does nothing unless the package is already in the manifest. Couldn't we just specify the packages name for the various different platforms?

[sodabrew]

And then do it for each type of VCS that this module supports and each platform that we want to support. I'd consider a comprehensive pull request, but prefer not to do it piecemeal.

[reidmv]

This pull request is for is an incremental improvement of the vcsrepo type. Assuming that the addition is determined to be a desirable behavior, git-specific autorequire is a first iteration.

Adding additional package names to the list is certainly something that could happen down the road, but I disagree that it should be a prerequisite for making a decision on whether to merge this pull request or not. Adding this autorequire will have tangible benefits now to people using the git provider for vcsrepo. It's not comprehensive. It's an iteration. If it turns out that there are scenarios in which the autorequire would be undesirable (I can't think of any), it would be better to minimize the blast zone and release small changes like this one on an agile cadence rather than bunching up changes together into larger-impact artifact.

I can write a trivial test for the autorequire in a bit. However it would be helpful to get a firm feel for whether or not iterating in this direction is something that the maintainers of this module find useful outside of a comprehensive change which would be harder to test and get feedback on.

[apenney]

I personally disapprove of this, because as mentioned in the comments there's too many git package names to be comfortable trying to auto require like this. Having said that I would be OK with an enhancement to vcsrepo to include something like vcsrepo::setup::git or so forth that defined a git package and then autorequired it from in the providers.

I feel like if we're autorequiring it we should be managing it too. The biggest issue we have is that autorequire doesn't work for classes (last I checked) or I would have suggested we just autorequire the Class['git'] and allow 3rd party modules to fill in the gaps here.

Either way, I'm nervous about forcing distribution specific information into a provider. If we're going to go that route we should write manifests that install git and just autorequire those resources for safety.

[hunner]

Ever since puppet 2.7.8, providers delay processing until it is suitable (lazy suitability). Suitability is determined in part by all binaries specified with commands being present. The git provider for vcsrepo has this already https://github.com/reidmv/puppetlabs-vcsrepo/blob/2b190756260346931b8f9a0dda8afc0c815710d6/lib/puppet/provider/vcsrepo/git.rb#L7-L8 (it's actually optional_commands because it predates puppet 2.7.8 when commands was strict instead of lazy, but now has the same effect).

Unless I'm missing something, this PR shouldn't be needed. Did you see behavior where it failed because /something/bin/git wasn't yet available, even though git was to be installed later in the puppet run?

[reidmv]

Yes, it absolutely doesn't work without the autorequire or explicit dependencies in Puppet code. E.g. consider A) the jpadams/puppet_vim_env module, which makes heavy use of vcsrepo resources, alongside B) a simple Package['git'] resource on a fresh system. As implemented the vcsrepo resources will NOT be intelligently ordered with regards to the Package['git'] resource, and initial runs will fail as the vcsrepo resources are evaluated prior to git being available.

EDIT1: This is using module versions as published on the Forge and PE 3.1.0.
EDIT2: clarified wording

[jhoblitt]

@reidmv One solution for that case is to add Package['git'] -> Vcsrepo<| |> to the manifest.

[reidmv]

@jhoblitt Absolutely. That's one form of the "explicit dependencies in Puppet code" solution. On a case-by-case basis those things can be added. This pull request is not suggesting that can't be done; it's aim is to open a discussion about whether or not we can use autorequire to eliminate the need to.

With regards to unfiltered collectors specifically, I personally dislike that solution due to the note here: "Chained collectors [...] can also be dangerous when used with virtual resources, which are implicitly realized by collectors." This means that the syntax you've given will have the side effect of realizing ALL virtual instances of type Vcsrepo, if virtual resources are being used. Given the low incidence of virtual resource use, that's a solution that will work for most people but it does have some very dangerous edge cases.

EDIT: grammar

[mbrodala]

@reidmv Well, it could be Package['git'] -> Vcsrepo <| provider == git |> to reduce the impact.

[reidmv]

@mbrodala Unfortunately that modification has potential to reduce the blastzone, but the primary concern is left unresolved. Namely, that if you use virtual resources of type Vcsrepo, an unknown number of them could unintentionally be deployed to a system due to this syntax. Chances are if you're managing via multiple repositories and at least one of them is git, there are more that are git.

Autorequires are safer in that they ONLY have an effect IF e.g. you have explicitly defined a Package['git'] resource that is being included in your catalog, and if something goes wrong the worst thing that can happen is a dependency cycle which will cause an error to be thrown by the compiler. No unexpected changes will be made to a system, and no code will be deployed unintentionally.

@hunner and/or @apenney please provide some direction on this issue. If you feel that trying to intelligently order Vcsrepo instances is fundamentally a bad idea, please express your concerns and close the pull request. If you feel that the idea is sound but there are implementation problems (needs to only take effect on RedHat/Debian osfamily, needs to take into consideration provider (is that possible?), etc), please provide feedback, objections, and possible resolutions.

Thanks!

[apenney]

I struggle to imagine an implementation I'd be happy with that:

a) doesn't require managing git via puppet.
b) works for all the systems that can run git

Yes, we could extend this to check osfamily and only work against RedHat/Debian but that's not a drastically better solution and it pushes os specific stuff into the type where it doesn't belong. I still don't understand why this can't be managed at the Puppet layer. What's wrong with expressing Package['git'] -> Vcsrepo <| provider == git |> within your manifests, why does it have to happen at the type?

I still feel that if we want vcsrepo{} to rely on git then the appropriate solution is to have a package { 'git': } in vcsrepo and do the appropriate thing to support distributions there. If we want a dependency then we should make one and manage it, and not rely on them hopefully defining the right package name for us.

[apenney]

@reidmv Just blew my mind by telling me that autorequires is a soft dependency and this won't do anything if the packages aren't in the catalog, making it a safe change. I had no idea.

[jhoblitt]

I think this is going to turn into a bottomless rabbit hole. Are we going to require ssh as well?

[reidmv]

@jhoblitt after having talked to Ash I think there may be some misunderstanding of what autorequire does. It's basically a soft-dependency, which is only evaluated after the parser has finished going through all the manifests to be included, and it will establish a relationship if-and-only-if the referenced resource happens to have been declared elsewhere. If the resource isn't found, an autorequire does exactly nothing. Have you read the autorequire section on docs.puppetlabs.com? http://docs.puppetlabs.com/guides/custom_types.html#automatic-relationships

[jhoblitt]

@reidmv I understand how autorequire works. The point is that merely installing the git package isn't enough to make it function for the majority of use cases.