ansible-collections / community.kubernetes

Kubernetes Collection for Ansible
https://galaxy.ansible.com/community/kubernetes
GNU General Public License v3.0
265 stars 104 forks source link

helm: use the diff generated before #363

Closed joschi36 closed 3 years ago

joschi36 commented 3 years ago
SUMMARY

In the previous PR #355 @Akasurde has implemented optional helm check by the helm plugin diff. I was unsure why we would not use this for --diff in Ansible. This would be very useful. I have added a gist with a simple module I build which is actually using it. Also, I think this PR needs some more love, but first I wanted to gather a little bit of feedback from the original creator @Akasurde and @LucasBoisserie

ISSUE TYPE
COMPONENT NAME

Helm

ADDITIONAL INFORMATION

Also, I sorted parameters in exit_json

codecov[bot] commented 3 years ago

Codecov Report

Merging #363 (40fd325) into main (92e3732) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #363   +/-   ##
=======================================
  Coverage   36.80%   36.80%           
=======================================
  Files           3        3           
  Lines         758      758           
  Branches      148      148           
=======================================
  Hits          279      279           
  Misses        430      430           
  Partials       49       49           

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 92e3732...4359820. Read the comment docs.

tima commented 3 years ago

Thanks for the PR @joschi36. CI failures aside I had a couple of comments.

Should the presence of the diff be conditional on the presence of --diff? I'm not seeing that here. Also, since helm-diff is optional to this module, what is returned if --diff is used?

joschi36 commented 3 years ago

Thanks @tima for the quick feedback.

First sorry I've forgotten to add the gist I was talking about: https://gist.github.com/joschi36/f7df3a0aac460cebe11c36ec1b9a03bf That's the module we are currently using in production.

Should the presence of the diff be conditional on the presence of --diff?

I'm not sure if this is needed. If the --diff parameter is not present the diff is not showed. I'm not sure if it should be conditional.

Also, since helm-diff is optional to this module, what is returned if --diff is used?

No diff as before. When variable diff is empty it will result in: diff={'prepared': ''}, in exit_json and just no diff is shown.

I hope I understood your questions right, it's getting late here :smile:

Akasurde commented 3 years ago

@joschi36 Thanks for the contribution. Could you open a new PR against https://github.com/ansible-collections/kubernetes.core? Thanks.

github-actions[bot] commented 3 years ago

This repository does not accept pull requests, see the README for details.