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

Changes for BUG opened: 56995 #56996

Merged
merged 8 commits into from Aug 24, 2020
Merged

Changes for BUG opened: 56995 #56996

merged 8 commits into from Aug 24, 2020

Conversation

bfdacosta
Copy link
Contributor

@bfdacosta bfdacosta commented Apr 30, 2020

What does this PR do?

Fix mediatype_create and mediatype_get usage since new API 4.4.

  • Added a apiinfo_version call to check the API version when
    dealing with mediatype_create and get.
  • Changed the attribute called from "description" to "name",
    when API version is greater or equal to 4.4. changed both
    mediatype_create and mediatype_get.

What issues does this PR fix or reference?

Fixes #56995

Commits signed with GPG?

No

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

@bfdacosta bfdacosta requested a review from a team as a code owner April 30, 2020 02:40
@ghost ghost requested review from DmitryKuzmenko and removed request for a team April 30, 2020 02:40
@DmitryKuzmenko
Copy link
Contributor

@bfdacosta thank you for contribution! Could you please also write a regression tests for your changes?

@bfdacosta
Copy link
Contributor Author

DmitryKuzmenko, greetings! Can you point me out a regression test that is needed. Any example code for that ? Thanks !

@mchugh19
Copy link
Contributor

@bfdacosta there are unit tests for the zabbix module in salt/tests/unit/modules/test_zabbix.py. You could create a test which mocks a version response of 4.3 and validate that the json is generated in the expected format. This could then be copied and modified for the 4.4 version and expected response.

@DmitryKuzmenko
Copy link
Contributor

Thank you @mchugh19 .
@bfdacosta do you need any advice on that?

@dwoz dwoz added Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases has-failing-test labels May 12, 2020
@bfdacosta
Copy link
Contributor Author

I will produce the test.
The problem is not the answer, its how the call to the API is done. Thats is the difference between versions. I will lookup for the test.

@DmitryKuzmenko
Copy link
Contributor

@bfdacosta I think you can mock it with stubs just checking the logic goes in a proper ways with different stubbed version values. Also you can add links to the corresponding documentation to the stubs to make it easily understandable. I don't think we have to test this particular change with a real zabbix servers.

@bfdacosta
Copy link
Contributor Author

Test added to the code.

@bfdacosta
Copy link
Contributor Author

@DmitryKuzmenko let me know if anything else still needed for merge! Thanks!

@dwoz dwoz removed the Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Jul 28, 2020
@DmitryKuzmenko
Copy link
Contributor

@bfdacosta yes, could you please update your branch and re-run pre-commit on it? There're some issues

05:38:19  Drop six usage and Py2 support...........................................Failed
05:38:19  - hook id: pyupgrade
05:38:19  - exit code: 1
05:38:19  - files were modified by this hook
05:38:19  
05:38:19  Rewriting salt/modules/zabbix.py
05:38:19  Rewriting tests/unit/modules/test_zabbix.py
05:38:19  
05:38:20  isort....................................................................Passed
05:38:25  black....................................................................Failed
05:38:25  - hook id: black
05:38:25  - files were modified by this hook
05:38:25  
05:38:25  reformatted /var/jenkins/workspace/pr-pre-commit_PR-56996/salt/modules/zabbix.py
05:38:25  All done! ✨ 🍰 ✨
05:38:25  1 file reformatted, 1 file left unchanged.

Bruno Figueiredo da Costa and others added 6 commits August 19, 2020 15:37
Fix mediatype_create and mediatype_get usage since new API 4.4.

- Added a apiinfo_version call to check the API version when
  dealing with mediatype_create and get.
- Changed the attribute called from "description" to "name",
  when API version is greater or equal to 4.4. changed both
  mediatype_create and mediatype_get.
@bfdacosta
Copy link
Contributor Author

@DmitryKuzmenko Greetings!!! All problems solved. Thanks!

@dwoz dwoz merged commit 95cd625 into saltstack:master Aug 24, 2020
27 checks passed
@sagetherage sagetherage added the Magnesium Mg release after Na prior to Al label Sep 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Magnesium Mg release after Na prior to Al
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Problem using zabbix.mediatype_create and zabbix.mediatype_get due to API changes on 4.4
5 participants