Skip to content

Commit a890d3f

Browse files
iankhougithub-actions
andauthored
fix(cli): cdk flags --set without additional options should fail (#833)
Fixes #832 Currently, running `cdk flags --set` or `cdk flags --set --recommended` is a no-op. Either the `--all` or `--unconfigured` options should be passed alongside `--set`. This can be attributed to insufficient testing during development of the feature. Changes: - Added validation to fix the problem above - Brought `flag-operations.ts` to 100% unit test line coverage (previously 92%) <img width="1498" height="137" alt="Screenshot 2025-09-03 at 12 41 21" src="https://github.com/user-attachments/assets/1a9ad22d-4a78-411a-bb09-de4975d8752a" /> Validated with unit tests and manual testing. --- By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license --------- Signed-off-by: github-actions <[email protected]> Co-authored-by: github-actions <[email protected]>
1 parent 1ab5a09 commit a890d3f

File tree

2 files changed

+357
-1
lines changed

2 files changed

+357
-1
lines changed

packages/aws-cdk/lib/commands/flag-operations.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,7 @@ export async function handleFlags(flagData: FeatureFlag[], ioHelper: IoHelper, o
128128
}
129129

130130
if (options.unconfigured && options.FLAGNAME) {
131-
await ioHelper.defaults.error('Error: Cannot use --unconfigured with a specific flag name. --unconfigured works on multiple flags.');
131+
await ioHelper.defaults.error('Error: Cannot use --unconfigured with a specific flag name. --unconfigured works with multiple flags.');
132132
return;
133133
}
134134

@@ -147,6 +147,11 @@ export async function handleFlags(flagData: FeatureFlag[], ioHelper: IoHelper, o
147147
return;
148148
}
149149

150+
if (options.set && !options.all && !options.unconfigured && !options.FLAGNAME) {
151+
await ioHelper.defaults.error('Error: When using --set, you must specify either --all, --unconfigured, or provide a specific flag name.');
152+
return;
153+
}
154+
150155
if (options.FLAGNAME && !options.set && !options.value) {
151156
await displayFlags(params);
152157
return;

packages/aws-cdk/test/commands/flag-operations.test.ts

Lines changed: 351 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -189,6 +189,68 @@ describe('displayFlags', () => {
189189
expect(plainTextOutput).toContain('different-module');
190190
});
191191

192+
test('sorts flags by module name and then by flag name within module', async () => {
193+
// This test targets the sorting logic in displayFlagTable:
194+
// if (a.module !== b.module) { return a.module.localeCompare(b.module); }
195+
// return a.name.localeCompare(b.name);
196+
197+
const flagsForSortingTest: FeatureFlag[] = [
198+
{
199+
module: 'z-module',
200+
name: '@aws-cdk/z:flagB',
201+
recommendedValue: 'true',
202+
userValue: undefined,
203+
explanation: 'Flag B in Z module',
204+
},
205+
{
206+
module: 'a-module',
207+
name: '@aws-cdk/a:flagZ',
208+
recommendedValue: 'true',
209+
userValue: undefined,
210+
explanation: 'Flag Z in A module',
211+
},
212+
{
213+
module: 'a-module',
214+
name: '@aws-cdk/a:flagA',
215+
recommendedValue: 'true',
216+
userValue: undefined,
217+
explanation: 'Flag A in A module',
218+
},
219+
{
220+
module: 'z-module',
221+
name: '@aws-cdk/z:flagA',
222+
recommendedValue: 'true',
223+
userValue: undefined,
224+
explanation: 'Flag A in Z module',
225+
},
226+
];
227+
228+
const params = {
229+
flagData: flagsForSortingTest,
230+
toolkit: mockToolkit,
231+
ioHelper,
232+
all: true,
233+
};
234+
await displayFlags(params);
235+
236+
const plainTextOutput = output();
237+
238+
// Verify that modules are sorted alphabetically (a-module before z-module)
239+
const aModuleIndex = plainTextOutput.indexOf('Module: a-module');
240+
const zModuleIndex = plainTextOutput.indexOf('Module: z-module');
241+
expect(aModuleIndex).toBeLessThan(zModuleIndex);
242+
243+
// Verify that within a-module, flags are sorted alphabetically (flagA before flagZ)
244+
const flagAIndex = plainTextOutput.indexOf('@aws-cdk/a:flagA');
245+
const flagZIndex = plainTextOutput.indexOf('@aws-cdk/a:flagZ');
246+
expect(flagAIndex).toBeLessThan(flagZIndex);
247+
248+
// Verify that within z-module, flags are sorted alphabetically (flagA before flagB)
249+
const zFlagAIndex = plainTextOutput.indexOf('@aws-cdk/z:flagA');
250+
const zFlagBIndex = plainTextOutput.indexOf('@aws-cdk/z:flagB');
251+
expect(zFlagAIndex).toBeLessThan(zFlagBIndex);
252+
});
253+
192254
test('does not display flag when unconfigured behavior is the same as recommended behavior', async () => {
193255
const params = {
194256
flagData: mockFlagsData,
@@ -491,6 +553,295 @@ describe('handleFlags', () => {
491553
const plainTextOutput = output();
492554
expect(plainTextOutput).toContain('The \'cdk flags\' command is not compatible with the AWS CDK library used by your application. Please upgrade to 2.212.0 or above.');
493555
});
556+
557+
test('shows error when --set is used without required options', async () => {
558+
const options: FlagsOptions = {
559+
set: true,
560+
};
561+
562+
await handleFlags(mockFlagsData, ioHelper, options, mockToolkit);
563+
564+
const plainTextOutput = output();
565+
expect(plainTextOutput).toContain('Error: When using --set, you must specify either --all, --unconfigured, or provide a specific flag name.');
566+
});
567+
568+
test('shows error when --set is used with --recommended but no target flags', async () => {
569+
const options: FlagsOptions = {
570+
set: true,
571+
recommended: true,
572+
};
573+
574+
await handleFlags(mockFlagsData, ioHelper, options, mockToolkit);
575+
576+
const plainTextOutput = output();
577+
expect(plainTextOutput).toContain('Error: When using --set, you must specify either --all, --unconfigured, or provide a specific flag name.');
578+
});
579+
580+
test('shows error when using both --all and a specific flag name', async () => {
581+
const options: FlagsOptions = {
582+
FLAGNAME: ['@aws-cdk/core:testFlag'],
583+
all: true,
584+
};
585+
586+
await handleFlags(mockFlagsData, ioHelper, options, mockToolkit);
587+
588+
const plainTextOutput = output();
589+
expect(plainTextOutput).toContain('Error: Cannot use both --all and a specific flag name. Please use either --all to show all flags or specify a single flag name.');
590+
});
591+
592+
test('shows error when using options without --set', async () => {
593+
const options: FlagsOptions = {
594+
value: 'true',
595+
};
596+
597+
await handleFlags(mockFlagsData, ioHelper, options, mockToolkit);
598+
599+
const plainTextOutput = output();
600+
expect(plainTextOutput).toContain('Error: This option can only be used with --set.');
601+
});
602+
603+
test('shows error when using --value without a specific flag name', async () => {
604+
const options: FlagsOptions = {
605+
value: 'true',
606+
set: true,
607+
};
608+
609+
await handleFlags(mockFlagsData, ioHelper, options, mockToolkit);
610+
611+
const plainTextOutput = output();
612+
expect(plainTextOutput).toContain('Error: --value requires a specific flag name. Please specify a flag name when providing a value.');
613+
});
614+
615+
test('shows error when using both --recommended and --default', async () => {
616+
const options: FlagsOptions = {
617+
recommended: true,
618+
default: true,
619+
set: true,
620+
all: true,
621+
};
622+
623+
await handleFlags(mockFlagsData, ioHelper, options, mockToolkit);
624+
625+
const plainTextOutput = output();
626+
expect(plainTextOutput).toContain('Error: Cannot use both --recommended and --default. Please choose one option.');
627+
});
628+
629+
test('shows error when using both --unconfigured and --all', async () => {
630+
const options: FlagsOptions = {
631+
set: true,
632+
unconfigured: true,
633+
all: true,
634+
};
635+
636+
await handleFlags(mockFlagsData, ioHelper, options, mockToolkit);
637+
638+
const plainTextOutput = output();
639+
expect(plainTextOutput).toContain('Error: Cannot use both --unconfigured and --all. Please choose one option.');
640+
});
641+
642+
test('shows error when using both --unconfigured and a specific flag name', async () => {
643+
const options: FlagsOptions = {
644+
set: true,
645+
unconfigured: true,
646+
FLAGNAME: ['@aws-cdk/core:testFlag'],
647+
};
648+
649+
await handleFlags(mockFlagsData, ioHelper, options, mockToolkit);
650+
651+
const plainTextOutput = output();
652+
expect(plainTextOutput).toContain('Error: Cannot use --unconfigured with a specific flag name. --unconfigured works with multiple flags.');
653+
});
654+
655+
test('shows error when setting a flag without providing a value', async () => {
656+
const options: FlagsOptions = {
657+
set: true,
658+
FLAGNAME: ['@aws-cdk/core:testFlag'],
659+
};
660+
661+
await handleFlags(mockFlagsData, ioHelper, options, mockToolkit);
662+
663+
const plainTextOutput = output();
664+
expect(plainTextOutput).toContain('Error: When setting a specific flag, you must provide a --value.');
665+
});
666+
667+
test('shows error when using --set with --all without --recommended or --default', async () => {
668+
const options: FlagsOptions = {
669+
set: true,
670+
all: true,
671+
};
672+
673+
await handleFlags(mockFlagsData, ioHelper, options, mockToolkit);
674+
675+
const plainTextOutput = output();
676+
expect(plainTextOutput).toContain('Error: When using --set with --all, you must specify either --recommended or --default.');
677+
});
678+
679+
test('shows error when using --set with --unconfigured without --recommended or --default', async () => {
680+
const options: FlagsOptions = {
681+
set: true,
682+
unconfigured: true,
683+
};
684+
685+
await handleFlags(mockFlagsData, ioHelper, options, mockToolkit);
686+
687+
const plainTextOutput = output();
688+
expect(plainTextOutput).toContain('Error: When using --set with --unconfigured, you must specify either --recommended or --default.');
689+
});
690+
691+
test('shows error when trying to set a flag that does not exist', async () => {
692+
const options: FlagsOptions = {
693+
set: true,
694+
FLAGNAME: ['@aws-cdk/core:nonExistentFlag'],
695+
value: 'true',
696+
};
697+
698+
await handleFlags(mockFlagsData, ioHelper, options, mockToolkit);
699+
700+
const plainTextOutput = output();
701+
expect(plainTextOutput).toContain('Flag not found.');
702+
});
703+
704+
test('calls setMultipleFlagsIfSupported when using --set with --unconfigured and --default', async () => {
705+
const flagsWithUnconfiguredBehavior: FeatureFlag[] = [
706+
{
707+
module: 'aws-cdk-lib',
708+
name: '@aws-cdk/core:flagWithV2True',
709+
recommendedValue: 'false',
710+
userValue: undefined,
711+
explanation: 'Flag with unconfiguredBehavesLike.v2 = true',
712+
unconfiguredBehavesLike: { v2: 'true' },
713+
},
714+
{
715+
module: 'aws-cdk-lib',
716+
name: '@aws-cdk/core:flagWithV2False',
717+
recommendedValue: 'false',
718+
userValue: undefined,
719+
explanation: 'Flag with unconfiguredBehavesLike.v2 = false',
720+
unconfiguredBehavesLike: { v2: 'false' },
721+
},
722+
];
723+
724+
const cdkJsonPath = await createCdkJsonFile({});
725+
726+
setupMockToolkitForPrototyping(mockToolkit);
727+
728+
const requestResponseSpy = jest.spyOn(ioHelper, 'requestResponse');
729+
requestResponseSpy.mockResolvedValue(true);
730+
731+
const options: FlagsOptions = {
732+
set: true,
733+
unconfigured: true,
734+
default: true,
735+
};
736+
737+
await handleFlags(flagsWithUnconfiguredBehavior, ioHelper, options, mockToolkit);
738+
739+
// Verify that the prototyping process was called (indicating setMultipleFlagsIfSupported was executed)
740+
expect(mockToolkit.fromCdkApp).toHaveBeenCalled();
741+
expect(mockToolkit.synth).toHaveBeenCalled();
742+
expect(mockToolkit.diff).toHaveBeenCalled();
743+
expect(requestResponseSpy).toHaveBeenCalled();
744+
745+
// Verify that the flags were set to their default values based on unconfiguredBehavesLike.v2
746+
const updatedContent = await fs.promises.readFile(cdkJsonPath, 'utf-8');
747+
const updatedJson = JSON.parse(updatedContent);
748+
749+
expect(updatedJson.context['@aws-cdk/core:flagWithV2True']).toBe(true);
750+
expect(updatedJson.context['@aws-cdk/core:flagWithV2False']).toBe(false);
751+
752+
await cleanupCdkJsonFile(cdkJsonPath);
753+
requestResponseSpy.mockRestore();
754+
});
755+
756+
test('handles boolean flag values correctly in toBooleanValue function', async () => {
757+
const flagsWithBooleanRecommendedValues: FeatureFlag[] = [
758+
{
759+
module: 'aws-cdk-lib',
760+
name: '@aws-cdk/core:booleanTrueFlag',
761+
recommendedValue: true,
762+
userValue: undefined,
763+
explanation: 'Flag with boolean true recommended value',
764+
},
765+
{
766+
module: 'aws-cdk-lib',
767+
name: '@aws-cdk/core:booleanFalseFlag',
768+
recommendedValue: false,
769+
userValue: undefined,
770+
explanation: 'Flag with boolean false recommended value',
771+
},
772+
];
773+
774+
const cdkJsonPath = await createCdkJsonFile({});
775+
776+
setupMockToolkitForPrototyping(mockToolkit);
777+
778+
const requestResponseSpy = jest.spyOn(ioHelper, 'requestResponse');
779+
requestResponseSpy.mockResolvedValue(true);
780+
781+
const options: FlagsOptions = {
782+
set: true,
783+
all: true,
784+
recommended: true,
785+
};
786+
787+
await handleFlags(flagsWithBooleanRecommendedValues, ioHelper, options, mockToolkit);
788+
789+
// Verify that the flags were set correctly using boolean values
790+
const updatedContent = await fs.promises.readFile(cdkJsonPath, 'utf-8');
791+
const updatedJson = JSON.parse(updatedContent);
792+
793+
// These should be set to their boolean recommended values, testing the toBooleanValue boolean branch
794+
expect(updatedJson.context['@aws-cdk/core:booleanTrueFlag']).toBe(true);
795+
expect(updatedJson.context['@aws-cdk/core:booleanFalseFlag']).toBe(false);
796+
797+
await cleanupCdkJsonFile(cdkJsonPath);
798+
requestResponseSpy.mockRestore();
799+
});
800+
801+
test('shows error when flag is not found during prototypeChanges', async () => {
802+
// This test targets the validation in prototypeChanges function:
803+
// if (!flag) { await ioHelper.defaults.error(`Flag ${flagName} not found.`); return false; }
804+
805+
const cdkJsonPath = await createCdkJsonFile({});
806+
807+
setupMockToolkitForPrototyping(mockToolkit);
808+
809+
// Create a scenario where we try to set multiple flags but one doesn't exist
810+
// We'll mock the internal flag lookup to simulate a missing flag during the prototyping process
811+
const originalFind = Array.prototype.find;
812+
let findCallCount = 0;
813+
814+
// Mock Array.find to return undefined for the second call (simulating missing flag in prototypeChanges)
815+
Array.prototype.find = function(this: any[], callback: any) {
816+
findCallCount++;
817+
// First call is in handleFlags validation (should find the flag)
818+
// Second call is in prototypeChanges (should not find the flag to trigger our test case)
819+
if (findCallCount === 2) {
820+
return undefined; // Simulate flag not found in prototypeChanges
821+
}
822+
return originalFind.call(this, callback);
823+
};
824+
825+
const options: FlagsOptions = {
826+
set: true,
827+
all: true,
828+
recommended: true,
829+
};
830+
831+
await handleFlags(mockFlagsData, ioHelper, options, mockToolkit);
832+
833+
const plainTextOutput = output();
834+
expect(plainTextOutput).toContain('Flag @aws-cdk/s3:anotherFlag not found.');
835+
836+
// Verify that prototyping was attempted but failed due to missing flag
837+
expect(mockToolkit.fromCdkApp).toHaveBeenCalledTimes(1); // Only the initial call, not the modified one
838+
expect(mockToolkit.diff).not.toHaveBeenCalled(); // Diff should not be called due to early return
839+
840+
// Restore original Array.find
841+
Array.prototype.find = originalFind;
842+
843+
await cleanupCdkJsonFile(cdkJsonPath);
844+
});
494845
});
495846

496847
describe('modifyValues', () => {

0 commit comments

Comments
 (0)