Skip to content

Commit 3d7b09b

Browse files
authored
feat(cli): display warning when --role-arn is used with gc command (#893)
The gc command currently ignores the --role-arn parameter and always uses the user's default credentials, regardless of whether a role ARN is specified. This can potentially lead to silent failures, permission issues, as well as a bad user experience due to confusion. This pr adds a check to throw a warning when the --role-arn parameter is used in the gc command. This helps improve user experience and avoid mismanaged expectations. A consideration to throw an error was made. However, this could be a breaking change for existing ci/cd pipeline users. Therefore, this idea was discarded --- By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license
1 parent 75b8256 commit 3d7b09b

File tree

2 files changed

+59
-0
lines changed

2 files changed

+59
-0
lines changed

packages/aws-cdk/lib/cli/cli.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -475,6 +475,11 @@ export async function exec(args: string[], synthesizer?: Synthesizer): Promise<n
475475
if (args.bootstrapStackName) {
476476
await ioHost.defaults.warn('--bootstrap-stack-name is deprecated and will be removed when gc is GA. Use --toolkit-stack-name.');
477477
}
478+
// roleArn is defined for when cloudformation is invoked
479+
// This conflicts with direct sdk calls existing in the gc command to s3 and ecr
480+
if (args.roleArn) {
481+
await ioHost.defaults.warn('The --role-arn option is not supported for the gc command and will be ignored.');
482+
}
478483
return cli.garbageCollect(args.ENVIRONMENTS, {
479484
action: args.action,
480485
type: args.type,

packages/aws-cdk/test/cli/cli.test.ts

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,14 @@ jest.mock('../../lib/cli/parse-command-line-arguments', () => ({
5151
if (args.includes('ts')) {
5252
result = { ...result, language: 'typescript' };
5353
}
54+
} else if (args.includes('gc')) {
55+
result = { ...result, _: ['gc'] };
56+
57+
// Handle role-arn flag for gc command validation testing
58+
// This simulates parser behavior to test that the CLI properly rejects roleArn
59+
if (args.includes('--role-arn')) {
60+
result = { ...result, roleArn: 'arn:aws:iam::123456789012:role/TestRole' };
61+
}
5462
}
5563

5664
// Handle notices flags
@@ -427,3 +435,49 @@ describe('notices configuration tests', () => {
427435
);
428436
});
429437
});
438+
439+
describe('gc command tests', () => {
440+
let originalCliIoHostInstance: any;
441+
442+
beforeEach(() => {
443+
jest.clearAllMocks();
444+
originalCliIoHostInstance = CliIoHost.instance;
445+
});
446+
447+
afterEach(() => {
448+
CliIoHost.instance = originalCliIoHostInstance;
449+
});
450+
451+
test('should warn when --role-arn is used with gc command', async () => {
452+
const gcSpy = jest.spyOn(cdkToolkitModule.CdkToolkit.prototype, 'garbageCollect').mockResolvedValue();
453+
454+
// Make exec use our TestIoHost and adds properties to TestIoHost to match CliIoHost
455+
const warnSpy = jest.fn();
456+
(ioHost as any).defaults = { warn: warnSpy, debug: jest.fn(), result: jest.fn() };
457+
(ioHost as any).asIoHelper = () => ioHelper;
458+
(ioHost as any).logLevel = 'info';
459+
jest.spyOn(CliIoHost, 'instance').mockReturnValue(ioHost as any);
460+
461+
const mockConfig = {
462+
loadConfigFiles: jest.fn().mockResolvedValue(undefined),
463+
settings: {
464+
get: jest.fn().mockImplementation((key: string[]) => {
465+
if (key[0] === 'unstable') return ['gc'];
466+
return [];
467+
}),
468+
},
469+
context: {
470+
get: jest.fn().mockReturnValue([]),
471+
},
472+
};
473+
474+
Configuration.fromArgsAndFiles = jest.fn().mockResolvedValue(mockConfig);
475+
476+
await exec(['gc', '--unstable=gc', '--role-arn', 'arn:aws:iam::123456789012:role/TestRole']);
477+
478+
expect(warnSpy).toHaveBeenCalledWith(
479+
'The --role-arn option is not supported for the gc command and will be ignored.',
480+
);
481+
expect(gcSpy).toHaveBeenCalled();
482+
});
483+
});

0 commit comments

Comments
 (0)