Remove compose from requirements and update dependencies for fixing the build #394
Conversation
kiview
changed the title
| @@ -11,8 +12,9 @@ def test_webdriver_container_container(caps): | |||
|
|
|||
| with BrowserWebDriverContainer(caps).maybe_emulate_amd64() as chrome: | |||
| webdriver = chrome.get_driver() | |||
| webdriver.get("http://google.com") | |||
There was a problem hiding this comment.
google.com has a cookie consent banner, so the test was failing
| @@ -60,9 +61,12 @@ def _configure(self) -> None: | |||
|
|
|||
| @wait_container_is_ready(urllib3.exceptions.HTTPError) | |||
| def _connect(self) -> webdriver.Remote: | |||
| options = Options() | |||
| for key, value in self.capabilities.items(): | |||
| options.set_capability(key, value) | |||
There was a problem hiding this comment.
The get_requirements.py script pulled in the new Selenium dependency, that had breaking changes. We don't have a breaking change in the Testcontainers API though this, but if users use the transitive Selenium dependency in their tests, it can be breaking change for them.
There was a problem hiding this comment.
this is using chrome options unconditionally, even though the library support both that and firefox - i used a base class in my fix for the same in #392
There was a problem hiding this comment.
You are right, great catch @alexanderankin.
I could not apply your change 1:1, because of the implicitly bumped dependency version, but will try to model it after your approach 👍
There was a problem hiding this comment.
What is interesting though, that the test we have using Firefox does not fail, do you have an idea why?
There was a problem hiding this comment.
Yeah if you read the source code of the selenium python package it seems like not too many differences - I forget what they are now but it seemed like a code smell to just use one even though it works. Probably safer in terms of future updates too
There was a problem hiding this comment.
It communicates intent better too, opting into common behavior but opting out of browser specific behavior
kiview
changed the title
|
This kinda makes me a little angry at this community - I literally opened an issue to migrate to Compose V2 (instead of ditching the component) and you didn't bother looking through your own issues to see if this is being addressed. On top of that I've waited a good couple of months for replies on #358 and nobody from the maintainers bothered to answer or look. |
|
I can't speak on behalf of the maintainers, but from the standpoint of an ex maintainer (of different projects) who is maybe not 100% an expert in the various domains, i would say, and i hope others can agree, that it is important to have confidence in the codebase, so that when things happen, at least you have this known good point in time when the tests passed, and you can resume work from there. I personally think that compose is an important features due to its attraction for users who interop using docker-compose outside of testcontainers. and im looking forward to advocating for it in other(or future) prs in this repo. I have begun to spend time on this project this month and will likely continue; its on my list to go through and record my own thoughts on all the open prs but havent done so as i am not typically working from home and so have to do this when i am either rushing out the door or very tired in the evening. regarding issue 358, i see it is linked to a number of other issues, but only one pr is easily discoverable (again, i plan to go through all open (and probably should go through closed ones) prs - it does not remove the dependency, which is required at present for tests to pass. So i can see a path forward where both this pr is necessary and docker-compose functionality will return. Actually just as i say that, it appears that #376 seems to also be a fix but ultimately the docker-compose python library must go, right? and replaced by either the cli or dind approaches... |
|
If it was merely mentioned and explained, I would have been happy and accept the consequences that What made me upset is that instead of any mention of different issues, it was merely removed like it was a deprecated feature. The feature has a deprecated dependency and that is different. Having said that, I'll find some time to propose a new implementation for the V2, now that technically there is nothing on |
|
totally - ive mentioned on the testcontainers slack that compose support should be considered sooner rather than later as we are already getting feedback about it. tbh its also frustrating for me that communication is so limited on all mediums (people are busy i guess), but i am able to get some feedback via the slack. |
|
Hey @balint-backmaker, sorry if you had a frustrating experience with the project in the last months and through the recent activities. As you have correctly identified, the maintenance status was recently hanging a bit in the air. Therefore, we (the maintainers of the other Testcontainers languages) decided to step in and help out with maintenance on a best-effort basis for now. I myself am a maintainer of Testcontainers for Java and not a Python expert, nor do I have extensive experience with the current implementation and history of Testcontainers for Python in full detail. You are right, that it would have appropriate to reference the open issue #358, in which you lay out in the detail the current situation and how to move forward. My immediate intention was unblocking the build and do it in a way, in which I can understand the consequences. In the PR description I have already mentioned, that a new compose module with v2 support should follow an approach of delegating to the binary, similar to what we do in Testcontainers for Java and what you have proposed in #358 (in addition we also provide a container-based solution). I'd be happy to work collaborate with you on PR to bring compose v2 support back into tc-python. |
|
@kiview sorry for the delay on my part too. I would love to step up in helping with maintenance, actually. |
What does this PR do?
Removes the Docker Compose module and restores the build.
It also updates all dependencies based on the
get_requirements.pyprocess. This unfortunately bumped the Selenium version, so the Selenium module needed to incorporate changes to make this transparent for users.Why is it important?
The Docker Compose module was implemented using Python-based Docker Compose v1 directly as a dependency. Docker Compose v1 is EoL since July 2023 and brought in transitive dependencies (such as PyYAML, which is forced to a specific version that breaks the build) that made CI fail. Docker Compose v2 is now Go based, so future implementations should probably integrate with Docker Compose in a way more similar to e.g. tc-java, directly by calling the Compose binary.
Related Issues