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

zabbix_template: Fix the issue for the unicode strings doesn’t decode in python2 #322

Merged
merged 4 commits into from
Feb 2, 2021

Conversation

sky-joker
Copy link
Contributor

SUMMARY

I added the codecs processing for decoding unicode in Python2 to the zabbix_template module previously.
#226

It turns out that this process also removes the necessary escape characters(slash) in Python2 and 3.
So, this PR will fix that issue.

fixes: #314

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

plugins/modules/zabbix_template.py

ADDITIONAL INFORMATION

tested on Zabbix 5.0

@codecov
Copy link

codecov bot commented Jan 31, 2021

Codecov Report

Merging #322 (db86935) into main (51630d7) will increase coverage by 0.12%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #322      +/-   ##
==========================================
+ Coverage   77.20%   77.32%   +0.12%     
==========================================
  Files          19       19              
  Lines        2685     2686       +1     
  Branches      689      690       +1     
==========================================
+ Hits         2073     2077       +4     
+ Misses        409      407       -2     
+ Partials      203      202       -1     
Impacted Files Coverage Δ
...ommunity/zabbix/plugins/modules/zabbix_template.py 77.99% <0.00%> (+1.24%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f8c3a4...875b77d. Read the comment docs.

Copy link
Contributor

@D3DeFi D3DeFi left a comment

Choose a reason for hiding this comment

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

Looks ok to me

@sky-joker sky-joker changed the title [WIP] zabbix_template: Fix the issue for the unicode strings doesn’t decode in python2 zabbix_template: Fix the issue for the unicode strings doesn’t decode in python2 Feb 1, 2021
@sky-joker
Copy link
Contributor Author

Thank you @D3DeFi for your review.
I removed WIP, so can you please look at this PR again?

@D3DeFi
Copy link
Contributor

D3DeFi commented Feb 1, 2021

@sky-joker I think that added integration test will not be able to detect the problem by itself as the bug will not prevent it from importing the template, rather would cause the string with "\" to end up in zabbix as "\\".

I think this would require exporting the string via zabbix_template_info and comparing values between original template and result from export to be sure.

@sky-joker
Copy link
Contributor Author

Thank you @D3DeFi for your feedback.
I added the test for comparing the Unicode!

Copy link
Contributor

@D3DeFi D3DeFi left a comment

Choose a reason for hiding this comment

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

Amazing! :)

@D3DeFi D3DeFi merged commit 14cf8b5 into ansible-collections:main Feb 2, 2021
@D3DeFi
Copy link
Contributor

D3DeFi commented Feb 2, 2021

Thank you for the fix @sky-joker . Now, the question is: do we want to release 1.2.1 with this? I feel like we should some how do that

@sky-joker
Copy link
Contributor Author

sky-joker commented Feb 3, 2021

Thank you @D3DeFi for merging!

@dj-wasabi
Is there anything you want to include in v1.2.1 from the PRs that have not yet been merged?
If there's nothing, I think that we release v1.2.1 is ok.

@sky-joker sky-joker deleted the fix_314 branch February 3, 2021 12:47
@D3DeFi
Copy link
Contributor

D3DeFi commented Feb 4, 2021

Just for the completeness in this thread:

As mentioned on Gitter, we skip 1.2.1 and go for 2.0.0 directly

@sky-joker
Copy link
Contributor Author

Thank you @D3DeFi for the follow.

l00ptr pushed a commit to l00ptr/community.zabbix that referenced this pull request Feb 26, 2021
… in python2 (ansible-collections#322)

* fix the issue for the unicode strings doesn’t decode in python2

* add the integration test for ansible-collections#314

* add changelog file

* add the test for comparing the unicode ansible-collections#322 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

unicode escape breaks templates
2 participants