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 hcloud python dependency #244

Merged
merged 15 commits into from
Jul 11, 2023

Conversation

jooola
Copy link
Collaborator

@jooola jooola commented Jun 29, 2023

SUMMARY

This PR vendors the hcloud python dependency inside the ansible collection.

Fixes #211
Fixes #217
Fixes #218

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

module_utils

@jooola jooola force-pushed the bundle_hcloud branch 3 times, most recently from 1331134 to 478696c Compare July 3, 2023 10:33
@jooola jooola added this to the v2.0.0 milestone Jul 3, 2023
@jooola jooola marked this pull request as ready for review July 3, 2023 12:50
@webknjaz
Copy link
Member

webknjaz commented Jul 4, 2023

@jooola so I looked into this PR and have a few points to make:

  1. Using pre-commit is probably not right fit for this kind of check since it's expensive. Dropping it from this tool would allow for blazing fast checks with pre-commit.ci. It should be fine to have it separate because vendoring is a manually triggered process. Every PR doesn't need to run it.
  2. It seems like the script here reinvents the wheel. Instead, I'd recommend relying on prior art. Specially, why not use the project that is called vendoring — it grew out of pip and is now available as a standalone distribution. Pip still uses it, of course. AFAIK, it also allows patching the vendored projects in a generic manner too.
  3. I'd check if it's possible to integrate this into ansible-test sanity. ansible/ansible has a special location for such linters but I don't remember if this mechanism is supported in collections...

@jooola
Copy link
Collaborator Author

jooola commented Jul 4, 2023

Oh nice, I didn't know about vendoring. I just tried to understand the tool, and sadly it is lacking the first bits of documentation. The CLI isn't describing itself either.

In addition, in the README I read: "This tool has no stability promises -- it has only one intended user: pip"

So I think I'd prefer to stay away from this tool, and use a simple script with no dependency.

I am not sure to understand what "this" is referring to. Could you elaborate please?

@jooola jooola force-pushed the bundle_hcloud branch 2 times, most recently from f830de4 to 21d9529 Compare July 4, 2023 15:09
@webknjaz
Copy link
Member

webknjaz commented Jul 4, 2023

I am not sure to understand what "this" is referring to. Could you elaborate please?

This==your vendoring check.
ansible-core has this directory https://github.com/ansible/ansible/tree/devel/test/sanity/code-smell. And ansible-test sanity picks up the scripts put there. I was hoping this is also supported for collections, which would make it a better place for putting your vendoring script.

@webknjaz
Copy link
Member

webknjaz commented Jul 4, 2023

Oh nice, I didn't know about vendoring. I just tried to understand the tool, and sadly it is lacking the first bits of documentation. The CLI isn't describing itself either.

It's integrated somewhere in noxfile.py, it shouldn't be too hard to figure it out, if you're interested..

In addition, in the README I read: "This tool has no stability promises -- it has only one intended user: pip"

True, though this is solvable by pinning it to a precise version that works for you.

@webknjaz
Copy link
Member

webknjaz commented Jul 4, 2023

It's integrated somewhere in noxfile.py, it shouldn't be too hard to figure it out, if you're interested..

Here's some calls: https://github.com/pypa/pip/blob/b88adde/noxfile.py#L198-L243.

Here's some docs: https://github.com/pypa/pip/tree/b88adde/src/pip/_vendor#automatic-vendoring

@jooola jooola force-pushed the bundle_hcloud branch 2 times, most recently from ae1776e to 143a2ab Compare July 4, 2023 16:11
@jooola
Copy link
Collaborator Author

jooola commented Jul 5, 2023

@webknjaz I appreciate the links, but we will not be using the vendoring tool for the reasons I already stated.

Regarding the pre-commit point (1.), I prefer to move the discussion to a separate issue, so we don't mix everything up. We can always remove this pre-commit hook in the future.

@jooola
Copy link
Collaborator Author

jooola commented Jul 5, 2023

Hi @resmo @felixfontein, if you find some time, could I have a small review please? This PR should fix #211 #217 #218

@jooola jooola changed the title feat: vendor hcloud python dependency feat!: vendor hcloud python dependency Jul 5, 2023
apricote
apricote previously approved these changes Jul 5, 2023
Copy link
Collaborator

@apricote apricote left a comment

Choose a reason for hiding this comment

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

🥳 Works for me!

@jooola jooola changed the base branch from main to stable-1 July 10, 2023 08:35
@jooola jooola changed the title feat!: vendor hcloud python dependency feat: vendor hcloud python dependency Jul 10, 2023
@jooola jooola removed this from the v2.0.0 milestone Jul 10, 2023
@jooola jooola changed the base branch from stable-1 to main July 11, 2023 08:54
@jooola jooola merged commit 8a6157e into ansible-collections:main Jul 11, 2023
@jooola jooola deleted the bundle_hcloud branch July 11, 2023 09:15
jooola added a commit to jooola/hetzner.hcloud that referenced this pull request Jul 12, 2023
* chore: ignore venv directories

* chore: ignore integration test generated inventory

* feat: vendor hcloud package

* import https://github.com/hetznercloud/hcloud-python

* use vendored hcloud in modules

* update integration test requirements

* make vendor script self contained

* chore: add  check-hcloud-vendor pre-commit hook

* pin hcloud version to v.1.24.0

* move vendored __version__.py file to _version.py

* update comment about galaxy-importer filename lint
jooola added a commit to jooola/hetzner.hcloud that referenced this pull request Jul 12, 2023
* chore: ignore venv directories

* chore: ignore integration test generated inventory

* feat: vendor hcloud package

* import https://github.com/hetznercloud/hcloud-python

* use vendored hcloud in modules

* update integration test requirements

* make vendor script self contained

* chore: add  check-hcloud-vendor pre-commit hook

* pin hcloud version to v.1.24.0

* move vendored __version__.py file to _version.py

* update comment about galaxy-importer filename lint
jooola added a commit that referenced this pull request Jul 13, 2023
* chore: ignore venv directories

* chore: ignore integration test generated inventory

* feat: vendor hcloud package

* import https://github.com/hetznercloud/hcloud-python

* use vendored hcloud in modules

* update integration test requirements

* make vendor script self contained

* chore: add  check-hcloud-vendor pre-commit hook

* pin hcloud version to v.1.24.0

* move vendored __version__.py file to _version.py

* update comment about galaxy-importer filename lint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants