Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix function calls with inconsistent argument breaking #932

Merged
merged 2 commits into from
Aug 3, 2023
Merged

Fix function calls with inconsistent argument breaking #932

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0356953
Fix function calls with inconsistent argument breaking
pleshakov Aug 2, 2023
780a39c
Merge branch 'main' into tech-debt/fix-inconsistent-line-breaks
pleshakov Aug 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions cmd/gateway/commands.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,8 @@ func createStaticModeCommand() *cobra.Command {
Short: "Configure NGINX in the scope of a single Gateway resource",
RunE: func(cmd *cobra.Command, args []string) error {
logger := zap.New()
logger.Info("Starting NGINX Kubernetes Gateway in static mode",
logger.Info(
"Starting NGINX Kubernetes Gateway in static mode",
"version", version,
"commit", commit,
"date", date,
Expand Down Expand Up @@ -184,7 +185,8 @@ func createProvisionerModeCommand() *cobra.Command {
Hidden: true,
RunE: func(cmd *cobra.Command, args []string) error {
logger := zap.New()
logger.Info("Starting NGINX Kubernetes Gateway Provisioner",
logger.Info(
"Starting NGINX Kubernetes Gateway Provisioner",
"version", version,
"commit", commit,
"date", date,
Expand Down
27 changes: 24 additions & 3 deletions docs/developer/go-style-guide.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,20 +45,27 @@ signaller <- false // is this a signal? is this an error?

### Consistent Line Breaks

When breaking up a long function definition, call, or struct initialization, choose to break after each parameter,
When breaking up a (1) long function definition, (2) call, or (3) struct initialization, choose to break after each parameter,
argument, or field.

DO:

```go
// 1
func longFunctionDefinition(
paramX int,
paramY string,
paramZ bool,
) (string, error){}

// and
// 2
callWithManyArguments(
arg1,
arg2,
arg3,
)

// 3
s := myStruct{
field1: 1,
field2: 2,
Expand All @@ -69,6 +76,7 @@ s := myStruct{
DO NOT:

```go
// 1
func longFunctionDefinition(paramX int, paramY string,
paramZ bool,
) (string, error){}
Expand All @@ -80,12 +88,25 @@ func longFunctionDefinition(
paramZ bool,
) (string, error){}

// or
// 2
callWithManyArguments(arg1, arg2,
arg3)

// 3
s := myStruct{field1: 1, field2: 2,
field3: 3}

```

> **Exception**: Calls to ginkgo helper functions like below in tests are OK (because of the readability):
>
>```go
>DescribeTable("Edge cases for events",
> func(e interface{}) {
> . . .
>)
>```

When constructing structs pass members during initialization.

Example:
Expand Down
8 changes: 6 additions & 2 deletions internal/framework/status/updater.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,9 @@ func (upd *updaterImpl) update(
err := upd.cfg.Client.Get(ctx, nsname, obj)
if err != nil {
if !apierrors.IsNotFound(err) {
upd.cfg.Logger.Error(err, "Failed to get the recent version the resource when updating status",
upd.cfg.Logger.Error(
err,
"Failed to get the recent version the resource when updating status",
"namespace", nsname.Namespace,
"name", nsname.Name,
"kind", obj.GetObjectKind().GroupVersionKind().Kind)
Expand All @@ -145,7 +147,9 @@ func (upd *updaterImpl) update(

err = upd.cfg.Client.Status().Update(ctx, obj)
if err != nil {
upd.cfg.Logger.Error(err, "Failed to update status",
upd.cfg.Logger.Error(
err,
"Failed to update status",
"namespace", nsname.Namespace,
"name", nsname.Name,
"kind", obj.GetObjectKind().GroupVersionKind().Kind)
Expand Down
6 changes: 4 additions & 2 deletions internal/mode/provisioner/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,8 @@ func (h *eventHandler) ensureDeploymentsMatchGateways(ctx context.Context) {

h.provisions[nsname] = deployment

h.logger.Info("Created deployment",
h.logger.Info(
"Created deployment",
"deployment", client.ObjectKeyFromObject(deployment),
"gateway", nsname,
)
Expand All @@ -135,7 +136,8 @@ func (h *eventHandler) ensureDeploymentsMatchGateways(ctx context.Context) {

delete(h.provisions, nsname)

h.logger.Info("Deleted deployment",
h.logger.Info(
"Deleted deployment",
"deployment", client.ObjectKeyFromObject(deployment),
"gateway", nsname,
)
Expand Down
7 changes: 5 additions & 2 deletions internal/mode/static/nginx/config/validation/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,11 @@ var escapedStringsNoVarExpansionFmtRegexp = regexp.MustCompile("^" + escapedStri
// If the value is invalid, the function returns an error that includes the specified examples of valid values.
func validateEscapedStringNoVarExpansion(value string, examples []string) error {
if !escapedStringsNoVarExpansionFmtRegexp.MatchString(value) {
msg := k8svalidation.RegexError(escapedStringsNoVarExpansionErrMsg, escapedStringsNoVarExpansionFmt,
examples...)
msg := k8svalidation.RegexError(
escapedStringsNoVarExpansionErrMsg,
escapedStringsNoVarExpansionFmt,
examples...,
)
return errors.New(msg)
}
return nil
Expand Down
42 changes: 30 additions & 12 deletions internal/mode/static/nginx/config/validation/common_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,39 +8,56 @@ import (
func TestValidateEscapedString(t *testing.T) {
validator := func(value string) error { return validateEscapedString(value, []string{"example"}) }

testValidValuesForSimpleValidator(t, validator,
testValidValuesForSimpleValidator(
t,
validator,
`test`,
`test test`,
`\"`,
`\\`)
testInvalidValuesForSimpleValidator(t, validator,
`\\`,
)
testInvalidValuesForSimpleValidator(
t,
validator,
`\`,
`test"test`)
`test"test`,
)
}

func TestValidateEscapedStringNoVarExpansion(t *testing.T) {
validator := func(value string) error { return validateEscapedStringNoVarExpansion(value, []string{"example"}) }

testValidValuesForSimpleValidator(t, validator,
testValidValuesForSimpleValidator(
t,
validator,
`test`,
`test test`,
`\"`,
`\\`)
testInvalidValuesForSimpleValidator(t, validator,
`\\`,
)
testInvalidValuesForSimpleValidator(
t,
validator,
`\`,
`test"test`,
`$test`)
`$test`,
)
}

func TestValidateValidHeaderName(t *testing.T) {
validator := func(value string) error { return validateHeaderName(value) }

testValidValuesForSimpleValidator(t, validator,
testValidValuesForSimpleValidator(
t,
validator,
`Content-Encoding`,
`X-Forwarded-For`,
// max supported length is 256, generate string with 16*16 chars (256)
strings.Repeat("very-long-header", 16))
testInvalidValuesForSimpleValidator(t, validator,
strings.Repeat("very-long-header", 16),
)
testInvalidValuesForSimpleValidator(
t,
validator,
`\`,
`test test`,
`test"test`,
Expand All @@ -49,5 +66,6 @@ func TestValidateValidHeaderName(t *testing.T) {
"host",
"my-header[]",
"my-header&",
strings.Repeat("very-long-header", 16)+"1")
strings.Repeat("very-long-header", 16)+"1",
)
}
45 changes: 31 additions & 14 deletions internal/mode/static/nginx/config/validation/framework_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,16 @@ func testValidValuesForSupportedValuesValidator[T configValue](
f supportedValuesValidatorFunc[T],
values ...T,
) {
runValidatorTests(t, func(g *WithT, v T) {
valid, supportedValues := f(v)
g.Expect(valid).To(BeTrue(), createFailureMessage(v))
g.Expect(supportedValues).To(BeNil(), createFailureMessage(v))
}, "valid_value", values...)
runValidatorTests(
t,
func(g *WithT, v T) {
valid, supportedValues := f(v)
g.Expect(valid).To(BeTrue(), createFailureMessage(v))
g.Expect(supportedValues).To(BeNil(), createFailureMessage(v))
},
"valid_value",
values...,
)
}

func testInvalidValuesForSupportedValuesValidator[T configValue](
Expand All @@ -56,11 +61,16 @@ func testInvalidValuesForSupportedValuesValidator[T configValue](
supportedValuesMap map[T]struct{},
values ...T,
) {
runValidatorTests(t, func(g *WithT, v T) {
valid, supportedValues := f(v)
g.Expect(valid).To(BeFalse(), createFailureMessage(v))
g.Expect(supportedValues).To(Equal(getSortedKeysAsString(supportedValuesMap)), createFailureMessage(v))
}, "invalid_value", values...)
runValidatorTests(
t,
func(g *WithT, v T) {
valid, supportedValues := f(v)
g.Expect(valid).To(BeFalse(), createFailureMessage(v))
g.Expect(supportedValues).To(Equal(getSortedKeysAsString(supportedValuesMap)), createFailureMessage(v))
},
"invalid_value",
values...,
)
}

func TestValidateInSupportedValues(t *testing.T) {
Expand All @@ -74,12 +84,19 @@ func TestValidateInSupportedValues(t *testing.T) {
return validateInSupportedValues(value, supportedValues)
}

testValidValuesForSupportedValuesValidator(t, validator,
testValidValuesForSupportedValuesValidator(
t,
validator,
"value1",
"value2",
"value3")
testInvalidValuesForSupportedValuesValidator(t, validator, supportedValues,
"value4")
"value3",
)
testInvalidValuesForSupportedValuesValidator(
t,
validator,
supportedValues,
"value4",
)
}

func TestGetSortedKeysAsString(t *testing.T) {
Expand Down
Loading
Loading