[Python] Add ecs Basics scenario + Hello - #8048
Conversation
github-actions
Bot
added
the
Python
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 AI Code Example Review
This PR is a conditional pass with several issues that need to be addressed before merging. The code is functional and well-structured overall, but has import path problems that would prevent execution in certain configurations, missing conftest.py/pytest fixtures following standard patterns, unused imports, an incorrect type annotation, and the test file misidentified as an integration test when it uses only stubs.
Detailed Review
-
Import path issue (blocking): In
scenarios/ecs_basics_scenario.py, the importfrom ecs_wrapper import EcsWrapperwill fail when run from thescenarios/subdirectory sinceecs_wrapper.pylives one level up atpython/example_code/ecs/ecs_wrapper.py. Either asys.pathadjustment is needed or the file should be moved to the same directory as the wrapper. Similarly,test/test_ecs_integ.pyusessys.path.insert(0, ...)which only adds thetest/directory, then importsfrom ecs_wrapper import EcsWrapper— the path should point to the parentecs/folder, not thetest/folder itself (os.path.dirname(os.path.dirname(...))). This is a blocking runtime issue. -
Unused import: In
ecs_wrapper.py,Optionalis imported fromtypingbut never used. This will trigger a pylint warning. -
Incorrect type annotation: In
ecs_wrapper.py,def __init__(self, ecs_client: boto3.client)—boto3.clientis a function/factory, not a type. The correct annotation isboto3.clientas a callable result, which should be typed asbotocore.client.BaseClientor simply left untyped. This is a minor but recurring issue also present in comparable examples. -
Missing conftest.py: Premium reference examples (controltower, conditional_requests, etc.) all include a
conftest.pyin the test folder. This PR has noconftest.py, meaning there are no shared fixtures or propersys.pathsetup following the repository standard pattern. -
Test file naming inconsistency: The file is named
test_ecs_integ.py(integration) but the docstring and implementation use only Stubber/offline stubs — no actual AWS calls. It should be renamedtest_ecs_wrapper.pyortest_ecs_scenario.pyto avoid confusion and match the unit/integration split used in comparable examples. -
No scenario-level test: There is no test for
run_scenario()inecs_basics_scenario.pyitself (analogous totest_scenario_run.pyin controltower). The tests only cover the wrapper methods. A scenario-level test using mocked/stubbed CloudFormation and ECS calls would be expected per the premium patterns. -
time.sleep(5)in cleanup (minor): Inecs_basics_scenario.pycleanup, there is a baretime.sleep(5)between scaling to 0 and deleting. This is fragile and unnecessary sincedelete_servicewithforce=Truehandles this. Better to remove or use the waiter instead. -
list()constructor overuse: Throughout the code,list()is used as an empty list literal where[]is idiomatic Python (e.g.,cluster_arns = list(),task_arns = list()). This doesn't affect functionality but deviates from PEP 8 style. -
Missing README.md: Comparable examples all include a
README.md(see controltower, conditional_requests). The ECS example lacks one, which is typically required for the repository. -
Good aspects: The wrapper class is well-organized with proper paginator usage, waiter support (
wait_for_service_stable/wait_for_service_inactive), correct CloudFormation-based prerequisite setup, comprehensive Stubber-based unit tests for all happy path and error scenarios, properfinallyblock for guaranteed cleanup in the scenario, and good docstrings throughout. The metadata YAML changes are correctly structured.
This review was generated automatically using Amazon Bedrock. It compares your changes against existing examples and coding guidelines. Please use your judgment — this is advisory, not authoritative.
| from ecs_wrapper import EcsWrapper | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
There was a problem hiding this comment.
🤖 This import will fail when the scenario is run from the scenarios/ subdirectory since ecs_wrapper.py is in the parent ecs/ directory. Add import sys, os; sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) before this import, or restructure the files so they are in the same directory.
|
|
||
| import sys | ||
| import os | ||
|
|
There was a problem hiding this comment.
🤖 sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) inserts the test/ directory itself, but ecs_wrapper.py is one level up in the ecs/ folder. This should be os.path.dirname(os.path.dirname(os.path.abspath(__file__))) to resolve the import correctly.
| """ | ||
|
|
||
| import logging | ||
| from typing import Any, Dict, List, Optional |
There was a problem hiding this comment.
🤖 Optional is imported but never used anywhere in this file. Remove it to avoid pylint warnings.
|
|
||
| def __init__(self, ecs_client: boto3.client): | ||
| """ | ||
| Initializes the EcsWrapper with an ECS client. |
There was a problem hiding this comment.
🤖 boto3.client is a factory function, not a type — using it as a type annotation is incorrect. Use Any from typing, or more precisely botocore.client.BaseClient. This is a typing correctness issue that mypy will flag.
| # Step 9: Delete the ECS Service | ||
| try: | ||
| print(f"\n--- Cleanup: Scale down and delete service '{service_name}' ---") | ||
| try: |
There was a problem hiding this comment.
🤖 time.sleep(5) is fragile and arbitrary. Since delete_service(..., force=True) is called immediately after, ECS will handle draining regardless. Consider removing the sleep entirely, or if a delay is intentional, add a comment explaining why and consider using the wait_for_service_inactive waiter instead.
| try: | ||
| paginator = ecs_client.get_paginator("list_clusters") | ||
| cluster_arns = list() | ||
| for page in paginator.paginate(): |
There was a problem hiding this comment.
🤖 cluster_arns = list() — per Python idioms and PEP 8, prefer cluster_arns = [] over the list() constructor for an empty list literal. This pattern recurs throughout the codebase.
Superficial change to test commit and push workflow.
There was a problem hiding this comment.
🤖 AI Code Example Review
{
"summary": "This follow-up PR has addressed several previous issues (the time.sleep(5) now uses the wait_for_service_inactive waiter, list literals feedback is partially addressed) but two blocking issues from the previous review remain unresolved: the sys.path.insert in test_ecs_integ.py still inserts the wrong directory (test/ instead of ecs/), and the Optional import in ecs_wrapper.py is still unused. Additionally, the import path issue in ecs_basics_scenario.py for ecs_wrapper remains fragile when run from the scenarios/ subdirectory.",
"detailed_review": "1. [BLOCKING - UNRESOLVED] Wrong sys.path in test file (python/example_code/ecs/test/test_ecs_integ.py, line 11): sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) inserts the test/ directory, but ecs_wrapper.py lives one level up in ecs/. This means from ecs_wrapper import EcsWrapper will fail when running pytest from any directory other than ecs/test/. It should be os.path.dirname(os.path.dirname(os.path.abspath(__file__))). This was flagged in the previous review and is still unfixed. Comparable examples (e.g., controltower/test/conftest.py) use a conftest.py with the correct parent-directory path append.\n\n2. [BLOCKING - UNRESOLVED] Unused Optional import (python/example_code/ecs/ecs_wrapper.py, line 9): Optional is imported from typing but never used anywhere in the file. This will cause a pylint warning and should be removed. Flagged in previous review, still present.\n\n3. [BLOCKING - UNRESOLVED] Import fragility in scenario (python/example_code/ecs/scenarios/ecs_basics_scenario.py, line 32): from ecs_wrapper import EcsWrapper will fail when running the script directly from the scenarios/ subdirectory because ecs_wrapper.py is in the parent ecs/ directory. The previous review suggested adding a sys.path manipulation or restructuring files. Neither was done. This is a runability issue.\n\n4. [UNRESOLVED - Minor] list() constructor vs [] literal (ecs_wrapper.py and ecs_hello.py): The previous review noted that list() should be replaced with [] per PEP 8 / Python idiom. This pattern remains throughout: cluster_arns = list(), task_arns = list(), output_map = dict(), and list() as a .get() default. These should be [] and {} respectively.\n\n5. [ADDRESSED] time.sleep(5) + service delete pattern: The previous review flagged the time.sleep(5) as fragile. The code now calls update_service with desired_count=0, sleeps briefly (still 5 seconds), then delete_service(force=True) followed by wait_for_service_inactive. The waiter is now used, which is an improvement. The time.sleep(5) that remains after update_service is still somewhat arbitrary but is now less critical since the real wait is the waiter. Consider adding an inline comment explaining it.\n\n6. [ADDRESSED] boto3.client type annotation was flagged previously — the annotation ecs_client: boto3.client is still present in ecs_wrapper.py line 23. boto3.client is a factory function, not a type. This should be Any or botocore.client.BaseClient. This was flagged previously and remains unaddressed.\n\n7. [NEW ISSUE] Missing conftest.py in python/example_code/ecs/test/: The comparable premium examples (controltower, conditional_requests) all use a conftest.py to configure sys.path. This repo's pattern is to use conftest.py for path manipulation rather than inline sys.path.insert in test files. Adding a conftest.py would be cleaner and consistent.\n\n8. [NEW ISSUE] No README.md for the ECS example: Other examples in the repo include a README.md describing prerequisites, usage, and what is demonstrated. The ECS directory is missing one.\n\n9. [GOOD] Test coverage: The test file is comprehensive — it covers all wrapper methods with both success and error cases using botocore Stubber. The stubs are correctly structured with expected params and responses. This is solid work.\n\n10. [GOOD] CloudFormation-based prerequisite provisioning: Using CloudFormation to provision VPC/subnets/IAM role is a best practice that avoids hardcoded infrastructure values and ensures clean teardown. The template is well-structured and the waiter-based polling is correct.",
"inline_comments": [
{
"path": "python/example_code/ecs/test/test_ecs_integ.py",
"line": 11,
"body": "[UNRESOLVED from previous review] This inserts the test/ directory itself into sys.path, but ecs_wrapper.py lives in the parent ecs/ directory. Change to:\n
This review was generated automatically using Amazon Bedrock. It compares your changes against existing examples and coding guidelines. Please use your judgment — this is advisory, not authoritative.
This reverts commit df2ed3b.
There was a problem hiding this comment.
🤖 AI Code Example Review
This follow-up PR addresses several previous review issues but two blockers remain: the sys.path fix in the test file still inserts the wrong directory (test/ instead of the parent ecs/), causing the from ecs_wrapper import EcsWrapper import to fail at runtime; and the scenario file's from ecs_wrapper import EcsWrapper import will still fail when run from the scenarios/ subdirectory. The Optional import and the list() constructor issues noted previously have not been addressed. The code is otherwise well-structured with good test coverage, proper waiter usage, and solid error handling.
Detailed Review
-
[BLOCKER] Test import path is still wrong (test_ecs_integ.py:11):
sys.path.insert(0, os.path.dirname(os.path.abspath(__file__)))inserts thetest/directory itself into the path. Sinceecs_wrapper.pylives one level up inecs/, thefrom ecs_wrapper import EcsWrapperimport on line 17 will raiseModuleNotFoundErrorwhen tests are run from thetest/directory. This should beos.path.dirname(os.path.dirname(os.path.abspath(__file__)))— identical to the feedback in the previous review and still unresolved. -
[BLOCKER] Scenario import still broken (ecs_basics_scenario.py:32):
from ecs_wrapper import EcsWrapperwill fail when the script is executed from thescenarios/subdirectory becauseecs_wrapper.pyis in the parentecs/directory. Aconftest.pyor explicitsys.pathmanipulation before the import (as recommended in the prior review) is still absent. This is the same issue raised before, unaddressed. -
[REMAINING]
Optionalstill imported but unused (ecs_wrapper.py:9):from typing import Any, Dict, List, Optional—Optionalis never used anywhere in the file. This was flagged in the previous review and is unchanged, which will generate a pylint/mypy warning. -
[REMAINING]
list()constructor instead of[](ecs_hello.py:20, ecs_wrapper.py throughout): Multiple occurrences ofcluster_arns = list(),task_arns = list(),output_map = dict(), etc. PEP 8 and the previous review both recommend using[]and{}literals. These are still present. -
[REMAINING]
boto3.clientas a type annotation (ecs_wrapper.py:23): The__init__parameterecs_client: boto3.clientusesboto3.client(a factory function) as a type annotation. This is incorrect and mypy will flag it. The previous review suggested usingAnyorbotocore.client.BaseClient. Still unchanged. -
[GOOD] Waiter usage is correct and the previous
time.sleep(5)issue has been partially addressed: The scenario now properly useswait_for_service_inactiveafterdelete_service. However, thetime.sleep(5)call betweenupdate_service(desired_count=0)anddelete_service(..., force=True)on cleanup (ecs_basics_scenario.py ~358) is still present and still lacks an explanatory comment. Sinceforce=Trueis used, this sleep is unnecessary. -
[GOOD] Comprehensive test coverage: The test file covers all 10+ wrapper methods with both success and error paths, using the botocore Stubber correctly. The stubbed responses match the expected API shapes. This is a strong positive.
-
[GOOD] CloudFormation-based infrastructure setup: Using a CFN stack to provision VPC, subnets, security group, and IAM role is a clean approach — it avoids hardcoded resource IDs, supports teardown, and is reusable.
-
[GOOD] Error handling pattern is consistent: All wrapper methods follow the same pattern of catching
ClientError, logging a specific message for known error codes, and re-raising. This is aligned with SDK example best practices. -
[MINOR] No
conftest.pyfor tests: Premium comparable examples (e.g.,controltower,conditional_requests) use aconftest.pyin thetest/directory to handle path setup and shared fixtures. Adding aconftest.pywith propersys.pathconfiguration would be the idiomatic pattern and would fix the import issue cleanly rather than using an inlinesys.path.insertin the test file itself.
This review was generated automatically using Amazon Bedrock. It compares your changes against existing examples and coding guidelines. Please use your judgment — this is advisory, not authoritative.
|
|
||
| import sys | ||
| import os | ||
|
|
There was a problem hiding this comment.
🤖 This still inserts the test/ directory itself, not the ecs/ parent where ecs_wrapper.py lives. Change to os.path.dirname(os.path.dirname(os.path.abspath(__file__))) so that from ecs_wrapper import EcsWrapper resolves correctly. This was flagged in the previous review and remains a blocking issue.
| from ecs_wrapper import EcsWrapper | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
There was a problem hiding this comment.
🤖 from ecs_wrapper import EcsWrapper will raise ModuleNotFoundError when this script is run from the scenarios/ subdirectory. Add import sys, os; sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) before this import, or move both files to the same directory. This was flagged in the previous review and is still unresolved.
| """ | ||
|
|
||
| import logging | ||
| from typing import Any, Dict, List, Optional |
There was a problem hiding this comment.
🤖 Optional is imported but never used anywhere in this file. Remove it to avoid pylint/mypy warnings. Flagged in the previous review; still present.
|
|
||
| def __init__(self, ecs_client: boto3.client): | ||
| """ | ||
| Initializes the EcsWrapper with an ECS client. |
There was a problem hiding this comment.
🤖 boto3.client is a factory function, not a type — using it as ecs_client: boto3.client is incorrect and mypy will flag it. Use Any from typing or botocore.client.BaseClient instead. This was flagged in the previous review and remains unchanged.
| ecs_client = boto3.client("ecs") | ||
|
|
||
| try: | ||
| paginator = ecs_client.get_paginator("list_clusters") |
There was a problem hiding this comment.
🤖 cluster_arns = list() — prefer the list literal cluster_arns = [] per PEP 8. Same pattern appears throughout ecs_wrapper.py (e.g., task_arns = list(), output_map = dict()). Flagged in the previous review; still present.
2 participants
Auto-generated Python code examples for ecs (Hello + Basics scenario + wrapper + tests)