-
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): can't bootstrap environment not in app #7510
Changes from 2 commits
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 |
---|---|---|
|
@@ -113,8 +113,8 @@ export class SdkProvider { | |
* The `region` and `accountId` parameters are interpreted as in `resolveEnvironment()` (which is to | ||
* say, `undefined` doesn't do what you expect). | ||
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. this comment needs updating |
||
*/ | ||
public async forEnvironment(accountId: string | undefined, region: string | undefined, mode: Mode): Promise<ISDK> { | ||
const env = await this.resolveEnvironment(accountId, region); | ||
public async forEnvironment(environment: cxapi.Environment, mode: Mode): Promise<ISDK> { | ||
const env = await this.resolveEnvironment(environment); | ||
const creds = await this.obtainCredentials(env.account, mode); | ||
return new SDK(creds, env.region, this.sdkOptions); | ||
} | ||
|
@@ -153,27 +153,19 @@ export class SdkProvider { | |
* `undefined` actually means `undefined`, and is NOT changed to default values! Only the magic values UNKNOWN_REGION | ||
* and UNKNOWN_ACCOUNT will be replaced with looked-up values! | ||
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. ditto |
||
*/ | ||
public async resolveEnvironment(accountId: string | undefined, region: string | undefined) { | ||
region = region !== cxapi.UNKNOWN_REGION ? region : this.defaultRegion; | ||
accountId = accountId !== cxapi.UNKNOWN_ACCOUNT ? accountId : (await this.defaultAccount())?.accountId; | ||
public async resolveEnvironment(env: cxapi.Environment): Promise<cxapi.Environment> { | ||
const region = env.region !== cxapi.UNKNOWN_REGION ? env.region : this.defaultRegion; | ||
const account = env.account !== cxapi.UNKNOWN_ACCOUNT ? env.account : (await this.defaultAccount())?.accountId; | ||
|
||
if (!region) { | ||
throw new Error('AWS region must be configured either when you configure your CDK stack or through the environment'); | ||
} | ||
Comment on lines
-160
to
-162
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. was this dead code? 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. Yep |
||
|
||
if (!accountId) { | ||
if (!account) { | ||
throw new Error('Unable to resolve AWS account to use. It must be either configured when you define your CDK or through the environment'); | ||
} | ||
|
||
const environment: cxapi.Environment = { | ||
region, account: accountId, name: cxapi.EnvironmentUtils.format(accountId, region), | ||
return { | ||
region, | ||
account, | ||
name: cxapi.EnvironmentUtils.format(account, region), | ||
}; | ||
|
||
return environment; | ||
} | ||
|
||
public async resolveEnvironmentObject(env: cxapi.Environment) { | ||
return this.resolveEnvironment(env.account, env.region); | ||
Comment on lines
-175
to
-176
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. there was nothing using this? 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. There was one place, but this was the method that |
||
} | ||
|
||
/** | ||
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,65 @@ | ||
import * as cxapi from '@aws-cdk/cx-api'; | ||
import * as path from 'path'; | ||
import { loadStructuredFile } from '../../serialize'; | ||
import { SdkProvider } from '../aws-auth'; | ||
import { DeployStackResult } from '../deploy-stack'; | ||
import { BootstrapEnvironmentOptions } from './bootstrap-props'; | ||
import { deployBootstrapStack } from './deploy-bootstrap'; | ||
import { legacyBootstrapTemplate } from './legacy-template'; | ||
|
||
// tslint:disable:max-line-length | ||
|
||
/** | ||
* Deploy legacy bootstrap stack | ||
* | ||
* @experimental | ||
*/ | ||
export async function bootstrapEnvironment(environment: cxapi.Environment, sdkProvider: SdkProvider, options: BootstrapEnvironmentOptions): Promise<DeployStackResult> { | ||
const params = options.parameters ?? {}; | ||
|
||
if (params.trustedAccounts?.length) { | ||
throw new Error('--trust can only be passed for the new bootstrap experience.'); | ||
} | ||
if (params.cloudFormationExecutionPolicies?.length) { | ||
throw new Error('--cloudformation-execution-policies can only be passed for the new bootstrap experience.'); | ||
} | ||
|
||
return deployBootstrapStack( | ||
legacyBootstrapTemplate(params), | ||
{}, | ||
environment, | ||
sdkProvider, | ||
options); | ||
} | ||
|
||
/** | ||
* Deploy CI/CD-ready bootstrap stack from template | ||
* | ||
* @experimental | ||
*/ | ||
export async function bootstrapEnvironment2( | ||
environment: cxapi.Environment, | ||
sdkProvider: SdkProvider, | ||
options: BootstrapEnvironmentOptions): Promise<DeployStackResult> { | ||
|
||
const params = options.parameters ?? {}; | ||
|
||
if (params.trustedAccounts?.length && !params.cloudFormationExecutionPolicies?.length) { | ||
throw new Error('--cloudformation-execution-policies are required if --trust has been passed!'); | ||
} | ||
|
||
const bootstrapTemplatePath = path.join(__dirname, 'bootstrap-template.yaml'); | ||
const bootstrapTemplate = await loadStructuredFile(bootstrapTemplatePath); | ||
|
||
return deployBootstrapStack( | ||
bootstrapTemplate, | ||
{ | ||
FileAssetsBucketName: params.bucketName, | ||
FileAssetsBucketKmsKeyId: params.kmsKeyId, | ||
TrustedAccounts: params.trustedAccounts?.join(','), | ||
CloudFormationExecutionPolicies: params.cloudFormationExecutionPolicies?.join(','), | ||
}, | ||
environment, | ||
sdkProvider, | ||
options); | ||
} |
This file was deleted.
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.
makes sense to protect against it, but what's the use case behind forcing a bootstrap and the implication of using a downgraded template version?
when would it be used?
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.
I don't want to prevent people from doing what they're trying to do, in case we get it wrong (for whatever reason).
Just feels like any protection should have an 'override' feature'.
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.
fair enough