Skip to content
This repository was archived by the owner on Sep 26, 2025. It is now read-only.

Misc. fixes and improvements#69

Open
bugfood wants to merge 7 commits into
signalfx:masterfrom
ifwe:for-upstream
Open

Misc. fixes and improvements#69
bugfood wants to merge 7 commits into
signalfx:masterfrom
ifwe:for-upstream

Conversation

@bugfood

@bugfood bugfood commented Sep 18, 2019

Copy link
Copy Markdown

Hi,

We've been using this repository internally and carrying some local changes. This PR consists of the most important of those changes, updated and cleaned up.

The only change to overall behavior is in the final patch, which makes a notify non-default. If that change is controversial, I can drop that patch individually.

Thanks,
Corey

ctoa and others added 7 commits September 18, 2019 11:44
By not sorting the hash key, this will most likely change on every
puppet run.

(Two changes by Christi Toa squashed into one)
Ensure that the keys are values are string else a Fixnum error could be
thrown.
In a large managed environment, common practice is to block direct
Internet access from most hosts and to store all packages in locally
curated repositories.

This provides a parameter to block the default behavior of installing a
remote repository. The user is then responsible for putting appropriate
packages in a local repository that is accessible to the client hosts.
By default, the module reports a message:
Notice: /Stage[main]/Collectd::Plugins::Signalfx/Notify[Collectd will
transmit metrics to this url: https://ingest.signalfx.com/v1/collectd?
[...details omitted...]

The message seems useful for debugging, but not everybody will want this
printed every time.

This provides a parameter to disable that message.
I can't find any published rules/conventions about this, but I have not
yet encountered another module that reports informational messages like
this.

Personally, I like puppet runs to be quiet unless puppet is applying an
actual change. Gratuitous notification makes it looks like a change is
happening.
This allows for data types to be used, among other things.
https://puppet.com/docs/puppet/4.10/lang_data_type.html

I think this is justified due to fairly maintream puppet modules no
longer supporting old versions. Many common modules require 5.x, even.

Just a few examples:
elastic-elastic_stack: ">= 4.13.1 < 7.0.0"
puppetlabs-apt: ">= 5.5.10 < 7.0.0"
puppetlabs-docker: ">= 5.5.10 < 7.0.0"
puppetlabs-stdlib: ">= 5.5.10 < 7.0.0"
This fixes a long-winded deprecation warning:

Warning: This method is deprecated, please use the stdlib validate_legacy function,
                    with Stdlib::Compat::Hash. There is further documentation for validate_legacy function in the README. at ["/etc/puppetlabs/code/modules/collectd/manifests/plugins/apache.pp", 10]:["/etc/puppetlabs/code/environments/production/modules/webconf/manifests/apache.pp", 69]
   (at /etc/puppetlabs/code/modules/stdlib/lib/puppet/functions/deprecation.rb:28:in `deprecation')
Warning: This method is deprecated, please use the stdlib validate_legacy function,
                    with Stdlib::Compat::String. There is further documentation for validate_legacy function in the README. at ["/etc/puppetlabs/code/modules/collectd/manifests/plugins/plugin_common.pp", 13]:
   (at /etc/puppetlabs/code/modules/stdlib/lib/puppet/functions/deprecation.rb:28:in `deprecation')
@atoulme

atoulme commented Oct 5, 2022

Copy link
Copy Markdown
Contributor

Closing for inactivity. Please reopen if these changes are still needed.

@atoulme atoulme closed this Oct 5, 2022
@bugfood

bugfood commented Oct 5, 2022

Copy link
Copy Markdown
Author

@atoulme is this repo abandoned?

If the repo is still active, then I think the PR is still useful. We're still using it locally.

@atoulme atoulme reopened this Oct 5, 2022
@atoulme

atoulme commented Oct 5, 2022

Copy link
Copy Markdown
Contributor

Just doing the rounds and making sure we close inactive pull requests. Thanks for replying! I have reopened the PR.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants