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

Upgrade zabbixapi package for Zabbix 4.2 support #623

Closed
wants to merge 4 commits into from

Conversation

Bouke
Copy link

@Bouke Bouke commented Sep 19, 2019

Pull Request (PR) description

zabbixapi 4.0.0 doesn't support Zabbix 4.2.

This Pull Request (PR) fixes the following issues

@bastelfreak
Copy link
Member

Hi, thanks for the PR. Can you please rebase after #624 got merged?

@bastelfreak bastelfreak added bug Something isn't working needs-rebase labels Sep 23, 2019
@Bouke
Copy link
Author

Bouke commented Sep 25, 2019

I think you can merge the PR and have GitHub rebase it?

@bastelfreak
Copy link
Member

@Bouke yes that works. But that won't kick of travis before the merge. Please rebase. Afterwards travis will test your changes.

@Bouke
Copy link
Author

Bouke commented Oct 8, 2019

I've rebased the PR.

@bastelfreak
Copy link
Member

@Bouke can you please have a look at the failing travis tests?

@Bouke
Copy link
Author

Bouke commented Oct 9, 2019

It looks like the new API version has an unsolved issue which the tests run into express42/zabbixapi#99. I think the only solution here is to wait until that bug is resolved in a future release.

@Bouke
Copy link
Author

Bouke commented Oct 24, 2019

If anybody knows how to deploy a fork hosted on github using puppet_gem package provider, it would be very helpful.

@Bouke
Copy link
Author

Bouke commented Jan 7, 2020

With the following in place, everything seems to work fine on Zabbix 4.4. It just logs a lot of "unsupported messages", but new resources seem to be added just fine.

    Package<| title == 'zabbixapi' |> {
        ensure => 'latest',
    }

@baurmatt
Copy link
Contributor

baurmatt commented Mar 2, 2020

@Bouke I would prefer not to add 'latest' to packages because it will update the package as soon as a new release gets out. Can we just set the variable to "4.1.2"? That's the latest release currently available on Rubygems.

@bastelfreak bastelfreak added the needs-feedback Further information is requested label Mar 2, 2020
@ekohl ekohl mentioned this pull request Apr 16, 2020
@vox-pupuli-tasks
Copy link

Dear @Bouke, thanks for the PR!

This is Vox Pupuli Tasks, your friendly Vox Pupuli Github Bot. I noticed that your pull request has CI failures. Can you please have a look at the failing CI jobs?
If you need any help, you can reach out to us on our IRC channel voxpupuli on Freenode or our Slack channel voxpupuli at slack.puppet.com.
You can find my sourcecode at voxpupuli/vox-pupuli-tasks

@Bouke
Copy link
Author

Bouke commented Aug 24, 2020

@Bouke I would prefer not to add 'latest' to packages because it will update the package as soon as a new release gets out. Can we just set the variable to "4.1.2"? That's the latest release currently available on Rubygems.

Ideally as a consumer of this module, I'd like the ability to override such versions. Currently it just hardcodes some version (either 'latest' or '4.1.2'). Maybe we can change the parameters to allow overriding the version?

@bastelfreak
Copy link
Member

I agree that this should be a parameter. But we also need to provide sane default that work with up2date zabbix versions. I did a lot of fiddling around in https://github.com/voxpupuli/puppet-zabbix/pull/684/files to provide matching gem versions, but I failed hard (please ignore the labels).

@Bouke
Copy link
Author

Bouke commented Aug 27, 2020

I've rebased the PR and made the change to hardcode version 4.1.2. So should be all good now.

@bastelfreak
Copy link
Member

I now see the following message in the acceptance test output:

  [WARN] Zabbix API version: 3.0.31 is not supported by this version of zabbixapi

also we've a few places where we hardcode the zabbix version in the test to 3 because of old limitations from the gem:
https://github.com/voxpupuli/puppet-zabbix/blob/master/spec/acceptance/zabbix_template_spec.rb#L17
I think we should try to test all the supported zabbix versions to ensure we choose the correct zabbixapi gem version?

@Bouke
Copy link
Author

Bouke commented Sep 1, 2020

I agree that it would be good for this package to have good test coverage against recent releases of the various tools it configures. Meanwhile I would also suggest to leave that out of the scope of this PR.

@bastelfreak
Copy link
Member

@Bouke can you please rebase against our latest master branch? I still think that the zabbix versions in our acceptance tests should be updated in this PR. The PR claims to support zabbix 4.2, but we only test on 3.0.

@bastelfreak
Copy link
Member

thanks for all the work everybody. we merged the changes in #730

@bastelfreak bastelfreak added skip-changelog and removed bug Something isn't working needs-feedback Further information is requested needs-rebase tests-fail labels Jan 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants