Skip to content

Commit f2d95e1

Browse files
bvtujobvtujo
authored
fix(cli): Correctly set NoLSI field when askDynamoLSIConfig returns false (#1057)
<!-- Provide summary of changes --> This change solves an issue where if a customer selected "No" when asked "Would you like to add any alternate sort keys to this DynamoDB table?", the `noLsi` option was not properly set and an erroneous line was injected into the CF template. <!-- Issue number, if available. E.g. "Fixes #31", "Addresses #42, 77" --> By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
1 parent 8ad9bbe commit f2d95e1

5 files changed

Lines changed: 81 additions & 28 deletions

File tree

‎internal/pkg/cli/flag.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,7 @@ Must be of the format '<keyName>:<dataType>'.`
133133
storageSortKeyFlagDescription = `Optional. Sort key for the DDB table.
134134
Must be of the format '<keyName>:<dataType>'.`
135135
storageNoSortFlagDescription = "Optional. Skip configuring sort keys."
136-
storageNoLsiFlagDescription = `Optional. Don't ask about configuring alternate sort keys.`
136+
storageNoLSIFlagDescription = `Optional. Don't ask about configuring alternate sort keys.`
137137
storageLSIConfigFlagDescription = `Optional. Attribute to use as an alternate sort key. May be specified up to 5 times.
138138
Must be of the format '<keyName>:<dataType>'.`
139139

‎internal/pkg/cli/storage_init.go‎

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,7 @@ type initStorageVars struct {
122122
partitionKey string
123123
sortKey string
124124
lsiSorts []string // lsi sort keys collected as "name:T" where T is one of [SNB]
125-
noLsi bool
125+
noLSI bool
126126
noSort bool
127127
}
128128

@@ -198,7 +198,7 @@ func (o *initStorageOpts) Validate() error {
198198
}
199199
}
200200
// --no-lsi and --lsi are mutually exclusive.
201-
if o.noLsi && len(o.lsiSorts) != 0 {
201+
if o.noLSI && len(o.lsiSorts) != 0 {
202202
return fmt.Errorf("validate LSI configuration: cannot specify --no-lsi and --lsi options at once")
203203
}
204204

@@ -384,7 +384,7 @@ func (o *initStorageOpts) askDynamoLSIConfig() error {
384384
return nil
385385
}
386386
// If --no-lsi has been specified, there is no need to ask for local secondary indices.
387-
if o.noLsi {
387+
if o.noLSI {
388388
return nil
389389
}
390390
// If --no-sort has been specified, there is no need to ask for local secondary indices.
@@ -399,13 +399,16 @@ func (o *initStorageOpts) askDynamoLSIConfig() error {
399399
return fmt.Errorf("confirm add alternate sort key: %w", err)
400400
}
401401
for {
402-
if !moreLSI {
403-
break
404-
}
405402
if len(o.lsiSorts) > 5 {
406403
log.Infoln("You may not specify more than 5 alternate sort keys. Continuing...")
407-
break
404+
moreLSI = false
405+
}
406+
// This will execute last in the loop if moreLSI is set to false by any confirm prompts.
407+
if !moreLSI {
408+
o.noLSI = len(o.lsiSorts) == 0
409+
return nil
408410
}
411+
409412
lsiName, err := o.prompt.Get(storageInitDDBLSINamePrompt,
410413
storageInitDDBLSINameHelp,
411414
dynamoTableNameValidation,
@@ -577,7 +580,7 @@ func (o *initStorageOpts) newDynamoDBAddon() (*addon.DynamoDB, error) {
577580
props.Attributes = attributes
578581
// only configure LSI if we haven't specified the --no-lsi flag.
579582
props.HasLSI = false
580-
if !o.noLsi {
583+
if !o.noLSI {
581584
props.HasLSI = true
582585
lsiConfig, err := newLSI(
583586
*partKey.Name,
@@ -660,7 +663,7 @@ func BuildStorageInitCmd() *cobra.Command {
660663
cmd.Flags().StringVar(&vars.partitionKey, storagePartitionKeyFlag, "", storagePartitionKeyFlagDescription)
661664
cmd.Flags().StringVar(&vars.sortKey, storageSortKeyFlag, "", storageSortKeyFlagDescription)
662665
cmd.Flags().StringArrayVar(&vars.lsiSorts, storageLSIConfigFlag, []string{}, storageLSIConfigFlagDescription)
663-
cmd.Flags().BoolVar(&vars.noLsi, storageNoLSIFlag, false, storageNoLsiFlagDescription)
666+
cmd.Flags().BoolVar(&vars.noLSI, storageNoLSIFlag, false, storageNoLSIFlagDescription)
664667
cmd.Flags().BoolVar(&vars.noSort, storageNoSortFlag, false, storageNoSortFlagDescription)
665668

666669
requiredFlags := pflag.NewFlagSet("Required", pflag.ContinueOnError)

‎internal/pkg/cli/storage_init_test.go‎

Lines changed: 59 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -171,13 +171,15 @@ func TestStorageInitOpts_Ask(t *testing.T) {
171171
inPartition string
172172
inSort string
173173
inLSISorts []string
174-
inNoLsi bool
174+
inNoLSI bool
175175
inNoSort bool
176176

177177
mockPrompt func(m *mocks.Mockprompter)
178178
mockCfg func(m *mocks.MockconfigSelector)
179179

180180
wantedErr error
181+
182+
wantedVars *initStorageVars
181183
}{
182184
"Asks for storage type": {
183185
inAppName: wantedAppName,
@@ -265,7 +267,7 @@ func TestStorageInitOpts_Ask(t *testing.T) {
265267
inStorageType: dynamoDBStorageType,
266268
inStorageName: wantedTableName,
267269
inSort: wantedSortKey,
268-
inNoLsi: true,
270+
inNoLSI: true,
269271

270272
mockPrompt: func(m *mocks.Mockprompter) {
271273
keyPrompt := fmt.Sprintf(fmtStorageInitDDBKeyPrompt,
@@ -334,7 +336,7 @@ func TestStorageInitOpts_Ask(t *testing.T) {
334336
inStorageType: dynamoDBStorageType,
335337
inStorageName: wantedTableName,
336338
inPartition: wantedPartitionKey,
337-
inNoLsi: true,
339+
inNoLSI: true,
338340

339341
mockPrompt: func(m *mocks.Mockprompter) {
340342
m.EXPECT().Confirm(
@@ -438,7 +440,7 @@ func TestStorageInitOpts_Ask(t *testing.T) {
438440
inStorageName: wantedTableName,
439441
inPartition: wantedPartitionKey,
440442
inNoSort: true,
441-
inNoLsi: true,
443+
inNoLSI: true,
442444

443445
mockPrompt: func(m *mocks.Mockprompter) {},
444446
mockCfg: func(m *mocks.MockconfigSelector) {},
@@ -452,7 +454,7 @@ func TestStorageInitOpts_Ask(t *testing.T) {
452454
inStorageName: wantedTableName,
453455
inPartition: wantedPartitionKey,
454456
inSort: wantedSortKey,
455-
inNoLsi: true,
457+
inNoLSI: true,
456458

457459
mockPrompt: func(m *mocks.Mockprompter) {},
458460
mockCfg: func(m *mocks.MockconfigSelector) {},
@@ -507,6 +509,50 @@ func TestStorageInitOpts_Ask(t *testing.T) {
507509
).Return(false, nil)
508510
},
509511
mockCfg: func(m *mocks.MockconfigSelector) {},
512+
513+
wantedVars: &initStorageVars{
514+
GlobalOpts: &GlobalOpts{
515+
appName: wantedAppName,
516+
},
517+
storageName: wantedTableName,
518+
storageSvc: wantedSvcName,
519+
storageType: dynamoDBStorageType,
520+
521+
partitionKey: wantedPartitionKey,
522+
sortKey: wantedSortKey,
523+
noLSI: false,
524+
lsiSorts: []string{"Email:S"},
525+
},
526+
},
527+
"noLSI is set correctly if no lsis specified": {
528+
inAppName: wantedAppName,
529+
inSvcName: wantedSvcName,
530+
inStorageType: dynamoDBStorageType,
531+
inStorageName: wantedTableName,
532+
inPartition: wantedPartitionKey,
533+
inSort: wantedSortKey,
534+
535+
mockPrompt: func(m *mocks.Mockprompter) {
536+
m.EXPECT().Confirm(
537+
gomock.Eq(storageInitDDBLSIPrompt),
538+
gomock.Eq(storageInitDDBLSIHelp),
539+
gomock.Any(),
540+
).Return(false, nil)
541+
},
542+
mockCfg: func(m *mocks.MockconfigSelector) {},
543+
544+
wantedVars: &initStorageVars{
545+
GlobalOpts: &GlobalOpts{
546+
appName: wantedAppName,
547+
},
548+
storageName: wantedTableName,
549+
storageSvc: wantedSvcName,
550+
storageType: dynamoDBStorageType,
551+
552+
partitionKey: wantedPartitionKey,
553+
sortKey: wantedSortKey,
554+
noLSI: true,
555+
},
510556
},
511557
"error if lsi name misspecified": {
512558
inAppName: wantedAppName,
@@ -616,7 +662,7 @@ func TestStorageInitOpts_Ask(t *testing.T) {
616662
partitionKey: tc.inPartition,
617663
sortKey: tc.inSort,
618664
lsiSorts: tc.inLSISorts,
619-
noLsi: tc.inNoLsi,
665+
noLSI: tc.inNoLSI,
620666
noSort: tc.inNoSort,
621667
},
622668
sel: mockConfig,
@@ -632,6 +678,10 @@ func TestStorageInitOpts_Ask(t *testing.T) {
632678
} else {
633679
require.Nil(t, err)
634680
}
681+
if tc.wantedVars != nil {
682+
tc.wantedVars.prompt = opts.prompt
683+
require.Equal(t, *tc.wantedVars, opts.initStorageVars)
684+
}
635685
})
636686
}
637687
}
@@ -654,7 +704,7 @@ func TestStorageInitOpts_Execute(t *testing.T) {
654704
inPartition string
655705
inSort string
656706
inLSISorts []string
657-
inNoLsi bool
707+
inNoLSI bool
658708
inNoSort bool
659709

660710
mockWs func(m *mocks.MockwsAddonManager)
@@ -678,7 +728,7 @@ func TestStorageInitOpts_Execute(t *testing.T) {
678728
inStorageType: dynamoDBStorageType,
679729
inSvcName: wantedSvcName,
680730
inStorageName: "my-table",
681-
inNoLsi: true,
731+
inNoLSI: true,
682732
inNoSort: true,
683733
inPartition: wantedPartitionKey,
684734

@@ -746,7 +796,7 @@ func TestStorageInitOpts_Execute(t *testing.T) {
746796
partitionKey: tc.inPartition,
747797
sortKey: tc.inSort,
748798
lsiSorts: tc.inLSISorts,
749-
noLsi: tc.inNoLsi,
799+
noLSI: tc.inNoLSI,
750800
noSort: tc.inNoSort,
751801
},
752802
ws: mockAddon,

‎internal/pkg/cli/validate.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,8 @@ var (
2929
errDDBValueBadSize = errors.New("value must be between 3 and 255 characters in length")
3030
errValueBadFormatWithPeriodUnderscore = errors.New("value must contain only alphanumeric characters and ._-")
3131
errDDBAttributeBadFormat = errors.New("value must be of the form <name>:<T> where T is one of S, N, or B")
32-
errLsiAttributeNotPresent = errors.New("lsi must be present in list of attributes")
33-
errTooManyLsiKeys = errors.New("number of specified LSI sort keys must be 5 or less")
32+
errLSIAttributeNotPresent = errors.New("lsi must be present in list of attributes")
33+
errTooManyLSIKeys = errors.New("number of specified LSI sort keys must be 5 or less")
3434
)
3535

3636
var fmtErrInvalidStorageType = "invalid storage type %s: must be one of %s"
@@ -279,7 +279,7 @@ func validateLSIs(val interface{}) error {
279279
return errValueNotAStringSlice
280280
}
281281
if len(s) > 5 {
282-
return errTooManyLsiKeys
282+
return errTooManyLSIKeys
283283
}
284284
for _, att := range s {
285285
err := validateKey(att)

‎internal/pkg/cli/validate_test.go‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -279,25 +279,25 @@ func TestGetAttributeFromKey(t *testing.T) {
279279
func TestValidateLSIs(t *testing.T) {
280280
testCases := map[string]struct {
281281
inputAttributes []string
282-
inputLsis []string
282+
inputLSIs []string
283283
wantError error
284284
}{
285285
"good case": {
286-
inputLsis: []string{"userID:S"},
286+
inputLSIs: []string{"userID:S"},
287287
wantError: nil,
288288
},
289289
"bad lsi structure": {
290-
inputLsis: []string{"userID"},
290+
inputLSIs: []string{"userID"},
291291
wantError: errDDBAttributeBadFormat,
292292
},
293293
"too many lsis": {
294-
inputLsis: []string{"bowie:S", "clyde:S", "keno:S", "kava:S", "meow:S", "hana:S"},
295-
wantError: errTooManyLsiKeys,
294+
inputLSIs: []string{"bowie:S", "clyde:S", "keno:S", "kava:S", "meow:S", "hana:S"},
295+
wantError: errTooManyLSIKeys,
296296
},
297297
}
298298
for name, tc := range testCases {
299299
t.Run(name, func(t *testing.T) {
300-
got := validateLSIs(tc.inputLsis)
300+
got := validateLSIs(tc.inputLSIs)
301301
if tc.wantError != nil {
302302
require.EqualError(t, got, tc.wantError.Error())
303303
} else {

0 commit comments

Comments
 (0)