Skip to content

Fix aws ecs deploy not defaulting an empty --cluster to "default" - #10557

Open
Adityaj0 wants to merge 1 commit into
aws:v2from
Adityaj0:fix/ecs-deploy-empty-cluster-string
Open

Fix aws ecs deploy not defaulting an empty --cluster to "default"#10557
Adityaj0 wants to merge 1 commit into
aws:v2from
Adityaj0:fix/ecs-deploy-empty-cluster-string

Conversation

@Adityaj0

Copy link
Copy Markdown

Issue

Fixes #10556

Description of changes

ECSClient.get_service_details() (used by aws ecs deploy) defaults an unspecified cluster to "default" with:

if cluster is None or '':
    cluster = 'default'

This parses as (cluster is None) or (''). The bare '' literal is always falsy, so it never affects the or — the whole condition is silently equivalent to if cluster is None:. The or '' is dead code.

--cluster's help text says: "If you do not specify a cluster, the 'default' cluster is assumed." That's true when --cluster is omitted (argparse leaves it None), but not when a caller passes --cluster "" — e.g. a CI/CD pipeline running --cluster "$CLUSTER_NAME" where the variable is unset or empty. In that case cluster is '', not None, so the fallback is skipped and the empty string is sent straight through to DescribeServices instead of resolving to the default cluster the command documents.

Fix: if not cluster:, which treats None and '' identically and matches the documented behavior in both cases.

>>> args.cluster = ''
>>> ECSClient(session, args, globals, agent).get_service_details()
# before: describe_services(cluster='', ...)
# after:  describe_services(cluster='default', ...)

Testing

get_service_details() had no existing test coverage at all for the cluster-defaulting behavior (only the ECSClient constructor/config was tested in test_ecsclient.py). Added three tests to tests/unit/customizations/ecs/test_ecsclient.py:

  • --cluster omitted (None) → falls back to default (already worked, now covered)
  • --cluster "" → falls back to default (was broken, now fixed)
  • --cluster my-cluster → passed through unchanged (already worked, now covered)

I confirmed the empty-string test fails with AssertionError: '' != 'default' on unmodified v2 and passes with the fix.

  • tests/unit/customizations/ecs/ + tests/functional/ecs/: 275 passed
  • ruff check / ruff format --check on both touched files: clean

ECSClient.get_service_details() defaults the cluster to "default" with:

    if cluster is None or '':
        cluster = 'default'

Operator precedence makes this `(cluster is None) or ('')`. The bare
string literal '' is always falsy, so it never changes the outcome of
the `or` - the check is silently equivalent to `if cluster is None:`.

--cluster is documented as: "If you do not specify a cluster, the
'default' cluster is assumed." That holds when --cluster is omitted
(argparse leaves it None), but not when a caller passes --cluster ""
(e.g. a CI/CD pipeline doing `--cluster "$CLUSTER_NAME"` where the
variable is unset), since '' is not None. The empty string is then
sent straight through to DescribeServices instead of falling back to
the default cluster the command promises.

Replace the condition with `if not cluster:`, which treats None and ''
identically and matches the documented behavior in both cases. Added
regression tests for get_service_details, which previously had none.

Fixes aws#10556

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant