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

Implement self.prefetch for zabbix_host #591

Conversation

baurmatt
Copy link
Contributor

Pull Request (PR) description

This commits adds the following improvements to zabbix_host:

  • Implement self.prefetch (This allows the use of puppet resource zabbix_host as well as purging unmanaged zabbix_host resources)
  • Implement proper setters for all properties
  • Rename the group parameter to groups to allow multiple groups to
    be specified

This is another change for #570.

This Pull Request (PR) fixes the following issues

@@ -223,6 +226,7 @@
$monitored_by_proxy = $zabbix::params::monitored_by_proxy,
$agent_use_ip = $zabbix::params::agent_use_ip,
$zbx_group = $zabbix::params::agent_zbx_group,
$zbx_groups = $zabbix::params::agent_zbx_groups,
Copy link
Member

Choose a reason for hiding this comment

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

Please add a datatype for the new option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done :)

@bastelfreak bastelfreak added enhancement New feature or request needs-work not ready to merge just yet labels Feb 11, 2019
@bastelfreak
Copy link
Member

@baurmatt can you take a look at the rubocop issues and add some tests for it? Thanks for the great work!

@baurmatt baurmatt force-pushed the feature/upstream_zabbix_host_self.prefetch branch from f31598e to e40fbb3 Compare February 11, 2019 14:04
@baurmatt
Copy link
Contributor Author

@bastelfreak Thanks! :) I've fixed the rubocop issues. Will take a look at unit tests for the type/provider.

This commits adds the following improvements to zabbix_host:
* Implement self.prefetch (This allows the use of `puppet resource
zabbix_host` as well as purging unmanaged zabbix_host resources)
* Implement proper setters for all properties
* Rename the `group` parameter to `groups` to allow multiple groups to
be specified

This is another change for voxpupuli#570.
@baurmatt
Copy link
Contributor Author

@bastelfreak I've implemented all tests for the custom type. But the provider seems to be out of my league. From my understanding, that would require a complete mock of the Zabbix API.

@baurmatt baurmatt force-pushed the feature/upstream_zabbix_host_self.prefetch branch from e40fbb3 to a4750d4 Compare February 19, 2019 14:55
@bastelfreak bastelfreak removed the needs-work not ready to merge just yet label Feb 19, 2019
@bastelfreak
Copy link
Member

Thanks for all the work!

@bastelfreak bastelfreak merged commit 580b484 into voxpupuli:master Feb 19, 2019
@guzmanbraso
Copy link

Hey, this commit broke our environments as the parameter group of manifests/resources/agent.pp was not renamed:
Error: Could not retrieve catalog from remote server: Error 500 on SERVER: Server Error: no parameter named 'groups' (file: /etc/puppetlabs/code/environments/x/modules/zabbix/manifests/resources/agent.pp, line: 38) on Zabbix_host[x] (file: /etc/puppetlabs/code/environments/x/modules/zabbix/manifests/resources/agent.pp, line: 38) on node x

@baurmatt
Copy link
Contributor Author

@guzmanbraso First of all, sorry that it broke your environment! I will look into it and open up a new PR with the fix soon.

@baurmatt
Copy link
Contributor Author

Hey @guzmanbraso, so I've just tried to understand what the problem could be, but sadly failed. manifests/resources/agent.pp still has the group parameter additionally to the new groups parameter:

$group = undef,
Array[String[1]] $groups = undef,

Could you please provide more information about what the problem is?

@alexjfisher
Copy link
Member

@guzmanbraso Are you running puppet generate types to prevent environment isolation issues?

To me, it looks like your puppetserver doesn't realise the type contains a new groups parameter.

@guzmanbraso
Copy link

@baurmatt, no need to be sorry, my message was not a complain, just wanted to share that information wiyh you guys. Because $groups was defined with spec I missed when fast searching for it and took as granted that $group had to be renamed. My bad

@alexjfisher will check that out and get back to you, I did not tested if it was an environment isolation issue, thought it was something more basic but clearly it's not. I'll test that and get back to you.

Thank you everyone for your support!

@alexjfisher
Copy link
Member

There were similar errors reported when puppetlabs/apt gained a new parameter in one of its ruby types. See #569 (comment)

@hferrag
Copy link

hferrag commented Apr 16, 2020

Hey, this commit broke our environments as the parameter group of manifests/resources/agent.pp was not renamed:
Error: Could not retrieve catalog from remote server: Error 500 on SERVER: Server Error: no parameter named 'groups' (file: /etc/puppetlabs/code/environments/x/modules/zabbix/manifests/resources/agent.pp, line: 38) on Zabbix_host[x] (file: /etc/puppetlabs/code/environments/x/modules/zabbix/manifests/resources/agent.pp, line: 38) on node x

I had exactly the same problem as @guzmanbraso

@hferrag
Copy link

hferrag commented Apr 16, 2020

I had exactly the same problem as @guzmanbraso
Error: Could not retrieve catalog from remote server: Error 500 on SERVER: Server Error: no parameter named 'groups' (file: /etc/puppetlabs/code/z/modules/zabbix/manifests/resources/agent.pp, line: 39) on Zabbix_host[z] (file: /etc/puppetlabs/code/z/modules/zabbix/manifests/resources/agent.pp, line: 39) on node my-node.local

Fixed after running puppet generate types --environment my_env.

Thanks @baurmatt and @alexjfisher for your support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants