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
Merged
merged 9 commits into from
Aug 22, 2025
Merged

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

merged 9 commits into from
Aug 22, 2025
+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! 💯

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! 💯

dfangl
dfangl approved these changes Aug 22, 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! Nice tests, and very good catch with the remove container function! Thanks for adding all those sweet new tests!

@bryansan-local bryansan-local merged commit 047b419 into main Aug 22, 2025
40 checks passed
@bryansan-local bryansan-local deleted the feature/improve_remove_container branch August 22, 2025 09:26
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

@dfangl dfangl dfangl 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

+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