Skip to content
This repository has been archived by the owner on Jun 13, 2024. It is now read-only.

Add helm, helm_info, and helm_repository modules #61

Merged
merged 7 commits into from
May 4, 2020
Merged

Add helm, helm_info, and helm_repository modules #61

merged 7 commits into from
May 4, 2020

Conversation

LucasBoisserie
Copy link
Contributor

@LucasBoisserie LucasBoisserie commented Mar 22, 2020

SUMMARY

Add helm, helm_info, and helm_repository modules to manage helm deployment inside kubernetes cluster

Fix: ansible/ansible#61546
Original PR on ansible repo: ansible/ansible#62450

ISSUE TYPE
  • New Modules Pull Request
COMPONENT NAME

helm
helm_info
helm_repository

ADDITIONAL INFORMATION

Tests are enabled only for Helm 3
Helm 2 support has been dropped

README.md Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 26, 2020

Codecov Report

Merging #61 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #61   +/-   ##
=======================================
  Coverage   43.33%   43.33%           
=======================================
  Files           3        3           
  Lines         540      540           
  Branches      109      109           
=======================================
  Hits          234      234           
  Misses        263      263           
  Partials       43       43           

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 dd8b716...9f788c6. Read the comment docs.

Copy link
Collaborator

@fabianvf fabianvf left a comment

Choose a reason for hiding this comment

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

some minor questions, for the most part looks good to me though, nice work on all the testing

plugins/modules/helm_cli_info.py Outdated Show resolved Hide resolved
plugins/modules/helm_cli_info.py Outdated Show resolved Hide resolved
plugins/modules/helm_cli_info.py Outdated Show resolved Hide resolved
@LucasBoisserie LucasBoisserie marked this pull request as ready for review March 27, 2020 10:27
@tatemz
Copy link

tatemz commented Mar 28, 2020

I'm excited about this PR and watching it for updates.

I'm testing this out locally and notice that we might need a way to add helm repos before updating the cache.

Thoughts?

Copy link
Collaborator

@fabianvf fabianvf left a comment

Choose a reason for hiding this comment

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

This all looks good to me, only question is do you think it makes sense to combine helm 2 + 3 into the same module or would it make sense to add a module_utils for shared logic and separate 2 and 3 into separate modules? I'm fine with either one, and I'm not super familiar with the differences.

plugins/modules/helm_cli.py Outdated Show resolved Hide resolved
plugins/modules/helm_cli.py Outdated Show resolved Hide resolved
plugins/modules/helm_cli.py Outdated Show resolved Hide resolved
plugins/modules/helm_cli.py Outdated Show resolved Hide resolved
plugins/modules/helm_cli.py Outdated Show resolved Hide resolved
plugins/modules/helm_cli.py Outdated Show resolved Hide resolved
plugins/modules/helm_cli.py Outdated Show resolved Hide resolved
plugins/modules/helm_cli_info.py Outdated Show resolved Hide resolved
plugins/modules/helm_cli_info.py Outdated Show resolved Hide resolved
@geerlingguy
Copy link
Collaborator

I was looking into what kind of support cycle there will be for Helm 2, because it would be nice to make a clean break (for some of the reasons @fabianvf pointed out with auth, mainly) and not have to support both 2 and 3...

Looking in the release notes for the last 2.x version, it states:

6 months after Helm 3’s public release, Helm 2 will stop accepting bug fixes. Only security issues will be accepted.

12 months after Helm 3’s public release, support for Helm 2 will formally end. Download links for the Helm 2 client through Google Cloud Storage, the Docker image for Tiller stored in Google Container Registry, and the Google Cloud buckets for the stable and incubator chart repositories may no longer work at any point.

So, Helm 3 was released on November 13, 2019, meaning:

  • Helm 2 bug fixes until: May 13, 2020
  • Helm 2 security fixes until: November 13, 2020

Since this is a new module, and Helm 2 is already on the way out... what do others think about only supporting Helm 3 out of the gate?

@LucasBoisserie
Copy link
Contributor Author

@geerlingguy

Helm 2 bug fixes has been prolonged to 13 August but I'm agree to drop helm 2 support.
When i start writting this module in october 2019, helm 3 was in beta, so i begin with helm 2
Now, i only used it with helm 3

For the auth part, i think the best solution is to outsource it and let users manage it.
There are many way to do it:
* helm registry commands are only for OCI registry and are experimentals.
* helm repo add --username --password for helm repo.

We can mention them in the doc for more precision

@xeor
Copy link

xeor commented Apr 20, 2020

