Skip to content

Commit

Permalink
refactor(cli): remove unused v1 bootstrapping detection (#32606)
Browse files Browse the repository at this point in the history
### Reason for this change

Removing dead code in `determineV1BootstrapSource` which could not be
called anymore since version always returns v2 these days.

### Description of changes

Remove the dead code and conditional.
Also slightly changes the contract of `CliToolkit.bootstrap()` to take a
`BootstrapSource` as part of options, instead of a the `Bootstrapper`.

### Describe any new or updated permissions being added

n/a

### Description of how you validated changes

existing tests and integ tests

### Checklist
- [x] My code adheres to the [CONTRIBUTING
GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and
[DESIGN
GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache-2.0 license*

---------

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
  • Loading branch information
mrgrain and mergify[bot] authored Dec 20, 2024
1 parent 189bae3 commit 8b48fe8
Show file tree
Hide file tree
Showing 6 changed files with 77 additions and 48 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import { Mode } from '../plugin';
export type BootstrapSource = { source: 'legacy' } | { source: 'default' } | { source: 'custom'; templateFile: string };

export class Bootstrapper {
constructor(private readonly source: BootstrapSource) {}
constructor(private readonly source: BootstrapSource = { source: 'default' }) {}

public bootstrapEnvironment(
environment: cxapi.Environment,
Expand Down
8 changes: 8 additions & 0 deletions packages/aws-cdk/lib/api/bootstrap/bootstrap-props.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { BootstrapSource } from './bootstrap-environment';
import { Tag } from '../../cdk-toolkit';
import { StringWithoutPlaceholders } from '../util/placeholders';

Expand All @@ -22,6 +23,13 @@ export interface BootstrapEnvironmentOptions {
readonly parameters?: BootstrappingParameters;
readonly force?: boolean;

/**
* The source of the bootstrap stack
*
* @default - modern v2-style bootstrapping
*/
readonly source?: BootstrapSource;

/**
* Whether to execute the changeset or only create it and leave it in review.
* @default true
Expand Down
3 changes: 1 addition & 2 deletions packages/aws-cdk/lib/cdk-toolkit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -918,14 +918,13 @@ export class CdkToolkit {
*
* @param userEnvironmentSpecs environment names that need to have toolkit support
* provisioned, as a glob filter. If none is provided, all stacks are implicitly selected.
* @param bootstrapper Legacy or modern.
* @param options The name, role ARN, bootstrapping parameters, etc. to be used for the CDK Toolkit stack.
*/
public async bootstrap(
userEnvironmentSpecs: string[],
bootstrapper: Bootstrapper,
options: BootstrapEnvironmentOptions,
): Promise<void> {
const bootstrapper = new Bootstrapper(options.source);
// If there is an '--app' argument and an environment looks like a glob, we
// select the environments from the app. Otherwise, use what the user said.

Expand Down
34 changes: 5 additions & 29 deletions packages/aws-cdk/lib/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -242,15 +242,15 @@ export async function exec(args: string[], synthesizer?: Synthesizer): Promise<n
});

case 'bootstrap':
const source: BootstrapSource = determineBootstrapVersion(args, configuration);

const bootstrapper = new Bootstrapper(source);
const source: BootstrapSource = determineBootstrapVersion(args);

if (args.showTemplate) {
const bootstrapper = new Bootstrapper(source);
return bootstrapper.showTemplate(args.json);
}

return cli.bootstrap(args.ENVIRONMENTS, bootstrapper, {
return cli.bootstrap(args.ENVIRONMENTS, {
source,
roleArn: args.roleArn,
force: argv.force,
toolkitStackName: toolkitStackName,
Expand Down Expand Up @@ -468,32 +468,8 @@ export async function exec(args: string[], synthesizer?: Synthesizer): Promise<n

/**
* Determine which version of bootstrapping
* (legacy, or "new") should be used.
*/
function determineBootstrapVersion(args: { template?: string }, configuration: Configuration): BootstrapSource {
const isV1 = version.DISPLAY_VERSION.startsWith('1.');
return isV1 ? determineV1BootstrapSource(args, configuration) : determineV2BootstrapSource(args);
}

function determineV1BootstrapSource(args: { template?: string }, configuration: Configuration): BootstrapSource {
let source: BootstrapSource;
if (args.template) {
print(`Using bootstrapping template from ${args.template}`);
source = { source: 'custom', templateFile: args.template };
} else if (process.env.CDK_NEW_BOOTSTRAP) {
print('CDK_NEW_BOOTSTRAP set, using new-style bootstrapping');
source = { source: 'default' };
} else if (isFeatureEnabled(configuration, cxapi.NEW_STYLE_STACK_SYNTHESIS_CONTEXT)) {
print(`'${cxapi.NEW_STYLE_STACK_SYNTHESIS_CONTEXT}' context set, using new-style bootstrapping`);
source = { source: 'default' };
} else {
// in V1, the "legacy" bootstrapping is the default
source = { source: 'legacy' };
}
return source;
}

function determineV2BootstrapSource(args: { template?: string }): BootstrapSource {
function determineBootstrapVersion(args: { template?: string }): BootstrapSource {
let source: BootstrapSource;
if (args.template) {
print(`Using bootstrapping template from ${args.template}`);
Expand Down
36 changes: 36 additions & 0 deletions packages/aws-cdk/test/bootstrap.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import { Bootstrapper } from '../lib/api/bootstrap/bootstrap-environment';
import { exec } from '../lib/cli';

beforeEach(() => {
jest.clearAllMocks();
});

describe('cdk bootstrap', () => {
const bootstrapEnvironmentMock = jest.spyOn(Bootstrapper.prototype, 'bootstrapEnvironment');

test('will bootstrap the a provided environment', async () => {
bootstrapEnvironmentMock.mockResolvedValueOnce({
noOp: false,
outputs: {},
type: 'did-deploy-stack',
stackArn: 'fake-arn',
});

await exec(['bootstrap', 'aws://123456789012/us-east-1']);
expect(bootstrapEnvironmentMock).toHaveBeenCalledTimes(1);
expect(bootstrapEnvironmentMock).toHaveBeenCalledWith({
name: 'aws://123456789012/us-east-1',
account: '123456789012',
region: 'us-east-1',
}, expect.anything(), expect.anything());
});
});

describe('cdk bootstrap --show-template', () => {
const stdoutSpy = jest.spyOn(process.stdout, 'write').mockImplementation(() => { return true; });

test('prints the default bootstrap template', async () => {
await exec(['bootstrap', '--show-template']);
expect(stdoutSpy).toHaveBeenCalledWith(expect.stringContaining('BootstrapVersion'));
});
});
42 changes: 26 additions & 16 deletions packages/aws-cdk/test/cdk-toolkit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ import {
mockSSMClient,
restoreSdkMocksToDefault,
} from './util/mock-sdk';
import { Bootstrapper } from '../lib/api/bootstrap';
import { Bootstrapper, type BootstrapSource } from '../lib/api/bootstrap';
import { DeployStackResult, SuccessfulDeployStackResult } from '../lib/api/deploy-stack';
import {
Deployments,
Expand All @@ -97,8 +97,9 @@ markTesting();

process.env.CXAPI_DISABLE_SELECT_BY_ID = '1';

const defaultBootstrapSource: BootstrapSource = { source: 'default' };
const bootstrapEnvironmentMock = jest.spyOn(Bootstrapper.prototype, 'bootstrapEnvironment');
let cloudExecutable: MockCloudExecutable;
let bootstrapper: jest.Mocked<Bootstrapper>;
let stderrMock: jest.SpyInstance;
beforeEach(() => {
jest.resetAllMocks();
Expand All @@ -108,11 +109,12 @@ beforeEach(() => {
// on() in chokidar's Watcher returns 'this'
mockChokidarWatcherOn.mockReturnValue(fakeChokidarWatcher);

bootstrapper = instanceMockFrom(Bootstrapper);
bootstrapper.bootstrapEnvironment.mockResolvedValue({
bootstrapEnvironmentMock.mockResolvedValue({
noOp: false,
outputs: {},
} as any);
type: 'did-deploy-stack',
stackArn: 'fake-arn',
});

cloudExecutable = new MockCloudExecutable({
stacks: [MockStack.MOCK_STACK_A, MockStack.MOCK_STACK_B],
Expand Down Expand Up @@ -539,17 +541,19 @@ describe('bootstrap', () => {
configuration.context.set('@aws-cdk/core:bootstrapQualifier', 'abcde');

// WHEN
await toolkit.bootstrap(['aws://56789/south-pole'], bootstrapper, {
await toolkit.bootstrap(['aws://56789/south-pole'], {
source: defaultBootstrapSource,
parameters: {
qualifier: configuration.context.get('@aws-cdk/core:bootstrapQualifier'),
},
});

// THEN
expect(bootstrapper.bootstrapEnvironment).toHaveBeenCalledWith(expect.anything(), expect.anything(), {
expect(bootstrapEnvironmentMock).toHaveBeenCalledWith(expect.anything(), expect.anything(), {
parameters: {
qualifier: 'abcde',
},
source: defaultBootstrapSource,
});
});
});
Expand Down Expand Up @@ -868,10 +872,12 @@ describe('deploy', () => {
const toolkit = defaultToolkitSetup();

// WHEN
await toolkit.bootstrap(['aws://56789/south-pole'], bootstrapper, {});
await toolkit.bootstrap(['aws://56789/south-pole'], {
source: defaultBootstrapSource,
});

// THEN
expect(bootstrapper.bootstrapEnvironment).toHaveBeenCalledWith(
expect(bootstrapEnvironmentMock).toHaveBeenCalledWith(
{
account: '56789',
region: 'south-pole',
Expand All @@ -880,7 +886,7 @@ describe('deploy', () => {
expect.anything(),
expect.anything(),
);
expect(bootstrapper.bootstrapEnvironment).toHaveBeenCalledTimes(1);
expect(bootstrapEnvironmentMock).toHaveBeenCalledTimes(1);
});

test('globby bootstrap uses whats in the stacks', async () => {
Expand All @@ -889,10 +895,12 @@ describe('deploy', () => {
cloudExecutable.configuration.settings.set(['app'], 'something');

// WHEN
await toolkit.bootstrap(['aws://*/bermuda-triangle-1'], bootstrapper, {});
await toolkit.bootstrap(['aws://*/bermuda-triangle-1'], {
source: defaultBootstrapSource,
});

// THEN
expect(bootstrapper.bootstrapEnvironment).toHaveBeenCalledWith(
expect(bootstrapEnvironmentMock).toHaveBeenCalledWith(
{
account: '123456789012',
region: 'bermuda-triangle-1',
Expand All @@ -901,7 +909,7 @@ describe('deploy', () => {
expect.anything(),
expect.anything(),
);
expect(bootstrapper.bootstrapEnvironment).toHaveBeenCalledTimes(1);
expect(bootstrapEnvironmentMock).toHaveBeenCalledTimes(1);
});

test('bootstrap can be invoked without the --app argument', async () => {
Expand All @@ -913,10 +921,12 @@ describe('deploy', () => {
const toolkit = defaultToolkitSetup();

// WHEN
await toolkit.bootstrap(['aws://123456789012/west-pole'], bootstrapper, {});
await toolkit.bootstrap(['aws://123456789012/west-pole'], {
source: defaultBootstrapSource,
});

// THEN
expect(bootstrapper.bootstrapEnvironment).toHaveBeenCalledWith(
expect(bootstrapEnvironmentMock).toHaveBeenCalledWith(
{
account: '123456789012',
region: 'west-pole',
Expand All @@ -925,7 +935,7 @@ describe('deploy', () => {
expect.anything(),
expect.anything(),
);
expect(bootstrapper.bootstrapEnvironment).toHaveBeenCalledTimes(1);
expect(bootstrapEnvironmentMock).toHaveBeenCalledTimes(1);

expect(cloudExecutable.hasApp).toEqual(false);
expect(mockSynthesize).not.toHaveBeenCalled();
Expand Down

0 comments on commit 8b48fe8

Please sign in to comment.