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

feat: Vendor/bundle dependency hcloud-python #218

Closed
apricote opened this issue Jun 2, 2023 · 3 comments · Fixed by #244
Closed

feat: Vendor/bundle dependency hcloud-python #218

apricote opened this issue Jun 2, 2023 · 3 comments · Fixed by #244

Comments

@apricote
Copy link
Collaborator

apricote commented Jun 2, 2023

SUMMARY

We should include the hcloud-python code in the release assets for the ansible collection.

Adding a new feature in our API to the ansible collection, usually also requires a new matching release of hcloud-python. This has caused multiple issues in the past where people did not update hcloud-python together with the ansible collection, which broke their setup (#209, #210).

In one of the latest versions, I added the requirement to update hcloud-python as a breaking change to the release notes, which (rightfully) summoned #217.

By bundling hcloud-python with the collection, we can avoid all these issues.

ISSUE TYPE
  • Feature Idea
COMPONENT NAME
  • dependency hcloud-python
ADDITIONAL INFORMATION

Replaces #211
Fixes #217

@resmo
Copy link
Member

resmo commented Jun 14, 2023

I am not sure if I understand this correctly but I would not bundle it but test for the hcloud-python version in the code. This is what other collections do as well.

@jooola
Copy link
Collaborator

jooola commented Jun 28, 2023

Hi @resmo,
Could you expand on your comment above please ?
I am curious about your perspective on the pros/cons of bundling the hcloud library in the collection.

Also, could you paste the links to the collections that choose to check the library version ?
I found:

and those who choose to follow the vendoring path:

@felixfontein
Copy link

In community.docker (stable-2 branch, before we vendored some parts of Docker SDK for Python - now we only have checks left for the API version) we were checking both the API version and the Docker SDK for Python version per feature. In the case of the docker_container module, it looks like: https://github.com/ansible-collections/community.docker/blob/stable-2/plugins/modules/docker_container.py#L3482-L3507

So for example if the user supplies the device_write_iops module option, the module would check whether the Docker daemon's API version is >= 1.22, and whether the Docker SDK for Python version is >= 1.9.0 - if one isn't the case, the module will fail.

Obviously this requires some bookkeeping, and making sure that the module code doesn't try to use a feature that isn't there. That way the module was supporting a large range of Docker daemon API versions and Docker SDK for Python versions.

(For the current main branch, the module only needs to check the API version: https://github.com/ansible-collections/community.docker/blob/main/plugins/module_utils/module_container/docker_api.py#L1196 - we also dropped support for some older API versions, which removed quite a few of these version checks.)

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 a pull request may close this issue.

4 participants