Copilot commented on code in PR #224:
URL: https://github.com/apache/skywalking-cli/pull/224#discussion_r2121471228
##########
pkg/graphql/client/client.go:
##########
@@ -29,18 +29,37 @@ import (
"github.com/apache/skywalking-cli/pkg/logger"
)
+// newClient creates a new GraphQL client with configuration from context.
+//
+// CLI has its own defaults, but for calls from other servers,
+// we also add validation and certain default values here.
func newClient(ctx context.Context) *graphql.Client {
options := []graphql.ClientOption{}
- insecure := ctx.Value(contextkey.Insecure{}).(bool)
+ var insecure bool
Review Comment:
[nitpick] Consider extracting repeated ctx.Value type assertions into a
helper function (e.g. `getStringValue(ctx, key)`) to reduce duplication and
improve readability.
##########
pkg/graphql/client/client.go:
##########
@@ -29,18 +29,37 @@ import (
"github.com/apache/skywalking-cli/pkg/logger"
)
+// newClient creates a new GraphQL client with configuration from context.
+//
+// CLI has its own defaults, but for calls from other servers,
+// we also add validation and certain default values here.
func newClient(ctx context.Context) *graphql.Client {
options := []graphql.ClientOption{}
- insecure := ctx.Value(contextkey.Insecure{}).(bool)
+ var insecure bool
+ if val := ctx.Value(contextkey.Insecure{}); val != nil {
+ if flag, ok := val.(bool); ok {
+ insecure = flag
+ }
+ }
if insecure {
customTransport :=
http.DefaultTransport.(*http.Transport).Clone()
customTransport.TLSClientConfig =
&tls.Config{InsecureSkipVerify: insecure} // #nosec G402
httpClient := &http.Client{Transport: customTransport}
options = append(options, graphql.WithHTTPClient(httpClient))
}
- client := graphql.NewClient(ctx.Value(contextkey.BaseURL{}).(string),
options...)
+ var baseURL string
+ if val := ctx.Value(contextkey.BaseURL{}); val != nil {
+ if str, ok := val.(string); ok {
+ baseURL = str
+ }
+ }
+ if baseURL == "" {
+ baseURL = "http://127.0.0.1:12800/graphql";
Review Comment:
[nitpick] Extract the default baseURL literal into a named constant or
configuration variable to avoid a magic string and improve maintainability.
##########
pkg/graphql/client/client.go:
##########
@@ -29,18 +29,37 @@ import (
"github.com/apache/skywalking-cli/pkg/logger"
)
+// newClient creates a new GraphQL client with configuration from context.
+//
+// CLI has its own defaults, but for calls from other servers,
+// we also add validation and certain default values here.
func newClient(ctx context.Context) *graphql.Client {
options := []graphql.ClientOption{}
- insecure := ctx.Value(contextkey.Insecure{}).(bool)
+ var insecure bool
+ if val := ctx.Value(contextkey.Insecure{}); val != nil {
+ if flag, ok := val.(bool); ok {
+ insecure = flag
Review Comment:
[nitpick] Consider logging or otherwise handling the case where a context
value exists but fails the type assertion, to help diagnose misconfigurations.
```suggestion
insecure = flag
} else {
logger.Log.Warnf("Context value for 'Insecure' is not
of type bool: %v", val)
```
--
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: notifications-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
