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

Switch packages used to test the configuration #167

Merged
merged 1 commit into from
Jul 28, 2023

Conversation

mcdonnnj
Copy link
Member

🗣 Description

This pull request updates the test configuration to use the python-on-whales package instead of pytest-dockerc.

💭 Motivation and context

With the release of Cython v3 we now have a dependency issue with our testing configuration. This new version of Cython is incompatible with PyYAML 5.4.1, which is the last version of PyYAML that the docker-compose package will pull. Since the docker-compose Python package has been deprecated in favor of the v2 Golang application it is very unlikely (only maintenance for security fixes) that support for PyYAML v6 will be introduced. Due to build difficulties the odds of a PyYAML 5.4.2 which pins under the new Cython release is also very unlikely to happen.

This dependency problem coupled with the fact that the last release for the pytest-dockerc package was on 2020-10-09 and the latest commit on the default branch of the repository was the same day pushes us to migrate our testing configuration. After some research I settled on the python-on-whales package as being most similar in functionality to the pytest-dockerc package.

🧪 Testing

✅ Pre-approval checklist

  • This PR has an informative and human-readable title.
  • Changes are limited to a single goal - eschew scope creep!
  • All relevant type-of-change labels have been added.
  • I have read the CONTRIBUTING document.
  • These code changes follow cisagov code standards.
  • Tests have been added and/or modified to cover the changes in this PR.
  • All new and existing tests pass.

The pytest-dockerc plug is unmaintained and there is now a dependency
issues with PyYAML because of the release of Cython v3 (which is itself
a build dependency for PyYAML). After some research this seemed like
the most similar package in terms of functionality to the package we
are replacing. Although it is not a pytest plugin it still provides
similar access and uses the Docker composition defined in the
repository.
@mcdonnnj mcdonnnj added bug This issue or pull request addresses broken functionality improvement This issue or pull request will add or improve functionality, maintainability, or ease of use dependencies Pull requests that update a dependency file test This issue or pull request adds or otherwise modifies test code labels Jul 20, 2023
@mcdonnnj mcdonnnj self-assigned this Jul 20, 2023
@mcdonnnj mcdonnnj added the breaking change This issue or pull request involves changes to existing functionality label Jul 20, 2023
@jsf9k
Copy link
Member

jsf9k commented Jul 20, 2023

Can you comment briefly as to why this is a better choice than avast/pytest-docker?

@felddy
Copy link
Member

felddy commented Jul 20, 2023

About 8-months ago I tackled the issue of replacing pytest-dockerc in a personal project. After looking at a few of the alternatives I found that the best functionality came from the official python docker library itself.

Benefits:

  • Best bet on not losing support.
  • Easy to replace the dockerc fu with straight up docker calls.
  • Test configurations are defined wholly in code. i.e., no external docker-compose.yml required.
  • It's a lot less magical.

That said, I would like to suggest we use the official docker library as our mechanism for controlling containers under test. Thoughts?

See:

@mcdonnnj
Copy link
Member Author

Can you comment briefly as to why this is a better choice than avast/pytest-docker?

@jsf9k The pytest-docker plugin provides almost no information about the started containers. It seems well suited for using a Docker composition to test a Python package as opposed to testing a Docker image. The only helpful information it seems to provide is the IP of the running Docker container and port mappings.

About 8-months ago I tackled the issue of replacing pytest-dockerc in a personal project. After looking at a few of the alternatives I found that the best functionality came from the official python docker library itself.

Benefits:

* Best bet on not losing support.

* Easy to replace the dockerc fu with straight up docker calls.

* It's _a lot_ less magical.

That said, I would like to suggest we use the official docker library as our mechanism for controlling containers under test. Thoughts?

See:

* https://github.com/docker/docker-py

* https://pypi.org/project/docker/

