Skip to content

Commit

Permalink
chore(cli): refactor notices.ts to not use configuration settings (#3…
Browse files Browse the repository at this point in the history
…2558)

This is part of a larger effort to make sure that all command-level code does not use `configuraiton.settings` directly. If it needs a CLI setting, it should be passed in as a parameter to the command-level code. The idea is that the grab bag of `configuration.settings` stays in `cli.ts` and only the relevant information is passed along to each command.

### 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*
  • Loading branch information
kaizencc authored Dec 18, 2024
1 parent c1d3130 commit 043f9db
Show file tree
Hide file tree
Showing 4 changed files with 57 additions and 136 deletions.
7 changes: 6 additions & 1 deletion packages/aws-cdk/lib/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,12 @@ export async function exec(args: string[], synthesizer?: Synthesizer): Promise<n

const cmd = argv._[0];

const notices = Notices.create({ configuration, includeAcknowledged: cmd === 'notices' ? !argv.unacknowledged : false });
const notices = Notices.create({
context: configuration.context,
output: configuration.settings.get(['outdir']),
shouldDisplay: configuration.settings.get(['notices']),
includeAcknowledged: cmd === 'notices' ? !argv.unacknowledged : false,
});
await notices.refresh();

const sdkProvider = await SdkProvider.withAwsCliCompatibleDefaults({
Expand Down
57 changes: 26 additions & 31 deletions packages/aws-cdk/lib/notices.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import type { Environment } from '@aws-cdk/cx-api';
import * as fs from 'fs-extra';
import * as semver from 'semver';
import { debug, print, warning, error } from './logging';
import { Configuration } from './settings';
import { Context } from './settings';
import { loadTreeFromDir, some } from './tree';
import { flatMap } from './util';
import { cdkCacheDir } from './util/directories';
Expand All @@ -15,9 +15,9 @@ const CACHE_FILE_PATH = path.join(cdkCacheDir(), 'notices.json');

export interface NoticesProps {
/**
* A `Configuration` instance holding CDK context and settings.
* CDK context
*/
readonly configuration: Configuration;
readonly context: Context;

/**
* Include notices that have already been acknowledged.
Expand All @@ -26,6 +26,19 @@ export interface NoticesProps {
*/
readonly includeAcknowledged?: boolean;

/**
* Global CLI option for output directory for synthesized cloud assembly
*
* @default 'cdk.out'
*/
readonly output?: string;

/**
* Global CLI option for whether we show notices
*
* @default true
*/
readonly shouldDisplay?: boolean;
}

export interface NoticesPrintOptions {
Expand All @@ -36,7 +49,6 @@ export interface NoticesPrintOptions {
* @default false
*/
readonly showTotal?: boolean;

}

export interface NoticesRefreshOptions {
Expand All @@ -63,7 +75,6 @@ export interface NoticesFilterFilterOptions {
}

export class NoticesFilter {

public static filter(options: NoticesFilterFilterOptions): FilteredNotice[] {
return [
...this.findForCliVersion(options.data, options.cliVersion),
Expand Down Expand Up @@ -124,9 +135,7 @@ export class NoticesFilter {
function compareVersions(pattern: string, target: string | undefined): boolean {
return semver.satisfies(target ?? '', pattern);
}

});

}

private static findForBootstrapVersion(data: Notice[], bootstrappedEnvironments: BootstrappedEnvironment[]): FilteredNotice[] {
Expand Down Expand Up @@ -159,7 +168,6 @@ export class NoticesFilter {
filtered.addDynamicValue('ENVIRONMENTS', affected.map(s => s.environment.name).join(','));

return [filtered];

});
}

Expand All @@ -178,7 +186,6 @@ export class NoticesFilter {
}
});
}

}

