Skip to content

fix: propagate role_arn to companion stack CloudFormation calls (#5051) - #9263

Open
Som0111 wants to merge 4 commits into
aws:developfrom
Som0111:fix/companion-stack-service-role-5051
Open

fix: propagate role_arn to companion stack CloudFormation calls (#5051)#9263
Som0111 wants to merge 4 commits into
aws:developfrom
Som0111:fix/companion-stack-service-role-5051

Conversation

@Som0111

@Som0111 Som0111 commented Sep 8, 2026

Copy link
Copy Markdown

Which issue(s) does this change fix?

#5051

Why is this change necessary?

When sam deploy --role-arn is supplied, the companion stack's CloudFormation
calls did not pass the RoleArn argument. This caused permission errors when
the caller lacked ecr:CreateRepository but the service role had it.

How does it address the issue?

Propagates role_arn through the deploy call chain into CompanionStackManager
so create_stack and update_stack both receive RoleArn when set.

What side effects does this change have?

None — role_arn defaults to None, keeping existing behavior unchanged when
--role-arn is not supplied.

Mandatory Checklist

  • Review the generative AI contribution guidelines
  • Add input/output type hints to new functions/methods
  • Write design document if needed (not needed — narrow bug fix)
  • Write/update unit tests
  • Write/update integration tests (no integration env available locally)
  • Write/update functional tests if needed
  • make pr passes (no local dev env with all deps installed)
  • make update-reproducible-reqs (no dependencies changed)
  • Write documentation (no user-facing behavior change)
    By submitting this pull request, I confirm that my contribution is made under
    the terms of the Apache 2.0 license.

@Som0111
Som0111 requested a review from a team as a code owner September 8, 2026 04:58
@github-actions github-actions Bot added area/deploy sam deploy command pr/external stage/needs-triage Automatically applied to new issues and PRs, indicating they haven't been looked at. labels Sep 8, 2026

@aws-sam-tooling-bot aws-sam-tooling-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review Results

Reviewed: 94ee3f3..1499643
Files: 6
Comments: 2

Comment thread tests/unit/commands/deploy/test_command.py Outdated
Comment thread tests/unit/commands/deploy/test_guided_context.py Outdated

@aws-sam-tooling-bot aws-sam-tooling-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review Results

Reviewed: 94ee3f3..6676733
Files: 6
Comments: 2

Comment thread samcli/lib/bootstrap/companion_stack/companion_stack_manager.py Outdated
Comment thread samcli/lib/bootstrap/companion_stack/companion_stack_manager.py Outdated
@Som0111

Som0111 commented Sep 8, 2026

Copy link
Copy Markdown
Author

Fixed in latest commit (f6b18a9):

  1. Added Optional[str] type hints for role_arn in both companion_stack_manager.py and guided_context.py, plus _role_arn to the class-level annotation block.
  2. Added a comment in delete_unreferenced_repos() documenting that _ecr_client uses ambient credentials — routing ECR deletion through the assumed role requires STS session plumbing outside the scope of this fix.

@aws-sam-tooling-bot aws-sam-tooling-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review Results

Reviewed: 94ee3f3..f6b18a9
Files: 6
Comments: 2

Comment thread samcli/lib/bootstrap/companion_stack/companion_stack_manager.py Outdated
Comment thread samcli/lib/bootstrap/companion_stack/companion_stack_manager.py Outdated
@Som0111

Som0111 commented Sep 8, 2026

Copy link
Copy Markdown
Author

Addressed all bot findings in latest commits:

  1. s3_prefix assertions fixed in test_guided_context.py and test_command.py
  2. Optional[str] type hints added for role_arn in companion_stack_manager.py and guided_context.py
  3. extra_args annotated as Dict[str, Any]
  4. RoleARN now passed to delete_stack in _delete_companion_stack
  5. delete_unreferenced_repos catches AccessDeniedException and raises AWSServiceClientError with clear message about ecr:DeleteRepository requirement

All 58 unit tests pass.

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

Labels

area/deploy sam deploy command pr/external stage/needs-triage Automatically applied to new issues and PRs, indicating they haven't been looked at.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant