-
Notifications
You must be signed in to change notification settings - Fork 4k
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
fix(cli): diff with changeset fails if deploy role cannot be assumed #29718
Changes from 7 commits
d1190f8
037af3f
cf0abab
31c3c58
7e508dc
04099d6
c2da394
c0ef951
3559d44
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -265,6 +265,7 @@ export interface DestroyStackOptions { | |
export interface StackExistsOptions { | ||
stack: cxapi.CloudFormationStackArtifact; | ||
deployName?: string; | ||
tryLookupRole?: boolean; | ||
} | ||
|
||
export interface DeploymentsProps { | ||
|
@@ -430,7 +431,12 @@ export class Deployments { | |
} | ||
|
||
public async stackExists(options: StackExistsOptions): Promise<boolean> { | ||
const { stackSdk } = await this.prepareSdkFor(options.stack, undefined, Mode.ForReading); | ||
let stackSdk; | ||
if (options.tryLookupRole) { | ||
stackSdk = (await this.prepareSdkWithLookupOrDeployRole(options.stack)).stackSdk; | ||
} else { | ||
stackSdk = (await this.prepareSdkFor(options.stack, undefined, Mode.ForReading)).stackSdk; | ||
} | ||
Comment on lines
+435
to
+439
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Don't use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should still try to fallback to the deploy role, in case somebody doesn't have permissions to the lookup role. Why don't we catch any errors we get when trying to assume the deploy role? If we can't assume it then we stop changeset creation. Inside of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. discussed off(on?)line, we can investigate this in a follow up next week |
||
const stack = await CloudFormationStack.lookup(stackSdk.cloudFormation(), options.deployName ?? options.stack.stackName); | ||
return stack.exists; | ||
} | ||
|
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.
This option is confusing...if it's on, then try to assume the lookup role or the deploy role, but if it's off, we assume the lookup role only?
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.
we can follow this one up in another PR