Improve remove docker container function to take into account stopped containers #12979
Conversation
Test Results (amd64) - Integration, Bootstrap 5 files 5 suites 2h 21m 50s ⏱️ Results for commit 52742de. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
suggestion: This module is incredibly important and well-tested, and we basically try to have tests for every change whenever possible. Could you please add a test for this edge case to the test for this module?
The tests are done in this class here:
When adding a new test, it will automatically be executed for both - the SDK and the CMD client. :)
There was a problem hiding this comment.
@alexrashed @dfangl I have added tests covering these edge scenarios. I also took the initiative to expand beyond that since I didn't see tests specific for the get_running_container_names and remove_container methods. They were "kinda" covered by other tests but I am a firm believer we need to have dedicated ones.
From these dedicated ones I found a "bug" in the remove_container method. The sdk client throws an exception if the container doesnt exist and the force parameter is false but the cmd client doesn't do that. it swallows the error no matter what you pass in the force parameter. I have made the required changes to make it consistent with the sdk client. Let me know if you agree with that.
See the new tests test_remove_container_should_throw_exception_when_container_doesnt_exist_and_not_forcing and test_remove_container_should_work_even_when_container_doesnt_exist_because_of_forcing which covers that scenario where the container doesnt exist + the force parameter value
There was a problem hiding this comment.
Awesome! Thanks a lot for the detailed new tests! Really great that you also fixed a bug along the way, I totally agree with the testing principle and the force parameter semantics you outlined.
There was a problem hiding this comment.
Nice! Thank you so much for addressing the comments and even fixing a bug along the way!
I think it would be great to get a final review from @dfangl as owner of this module, but then we should be good to go! 💯
localstack-core/localstack/utils/container_utils/docker_cmd_client.py
Outdated
Show resolved
Hide resolved
localstack-core/localstack/utils/container_utils/docker_cmd_client.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Awesome! Thanks a lot for the detailed new tests! Really great that you also fixed a bug along the way, I totally agree with the testing principle and the force parameter semantics you outlined.
Motivation
Right now, this remove container method doesn't take into account stopped containers when you ask it to check for existence of it
Related PR for more context about why bother with this change -> https://github.com/localstack/localstack-pro/pull/5037
Changes
get_all_container_names) I didn't touch the old method (get_running_container_names) to keep compatibility and not break any code already using it.