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.

Sign up for GitHub

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

Jump to bottom

replace create_resources with resource types #874

Merged
merged 1 commit into from Mar 23, 2019
Merged

replace create_resources with resource types #874

merged 1 commit into from Mar 23, 2019

Conversation

zoojar
Copy link
Contributor

@zoojar zoojar commented Mar 20, 2019

Pull Request (PR) description

Replace create_resources with resource types

This Pull Request (PR) fixes the following issues

Fixes #716 (Replace create_resources() calls)

@juniorsysadmin juniorsysadmin added the enhancement New feature or request label Mar 20, 2019
alexjfisher
alexjfisher requested changes Mar 20, 2019
View reviewed changes
Copy link
Member

@alexjfisher alexjfisher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love the idea of getting rid of create_resources, but I think the replacement pattern used here is more complicated than it needs to be.

alexjfisher
alexjfisher reviewed Mar 21, 2019
View reviewed changes
manifests/plugin/postgresql.pp
$_manage_package = pick($manage_package, $::collectd::manage_package)
$databases_defaults = { 'ensure' => $ensure }
$queries_defaults = { 'ensure' => $ensure }
$writers_defaults = { 'ensure' => $ensure }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why create all these identical hashes?

@bastelfreak Did you not like #874 (comment) ?

Copy link
Contributor Author

@zoojar zoojar Mar 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looool i thought this too. It's one way of separating defaults for each resource

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

    collectd::plugin::postgresql::writer { $resource:
      * => { 'ensure' => $ensure } + $attributes,
    }

better?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With create_resources we had to pass a hash for the defaults (although it didn't strictly need to be assigned to a variable first).

Since we just have a single default property (ensure), putting it directly in the resource declaration is probably cleaner??

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

    collectd::plugin::postgresql::writer { $resource:
      * => { 'ensure' => $ensure } + $attributes,
    }

better?

No need to merge hashes. Just...

collectd::plugin::postgresql::writer { $resource:
  ensure => $ensure,
  *      => $attributes,
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That depends on $attributes not containing an override for ensure though. (which might be a perfectly reasonable assumption?)

Copy link
Contributor Author

@zoojar zoojar Mar 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

they do contain them....
https://github.com/voxpupuli/puppet-collectd/blob/master/manifests/plugin/postgresql/writer.pp#L3
default is 'present' anyhow

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually... I don't think that is a safe assumption.
* => { 'ensure' => $ensure } + $attributes, works for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. well, we still get one thing... replacement of create_resources :)

bastelfreak
bastelfreak approved these changes Mar 22, 2019
View reviewed changes
Copy link
Member

@bastelfreak bastelfreak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexjfisher I'm fine with this. What do you think?

@bastelfreak bastelfreak merged commit 8f5ff44 into voxpupuli:master Mar 23, 2019
traylenator added a commit to traylenator/puppet-collectd that referenced this pull request Mar 23, 2020
@traylenator
processmatch type now configures something
6d67ed2 
Since MR voxpupuli#874 the processmatch
type is not working.

For instance a configuration of

```puppet
collectd::plugin::processes::processmatch{'eosd':
  collect_file_descriptor => true,
  collect_memory_maps     => true,
  collect_context_switch  => false,
  regex => 'eosd .*$',
}
```

should create a file (on centos) `/etc/collectd.d/processes-config.conf` containing

```apache
<Plugin processes>
  <ProcessMatch "eosd" "eosd .*$">
    CollectContextSwitch false
    CollectFileDescriptor true
    CollectMemoryMaps true
  </ProcessMatch>
</Plugin>
```

but alas no configuration is created anywhere, the file itself is not even created.
@traylenator traylenator mentioned this pull request Mar 23, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexjfisher alexjfisher alexjfisher approved these changes

@bastelfreak bastelfreak bastelfreak approved these changes

@juniorsysadmin juniorsysadmin juniorsysadmin approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Replace create_resources() calls
4 participants
@zoojar @alexjfisher @bastelfreak @juniorsysadmin