/**
Expand All @@ -193,7 +200,6 @@ export interface BootstrappedEnvironment {
* Provides access to notices the CLI can display.
*/
export class Notices {

/**
* Create an instance. Note that this replaces the singleton.
*/
Expand All @@ -211,7 +217,9 @@ export class Notices {

private static _instance: Notices | undefined;

private readonly configuration: Configuration;
private readonly context: Context;
private readonly output: string;
private readonly shouldDisplay: boolean;
private readonly acknowledgedIssueNumbers: Set<Number>;
private readonly includeAcknowlegded: boolean;

Expand All @@ -221,9 +229,11 @@ export class Notices {
private readonly bootstrappedEnvironments: Map<string, BootstrappedEnvironment> = new Map();

private constructor(props: NoticesProps) {
this.configuration = props.configuration;
this.acknowledgedIssueNumbers = new Set(this.configuration.context.get('acknowledged-issue-numbers') ?? []);
this.context = props.context;
this.acknowledgedIssueNumbers = new Set(this.context.get('acknowledged-issue-numbers') ?? []);
this.includeAcknowlegded = props.includeAcknowledged ?? false;
this.output = props.output ?? 'cdk.out';
this.shouldDisplay = props.shouldDisplay ?? true;
}

/**
Expand All @@ -248,8 +258,7 @@ export class Notices {
* If context is configured to not display notices, this will no-op.
*/
public async refresh(options: NoticesRefreshOptions = {}) {

if (!this.shouldDisplay()) {
if (!this.shouldDisplay) {
return;
}

Expand All @@ -266,15 +275,14 @@ export class Notices {
* Display the relevant notices (unless context dictates we shouldn't).
*/
public display(options: NoticesPrintOptions = {}) {

if (!this.shouldDisplay()) {
if (!this.shouldDisplay) {
return;
}

const filteredNotices = NoticesFilter.filter({
data: Array.from(this.data),
cliVersion: versionNumber(),
outDir: this.configuration.settings.get(['output']) ?? 'cdk.out',
outDir: this.output,
bootstrappedEnvironments: Array.from(this.bootstrappedEnvironments.values()),
});

Expand Down Expand Up @@ -303,17 +311,7 @@ export class Notices {
print('');
print(`There are ${filteredNotices.length} unacknowledged notice(s).`);
}

}

/**
* Determine whether or not notices should be displayed based on the
* configuration provided at instantiation time.
*/
private shouldDisplay(): boolean {
return this.configuration.settings.get(['notices']) ?? true;
}

}

export interface Component {
Expand All @@ -335,7 +333,6 @@ export interface Notice {
* dynamic values as it has access to the dynamic matching data.
*/
export class FilteredNotice {

private readonly dynamicValues: { [key: string]: string } = {};

public constructor(public readonly notice: Notice) {}
Expand All @@ -353,7 +350,6 @@ export class FilteredNotice {
`\tAffected versions: ${componentsValue}`,
`\tMore information at: https://github.com/aws/aws-cdk/issues/${this.notice.issueNumber}`,
].join('\n\n') + '\n');

}

private formatOverview() {
Expand All @@ -372,7 +368,6 @@ export class FilteredNotice {
const pattern = new RegExp(Object.keys(this.dynamicValues).join('|'), 'g');
return input.replace(pattern, (matched) => this.dynamicValues[matched] ?? matched);
}

}

export interface NoticeDataSource {
Expand Down
4 changes: 2 additions & 2 deletions packages/aws-cdk/test/api/environment-resources.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { GetParameterCommand } from '@aws-sdk/client-ssm';
import { ToolkitInfo } from '../../lib/api';
import { EnvironmentResourcesRegistry } from '../../lib/api/environment-resources';
import { CachedDataSource, Notices, NoticesFilter } from '../../lib/notices';
import { Configuration } from '../../lib/settings';
import { Context } from '../../lib/settings';
import * as version from '../../lib/version';
import { MockSdk, mockBootstrapStack, mockSSMClient } from '../util/mock-sdk';
import { MockToolkitInfo } from '../util/mock-toolkitinfo';
Expand Down Expand Up @@ -98,7 +98,7 @@ describe('validateversion without bootstrap stack', () => {
jest.spyOn(version, 'versionNumber').mockImplementation(() => '1.0.0');

// THEN
const notices = Notices.create({ configuration: new Configuration() });
const notices = Notices.create({ context: new Context() });
await notices.refresh({ dataSource: { fetch: async () => [] } });
await expect(envResources().validateVersion(8, '/abc')).resolves.toBeUndefined();

Expand Down
Loading

0 comments on commit 043f9db

Please sign in to comment.