From e59b2784f1ed1373c3b15375344f2bbe5a50a9e0 Mon Sep 17 00:00:00 2001 From: frrist Date: Fri, 26 Jan 2024 12:46:17 -0800 Subject: [PATCH] refactor: Make base URL a required parameter in client constructor - Enforce base URL as mandatory in constructor; removes the illusion of it being an 'optional' setting when it's fundamentally required for operation --- cmd/cli/serve/serve_test.go | 4 +--- cmd/cli/serve/timeout_test.go | 4 +--- cmd/testing/base.go | 4 +--- cmd/util/api.go | 2 +- ops/aws/canary/lambda/pkg/scenarios/utils.go | 4 +--- pkg/publicapi/client/v2/client.go | 13 +++++++++++-- pkg/publicapi/client/v2/options.go | 10 ---------- pkg/publicapi/test/setup_test.go | 8 ++------ pkg/publicapi/test/util_test.go | 4 +--- pkg/test/scenario/suite.go | 4 +--- 10 files changed, 20 insertions(+), 37 deletions(-) diff --git a/cmd/cli/serve/serve_test.go b/cmd/cli/serve/serve_test.go index c3d0349294..6b3ccb8977 100644 --- a/cmd/cli/serve/serve_test.go +++ b/cmd/cli/serve/serve_test.go @@ -189,9 +189,7 @@ func (s *ServeSuite) TestCanSubmitJob() { docker.MustHaveDocker(s.T()) port, _ := s.serve("--node-type", "requester", "--node-type", "compute") client := client.NewAPIClient(client.NoTLS, "localhost", port) - clientV2 := clientv2.New(clientv2.Config{ - Address: fmt.Sprintf("http://127.0.0.1:%d", port), - }) + clientV2 := clientv2.New(fmt.Sprintf("http://127.0.0.1:%d", port)) s.Require().NoError(apitest.WaitForAlive(s.ctx, clientV2)) job, err := model.NewJobWithSaneProductionDefaults() diff --git a/cmd/cli/serve/timeout_test.go b/cmd/cli/serve/timeout_test.go index 182b7c6307..852710ff4d 100644 --- a/cmd/cli/serve/timeout_test.go +++ b/cmd/cli/serve/timeout_test.go @@ -55,9 +55,7 @@ func (s *ServeSuite) TestNoTimeoutSetOrApplied() { s.Require().NoError(err) client := client.NewAPIClient(client.NoTLS, "localhost", port) - clientV2 := clientv2.New(clientv2.Config{ - Address: fmt.Sprintf("http://127.0.0.1:%d", port), - }) + clientV2 := clientv2.New(fmt.Sprintf("http://127.0.0.1:%d", port)) s.Require().NoError(apitest.WaitForAlive(s.ctx, clientV2)) testJob := model.NewJob() diff --git a/cmd/testing/base.go b/cmd/testing/base.go index e049b46842..81f3134f2a 100644 --- a/cmd/testing/base.go +++ b/cmd/testing/base.go @@ -62,9 +62,7 @@ func (s *BaseSuite) SetupTest() { s.Host = s.Node.APIServer.Address s.Port = s.Node.APIServer.Port s.Client = client.NewAPIClient(client.NoTLS, s.Host, s.Port) - s.ClientV2 = clientv2.New(clientv2.Config{ - Address: fmt.Sprintf("http://%s:%d", s.Host, s.Port), - }) + s.ClientV2 = clientv2.New(fmt.Sprintf("http://%s:%d", s.Host, s.Port)) } // After each test diff --git a/cmd/util/api.go b/cmd/util/api.go index 42acd8970b..fee98dfd11 100644 --- a/cmd/util/api.go +++ b/cmd/util/api.go @@ -49,5 +49,5 @@ func GetAPIClientV2() *clientv2.Client { })) } - return clientv2.New(clientv2.Config{Address: base}, opts...) + return clientv2.New(base, opts...) } diff --git a/ops/aws/canary/lambda/pkg/scenarios/utils.go b/ops/aws/canary/lambda/pkg/scenarios/utils.go index b5723590cb..cc63afd84e 100644 --- a/ops/aws/canary/lambda/pkg/scenarios/utils.go +++ b/ops/aws/canary/lambda/pkg/scenarios/utils.go @@ -149,9 +149,7 @@ func getClient() *client.APIClient { func getClientV2() *clientv2.Client { host, port := getClientHostAndPort() - return clientv2.New(clientv2.Config{ - Address: fmt.Sprintf("http://%s:%d", host, port), - }) + return clientv2.New(fmt.Sprintf("http://%s:%d", host, port)) } func getNodeSelectors() ([]model.LabelSelectorRequirement, error) { diff --git a/pkg/publicapi/client/v2/client.go b/pkg/publicapi/client/v2/client.go index 222213280b..3519032d15 100644 --- a/pkg/publicapi/client/v2/client.go +++ b/pkg/publicapi/client/v2/client.go @@ -10,18 +10,24 @@ import ( ) type Client struct { + address string + httpClient *http.Client config Config } // New creates a new client. -func New(cfg Config, optFns ...OptionFn) *Client { +func New(address string, optFns ...OptionFn) *Client { + // define default filed on the config by setting them here, then + // modify with options to override. + var cfg Config for _, optFn := range optFns { optFn(&cfg) } resolveHTTPClient(&cfg) return &Client{ + address: address, httpClient: cfg.HTTPClient, config: cfg, } @@ -178,7 +184,10 @@ func (c *Client) toHTTP(ctx context.Context, method, endpoint string, r *apimode // generate URL for a given endpoint func (c *Client) url(endpoint string) (*url.URL, error) { - base, _ := url.Parse(c.config.Address) + base, err := url.Parse(c.address) + if err != nil { + return nil, err + } u, err := url.Parse(endpoint) if err != nil { return nil, err diff --git a/pkg/publicapi/client/v2/options.go b/pkg/publicapi/client/v2/options.go index bfe41fa239..29224de425 100644 --- a/pkg/publicapi/client/v2/options.go +++ b/pkg/publicapi/client/v2/options.go @@ -18,9 +18,6 @@ import ( ) type Config struct { - // Address is the address of the node's public REST API. - Address string - // Namespace is the default namespace to use for all requests. Namespace string @@ -80,13 +77,6 @@ func WithInsecureTLS(insecure bool) OptionFn { } } -// WithAddress sets the address of the node's public REST API. -func WithAddress(address string) OptionFn { - return func(o *Config) { - o.Address = address - } -} - // WithNamespace sets the default namespace to use for all requests. func WithNamespace(namespace string) OptionFn { return func(o *Config) { diff --git a/pkg/publicapi/test/setup_test.go b/pkg/publicapi/test/setup_test.go index 0c5248f7ae..2e6c3ae229 100644 --- a/pkg/publicapi/test/setup_test.go +++ b/pkg/publicapi/test/setup_test.go @@ -40,14 +40,10 @@ func (s *ServerSuite) SetupSuite() { ) s.requesterNode = stack.Nodes[0] - s.client = client.New(client.Config{ - Address: s.requesterNode.APIServer.GetURI().String(), - }) + s.client = client.New(s.requesterNode.APIServer.GetURI().String()) s.computeNode = stack.Nodes[1] - s.computeClient = client.New(client.Config{ - Address: s.computeNode.APIServer.GetURI().String(), - }) + s.computeClient = client.New(s.computeNode.APIServer.GetURI().String()) s.Require().NoError(WaitForAlive(ctx, s.client)) s.Require().NoError(WaitForAlive(ctx, s.computeClient)) diff --git a/pkg/publicapi/test/util_test.go b/pkg/publicapi/test/util_test.go index ca4824be60..035f8b5563 100644 --- a/pkg/publicapi/test/util_test.go +++ b/pkg/publicapi/test/util_test.go @@ -87,9 +87,7 @@ func setupNodeForTestWithConfig(t *testing.T, apiCfg publicapi.Config) (*node.No err = n.Start(ctx) require.NoError(t, err) - apiClient := client.New(client.Config{ - Address: n.APIServer.GetURI().String(), - }) + apiClient := client.New(n.APIServer.GetURI().String()) require.NoError(t, WaitForNodes(ctx, apiClient)) return n, apiClient } diff --git a/pkg/test/scenario/suite.go b/pkg/test/scenario/suite.go index 8e4112aeea..637b896742 100644 --- a/pkg/test/scenario/suite.go +++ b/pkg/test/scenario/suite.go @@ -152,9 +152,7 @@ func (s *ScenarioRunner) RunScenario(scenario Scenario) string { apiServer := stack.Nodes[0].APIServer apiClient := client.NewAPIClient(client.NoTLS, apiServer.Address, apiServer.Port) - apiClientV2 := clientv2.New(clientv2.Config{ - Address: fmt.Sprintf("http://%s:%d", apiServer.Address, apiServer.Port), - }) + apiClientV2 := clientv2.New(fmt.Sprintf("http://%s:%d", apiServer.Address, apiServer.Port)) submittedJob, submitError := apiClient.Submit(s.Ctx, j) if scenario.SubmitChecker == nil {