missing dependency in metdata.json (theforeman-git)

[term1n4l]

Based on puppet::server::config we are missing 'git':

manifests/server/config.pp, Line 253 says 'include git'

going back through the version history to 1.4.0, this is a reference to 'theforeman-git' as specified in 'Modulefile':

Modulefile, Line 12

As this is intended to be here still for managing git repos, I suggest adding the following to the dependencies:

    {
      "name": "theforeman/git",
      "version_requirement": ">= 6.3.0 < 8.0.0"
    },

This will cover Puppet 5, 6, 7, however if you would rather deprecate 5 like most other modules have done so far then:

    {
      "name": "theforeman/git",
      "version_requirement": ">= 7.0.0 < 8.0.0"
    },

This will provide the missing dependency specification in the module data on the forge. the below is an example of the updated metadata.json

metadata.json

[term1n4l]

given time, I may be able to create a PR for this, but putting the issue in here for awareness while I am thinking of it

[ekohl]

It's mentioned in https://github.com/theforeman/puppet-puppet#git-repo-support that it's needed. Soft dependencies in Puppet aren't great to say the least.

Perhaps it would be better to have something like extlib::has_module. So we can change this part here:

puppet-puppet/manifests/server/config.pp

Lines 253 to 254 in 01a716c

	if $puppet::server::git_repo {
	include git

To

  if $puppet::server::git_repo {
    unless extlib::has_module('theforeman/git') {
      fail('theforeman/git module is not present')
    }
    include git

Alternatively you could use load_module_metadata to load the metadata and verify the version is compatible. I can also imagine has_module could be extended with another argument to verify the version.

[term1n4l]

Making it a hard dependency means it should to be visible to the end user also. Without having it visible in the dependencies on the forge it might be missed. By all means make it a hard dependency, but please consider adding it to the module metadata so it's picked up by the forge as well as some people will only do a cursory scan of the README and still miss that it has undefined dependencies.