* [felddy/foundryvtt-docker@717aa53](https://github.com/felddy/foundryvtt-docker/commit/717aa53aa987c0a401f5affbd5d31b580acb9fbd)

@felddy I specifically chose not to use docker-py because it is focused around low-level access to the Docker Engine HTTP API. I believe that our existing system of using a docker-compose.yml to define a configuration that is then used for testing is the most appropriate. The docker-py package has no intention of supporting compose. The python-on-whales package has been around for a while, seen regular updates, and has even been featured on the Docker blog. It is a much more mature package than the short-lived pytest-dockerc since it provides a Python API to the Docker CLI.

@dav3r
Copy link
Member

dav3r commented Jul 21, 2023

@mcdonnnj do you have any examples of projects where we have to (or would prefer to) test using a Docker composition, as opposed to testing the container on its own without a composition? In theory, it sounds cleaner and more in line with our best practices to be able to test each container in isolation, but practically, I'm not so sure if that is always possible or preferable.

This one is a bit of a toss-up for me so far. The fact that @felddy has already been using docker-py in a real project for a while earns it some points in my book.

On the other hand, there are some potentially useful Python-on-whales features in the Docker blog post that @mcdonnnj pointed out:

...some of the features that are not available in docker-py but are available in Python-on-whales:

Building with Docker buildx
Deploying to Swarm with docker stack
Deploying to the local Engine with Compose

I guess it comes down to Docker CLI vs engine and I don't have a super strong preference either way:

If you need to call the Docker command line, use Python-on-whales. And if you need to call the Docker engine directly, use docker-py.

@jsf9k
Copy link
Member

jsf9k commented Jul 21, 2023

On the other hand, there are some potentially useful Python-on-whales features in the Docker blog post that @mcdonnnj pointed out:

...some of the features that are not available in docker-py but are available in Python-on-whales:
Building with Docker buildx
Deploying to Swarm with docker stack
Deploying to the local Engine with Compose

I guess it comes down to Docker CLI vs engine and I don't have a super strong preference either way:

If you need to call the Docker command line, use Python-on-whales. And if you need to call the Docker engine directly, use docker-py.

I could envision the buildx functionality being useful. I tend to lean slightly toward @mcdonnnj's solution since the Docker CLI has more features than the Docker engine API.

@mcdonnnj
Copy link
Member Author

@mcdonnnj do you have any examples of projects where we have to (or would prefer to) test using a Docker composition, as opposed to testing the container on its own without a composition? In theory, it sounds cleaner and more in line with our best practices to be able to test each container in isolation, but practically, I'm not so sure if that is always possible or preferable.

This one is a bit of a toss-up for me so far. The fact that @felddy has already been using docker-py in a real project for a while earns it some points in my book.

On the other hand, there are some potentially useful Python-on-whales features in the Docker blog post that @mcdonnnj pointed out:

...some of the features that are not available in docker-py but are available in Python-on-whales:
Building with Docker buildx
Deploying to Swarm with docker stack
Deploying to the local Engine with Compose

I guess it comes down to Docker CLI vs engine and I don't have a super strong preference either way:

If you need to call the Docker command line, use Python-on-whales. And if you need to call the Docker engine directly, use docker-py.

Anything that is designed around using secrets with Docker Compose would benefit from this library choice. All of our current testing is designed around the use of a Docker composition. Most, if not all, of our Docker image configurations are also designed around usage with a Docker composition. I feel that using the Docker API directly is over-complicated for the purposes of testing our images. Using Compose files as we have been doing provides a static and separately testable format. When I say separately testable I mean that I can run a docker compose up and both easily confirm the pytest results as well as perform additional testing/debugging with the configuration.

@jsf9k
Copy link
Member

jsf9k commented Jul 24, 2023

It seems that we have the following:

User Vote
@dav3r Ambivalent and refuses to vote
@felddy docker-py
@jsf9k python-on-whales
@mcdonnnj python-on-whales
@jasonodoom docker-py
@jmorrowomni python-on-whales

How about a deadline of 9AM Wednesday morning, and then we call it? If it is still a tie at that point then how about we force @dav3r to vote?

@jasonodoom
Copy link
Contributor

I think docker-on-whales is tempting but from what I've gathered the support for using it boils down to simply being convenient which I understand. However, we don't need more abstractions. I am also thinking about the future here. docker-py is the official library from Docker themselves which has proven to be reliable and current with the latest Docker API changes. I know there's no support for compose which sucks, but... it's not the end of the world.
docker-py has also existed longer than docker-on-whales so it's got a big fanbase and tons of folks using it, which means better support and documentation. The flexibility provided is also nice. You get to fine-tune your Docker interactions and have more control over containers, images, volumes, and networks. But what's really important here is that it's lightweight and has fewer dependencies compared to docker-on-whales. Not to mention, the implementation for docker-py will be quick as we'd be working with what's already available. I don't see any headaches here. .@mcdonnnj also mentioned docker-on-whales was featured on the Docker blog which we shouldn't assume is an endorsement. It's a cool project but we shouldn't factor publicity as a selling point. I like docker-py as an option.

@jsf9k
Copy link
Member

jsf9k commented Jul 25, 2023

Looks like we need @dav3r to cast the tie-breaking vote.

@jmorrowomni
Copy link
Contributor

While not an official vote, I also like the fact that Python-on-whales gives us the flexibility to utilize additional container ecosystems like Podman.

@jsf9k
Copy link
Member

jsf9k commented Jul 26, 2023

@felddy - Is the abiosoft/colima jazz you have been recommending supported by docker/docker-py? In other words, does colima offer the same API functionality that docker-py relies on?

@jasonodoom
Copy link
Contributor

@felddy - Is the abiosoft/colima jazz you have been recommending supported by docker/docker-py? In other words, does colima offer the same API functionality that docker-py relies on?

It supports Docker so yes, it should.

@jsf9k
Copy link
Member

jsf9k commented Jul 27, 2023

@felddy - Is the abiosoft/colima jazz you have been recommending supported by docker/docker-py? In other words, does colima offer the same API functionality that docker-py relies on?

It supports Docker so yes, it should.

I thought the point of @felddy's move to colima was to use non-Docker runtimes. I may be wrong about that, but if so then the use of docker-py might prove problematic for local testing.

@dav3r
Copy link
Member

dav3r commented Jul 27, 2023

@felddy - Is the abiosoft/colima jazz you have been recommending supported by docker/docker-py? In other words, does colima offer the same API functionality that docker-py relies on?

It supports Docker so yes, it should.

I thought the point of @felddy's move to colima was to use non-Docker runtimes. I may be wrong about that, but if so then the use of docker-py might prove problematic for local testing.

Colima can still use the Docker engine (if you want), without the need for running the Docker Desktop client (at least on Mac).

This post has the graphic below that I found to be helpful in understanding where Colima fits:
image

@jsf9k
Copy link
Member

jsf9k commented Jul 27, 2023

@felddy - Is the abiosoft/colima jazz you have been recommending supported by docker/docker-py? In other words, does colima offer the same API functionality that docker-py relies on?

It supports Docker so yes, it should.

I thought the point of @felddy's move to colima was to use non-Docker runtimes. I may be wrong about that, but if so then the use of docker-py might prove problematic for local testing.

Colima can still use the Docker engine (if you want), without the need for running the Docker Desktop client (at least on Mac).

This post has the graphic below that I found to be helpful in understanding where Colima fits: image

How does it obviate the need for Docker Desktop or similar? Docker containers must run under a Linux kernel, so there would have to be a Linux VM running on OSX. Does Colima launch its own Linux VM in which to run Docker containers?

@dav3r
Copy link
Member

dav3r commented Jul 27, 2023

How does it obviate the need for Docker Desktop or similar? Docker containers must run under a Linux kernel, so there would have to be a Linux VM running on OSX. Does Colima launch its own Linux VM in which to run Docker containers?

Yes, Colima has its own VM - the Colima README has some info about that.

@jsf9k jsf9k requested a review from a team July 27, 2023 20:48
@jsf9k
Copy link
Member

jsf9k commented Jul 27, 2023

In today's dev team mullet meeting it was decided to move ahead with @mcdonnnj's python-on-whales solution. @dav3r said that he was leaning slightly toward that, and @felddy said this is not a hill he is prepared to die on, so that resolved the dilemma.

We just need a few more reviews on this PR from @cisagov/team-ois and then we can merge it and wrap up the current Kraken.

Copy link
Member

@dav3r dav3r left a comment

Choose a reason for hiding this comment

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

💀 🐳 👍

@jsf9k jsf9k added this pull request to the merge queue Jul 28, 2023
Merged via the queue into develop with commit a9d6c92 Jul 28, 2023
@jsf9k jsf9k deleted the improvement/update_testing_method branch July 28, 2023 14:11
@jsf9k jsf9k mentioned this pull request Sep 18, 2023
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change This issue or pull request involves changes to existing functionality bug This issue or pull request addresses broken functionality dependencies Pull requests that update a dependency file improvement This issue or pull request will add or improve functionality, maintainability, or ease of use test This issue or pull request adds or otherwise modifies test code
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants