-
Notifications
You must be signed in to change notification settings - Fork 295
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 : Add support for depends_on functionality #728
base: main
Are you sure you want to change the base?
Conversation
Hi, just checking in on this PR. I’d be happy to make adjustments if needed. Please let me know if there’s anything I can do to help move this forward. Thanks! |
started_dependencies = [] | ||
for dependency in self._dependencies: | ||
if not dependency._container: | ||
try: | ||
dependency._start_dependencies() | ||
dependency.start() | ||
started_dependencies.append(dependency) | ||
|
||
if not dependency.wait_until_running(): | ||
raise ContainerStartException(f"Dependency {dependency.image} did not reach 'running' state.") | ||
|
||
except Exception as e: | ||
# Clean up all dependencies started before the failure | ||
for dep in started_dependencies: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesn't seem like it work as the recursive call would create its own started_dependencies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for pointing this out! The current code creates a new started_dependencies list on each recursive call, which prevents complete cleanup if a nested dependency fails. I’ll update the code to pass started_dependencies across recursive calls to ensure full cleanup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexanderankin I have made the needed changes such that the _start_dependencies function passes the started_dependencies across the recursive function calls to maintain a consistent state. I have also added a new check for circular dependencies which is called while adding dependencies in depends_on function. The tests have also been updated to be more comprehensive. I will be happy to make changes in the future if any of the core parts change just tag me in that case.
@hari134 currently the priorities that I have for the project are making the core parts of it work correctly, rather than adding features (as its a high potential to need to be re-done later). I plan on doing another big push to move the project before the end of the year but not sure when. usually more downtime during holiday season, so this may get merged then. |
@alexanderankin I completely understand the focus on stabilizing the core functionality before adding new features. I'll address the issues raised, and you can review them whenever it's convenient. Thanks. |
Description:
This PR addresses issue #679 by introducing the depends_on functionality to DockerContainer, enhancing feature parity with the dependsOn functionality in TC-java by allowing containers to define dependencies. With this feature, a container can now specify dependencies on other containers, ensuring they will only start once all dependencies reach the running state.
Changes
Added a depends_on method to define dependencies between containers.
Implemented _start_dependencies to manage recursive startup and ensure each dependency reaches the running state.
Integrated exception handling for cleanup, ensuring that all dependencies are stopped if a failure occurs during the startup process.
Sample Usage
Testing
Added test cases to verify:
Complete cleanup when all dependencies fail to start.
Cleanup of all dependencies when some dependencies succeed, and others fail.
Ensuring the main container does not start if any dependency fails.