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: Implement support for "client_call" in python on whales, with a… #195

Closed
wants to merge 1 commit into from

Conversation

ambasta
Copy link
Contributor

@ambasta ambasta commented Sep 4, 2023

…uto-detection of podman/nerdctl

On systems that have podman/nerdctl available, this PR adds support for detecting the appropriate runtime, and use it via client_call feature provided by python on whales.

If no runtime is detected, it falls back to docker as is the default

…uto-detection of podman/nerdctl

Signed-off-by: Amit Prakash Ambasta <[email protected]>


def get_docker_client():
clients = ["docker", "podman", "nerdctl"]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I would feel better if this were a pytest_ini config option, which we alternatively (and preferentially) load from PMR_DOCKER_CLIENT env var. either way defaulting to docker.

My concern is that this function will be called once per test, and therefore potentailly thousands of times. whiching that way seems less than ideal. It could depend upon this function being cached, but that feels like less than ideal design.

By contrast, at each of the 3 call sites you modified, we have access to a pytest config already, which should let it be parametrized. And then env-var option would be what i expect most to actually use, because i guess i would expect one to just use docker in CI, since podman seems more like a personal choice, you'd opt into only locally.

Copy link
Contributor

Choose a reason for hiding this comment

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

what this loses is the automatic configuration aspect. but idk, i feel like opting in seems like alright UX, given the choice being made here. thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely will explore overrides based on pytest.ini. I'd still want to retain automatic configuration if no config is specified, but a caching solution definitely would be useful.

I am thinking that the approach should be:

  • Look for an override in pytest.ini (or maybe pyproject.toml?)
  • If no override is found, try getting the value from cache
  • If no value exists in cache, auto-detect and cache value

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose the auto-detected option could simply be written back to pytest session.config, at the same ini config value you'd otherwise read.

I still feel like auto-detection in the face of environment-runtime configuration (i.e. env var) would basically equate to the same thing, for a user that had podman installed, but not docker. But i dont feel strongly enough against it that i'd reject what you have now, once the "caching" bit is addressed.

Copy link
Contributor

@DanCardin DanCardin Oct 11, 2023

Choose a reason for hiding this comment

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

i may have some time to further your progress on this, if you're still interested and aren't planning to continue it soon

@DanCardin
Copy link
Contributor

I just merged the referenced PR, which includes your original commit with my prior suggestions about load order, cli options, environment variable option, and docs.

It should get released later today in 2.10.0, feel free to submit a follow up issue (or PR), if anything I've done doesn't work for your usecase!

@DanCardin DanCardin closed this Oct 17, 2023
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.

2 participants