Provide -filter and -override with command line generator

[dengsh12]

Proposed changes

In #101, we provided a command line generator for support files. The generator creates a directives map and matchFunc based on the directives definition in the source code.

However, some directives in source codes are outdated or unofficial (like post_action in OSS). You may only want to include some official directives in output and we provide a -filter option to achieve this.

Some directives have different definition in crossplane vs source code(like if in OSS, we added ngxConfExpr to it). You may want to override the output and we provide a -override option to achieve this.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

• I have read the CONTRIBUTING document
• If applicable, I have added tests that prove my fix is effective or that my feature works
• If applicable, I have checked that any relevant tests pass after adding my changes
• I have updated any relevant documentation (README.md)

[ryepup]

nit: there's another idiom for table-driven tests that uses a map to specify the name of the test case, I think that reads a little better:

	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>
		})
	}}

[dengsh12]

Oh yeah, this one is better and align with other ut in crossplane. Applied.

[ornj]

@ryepup Many crossplane tests use a slice, but I agree that a map is better. You start to see hints of this when it becomes necessary to add a "name" or other key to the testcase struct.

@dengsh12 A more important benefit to using a map vs slice is that Go randomizes the order of keys when ranging over the map. I believe-- I do not have documentation that states this as fact on hand at the moment-- this is to dissuade anyone from getting used to maps being ordered in the event the internal details are changed. You can generally use this to your benefit as it will randomize the order the tests are run which can help to detect any unintentional shared memory that leaks through tests in the form of test flake.

[ryepup]

nit: naming: Golang likes short variables names (see https://go.dev/wiki/CodeReviewComments#variable-names), in tests you'll often see two variables want and got to represent what the test wanted to the code to return, and what was actually returned.

Recommend using something besides "parser"

Suggested change

	var parser overrideItem
	err := parser.UnmarshalText([]byte(tc.input))
	var got overrideItem
	err := got.UnmarshalText([]byte(tc.input))

or maybe o

Suggested change

	var parser overrideItem
	err := parser.UnmarshalText([]byte(tc.input))
	var o overrideItem
	err := o.UnmarshalText([]byte(tc.input))

[dengsh12]

Get it. Applied

[ryepup]

nit: if we're using the require library, it has some helpers to assert around errors. That can let us eliminate the leftover if err != nil

Suggested change

	if !tc.wantErr && err != nil {
	t.Fatal(err)
	}
	if tc.wantErr && err == nil {
	t.Fatal("expected error, got nil")
	}
	// 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
	}
	if tc.wantErr {
	require.Error(t, err)
	} else {
	require.NoError(t, err)
	}

[dengsh12]

Awesome! Thanks for telling me this. Applied

[ornj]

Suggested change

	This is a simple example that takes the path of a third-party module source code to generate support for it. For detail usage of the tool, please run
	This is a simple example that takes the path of a third-party module source code to generate support for it. For detailed usage of the tool, please run

[dengsh12]

Applied

[ornj]

Suggested change

	`go run ./cmd/generate/ --help`
	Assume the source code path of that module is `./src`. You can call `go run ./cmd/generate/ --src-path=./src`. The stdout will be like
	`go run ./cmd/generate/ --help`. Assuming the source code path of that module is `./src`, you can call `go run ./cmd/generate/ --src-path=./src`. The output will be similar to:

[dengsh12]

Applied

[ornj]

Nit: no longer a "Reader"

Suggested change

	func (reader *filterFlag) Set(value string) error {
	if reader.filter == nil {
	reader.filter = make(map[string]struct{})
	}
	reader.filter[value] = struct{}{}
	return nil
	}
	func (reader *filterFlag) String() string {
	return fmt.Sprintf("%v", *reader)
	}
	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)
	}

[dengsh12]

Sure! Applied. Should've checked all the usages after rename...

[ornj]

Nit: The bytes package may offer all of the functions from the strings package that you may be able to use to avoid the (very small) cost of casting []byte to string

[dengsh12]

Oh, really surprised to see bytes provide 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).

[ornj]

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.

[dengsh12]

We moved it to Set to simplify the errors in item.UnmarshalText
#106 (comment)

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
}

[ornj]

I'm not sure why you've added an extra comma and space?

Suggested change

	return fmt.Errorf("colon not found, override=%s, ", rawOverride)
	return fmt.Errorf("colon not found, override=%s", rawOverride)

If you're concerned that printing the override could look ambiguous, you could use %q to wrap the string in quotes.

[dengsh12]

#106 (comment). Sorry for omitting removal of rawOverride output here. Just removed

[ornj]

Nit: These patterns always give me pause. I think calling a method on a nil value is generally a programming error where a panic is warranted, but there are reasons to do this. I'm not sure that it makes sense here though. I think you want the caller to initialize the value.

[dengsh12]

I went in this way because most of the situations user doesn't provide a -override, lazy initialization can avoid unnecessary allocation(and Set is not run in parallel so lazy initialization is safe here).

[ornj]

Errors more commonly follow this idiom:

Suggested change

	return fmt.Errorf("invalid override %s, reason:%w", value, err)
	return fmt.Errorf("invalid override %s: %w", value, err)

[dengsh12]

Get it, applied.