lostluck commented on code in PR #28893: URL: https://github.com/apache/beam/pull/28893#discussion_r1355786937
########## .test-infra/mock-apis/src/main/go/cmd/service/refresher/main.go: ########## @@ -0,0 +1,120 @@ +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to You under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// refresher is an executable that runs the cache.Refresher service. +package main + +import ( + "context" + "fmt" + "github.com/apache/beam/test-infra/mock-apis/src/main/go/internal/cache" + "github.com/apache/beam/test-infra/mock-apis/src/main/go/internal/common" + "github.com/apache/beam/test-infra/mock-apis/src/main/go/internal/environment" + "github.com/apache/beam/test-infra/mock-apis/src/main/go/internal/logging" + "github.com/redis/go-redis/v9" + "os" + "os/signal" + "time" +) + +var ( + logger = logging.New("github.com/apache/beam/test-infra/mock-apis/src/main/go/cmd/refresher") + + loggerFields []logging.Field + + env = []environment.Variable{ + common.CacheHost, + common.QuotaId, + common.QuotaRefreshInterval, + common.QuotaSize, + } + + interval time.Duration + size uint64 + ref *cache.Refresher +) + +func init() { + for _, v := range env { + loggerFields = append(loggerFields, logging.Field{ + Key: v.Key(), + Value: v.Value(), + }) + } +} + +func main() { + ctx := context.Background() + if err := run(ctx); err != nil { + err = fmt.Errorf("fatal: run() err = %w", err) + logger.Fatal(ctx, err, loggerFields...) + } +} + +func run(ctx context.Context) error { + ctx, cancel := signal.NotifyContext(ctx, os.Interrupt) + defer cancel() + if err := setup(ctx); err != nil { + return err + } + + errChan := make(chan error) + go func() { + if err := ref.Refresh(ctx, common.QuotaId.Value(), size, interval); err != nil { + logger.Error(ctx, err, loggerFields...) + errChan <- err + } + }() + + for { Review Comment: Same comment here. No need for the `for` ########## .test-infra/mock-apis/src/main/go/cmd/service/echo/main.go: ########## @@ -0,0 +1,148 @@ +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to You under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// echo is an executable that runs the echov1.EchoService. +package main + +import ( + "context" + "fmt" + "net" + "net/http" + "os" + "os/signal" + + "github.com/apache/beam/test-infra/mock-apis/src/main/go/internal/cache" + "github.com/apache/beam/test-infra/mock-apis/src/main/go/internal/common" + "github.com/apache/beam/test-infra/mock-apis/src/main/go/internal/environment" + "github.com/apache/beam/test-infra/mock-apis/src/main/go/internal/logging" + "github.com/apache/beam/test-infra/mock-apis/src/main/go/internal/service/echo" + "github.com/redis/go-redis/v9" + "google.golang.org/grpc" +) + +var ( + logger = logging.New("github.com/apache/beam/test-infra/mock-apis/src/main/go/cmd/echo") + env = []environment.Variable{ + common.CacheHost, + common.GrpcPort, + common.HttpPort, + } + loggingFields []logging.Field + decrementer cache.Decrementer + grpcAddress string + + httpAddress string +) + +func init() { + ctx := context.Background() + for _, v := range env { + loggingFields = append(loggingFields, logging.Field{ + Key: v.Key(), + Value: v.Value(), + }) + } + if err := initE(ctx); err != nil { + logger.Fatal(ctx, err, loggingFields...) + } Review Comment: Given this is in the main program, it doesn't technically need to be in an init block, they could just be at the top of Main instead. ########## .test-infra/mock-apis/src/main/go/internal/service/echo/echo.go: ########## @@ -0,0 +1,224 @@ +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to You under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package echo Review Comment: Missing package doc. ########## .test-infra/mock-apis/src/main/go/internal/service/echo/echo.go: ########## @@ -0,0 +1,224 @@ +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to You under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package echo + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "path" + "time" + + "github.com/apache/beam/test-infra/mock-apis/src/main/go/internal/cache" + "github.com/apache/beam/test-infra/mock-apis/src/main/go/internal/logging" + "github.com/apache/beam/test-infra/mock-apis/src/main/go/internal/metric" + echov1 "github.com/apache/beam/test-infra/mock-apis/src/main/go/internal/proto/echo/v1" + "google.golang.org/grpc" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/health/grpc_health_v1" + "google.golang.org/grpc/status" +) + +const ( + metricsNamePrefix = "echo" + echoPath = "/proto.echo.v1.EchoService/Echo" + echoPathAlias = "/v1/echo" + healthPath = "/grpc.health.v1.Health/Check" + healthPathAlias = "/v1/healthz" +) + +var ( + defaultLogger = logging.New("github.com/apache/beam/test-infra/mock-apis/src/main/go/internal/service/echo") +) + +// Option applies optional parameters to the echo service. +type Option interface { + apply(srv *echo) +} + +// WithLogger overrides the default logging.Logger. +func WithLogger(logger logging.Logger) Option { + return &loggerOpt{ + v: logger, + } +} + +// WithLoggerFields WitLoggerFields supplies the echo service's logging.Logger with logging.Field slice. +func WithLoggerFields(fields ...logging.Field) Option { + return &loggerFieldsOpts{ + v: fields, + } +} + +// WithMetricWriter supplies the echo service with a metric.Writer. +func WithMetricWriter(writer metric.Writer) Option { + return &metricWriterOpt{ + v: writer, + } +} + +// Register a grpc.Server with the echov1.EchoService. Returns a http.Handler or error. +func Register(s *grpc.Server, decrementer cache.Decrementer, opts ...Option) (http.Handler, error) { + srv := &echo{ + decrementer: decrementer, + logger: defaultLogger, + } + for _, opt := range opts { + opt.apply(srv) + } + + echov1.RegisterEchoServiceServer(s, srv) + grpc_health_v1.RegisterHealthServer(s, srv) + + return srv, nil +} + +type echo struct { + echov1.UnimplementedEchoServiceServer + grpc_health_v1.UnimplementedHealthServer + decrementer cache.Decrementer + logger logging.Logger + loggerFields []logging.Field + metricsWriter metric.Writer +} + +// ServeHTTP implements http.Handler, allowing echo to support HTTP clients in addition to gRPC. +func (srv *echo) ServeHTTP(w http.ResponseWriter, r *http.Request) { + if f, ok := map[string]http.HandlerFunc{ + echoPath: srv.httpHandler, + echoPathAlias: srv.httpHandler, + healthPath: srv.checkHandler, + healthPathAlias: srv.checkHandler, + }[r.URL.Path]; ok { + f(w, r) + return + } + http.Error(w, fmt.Sprintf("%s not found", r.URL.Path), http.StatusNotFound) Review Comment: ```suggestion switch r.URL.Path { case echoPath, echoPathAlias: srv.httpHandler(w, r) case healthPath, healthPathAlias: srv.checkHandler(w,r) default: http.Error(w, fmt.Sprintf("%s not found", r.URL.Path), http.StatusNotFound) } ``` Using a switch statement clarifies that some of the cases are identical, and avoids the "indent error path" reversal. ########## .test-infra/mock-apis/src/main/go/internal/common/environment.go: ########## @@ -0,0 +1,39 @@ +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to You under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Package common exposes shared variables. +package common Review Comment: A couple of comments: 1. "common" is a bad package name. See https://google.github.io/styleguide/go/decisions#package-names and https://google.github.io/styleguide/go/best-practices#util-packages 2. All of these are Environment variables... so I propose we put them instead on the environment package, in a file named "vars.go" instead. Or just in the "variable.go" file. 3. Because of 2, using this doesn't reduce what's compiled in: The environment package has to be imported no matter what for the build. Thus it's not clear what benefit this separation provides. ########## .test-infra/mock-apis/src/main/go/internal/environment/variable_test.go: ########## @@ -0,0 +1,312 @@ +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to You under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package environment + +import ( + "errors" + "os" + "testing" + + "github.com/google/go-cmp/cmp" +) + +func TestMissing(t *testing.T) { + type args struct { + vars []Variable + values []string + } + tests := []struct { + name string + args args + want error + }{ + { + name: "{}", + args: args{}, + }, + { + name: "{A=}", + args: args{ + vars: []Variable{ + "A", + }, + values: []string{ + "", + }, + }, + want: errors.New("variables empty but expected from environment: A="), + }, + { + name: "{A=1}", + args: args{ + vars: []Variable{ + "A", + }, + values: []string{ + "1", + }, + }, + want: nil, + }, + { + name: "{A=; B=}", + args: args{ + vars: []Variable{ + "A", + "B", + }, + values: []string{ + "", + "", + }, + }, + want: errors.New("variables empty but expected from environment: A=; B="), + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var got, want string + clearVars(tt.args.vars...) + set(t, tt.args.vars, tt.args.values) + err := Missing(tt.args.vars...) + if err != nil { + got = err.Error() + } + if tt.want != nil { + want = tt.want.Error() + } + if diff := cmp.Diff(got, want); diff != "" { Review Comment: See https://google.github.io/styleguide/go/decisions#equality-comparison-and-diffs for best practices for cmp.Diff. In particular, reverse the got, and want parameters, and add an indication of how to interpret the test output: (-want, +got). This reads cleanly as "missing from the wanted result, and extra in the given result", vs (-got,+want)'s "missing in the given result, and extra in the wanted result". ########## .test-infra/mock-apis/src/main/go/cmd/service/echo/main.go: ########## @@ -0,0 +1,148 @@ +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to You under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// echo is an executable that runs the echov1.EchoService. +package main + +import ( + "context" + "fmt" + "net" + "net/http" + "os" + "os/signal" + + "github.com/apache/beam/test-infra/mock-apis/src/main/go/internal/cache" + "github.com/apache/beam/test-infra/mock-apis/src/main/go/internal/common" + "github.com/apache/beam/test-infra/mock-apis/src/main/go/internal/environment" + "github.com/apache/beam/test-infra/mock-apis/src/main/go/internal/logging" + "github.com/apache/beam/test-infra/mock-apis/src/main/go/internal/service/echo" + "github.com/redis/go-redis/v9" + "google.golang.org/grpc" +) + +var ( + logger = logging.New("github.com/apache/beam/test-infra/mock-apis/src/main/go/cmd/echo") + env = []environment.Variable{ + common.CacheHost, + common.GrpcPort, + common.HttpPort, + } + loggingFields []logging.Field + decrementer cache.Decrementer + grpcAddress string + + httpAddress string Review Comment: A better style to enable testing/overriding, is to just make these fields in a struct instead, and passing that around down the callstack. Turns something that requires side testing, into a single, independent call stack, rather than potentially conflicting globals. ########## .test-infra/mock-apis/src/main/go/internal/logging/logging.go: ########## @@ -0,0 +1,155 @@ +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to You under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Package logging performs structured output of log entries. Review Comment: Consider using the "log/slog" for a structured logging front end instead of your own library. It's in the Standard Library since Go 1.21 (which this set of code requires anyway) instead of a bespoke front end. You could write a handler yourself https://github.com/golang/example/blob/master/slog-handler-guide/README.md, or use one someone else wrote for the cloud logging portions. https://pkg.go.dev/github.com/PumpkinSeed/slog-cloudlogging#section-readme ########## .test-infra/mock-apis/src/main/go/cmd/service/refresher/main.go: ########## @@ -0,0 +1,120 @@ +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to You under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// refresher is an executable that runs the cache.Refresher service. +package main + +import ( + "context" + "fmt" + "github.com/apache/beam/test-infra/mock-apis/src/main/go/internal/cache" + "github.com/apache/beam/test-infra/mock-apis/src/main/go/internal/common" + "github.com/apache/beam/test-infra/mock-apis/src/main/go/internal/environment" + "github.com/apache/beam/test-infra/mock-apis/src/main/go/internal/logging" + "github.com/redis/go-redis/v9" + "os" + "os/signal" + "time" +) + +var ( + logger = logging.New("github.com/apache/beam/test-infra/mock-apis/src/main/go/cmd/refresher") + + loggerFields []logging.Field + + env = []environment.Variable{ + common.CacheHost, + common.QuotaId, + common.QuotaRefreshInterval, + common.QuotaSize, + } + + interval time.Duration + size uint64 + ref *cache.Refresher +) + +func init() { + for _, v := range env { + loggerFields = append(loggerFields, logging.Field{ + Key: v.Key(), + Value: v.Value(), + }) + } +} + +func main() { + ctx := context.Background() + if err := run(ctx); err != nil { + err = fmt.Errorf("fatal: run() err = %w", err) + logger.Fatal(ctx, err, loggerFields...) + } +} + +func run(ctx context.Context) error { + ctx, cancel := signal.NotifyContext(ctx, os.Interrupt) + defer cancel() + if err := setup(ctx); err != nil { + return err + } + + errChan := make(chan error) + go func() { + if err := ref.Refresh(ctx, common.QuotaId.Value(), size, interval); err != nil { + logger.Error(ctx, err, loggerFields...) + errChan <- err + } + }() + + for { + select { + case err := <-errChan: + return err + case <-ctx.Done(): + return nil + } + } +} + +func setup(ctx context.Context) error { + var err error Review Comment: Thee's no value in this pre-declaration since all the other `err`'s are in their own if block scopes. Just use `:=` instead. ########## .test-infra/mock-apis/src/main/go/cmd/service/echo/main.go: ########## @@ -0,0 +1,148 @@ +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to You under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// echo is an executable that runs the echov1.EchoService. +package main + +import ( + "context" + "fmt" + "net" + "net/http" + "os" + "os/signal" + + "github.com/apache/beam/test-infra/mock-apis/src/main/go/internal/cache" + "github.com/apache/beam/test-infra/mock-apis/src/main/go/internal/common" + "github.com/apache/beam/test-infra/mock-apis/src/main/go/internal/environment" + "github.com/apache/beam/test-infra/mock-apis/src/main/go/internal/logging" + "github.com/apache/beam/test-infra/mock-apis/src/main/go/internal/service/echo" + "github.com/redis/go-redis/v9" + "google.golang.org/grpc" +) + +var ( + logger = logging.New("github.com/apache/beam/test-infra/mock-apis/src/main/go/cmd/echo") + env = []environment.Variable{ + common.CacheHost, + common.GrpcPort, + common.HttpPort, + } + loggingFields []logging.Field + decrementer cache.Decrementer + grpcAddress string + + httpAddress string +) + +func init() { + ctx := context.Background() + for _, v := range env { + loggingFields = append(loggingFields, logging.Field{ + Key: v.Key(), + Value: v.Value(), + }) + } + if err := initE(ctx); err != nil { + logger.Fatal(ctx, err, loggingFields...) + } +} + +func initE(ctx context.Context) error { + if err := environment.Missing(env...); err != nil { + return err + } + + grpcPort, err := common.GrpcPort.Int() + if err != nil { + logger.Fatal(ctx, err, loggingFields...) + return err + } + grpcAddress = fmt.Sprintf(":%v", grpcPort) + + httpPort, err := common.HttpPort.Int() + if err != nil { + logger.Fatal(ctx, err, loggingFields...) + return err + } + httpAddress = fmt.Sprintf(":%v", httpPort) + + r := redis.NewClient(&redis.Options{ + Addr: common.CacheHost.Value(), + }) + rc := (*cache.RedisCache)(r) + decrementer = rc + + return decrementer.Alive(ctx) +} + +func main() { + ctx := context.Background() + if err := run(ctx); err != nil { + logger.Error(ctx, err, loggingFields...) + } +} + +func run(ctx context.Context) error { + ctx, cancel := signal.NotifyContext(ctx, os.Interrupt) + defer cancel() + logger.Info(ctx, "starting service", loggingFields...) + s, handler, err := setup(ctx) + if err != nil { + return err + } + defer s.GracefulStop() + + lis, err := net.Listen("tcp", grpcAddress) + if err != nil { + logger.Error(ctx, err, loggingFields...) + return err + } + + errChan := make(chan error) + go func() { + if err := s.Serve(lis); err != nil { + logger.Error(ctx, err, loggingFields...) + errChan <- err + } + }() + + go func() { + if err := http.ListenAndServe(httpAddress, handler); err != nil { + errChan <- err + } + }() + + for { Review Comment: There's no path here that loops, so we can remove that and just have the select. I think staticcheck would catch this. ########## .test-infra/mock-apis/src/main/go/internal/service/echo/echo.go: ########## @@ -0,0 +1,224 @@ +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to You under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package echo + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "path" + "time" + + "github.com/apache/beam/test-infra/mock-apis/src/main/go/internal/cache" + "github.com/apache/beam/test-infra/mock-apis/src/main/go/internal/logging" + "github.com/apache/beam/test-infra/mock-apis/src/main/go/internal/metric" + echov1 "github.com/apache/beam/test-infra/mock-apis/src/main/go/internal/proto/echo/v1" + "google.golang.org/grpc" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/health/grpc_health_v1" + "google.golang.org/grpc/status" +) + +const ( + metricsNamePrefix = "echo" + echoPath = "/proto.echo.v1.EchoService/Echo" + echoPathAlias = "/v1/echo" + healthPath = "/grpc.health.v1.Health/Check" + healthPathAlias = "/v1/healthz" +) + +var ( + defaultLogger = logging.New("github.com/apache/beam/test-infra/mock-apis/src/main/go/internal/service/echo") +) + +// Option applies optional parameters to the echo service. +type Option interface { + apply(srv *echo) +} + +// WithLogger overrides the default logging.Logger. +func WithLogger(logger logging.Logger) Option { + return &loggerOpt{ + v: logger, + } +} + +// WithLoggerFields WitLoggerFields supplies the echo service's logging.Logger with logging.Field slice. +func WithLoggerFields(fields ...logging.Field) Option { + return &loggerFieldsOpts{ + v: fields, + } +} + +// WithMetricWriter supplies the echo service with a metric.Writer. +func WithMetricWriter(writer metric.Writer) Option { + return &metricWriterOpt{ + v: writer, + } +} + +// Register a grpc.Server with the echov1.EchoService. Returns a http.Handler or error. +func Register(s *grpc.Server, decrementer cache.Decrementer, opts ...Option) (http.Handler, error) { + srv := &echo{ + decrementer: decrementer, + logger: defaultLogger, + } + for _, opt := range opts { + opt.apply(srv) + } + + echov1.RegisterEchoServiceServer(s, srv) + grpc_health_v1.RegisterHealthServer(s, srv) + + return srv, nil +} + +type echo struct { + echov1.UnimplementedEchoServiceServer + grpc_health_v1.UnimplementedHealthServer + decrementer cache.Decrementer + logger logging.Logger + loggerFields []logging.Field + metricsWriter metric.Writer +} + +// ServeHTTP implements http.Handler, allowing echo to support HTTP clients in addition to gRPC. +func (srv *echo) ServeHTTP(w http.ResponseWriter, r *http.Request) { + if f, ok := map[string]http.HandlerFunc{ + echoPath: srv.httpHandler, + echoPathAlias: srv.httpHandler, + healthPath: srv.checkHandler, + healthPathAlias: srv.checkHandler, + }[r.URL.Path]; ok { + f(w, r) + return + } + http.Error(w, fmt.Sprintf("%s not found", r.URL.Path), http.StatusNotFound) Review Comment: No action required, just a note: Typically (but not always) the error flow should be indented. See https://google.github.io/styleguide/go/decisions#indent-error-flow But in this case, they're both so short, and indending the error flow just makes the function slightly longer, so... this is fine. ########## .test-infra/mock-apis/src/main/go/internal/environment/variable_test.go: ########## @@ -0,0 +1,312 @@ +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to You under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package environment + +import ( + "errors" + "os" + "testing" + + "github.com/google/go-cmp/cmp" +) + +func TestMissing(t *testing.T) { + type args struct { + vars []Variable + values []string + } + tests := []struct { + name string + args args + want error + }{ + { + name: "{}", + args: args{}, + }, + { + name: "{A=}", + args: args{ + vars: []Variable{ + "A", + }, + values: []string{ + "", + }, + }, + want: errors.New("variables empty but expected from environment: A="), + }, + { + name: "{A=1}", + args: args{ + vars: []Variable{ + "A", + }, + values: []string{ + "1", + }, + }, + want: nil, + }, + { + name: "{A=; B=}", + args: args{ + vars: []Variable{ + "A", + "B", + }, + values: []string{ + "", + "", + }, + }, + want: errors.New("variables empty but expected from environment: A=; B="), + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var got, want string + clearVars(tt.args.vars...) + set(t, tt.args.vars, tt.args.values) + err := Missing(tt.args.vars...) + if err != nil { + got = err.Error() + } + if tt.want != nil { + want = tt.want.Error() + } + if diff := cmp.Diff(got, want); diff != "" { Review Comment: This applies whereever cmp.Diff is used. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
