Skip to content
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

Possible wrong Stdlib type in gmond.pp #95

Closed
jpn25 opened this issue Feb 16, 2021 · 7 comments · Fixed by #112
Closed

Possible wrong Stdlib type in gmond.pp #95

jpn25 opened this issue Feb 16, 2021 · 7 comments · Fixed by #112

Comments

@jpn25
Copy link

jpn25 commented Feb 16, 2021

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
Copy link
Collaborator

frizop commented Feb 16, 2021

@jpn25
Copy link
Author

jpn25 commented Feb 16, 2021

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
Copy link
Collaborator

frizop commented Feb 16, 2021

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
Copy link

ph448 commented Feb 16, 2021

@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
Copy link
Owner

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

@jhoblitt
Copy link
Owner

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

@jhoblitt
Copy link
Owner

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

Successfully merging a pull request may close this issue.

4 participants