jhoblitt / puppet-ganglia

Manages ganglia gmond & gmetad daemons + web front end
Other
12 stars 34 forks source link

Possible wrong Stdlib type in gmond.pp #95

Closed jpn25 closed 2 years ago

jpn25 commented 3 years ago

Hi

Just been using your module and have been getting this error:

Class[Ganglia::Gmond]: parameter 'cluster_url' expects a match for Stdlib::Fqdn = Pattern[/\A(([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9-][a-zA-Z0-9]).)([A-Za-z0-9]|[A-Za-z0-9][A-Za-z0-9-]*[A-Za-z0-9])\z/], got 'http://******/ganglia

Should the relevant line in gmond.pp be: Optional[Stdlib::HTTPUrl] $cluster_url = undef, rather than: Optional[Stdlib::Fqdn] $cluster_url = undef,

Regards John

frizop commented 3 years ago

https://github.com/jhoblitt/puppet-ganglia/blob/master/spec/classes/gmond_spec.rb#L70

No, it should be a Fqdn, see above test coverage

Edit: https://forge.puppet.com/modules/puppetlabs/stdlib#stdlibfqdn as well

jpn25 commented 3 years ago

Yes, but the gmond.conf man page (https://manpages.debian.org/testing/ganglia-monitor/gmond.conf.5.en.html) suggests that the cluster section should have the following:

The cluster section has four attributes: name, owner, latlong and url.

   For example,

     cluster {
       name = "Millennium Cluster"
       owner = "UC Berkeley CS Dept."
       latlong = "N37.37 W122.23"
       url = "http://www.millennium.berkeley.edu/"
     }

the url should contain a url, not just the fqdn.

Regards John

frizop commented 3 years ago

That might be correct for the product (or the end result) but it would be a breaking change to all those people who have the module installed.

ph448 commented 3 years ago

@frizop I think if this would be a breaking change for many you'd have had this bugreport a long time ago, as cluster_url has been in there since 2012. in any case, couldn't you just bump the major version number to let users know of an incompatible change?

jhoblitt commented 2 years ago

I concur that it should be Optional[Stdlib::HTTPUrl].

jhoblitt commented 2 years ago

Rather, lets go with Optional[Variant[Stdlib::HTTPUrl,Stdlib::Fqdn]] so that it won't break existing manifests.

jhoblitt commented 2 years ago

Fix released as part of https://forge.puppet.com/modules/jhoblitt/ganglia/4.0.0/readme