-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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.
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
feat(app-staging-synthesizer): cross-account support #26638
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request
. Additionally, if clarification is needed add Clarification Request
to a comment.
Exemption Request Looking at other commits adding new props I could not find new integration tests, e.g. 1b36124 |
0393f4f
to
4002f4c
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution!
For me the PR looks good and can be reviewed by a maintainer.
* Specify trusted principals for e.g. cross-account usage. | ||
* They will be added to the default principal. | ||
* Both file-role and image-role will be modified. | ||
* Will be ignored when custom roles are provided. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a validation for this please.
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
defaultStackSynthesizer: AppStagingSynthesizer.defaultResources({ | ||
appId: 'my-app-id', | ||
additionalTrustedPrincipals: [new iam.AccountPrincipal('999999999999')], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I correct in assuming that this example is not complete?
Because by default this would assume a bootstrapped Deploy Role, and only the Deploy Role needs to be able to assume the Asset Roles.
So are you using
deploymentIdentities: DeploymentIdentities.cliCredentials(),
In practice when you need this feature (and the credentials happen to be for account 999999999999
?)
Marking this as draft because @tenjaa and I are still having a conversation about the use cases of this PR |
This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week. |
Closes #26634
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license