Provide -filter and -override with command line generator
#106
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,99 @@ | ||
| /** | ||
| * Copyright (c) F5, Inc. | ||
| * | ||
| * This source code is licensed under the Apache License, Version 2.0 license found in the | ||
| * LICENSE file in the root directory of this source tree. | ||
| */ | ||
|
|
||
| package main | ||
|
|
||
| import ( | ||
| "errors" | ||
| "fmt" | ||
| "strings" | ||
|
|
||
| "github.com/nginxinc/nginx-go-crossplane/internal/generator" | ||
| ) | ||
|
|
||
| type filterFlag struct { | ||
| filter map[string]struct{} | ||
| } | ||
|
|
||
| func (f *filterFlag) Set(value string) error { | ||
| if f.filter == nil { | ||
| f.filter = make(map[string]struct{}) | ||
| } | ||
| f.filter[value] = struct{}{} | ||
| return nil | ||
| } | ||
|
|
||
| func (f *filterFlag) String() string { | ||
| return fmt.Sprintf("%v", *f) | ||
| } | ||
|
|
||
| type overrideItem struct { | ||
| directive string | ||
| masks []generator.Mask | ||
| } | ||
|
|
||
| func (item *overrideItem) UnmarshalText(text []byte) error { | ||
| rawOverride := string(text) | ||
|
|
||
| // rawStr should follow the format: directive:bitmask00|bitmask01|...,bitmask10|bitmask11|... | ||
| directive, definition, found := strings.Cut(rawOverride, ":") | ||
| if !found { | ||
| return errors.New("colon not found") | ||
| } | ||
| directive = strings.TrimSpace(directive) | ||
|
|
||
| item.directive = directive | ||
| if directive == "" { | ||
| return errors.New("directive name is empty") | ||
|
There was a problem hiding this comment. Include the override to communicate which one is being rejected? return fmt.Errorf("directive name is empty: override=%s", rawOverride)I think I would do this for most errors where it would be helpful to see the specific input being rejected. There was a problem hiding this comment. We moved it to func (ov *override) Set(value string) error {
if *ov == nil {
*ov = override{}
}
var item overrideItem
err := item.UnmarshalText([]byte(value))
if err != nil {
return fmt.Errorf("invalid override %s:%w", value, err)
}
(*ov)[item.directive] = item.masks
return nil
} |
||
| } | ||
|
|
||
| definition = strings.TrimSpace(definition) | ||
| if definition == "" { | ||
| return errors.New("directive definition is empty") | ||
| } | ||
|
|
||
| for _, varNamesStr := range strings.Split(definition, ",") { | ||
| varNamesList := strings.Split(varNamesStr, "|") | ||
| varNamesNum := len(varNamesList) | ||
| directiveMask := make(generator.Mask, varNamesNum) | ||
|
|
||
| for idx, varName := range varNamesList { | ||
| trimmedName := strings.TrimSpace(varName) | ||
| if trimmedName == "" { | ||
| return errors.New("one directive bitmask is empty, check if there are unnecessary |") | ||
| } | ||
|
|
||
| directiveMask[idx] = trimmedName | ||
| } | ||
| item.masks = append(item.masks, directiveMask) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| type override map[string][]generator.Mask | ||
|
|
||
| func (ov *override) String() string { | ||
| if ov == nil { | ||
| return "nil" | ||
| } | ||
| return fmt.Sprintf("%v", *ov) | ||
| } | ||
|
|
||
| func (ov *override) Set(value string) error { | ||
| if *ov == nil { | ||
| *ov = override{} | ||
| } | ||
|
Comment on lines
+88
to
+90
There was a problem hiding this comment. Nit: These patterns always give me pause. I think calling a method on a There was a problem hiding this comment. I went in this way because most of the situations user doesn't provide a |
||
| var item overrideItem | ||
| err := item.UnmarshalText([]byte(value)) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid override %s:%w", value, err) | ||
| } | ||
|
|
||
| (*ov)[item.directive] = item.masks | ||
| return nil | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,95 @@ | ||
| /** | ||
| * Copyright (c) F5, Inc. | ||
| * | ||
| * This source code is licensed under the Apache License, Version 2.0 license found in the | ||
| * LICENSE file in the root directory of this source tree. | ||
| */ | ||
|
|
||
| package main | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/nginxinc/nginx-go-crossplane/internal/generator" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| //nolint:funlen | ||
| func TestOverrideParser(t *testing.T) { | ||
| t.Parallel() | ||
| tests := []struct { | ||
| name string | ||
| input string | ||
| expected overrideItem | ||
| wantErr bool | ||
| }{ | ||
| { | ||
|
Comment on lines
+20
to
+26
There was a problem hiding this comment. nit: there's another idiom for table-driven tests that uses a testcases := map[string]struct{
input string
expected overrideItem
wantErr bool
}{
"normalFormat_pass": {
input: "location:ngxHTTPMainConf|ngxConfTake12,ngxStreamMainConf",
expected: overrideItem{
directive: "location",
masks: []generator.Mask{
{"ngxHTTPMainConf", "ngxConfTake12"},
{"ngxStreamMainConf"},
},
},
},
// <other test cases omitted for brevity>
}
for name, tc := range testcases{
tc := tc
t.Run(name, func(t *testing.T) {
t.Parallel()
// <test omitted for brevity>
})
}}There was a problem hiding this comment. Oh yeah, this one is better and align with other ut in crossplane. Applied. There was a problem hiding this comment. @ryepup Many crossplane tests use a slice, but I agree that a @dengsh12 A more important benefit to using a |
||
| name: "normalFormat_pass", | ||
| input: "location:ngxHTTPMainConf|ngxConfTake12,ngxStreamMainConf", | ||
|
|
||
| expected: overrideItem{ | ||
| directive: "location", | ||
| masks: []generator.Mask{ | ||
| {"ngxHTTPMainConf", "ngxConfTake12"}, | ||
| {"ngxStreamMainConf"}, | ||
| }, | ||
| }, | ||
| wantErr: false, | ||
| }, | ||
| { | ||
| name: "withSpaces_pass", | ||
| input: "hash:ngxHTTPUpsConf | ngxConfTake12, ngxStreamUpsConf | ngxConfTake12", | ||
| expected: overrideItem{ | ||
| directive: "hash", | ||
| masks: []generator.Mask{ | ||
| {"ngxHTTPUpsConf", "ngxConfTake12"}, | ||
| {"ngxStreamUpsConf", "ngxConfTake12"}, | ||
| }, | ||
| }, | ||
| wantErr: false, | ||
| }, | ||
| { | ||
| name: "withoutColon_fail", | ||
| input: "hashngxHTTPUpsConf | ngxConfTake12,ngxStreamUpsConf | ngxConfTake12", | ||
| wantErr: true, | ||
| }, | ||
| { | ||
| name: "colonLeftsideEmpty_fail", | ||
| input: " :ngxHTTPUpsConf | ngxConfTake12,ngxStreamUpsConf | ngxConfTake12", | ||
| wantErr: true, | ||
| }, | ||
| { | ||
| name: "colonRightsideEmpty_fail", | ||
| input: "hash: ", | ||
| wantErr: true, | ||
| }, | ||
| { | ||
| name: "emptyBitmask_fail", | ||
| input: "hash: ngxHTTPUpsConf| ", | ||
| wantErr: true, | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range tests { | ||
| tc := tc | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| t.Parallel() | ||
| var got overrideItem | ||
| err := got.UnmarshalText([]byte(tc.input)) | ||
|
|
||
| if tc.wantErr { | ||
| require.Error(t, err) | ||
| } else { | ||
| require.NoError(t, err) | ||
| } | ||
|
|
||
| // If the testcase wants an error and there is an error, skip the output file validation. | ||
| // Output makes no sense when there is an error. | ||
| if err != nil { | ||
| return | ||
| } | ||
|
|
||
| require.Equal(t, tc.expected, got) | ||
| }) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: The
bytespackage may offer all of the functions from thestringspackage that you may be able to use to avoid the (very small) cost of casting[]bytetostringUh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, really surprised to see
bytesprovide such strong capabilities. Thanks for telling me this. But for here I think the cost is acceptable? Transfer it into string seems more make sense(since override is a string originally).