Agree that this should only be for helm 3. I don't think there are many people that will end up wanting to use this for helm 2 since the same people would probably already be on v3, or days from it.
If they are using v2, they should follow https://helm.sh/docs/topics/v2_v3_migration/ to get to v3 before using this.

@LucasBoisserie
Copy link
Contributor Author

CI seems to be broken
Sanity and integration failed with the same errors:

$ ansible-test sanity --docker -v --color --python 3.6
...
ERROR: Command "ansible-doc -t connection community.kubernetes.kubectl" returned exit status 1.
>>> Standard Error
Traceback (most recent call last):
  File "/root/ansible/test/lib/ansible_test/_data/injector/ansible-doc", line 79, in <module>
    main()
  File "/root/ansible/test/lib/ansible_test/_data/injector/ansible-doc", line 49, in main
    args += [find_executable(name)]
  File "/root/ansible/test/lib/ansible_test/_data/injector/ansible-doc", line 75, in find_executable
    raise Exception('Executable "%s" not found in path: %s' % (name, path))
Exception: Executable "ansible-doc" not found in path: /tmp/python-g6s_1c7y-ansible:/root/ansible/test/lib/ansible_test/_data/injector:/root/ansible/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
...

After investigation, the issue provide from ansible-devel, installed from this task:

- name: Install ansible base (devel branch)
  run: pip install https://github.com/ansible/ansible/archive/devel.tar.gz --disable-pip-version-check

Ansible 2.9 seems to have no issue
ref: #63

@geerlingguy
Copy link
Collaborator

@LucasBoisserie - Thanks for the update—it looks like there's a fix upstream already for ansible/ansible base, but the current GitHub outage is causing some delays in releasing it :(

Once that's through, someone can kick a re-test. I'm hoping we can get this merged before the end of the week!

@geerlingguy
Copy link
Collaborator

@LucasBoisserie - For some reason now GitHub actions isn't even able to check out the code. I wonder if maybe one of your latest pushes didn't reach the repo correctly due to instability this morning (or if GitHub Actions is still experiencing weird outages)?

@geerlingguy
Copy link
Collaborator

@LucasBoisserie - I just fixed the annoying GitHub Actions issue that is causing the new failures with your latest commit (see #78) — can you rebase your branch and push it to kick off a new run using the new CI configuration?

@LucasBoisserie
Copy link
Contributor Author

Thanks @geerlingguy, the CI works well !

Copy link
Collaborator

@geerlingguy geerlingguy left a comment

Choose a reason for hiding this comment

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

Mostly some english grammar cleanup notes—but also left a couple pertinent questions (one is renaming modules possibly?).

molecule/default/roles/helm/tasks/tests_chart.yml Outdated Show resolved Hide resolved
molecule/default/roles/helm/tasks/run_test.yml Outdated Show resolved Hide resolved
molecule/default/roles/helm/tasks/run_test.yml Outdated Show resolved Hide resolved
molecule/default/roles/helm/tasks/tests_chart.yml Outdated Show resolved Hide resolved
molecule/default/roles/helm/tasks/tests_chart.yml Outdated Show resolved Hide resolved
molecule/default/roles/helm/tasks/tests_chart.yml Outdated Show resolved Hide resolved
molecule/default/roles/helm/tasks/tests_chart.yml Outdated Show resolved Hide resolved
plugins/modules/helm_cli_info.py Outdated Show resolved Hide resolved
@LucasBoisserie LucasBoisserie changed the title Add helm_cli & helm_cli_info modules Add helm & helm_info modules Apr 23, 2020
Copy link
Collaborator

@geerlingguy geerlingguy left a comment

Choose a reason for hiding this comment

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

Sorry about missing these in the last review...

plugins/modules/helm_info.py Outdated Show resolved Hide resolved
plugins/modules/helm.py Outdated Show resolved Hide resolved
plugins/modules/helm.py Outdated Show resolved Hide resolved
plugins/modules/helm.py Show resolved Hide resolved
README.md Show resolved Hide resolved
Copy link
Collaborator

@fabianvf fabianvf left a comment

Choose a reason for hiding this comment

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

Looks good to me

@geerlingguy geerlingguy changed the title Add helm & helm_info modules Add helm, helm_info, and helm_repository modules Apr 28, 2020
@geerlingguy
Copy link
Collaborator

geerlingguy commented Apr 28, 2020

I think this PR's been through the wringer, but is quite ready for a merge. Barring any further objections, I think it would be best to merge it and release a new collection version (0.11.0) by the end of this week.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Helm module replacement
8 participants