Skip to content

Improve remove docker container function to take into account stopped containers #12979

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Improve remove docker container function to take into account stopped containers #12979

wants to merge 9 commits into from
+108 −12

Conversation

bryansan-local
Copy link
Contributor

@bryansan-local bryansan-local commented Aug 8, 2025 •
edited
Loading

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

  • I have introduced a new method to return all containers including stopped ones (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.
  • I changed the remove container method to use this new method ^ so it takes into account stopped containers when you ask it to check for existence. This is kinda a breaking change BUT in my opinion this is an improvement to the remove operation.

@bryansan-local bryansan-local self-assigned this Aug 8, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 8, 2025 •
edited
Loading

Test Results - Preflight, Unit

22 144 tests  +5   20 407 ✅ +3   6m 19s ⏱️ +2s
     1 suites ±0    1 737 💤 +2 
     1 files   ±0        0 ❌ ±0 

Results for commit 0e71d26. ± Comparison against base commit 1d26825.

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 8, 2025 •
edited
Loading

Test Results (amd64) - Acceptance

7 tests  ±0   5 ✅ ±0   3m 20s ⏱️ +11s
1 suites ±0   2 💤 ±0 
1 files   ±0   0 ❌ ±0 

Results for commit 0e71d26. ± Comparison against base commit 1d26825.

♻️ This comment has been updated with latest results.

@bryansan-local bryansan-local marked this pull request as ready for review August 8, 2025 15:56
@bryansan-local bryansan-local added the semver: patch Non-breaking changes which can be included in patch releases label Aug 8, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 8, 2025 •
edited
Loading

Test Results (amd64) - Integration, Bootstrap

    5 files      5 suites   2h 23m 4s ⏱️
5 002 tests 4 404 ✅ 598 💤 0 ❌
5 008 runs  4 404 ✅ 604 💤 0 ❌

Results for commit 0e71d26.

♻️ This comment has been updated with latest results.

@coveralls
Copy link

Coverage Status

coverage: 83.016% (-3.8%) from 86.841%
when pulling 2f1b32b on feature/improve_remove_container
into 4464cd2 on main.

@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 8, 2025 •
edited
Loading

LocalStack Community integration with Pro

    2 files  ±0      2 suites  ±0   1h 45m 41s ⏱️ -4s
4 631 tests +1  4 191 ✅ +1  440 💤 ±0  0 ❌ ±0 
4 633 runs  +1  4 191 ✅ +1  442 💤 ±0  0 ❌ ±0 

Results for commit 0e71d26. ± Comparison against base commit 1d26825.

This pull request removes 2 and adds 3 tests. Note that renamed tests count towards both. 
tests.aws.services.cloudformation.engine.test_mappings.TestCloudFormationMappings ‑ test_async_mapping_error_first_level
tests.aws.services.cloudformation.engine.test_mappings.TestCloudFormationMappings ‑ test_async_mapping_error_second_level

tests.aws.services.s3.test_s3.TestS3 ‑ test_get_obj_attrs_multi_headers_behavior
tests.aws.services.s3.test_s3.TestS3 ‑ test_response_structure_get_obj_attrs
tests.aws.services.sns.test_sns.TestSNSSubscriptionSQS ‑ test_publish_message_group_id

♻️ This comment has been updated with latest results.

alexrashed
alexrashed reviewed Aug 12, 2025
View reviewed changes
Copy link
Member

@alexrashed alexrashed left a comment

Choose a reason for hiding this comment

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

Great catch and thanks for jumping on this! Once there is a test for this, we should be good to go! 💯

localstack-core/localstack/utils/container_utils/container_client.py Outdated
Copy link
Member

Choose a reason for hiding this comment

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

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:

localstack/tests/integration/docker_utils/test_docker.py

Line 131 in 34650cc

class TestDockerClient:

When adding a new test, it will automatically be executed for both - the SDK and the CMD client. :)

Copy link
Contributor Author

@bryansan-local bryansan-local Aug 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@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

Copy link
Member

Choose a reason for hiding this comment

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

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.

dfangl
dfangl reviewed Aug 13, 2025
View reviewed changes
Copy link
Member

@dfangl dfangl left a comment

Choose a reason for hiding this comment

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

LGTM, but I agree with Alex on the tests, would be nice to get one!

alexrashed
alexrashed approved these changes Aug 20, 2025
View reviewed changes
Copy link
Member

@alexrashed alexrashed left a comment

Choose a reason for hiding this comment

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

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/container_client.py Outdated
Copy link
Member

Choose a reason for hiding this comment

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexrashed alexrashed alexrashed approved these changes

@DrisDary DrisDary DrisDary approved these changes

@dominikschubert dominikschubert Awaiting requested review from dominikschubert dominikschubert is a code owner

@bblommers bblommers Awaiting requested review from bblommers

@alexlave100 alexlave100 Awaiting requested review from alexlave100

@dfangl dfangl Awaiting requested review from dfangl dfangl is a code owner

+1 more reviewer

@bryansan bryansan bryansan left review comments

Reviewers whose approvals may not affect merge requirements
Assignees

@bryansan-local bryansan-local

Labels
semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@bryansan-local @coveralls @alexrashed @dfangl @DrisDary @bryansan