From fde1bce50a3e19e9049a05de45f52a48d27c42e2 Mon Sep 17 00:00:00 2001 From: JulesFaucherre Date: Wed, 19 Jul 2023 11:23:27 +0200 Subject: [PATCH 1/5] refactor: Abstracted the different config validation routes --- api/rest/client.go | 9 ++- cmd/config.go | 12 ++- config/api_client.go | 71 ++++++++++++++++++ config/api_client_test.go | 61 +++++++++++++++ config/commands_test.go | 40 +++++++--- config/config.go | 74 ++++--------------- config/config_test.go | 56 +++++++------- config/{legacy.go => v1_api_client.go} | 40 +++++----- .../{legacy_test.go => v1_api_client_test.go} | 43 ++++++++--- config/v2_api_client.go | 62 ++++++++++++++++ config/v2_api_client_test.go | 17 +++++ local/local.go | 5 +- 12 files changed, 359 insertions(+), 131 deletions(-) create mode 100644 config/api_client.go create mode 100644 config/api_client_test.go rename config/{legacy.go => v1_api_client.go} (88%) rename config/{legacy_test.go => v1_api_client_test.go} (74%) create mode 100644 config/v2_api_client.go create mode 100644 config/v2_api_client_test.go diff --git a/api/rest/client.go b/api/rest/client.go index 2d1628f41..8204237fe 100644 --- a/api/rest/client.go +++ b/api/rest/client.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "io" + "io/ioutil" "net/http" "net/url" "strings" @@ -84,7 +85,7 @@ func (c *Client) enrichRequestHeaders(req *http.Request, payload interface{}) { } } -func (c *Client) DoRequest(req *http.Request, resp interface{}) (statusCode int, err error) { +func (c *Client) DoRequest(req *http.Request, resp interface{}) (int, error) { httpResp, err := c.client.Do(req) if err != nil { fmt.Printf("failed to make http request: %s\n", err.Error()) @@ -96,10 +97,14 @@ func (c *Client) DoRequest(req *http.Request, resp interface{}) (statusCode int, httpError := struct { Message string `json:"message"` }{} - err = json.NewDecoder(httpResp.Body).Decode(&httpError) + body, err := ioutil.ReadAll(httpResp.Body) if err != nil { return httpResp.StatusCode, err } + err = json.Unmarshal(body, &httpError) + if err != nil { + return httpResp.StatusCode, &HTTPError{Code: httpResp.StatusCode, Message: string(body)} + } return httpResp.StatusCode, &HTTPError{Code: httpResp.StatusCode, Message: httpError.Message} } diff --git a/cmd/config.go b/cmd/config.go index 6c85de763..690fc78d1 100644 --- a/cmd/config.go +++ b/cmd/config.go @@ -50,7 +50,11 @@ func newConfigCommand(globalConfig *settings.Config) *cobra.Command { Aliases: []string{"check"}, Short: "Check that the config file is well formed.", RunE: func(cmd *cobra.Command, args []string) error { - compiler := config.New(globalConfig) + compiler, err := config.NewWithConfig(globalConfig) + if err != nil { + return err + } + orgID, _ := cmd.Flags().GetString("org-id") orgSlug, _ := cmd.Flags().GetString("org-slug") path := config.DefaultConfigPath @@ -86,7 +90,11 @@ func newConfigCommand(globalConfig *settings.Config) *cobra.Command { Use: "process ", Short: "Validate config and display expanded configuration.", RunE: func(cmd *cobra.Command, args []string) error { - compiler := config.New(globalConfig) + compiler, err := config.NewWithConfig(globalConfig) + if err != nil { + return err + } + pipelineParamsFilePath, _ := cmd.Flags().GetString("pipeline-parameters") orgID, _ := cmd.Flags().GetString("org-id") orgSlug, _ := cmd.Flags().GetString("org-slug") diff --git a/config/api_client.go b/config/api_client.go new file mode 100644 index 000000000..1d3493d05 --- /dev/null +++ b/config/api_client.go @@ -0,0 +1,71 @@ +package config + +import ( + "fmt" + "net/url" + "sync" + + "github.com/CircleCI-Public/circleci-cli/api/graphql" + "github.com/CircleCI-Public/circleci-cli/api/rest" + "github.com/CircleCI-Public/circleci-cli/settings" +) + +var ( + compiler APIClient + compilerError error + once sync.Once + + compilePath = "compile-config-with-defaults" +) + +type apiClientVersion string + +type APIClient interface { + CompileConfig(configContent string, orgID string, params Parameters, values Values) (*ConfigResponse, error) +} + +func GetAPIClient(config *settings.Config) (APIClient, error) { + if compiler == nil { + once.Do(func() { + compiler, compilerError = newAPIClient(config) + }) + } + return compiler, compilerError +} + +func newAPIClient(config *settings.Config) (APIClient, error) { + hostValue := getCompileHost(config.Host) + restClient := rest.NewFromConfig(hostValue, config) + + version, err := detectAPIClientVersion(restClient) + if err != nil { + return nil, err + } + + switch version { + case v1_string: + return &v1APIClient{graphql.NewClient(config.HTTPClient, config.Host, config.Endpoint, config.Token, config.Debug)}, nil + case v2_string: + return &v2APIClient{restClient}, nil + default: + return nil, fmt.Errorf("Unable to recognise your Server's config file API") + } +} + +func detectAPIClientVersion(restClient *rest.Client) (apiClientVersion, error) { + req, err := restClient.NewRequest("POST", &url.URL{Path: compilePath}, nil) + if err != nil { + return "", err + } + + statusCode, err := restClient.DoRequest(req, nil) + if err != nil { + if _, ok := err.(*rest.HTTPError); ok { + return "", err + } + } + if statusCode == 404 { + return v1_string, nil + } + return v2_string, nil +} diff --git a/config/api_client_test.go b/config/api_client_test.go new file mode 100644 index 000000000..7011afafc --- /dev/null +++ b/config/api_client_test.go @@ -0,0 +1,61 @@ +package config + +import ( + "fmt" + "net/http" + "net/http/httptest" + "net/url" + "testing" + + "github.com/CircleCI-Public/circleci-cli/api/rest" + "gotest.tools/v3/assert" +) + +func TestAPIClient(t *testing.T) { + t.Run("detectCompilerVersion", func(t *testing.T) { + t.Run("when the route returns a 400 tells that the version is v2", func(t *testing.T) { + svr := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(400) + w.Header().Set("Content-Type", "application/octet-stream") + fmt.Fprintf(w, "Invalid input") + })) + url, err := url.Parse(svr.URL) + assert.NilError(t, err) + + restClient := rest.New(url, "token", http.DefaultClient) + version, err := detectAPIClientVersion(restClient) + assert.NilError(t, err) + assert.Equal(t, version, v2_string) + }) + + t.Run("when the route returns a 404 tells that the version is v1", func(t *testing.T) { + svr := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(404) + w.Header().Set("Content-Type", "application/octet-stream") + fmt.Fprintf(w, "Invalid input") + })) + url, err := url.Parse(svr.URL) + assert.NilError(t, err) + + restClient := rest.New(url, "token", http.DefaultClient) + version, err := detectAPIClientVersion(restClient) + assert.NilError(t, err) + assert.Equal(t, version, v1_string) + }) + + t.Run("in any other case, return an error", func(t *testing.T) { + svr := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(500) + w.Header().Set("Content-Type", "application/octet-stream") + fmt.Fprintf(w, "Invalid input") + })) + url, err := url.Parse(svr.URL) + assert.NilError(t, err) + + restClient := rest.New(url, "token", http.DefaultClient) + version, err := detectAPIClientVersion(restClient) + assert.Equal(t, version, apiClientVersion("")) + assert.Error(t, err, "Invalid input") + }) + }) +} diff --git a/config/commands_test.go b/config/commands_test.go index dc6e7a16a..786eafa18 100644 --- a/config/commands_test.go +++ b/config/commands_test.go @@ -8,6 +8,8 @@ import ( "net/http/httptest" "testing" + "github.com/CircleCI-Public/circleci-cli/api/collaborators" + "github.com/CircleCI-Public/circleci-cli/api/rest" "github.com/CircleCI-Public/circleci-cli/settings" "github.com/stretchr/testify/assert" ) @@ -18,7 +20,11 @@ func TestGetOrgID(t *testing.T) { fmt.Fprintf(w, `[{"vcs_type":"circleci","slug":"gh/test","id":"2345"}]`) })) defer svr.Close() - compiler := New(&settings.Config{Host: svr.URL, HTTPClient: http.DefaultClient}) + cfg := &settings.Config{Host: svr.URL, HTTPClient: http.DefaultClient} + apiClient := &v2APIClient{rest.NewFromConfig(cfg.Host, cfg)} + collaboratorsClient, err := collaborators.NewCollaboratorsRestClient(*cfg) + assert.NoError(t, err) + compiler := New(apiClient, collaboratorsClient) t.Run("returns the original org-id passed if it is set", func(t *testing.T) { expected := "1234" @@ -66,9 +72,13 @@ func TestValidateConfig(t *testing.T) { fmt.Fprintf(w, `{"valid":true,"source-yaml":"%s","output-yaml":"%s","errors":[]}`, testYaml, testYaml) })) defer svr.Close() - compiler := New(&settings.Config{Host: svr.URL, HTTPClient: http.DefaultClient}) + cfg := &settings.Config{Host: svr.URL, HTTPClient: http.DefaultClient} + apiClient := &v2APIClient{rest.NewFromConfig(cfg.Host, cfg)} + collaboratorsClient, err := collaborators.NewCollaboratorsRestClient(*cfg) + assert.NoError(t, err) + compiler := New(apiClient, collaboratorsClient) - err := compiler.ValidateConfig(ValidateConfigOpts{ + err = compiler.ValidateConfig(ValidateConfigOpts{ ConfigPath: "testdata/config.yml", }) assert.NoError(t, err) @@ -87,9 +97,13 @@ func TestValidateConfig(t *testing.T) { fmt.Fprintf(w, `{"valid":true,"source-yaml":"%s","output-yaml":"%s","errors":[]}`, testYaml, testYaml) })) defer svr.Close() - compiler := New(&settings.Config{Host: svr.URL, HTTPClient: http.DefaultClient}) + cfg := &settings.Config{Host: svr.URL, HTTPClient: http.DefaultClient} + apiClient := &v2APIClient{rest.NewFromConfig(cfg.Host, cfg)} + collaboratorsClient, err := collaborators.NewCollaboratorsRestClient(*cfg) + assert.NoError(t, err) + compiler := New(apiClient, collaboratorsClient) - err := compiler.ValidateConfig(ValidateConfigOpts{ + err = compiler.ValidateConfig(ValidateConfigOpts{ ConfigPath: "testdata/config.yml", OrgID: "1234", }) @@ -119,9 +133,13 @@ func TestValidateConfig(t *testing.T) { svr := httptest.NewServer(mux) defer svr.Close() - compiler := New(&settings.Config{Host: svr.URL, HTTPClient: http.DefaultClient}) + cfg := &settings.Config{Host: svr.URL, HTTPClient: http.DefaultClient} + apiClient := &v2APIClient{rest.NewFromConfig(cfg.Host, cfg)} + collaboratorsClient, err := collaborators.NewCollaboratorsRestClient(*cfg) + assert.NoError(t, err) + compiler := New(apiClient, collaboratorsClient) - err := compiler.ValidateConfig(ValidateConfigOpts{ + err = compiler.ValidateConfig(ValidateConfigOpts{ ConfigPath: "testdata/config.yml", OrgSlug: "gh/test", }) @@ -151,9 +169,13 @@ func TestValidateConfig(t *testing.T) { svr := httptest.NewServer(mux) defer svr.Close() - compiler := New(&settings.Config{Host: svr.URL, HTTPClient: http.DefaultClient}) + cfg := &settings.Config{Host: svr.URL, HTTPClient: http.DefaultClient} + apiClient := &v2APIClient{rest.NewFromConfig(cfg.Host, cfg)} + collaboratorsClient, err := collaborators.NewCollaboratorsRestClient(*cfg) + assert.NoError(t, err) + compiler := New(apiClient, collaboratorsClient) - err := compiler.ValidateConfig(ValidateConfigOpts{ + err = compiler.ValidateConfig(ValidateConfigOpts{ ConfigPath: "testdata/config.yml", OrgSlug: "gh/nonexistent", }) diff --git a/config/config.go b/config/config.go index ba55404de..741384220 100644 --- a/config/config.go +++ b/config/config.go @@ -3,12 +3,9 @@ package config import ( "fmt" "io" - "net/url" "os" "github.com/CircleCI-Public/circleci-cli/api/collaborators" - "github.com/CircleCI-Public/circleci-cli/api/graphql" - "github.com/CircleCI-Public/circleci-cli/api/rest" "github.com/CircleCI-Public/circleci-cli/settings" "github.com/pkg/errors" ) @@ -22,29 +19,28 @@ var ( ) type ConfigCompiler struct { - host string - compileRestClient *rest.Client - collaborators collaborators.CollaboratorsClient - - cfg *settings.Config - legacyGraphQLClient *graphql.Client + apiClient APIClient + collaborators collaborators.CollaboratorsClient } -func New(cfg *settings.Config) *ConfigCompiler { - hostValue := GetCompileHost(cfg.Host) +func NewWithConfig(cfg *settings.Config) (*ConfigCompiler, error) { + apiClient, err := GetAPIClient(cfg) + if err != nil { + return nil, err + } collaboratorsClient, err := collaborators.NewCollaboratorsRestClient(*cfg) if err != nil { - panic(err) + return nil, err } + return New(apiClient, collaboratorsClient), nil +} +func New(apiClient APIClient, collaboratorsClient collaborators.CollaboratorsClient) *ConfigCompiler { configCompiler := &ConfigCompiler{ - host: hostValue, - compileRestClient: rest.NewFromConfig(hostValue, cfg), - collaborators: collaboratorsClient, - cfg: cfg, + apiClient: apiClient, + collaborators: collaboratorsClient, } - configCompiler.legacyGraphQLClient = graphql.NewClient(cfg.HTTPClient, cfg.Host, cfg.Endpoint, cfg.Token, cfg.Debug) return configCompiler } @@ -94,49 +90,7 @@ func (c *ConfigCompiler) ConfigQuery( return nil, fmt.Errorf("failed to load yaml config from config path provider: %w", err) } - compileRequest := CompileConfigRequest{ - ConfigYaml: configString, - Options: Options{ - OwnerID: orgID, - PipelineValues: values, - PipelineParameters: params, - }, - } - - req, err := c.compileRestClient.NewRequest( - "POST", - &url.URL{ - Path: "compile-config-with-defaults", - }, - compileRequest, - ) - if err != nil { - return nil, fmt.Errorf("an error occurred creating the request: %w", err) - } - - configCompilationResp := &ConfigResponse{} - statusCode, originalErr := c.compileRestClient.DoRequest(req, configCompilationResp) - if statusCode == 404 { - fmt.Fprintf(os.Stderr, "You are using a old version of CircleCI Server, please consider updating\n") - legacyResponse, err := c.legacyConfigQueryByOrgID(configString, orgID, params, values, c.cfg) - if err != nil { - return nil, err - } - return legacyResponse, nil - } - if originalErr != nil { - return nil, fmt.Errorf("config compilation request returned an error: %w", originalErr) - } - - if statusCode != 200 { - return nil, errors.New("unable to validate or compile config") - } - - if len(configCompilationResp.Errors) > 0 { - return nil, fmt.Errorf("config compilation contains errors: %s", configCompilationResp.Errors) - } - - return configCompilationResp, nil + return c.apiClient.CompileConfig(configString, orgID, params, values) } func loadYaml(path string) (string, error) { diff --git a/config/config_test.go b/config/config_test.go index 84af6c2cc..21a2b30f9 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -8,37 +8,25 @@ import ( "net/http/httptest" "testing" + "github.com/CircleCI-Public/circleci-cli/api/collaborators" + "github.com/CircleCI-Public/circleci-cli/api/rest" "github.com/CircleCI-Public/circleci-cli/settings" "github.com/stretchr/testify/assert" ) -func TestCompiler(t *testing.T) { - t.Run("test compiler setup", func(t *testing.T) { - svr := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") - fmt.Fprintf(w, `[{"vcs_type":"circleci","slug":"gh/test","id":"2345"}]`) - })) - defer svr.Close() - compiler := New(&settings.Config{Host: svr.URL, HTTPClient: http.DefaultClient}) - - t.Run("assert compiler has correct host", func(t *testing.T) { - assert.Equal(t, "http://"+compiler.compileRestClient.BaseURL.Host, svr.URL) - }) - - t.Run("assert compiler has default api host", func(t *testing.T) { - newCompiler := New(&settings.Config{Host: defaultHost, HTTPClient: http.DefaultClient}) - assert.Equal(t, "https://"+newCompiler.compileRestClient.BaseURL.Host, defaultAPIHost) - }) - - t.Run("tests that we correctly get the config api host when the host is not the default one", func(t *testing.T) { +func TestConfig(t *testing.T) { + t.Run("test getCompileHost", func(t *testing.T) { + t.Run("for Server instances", func(t *testing.T) { // if the host isn't equal to `https://circleci.com` then this is likely a server instance and // wont have the api.X.com subdomain so we should instead just respect the host for config commands host := GetCompileHost("test") assert.Equal(t, host, "test") + }) + t.Run("for CircleCI servers", func(t *testing.T) { // If the host passed in is the same as the defaultHost 'https://circleci.com' - then we know this is cloud // and as such should use the `api.circleci.com` subdomain - host = GetCompileHost("https://circleci.com") + host := GetCompileHost("https://circleci.com") assert.Equal(t, host, "https://api.circleci.com") }) }) @@ -50,7 +38,11 @@ func TestCompiler(t *testing.T) { fmt.Fprintf(w, `{"valid":true,"source-yaml":"source","output-yaml":"output","errors":[]}`) })) defer svr.Close() - compiler := New(&settings.Config{Host: svr.URL, HTTPClient: http.DefaultClient}) + cfg := &settings.Config{Host: svr.URL, HTTPClient: http.DefaultClient} + apiClient := &v2APIClient{rest.NewFromConfig(cfg.Host, cfg)} + collaboratorsClient, err := collaborators.NewCollaboratorsRestClient(*cfg) + assert.NoError(t, err) + compiler := New(apiClient, collaboratorsClient) result, err := compiler.ConfigQuery("testdata/config.yml", "1234", Parameters{}, Values{}) assert.NoError(t, err) @@ -65,9 +57,13 @@ func TestCompiler(t *testing.T) { fmt.Fprintf(w, `{"valid":true,"source-yaml":"source","output-yaml":"output","errors":[]}`) })) defer svr.Close() - compiler := New(&settings.Config{Host: svr.URL, HTTPClient: http.DefaultClient}) + cfg := &settings.Config{Host: svr.URL, HTTPClient: http.DefaultClient} + apiClient := &v2APIClient{rest.NewFromConfig(cfg.Host, cfg)} + collaboratorsClient, err := collaborators.NewCollaboratorsRestClient(*cfg) + assert.NoError(t, err) + compiler := New(apiClient, collaboratorsClient) - _, err := compiler.ConfigQuery("testdata/nonexistent.yml", "1234", Parameters{}, Values{}) + _, err = compiler.ConfigQuery("testdata/nonexistent.yml", "1234", Parameters{}, Values{}) assert.Error(t, err) assert.Contains(t, err.Error(), "Could not load config file at testdata/nonexistent.yml") }) @@ -90,9 +86,13 @@ func TestCompiler(t *testing.T) { w.WriteHeader(http.StatusInternalServerError) })) defer svr.Close() - compiler := New(&settings.Config{Host: svr.URL, HTTPClient: http.DefaultClient}) + cfg := &settings.Config{Host: svr.URL, HTTPClient: http.DefaultClient} + apiClient := &v2APIClient{rest.NewFromConfig(cfg.Host, cfg)} + collaboratorsClient, err := collaborators.NewCollaboratorsRestClient(*cfg) + assert.NoError(t, err) + compiler := New(apiClient, collaboratorsClient) - _, err := compiler.ConfigQuery("testdata/config.yml", "1234", Parameters{}, Values{}) + _, err = compiler.ConfigQuery("testdata/config.yml", "1234", Parameters{}, Values{}) assert.Error(t, err) assert.Contains(t, err.Error(), "config compilation request returned an error") }) @@ -111,7 +111,11 @@ func TestCompiler(t *testing.T) { fmt.Fprintf(w, `{"valid":true,"source-yaml":"source","output-yaml":"output","errors":[]}`) })) defer svr.Close() - compiler := New(&settings.Config{Host: svr.URL, HTTPClient: http.DefaultClient}) + cfg := &settings.Config{Host: svr.URL, HTTPClient: http.DefaultClient} + apiClient := &v2APIClient{rest.NewFromConfig(cfg.Host, cfg)} + collaboratorsClient, err := collaborators.NewCollaboratorsRestClient(*cfg) + assert.NoError(t, err) + compiler := New(apiClient, collaboratorsClient) resp, err := compiler.ConfigQuery("testdata/test.yml", "1234", Parameters{}, Values{}) assert.NoError(t, err) diff --git a/config/legacy.go b/config/v1_api_client.go similarity index 88% rename from config/legacy.go rename to config/v1_api_client.go index 02581bb74..a65e9a134 100644 --- a/config/legacy.go +++ b/config/v1_api_client.go @@ -4,13 +4,17 @@ import ( "encoding/json" "fmt" "sort" - "strings" "github.com/CircleCI-Public/circleci-cli/api/graphql" - "github.com/CircleCI-Public/circleci-cli/settings" "github.com/pkg/errors" ) +const v1_string apiClientVersion = "v1" + +type v1APIClient struct { + gql *graphql.Client +} + // GQLErrorsCollection is a slice of errors returned by the GraphQL server. // Each error is made up of a GQLResponseError type. type GQLErrorsCollection []GQLResponseError @@ -24,13 +28,13 @@ type BuildConfigResponse struct { // Error turns a GQLErrorsCollection into an acceptable error string that can be printed to the user. func (errs GQLErrorsCollection) Error() string { - messages := []string{} + message := "config compilation contains errors:" for i := range errs { - messages = append(messages, errs[i].Message) + message += fmt.Sprintf("\n\t- %s", errs[i].Message) } - return strings.Join(messages, "\n") + return message } // LegacyConfigResponse is a structure that matches the result of the GQL @@ -54,6 +58,12 @@ type GQLResponseError struct { Type string } +// KeyVal is a data structure specifically for passing pipeline data to GraphQL which doesn't support free-form maps. +type KeyVal struct { + Key string `json:"key"` + Val interface{} `json:"val"` +} + // PrepareForGraphQL takes a golang homogenous map, and transforms it into a list of keyval pairs, since GraphQL does not support homogenous maps. func PrepareForGraphQL(kvMap Values) []KeyVal { // we need to create the slice of KeyVals in a deterministic order for testing purposes @@ -70,13 +80,7 @@ func PrepareForGraphQL(kvMap Values) []KeyVal { return kvs } -func (c *ConfigCompiler) legacyConfigQueryByOrgID( - configString string, - orgID string, - params Parameters, - values Values, - cfg *settings.Config, -) (*ConfigResponse, error) { +func (client *v1APIClient) CompileConfig(configContent string, orgID string, params Parameters, values Values) (*ConfigResponse, error) { var response BuildConfigResponse // GraphQL isn't forwards-compatible, so we are unusually selective here about // passing only non-empty fields on to the API, to minimize user impact if the @@ -101,8 +105,8 @@ func (c *ConfigCompiler) legacyConfigQueryByOrgID( ) request := graphql.NewRequest(query) - request.SetToken(cfg.Token) - request.Var("config", configString) + request.SetToken(client.gql.Token) + request.Var("config", configContent) if values != nil { request.Var("pipelineValues", PrepareForGraphQL(values)) @@ -119,7 +123,7 @@ func (c *ConfigCompiler) legacyConfigQueryByOrgID( request.Var("orgId", orgID) } - err := c.legacyGraphQLClient.Run(request, &response) + err := client.gql.Run(request, &response) if err != nil { return nil, errors.Wrap(err, "Unable to validate config") } @@ -133,9 +137,3 @@ func (c *ConfigCompiler) legacyConfigQueryByOrgID( OutputYaml: response.BuildConfig.LegacyConfigResponse.OutputYaml, }, nil } - -// KeyVal is a data structure specifically for passing pipeline data to GraphQL which doesn't support free-form maps. -type KeyVal struct { - Key string `json:"key"` - Val interface{} `json:"val"` -} diff --git a/config/legacy_test.go b/config/v1_api_client_test.go similarity index 74% rename from config/legacy_test.go rename to config/v1_api_client_test.go index 5175a136f..2eb0ce1da 100644 --- a/config/legacy_test.go +++ b/config/v1_api_client_test.go @@ -2,15 +2,17 @@ package config import ( "fmt" + "io/ioutil" "net/http" "net/http/httptest" "testing" + "github.com/CircleCI-Public/circleci-cli/api/collaborators" "github.com/CircleCI-Public/circleci-cli/settings" "github.com/stretchr/testify/assert" ) -func TestLegacyFlow(t *testing.T) { +func TestAPIV1Flow(t *testing.T) { t.Run("tests that the compiler defaults to the graphQL resolver should the original API request fail with 404", func(t *testing.T) { mux := http.NewServeMux() @@ -31,12 +33,17 @@ func TestLegacyFlow(t *testing.T) { svr := httptest.NewServer(mux) defer svr.Close() - compiler := New(&settings.Config{ + cfg := &settings.Config{ Host: svr.URL, Endpoint: "/graphql-unstable", HTTPClient: http.DefaultClient, Token: "", - }) + } + apiClient, err := newAPIClient(cfg) + assert.NoError(t, err) + collaboratorsClient, err := collaborators.NewCollaboratorsRestClient(*cfg) + assert.NoError(t, err) + compiler := New(apiClient, collaboratorsClient) resp, err := compiler.ConfigQuery("testdata/config.yml", "1234", Parameters{}, Values{}) assert.Equal(t, true, resp.Valid) @@ -63,13 +70,18 @@ func TestLegacyFlow(t *testing.T) { svr := httptest.NewServer(mux) defer svr.Close() - compiler := New(&settings.Config{ + cfg := &settings.Config{ Host: svr.URL, Endpoint: "/graphql-unstable", HTTPClient: http.DefaultClient, Token: "", - }) - _, err := compiler.ConfigQuery("testdata/config.yml", "1234", Parameters{}, Values{}) + } + apiClient, err := newAPIClient(cfg) + assert.NoError(t, err) + collaboratorsClient, err := collaborators.NewCollaboratorsRestClient(*cfg) + assert.NoError(t, err) + compiler := New(apiClient, collaboratorsClient) + _, err = compiler.ConfigQuery("testdata/config.yml", "1234", Parameters{}, Values{}) assert.Error(t, err) assert.Contains(t, err.Error(), "failed to validate") }) @@ -79,8 +91,14 @@ func TestLegacyFlow(t *testing.T) { gqlHitCounter := 0 mux.HandleFunc("/compile-config-with-defaults", func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusInternalServerError) + body, err := ioutil.ReadAll(r.Body) + assert.NoError(t, err) + if len(body) == 0 { + w.WriteHeader(http.StatusBadRequest) + return + } + w.WriteHeader(http.StatusInternalServerError) }) mux.HandleFunc("/me/collaborations", func(w http.ResponseWriter, r *http.Request) { @@ -97,13 +115,18 @@ func TestLegacyFlow(t *testing.T) { svr := httptest.NewServer(mux) defer svr.Close() - compiler := New(&settings.Config{ + cfg := &settings.Config{ Host: svr.URL, Endpoint: "/graphql-unstable", HTTPClient: http.DefaultClient, Token: "", - }) - _, err := compiler.ConfigQuery("testdata/config.yml", "1234", Parameters{}, Values{}) + } + apiClient, err := newAPIClient(cfg) + assert.NoError(t, err) + collaboratorsClient, err := collaborators.NewCollaboratorsRestClient(*cfg) + assert.NoError(t, err) + compiler := New(apiClient, collaboratorsClient) + _, err = compiler.ConfigQuery("testdata/config.yml", "1234", Parameters{}, Values{}) assert.Error(t, err) assert.Contains(t, err.Error(), "config compilation request returned an error:") assert.Equal(t, 0, gqlHitCounter) diff --git a/config/v2_api_client.go b/config/v2_api_client.go new file mode 100644 index 000000000..6d9c6bbe1 --- /dev/null +++ b/config/v2_api_client.go @@ -0,0 +1,62 @@ +package config + +import ( + "fmt" + "net/url" + + "github.com/CircleCI-Public/circleci-cli/api/rest" + "github.com/pkg/errors" +) + +const v2_string apiClientVersion = "v2" + +type v2APIClient struct { + restClient *rest.Client +} + +func configErrorsAsError(configErrors []ConfigError) error { + message := "config compilation contains errors:" + for _, err := range configErrors { + message += fmt.Sprintf("\n\t- %s", err.Message) + } + return errors.New(message) +} + +func (client *v2APIClient) CompileConfig(configContent string, orgID string, params Parameters, values Values) (*ConfigResponse, error) { + compileRequest := CompileConfigRequest{ + ConfigYaml: configContent, + Options: Options{ + OwnerID: orgID, + PipelineValues: values, + PipelineParameters: params, + }, + } + + req, err := client.restClient.NewRequest( + "POST", + &url.URL{ + Path: "compile-config-with-defaults", + }, + compileRequest, + ) + if err != nil { + return nil, fmt.Errorf("an error occurred creating the request: %w", err) + } + + configCompilationResp := &ConfigResponse{} + statusCode, originalErr := client.restClient.DoRequest(req, configCompilationResp) + + if originalErr != nil { + return nil, fmt.Errorf("config compilation request returned an error: %w", originalErr) + } + + if statusCode != 200 { + return nil, errors.New("unable to validate or compile config") + } + + if len(configCompilationResp.Errors) > 0 { + return nil, fmt.Errorf("config compilation contains errors: %s", configErrorsAsError(configCompilationResp.Errors)) + } + + return configCompilationResp, nil +} diff --git a/config/v2_api_client_test.go b/config/v2_api_client_test.go new file mode 100644 index 000000000..d5ebec6f2 --- /dev/null +++ b/config/v2_api_client_test.go @@ -0,0 +1,17 @@ +package config + +import ( + "testing" + + "gotest.tools/v3/assert" +) + +func TestConfigErrorsAsError(t *testing.T) { + err := configErrorsAsError([]ConfigError{ + {Message: "error on line 1"}, + {Message: "error on line 42"}, + }) + assert.Error(t, err, `config compilation contains errors: + - error on line 1 + - error on line 42`) +} diff --git a/local/local.go b/local/local.go index 39272c91a..4aa60ae72 100644 --- a/local/local.go +++ b/local/local.go @@ -26,7 +26,10 @@ func Execute(flags *pflag.FlagSet, cfg *settings.Config, args []string) error { var configResponse *config.ConfigResponse processedArgs, configPath := buildAgentArguments(flags) - compiler := config.New(cfg) + compiler, err := config.NewWithConfig(cfg) + if err != nil { + return err + } //if no orgId provided use org slug orgID, _ := flags.GetString("org-id") From fd1087e2145dc784709bd7498a3cfcf2d529893a Mon Sep 17 00:00:00 2001 From: JulesFaucherre Date: Tue, 1 Aug 2023 16:50:16 +0200 Subject: [PATCH 2/5] style: Fix linter issue --- api/rest/client.go | 3 +-- config/api_client.go | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/api/rest/client.go b/api/rest/client.go index 8204237fe..a64d14778 100644 --- a/api/rest/client.go +++ b/api/rest/client.go @@ -6,7 +6,6 @@ import ( "errors" "fmt" "io" - "io/ioutil" "net/http" "net/url" "strings" @@ -97,7 +96,7 @@ func (c *Client) DoRequest(req *http.Request, resp interface{}) (int, error) { httpError := struct { Message string `json:"message"` }{} - body, err := ioutil.ReadAll(httpResp.Body) + body, err := io.ReadAll(httpResp.Body) if err != nil { return httpResp.StatusCode, err } diff --git a/config/api_client.go b/config/api_client.go index 1d3493d05..f90d16d75 100644 --- a/config/api_client.go +++ b/config/api_client.go @@ -34,7 +34,7 @@ func GetAPIClient(config *settings.Config) (APIClient, error) { } func newAPIClient(config *settings.Config) (APIClient, error) { - hostValue := getCompileHost(config.Host) + hostValue := GetCompileHost(config.Host) restClient := rest.NewFromConfig(hostValue, config) version, err := detectAPIClientVersion(restClient) From 42ca438920441b5ade0856975e994cb113f5804d Mon Sep 17 00:00:00 2001 From: JulesFaucherre Date: Wed, 2 Aug 2023 10:01:57 +0200 Subject: [PATCH 3/5] style: Linter error addressed --- cmd/policy/policy.go | 10 ++++++++-- config/api_client.go | 6 ++---- config/api_client_test.go | 23 +++-------------------- config/v1_api_client_test.go | 4 ++-- 4 files changed, 15 insertions(+), 28 deletions(-) diff --git a/cmd/policy/policy.go b/cmd/policy/policy.go index dfecd3371..6ccab181c 100644 --- a/cmd/policy/policy.go +++ b/cmd/policy/policy.go @@ -295,7 +295,10 @@ This group of commands allows the management of polices to be verified against b } if !noCompile && context == "config" { - compiler := config.New(globalConfig) + compiler, err := config.NewWithConfig(globalConfig) + if err != nil { + return err + } input, err = mergeCompiledConfig(compiler, config.ProcessConfigOpts{ ConfigPath: inputPath, OrgID: ownerID, @@ -376,7 +379,10 @@ This group of commands allows the management of polices to be verified against b } if !noCompile && context == "config" { - compiler := config.New(globalConfig) + compiler, err := config.NewWithConfig(globalConfig) + if err != nil { + return err + } input, err = mergeCompiledConfig(compiler, config.ProcessConfigOpts{ ConfigPath: inputPath, OrgID: ownerID, diff --git a/config/api_client.go b/config/api_client.go index f90d16d75..cd87a6fd6 100644 --- a/config/api_client.go +++ b/config/api_client.go @@ -59,10 +59,8 @@ func detectAPIClientVersion(restClient *rest.Client) (apiClientVersion, error) { } statusCode, err := restClient.DoRequest(req, nil) - if err != nil { - if _, ok := err.(*rest.HTTPError); ok { - return "", err - } + if _, ok := err.(*rest.HTTPError); !ok { + return "", err } if statusCode == 404 { return v1_string, nil diff --git a/config/api_client_test.go b/config/api_client_test.go index 7011afafc..cc61be18d 100644 --- a/config/api_client_test.go +++ b/config/api_client_test.go @@ -13,25 +13,9 @@ import ( func TestAPIClient(t *testing.T) { t.Run("detectCompilerVersion", func(t *testing.T) { - t.Run("when the route returns a 400 tells that the version is v2", func(t *testing.T) { - svr := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(400) - w.Header().Set("Content-Type", "application/octet-stream") - fmt.Fprintf(w, "Invalid input") - })) - url, err := url.Parse(svr.URL) - assert.NilError(t, err) - - restClient := rest.New(url, "token", http.DefaultClient) - version, err := detectAPIClientVersion(restClient) - assert.NilError(t, err) - assert.Equal(t, version, v2_string) - }) - t.Run("when the route returns a 404 tells that the version is v1", func(t *testing.T) { svr := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(404) - w.Header().Set("Content-Type", "application/octet-stream") fmt.Fprintf(w, "Invalid input") })) url, err := url.Parse(svr.URL) @@ -43,10 +27,9 @@ func TestAPIClient(t *testing.T) { assert.Equal(t, version, v1_string) }) - t.Run("in any other case, return an error", func(t *testing.T) { + t.Run("on other cases return v2", func(t *testing.T) { svr := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(500) - w.Header().Set("Content-Type", "application/octet-stream") fmt.Fprintf(w, "Invalid input") })) url, err := url.Parse(svr.URL) @@ -54,8 +37,8 @@ func TestAPIClient(t *testing.T) { restClient := rest.New(url, "token", http.DefaultClient) version, err := detectAPIClientVersion(restClient) - assert.Equal(t, version, apiClientVersion("")) - assert.Error(t, err, "Invalid input") + assert.NilError(t, err) + assert.Equal(t, version, v2_string) }) }) } diff --git a/config/v1_api_client_test.go b/config/v1_api_client_test.go index 2eb0ce1da..64a78c8fe 100644 --- a/config/v1_api_client_test.go +++ b/config/v1_api_client_test.go @@ -2,7 +2,7 @@ package config import ( "fmt" - "io/ioutil" + "io" "net/http" "net/http/httptest" "testing" @@ -91,7 +91,7 @@ func TestAPIV1Flow(t *testing.T) { gqlHitCounter := 0 mux.HandleFunc("/compile-config-with-defaults", func(w http.ResponseWriter, r *http.Request) { - body, err := ioutil.ReadAll(r.Body) + body, err := io.ReadAll(r.Body) assert.NoError(t, err) if len(body) == 0 { w.WriteHeader(http.StatusBadRequest) From 416053e9a6d51e9b4441c68a8e504ffae5795f9d Mon Sep 17 00:00:00 2001 From: JulesFaucherre Date: Wed, 2 Aug 2023 15:09:33 +0200 Subject: [PATCH 4/5] Changed the way we create config API client Fixed the tests that needed to be adapted to this change --- cmd/policy/policy_test.go | 20 ++++++++++++++++---- config/api_client.go | 14 -------------- config/config.go | 2 +- config/v2_api_client.go | 4 +--- 4 files changed, 18 insertions(+), 22 deletions(-) diff --git a/cmd/policy/policy_test.go b/cmd/policy/policy_test.go index 8fbe1a72c..227a6a66d 100644 --- a/cmd/policy/policy_test.go +++ b/cmd/policy/policy_test.go @@ -635,7 +635,10 @@ test: config CompilerServerHandler: func(w http.ResponseWriter, r *http.Request) { var req config.CompileConfigRequest err := json.NewDecoder(r.Body).Decode(&req) - require.NoError(t, err) + if err != nil { + w.WriteHeader(http.StatusBadRequest) + return + } // dummy compilation here (remove the _compiled_ key in compiled config, as compiled config can't have that at top-level key). var yamlResp map[string]any @@ -678,7 +681,10 @@ test: config CompilerServerHandler: func(w http.ResponseWriter, r *http.Request) { var req config.CompileConfigRequest err := json.NewDecoder(r.Body).Decode(&req) - require.NoError(t, err) + if err != nil { + w.WriteHeader(http.StatusBadRequest) + return + } response := config.ConfigResponse{Valid: true, SourceYaml: req.ConfigYaml, OutputYaml: req.ConfigYaml} @@ -837,7 +843,10 @@ test: config CompilerServerHandler: func(w http.ResponseWriter, r *http.Request) { var req config.CompileConfigRequest err := json.NewDecoder(r.Body).Decode(&req) - require.NoError(t, err) + if err != nil { + w.WriteHeader(http.StatusBadRequest) + return + } response := config.ConfigResponse{Valid: true, SourceYaml: req.ConfigYaml, OutputYaml: req.ConfigYaml} @@ -1082,7 +1091,10 @@ test: config CompilerServerHandler: func(w http.ResponseWriter, r *http.Request) { var req config.CompileConfigRequest err := json.NewDecoder(r.Body).Decode(&req) - require.NoError(t, err) + if err != nil { + w.WriteHeader(http.StatusBadRequest) + return + } response := config.ConfigResponse{Valid: true, SourceYaml: req.ConfigYaml, OutputYaml: req.ConfigYaml} diff --git a/config/api_client.go b/config/api_client.go index cd87a6fd6..6c62bef0b 100644 --- a/config/api_client.go +++ b/config/api_client.go @@ -3,7 +3,6 @@ package config import ( "fmt" "net/url" - "sync" "github.com/CircleCI-Public/circleci-cli/api/graphql" "github.com/CircleCI-Public/circleci-cli/api/rest" @@ -11,10 +10,6 @@ import ( ) var ( - compiler APIClient - compilerError error - once sync.Once - compilePath = "compile-config-with-defaults" ) @@ -24,15 +19,6 @@ type APIClient interface { CompileConfig(configContent string, orgID string, params Parameters, values Values) (*ConfigResponse, error) } -func GetAPIClient(config *settings.Config) (APIClient, error) { - if compiler == nil { - once.Do(func() { - compiler, compilerError = newAPIClient(config) - }) - } - return compiler, compilerError -} - func newAPIClient(config *settings.Config) (APIClient, error) { hostValue := GetCompileHost(config.Host) restClient := rest.NewFromConfig(hostValue, config) diff --git a/config/config.go b/config/config.go index 741384220..9d52be9a1 100644 --- a/config/config.go +++ b/config/config.go @@ -24,7 +24,7 @@ type ConfigCompiler struct { } func NewWithConfig(cfg *settings.Config) (*ConfigCompiler, error) { - apiClient, err := GetAPIClient(cfg) + apiClient, err := newAPIClient(cfg) if err != nil { return nil, err } diff --git a/config/v2_api_client.go b/config/v2_api_client.go index 6d9c6bbe1..2ae9a3152 100644 --- a/config/v2_api_client.go +++ b/config/v2_api_client.go @@ -34,9 +34,7 @@ func (client *v2APIClient) CompileConfig(configContent string, orgID string, par req, err := client.restClient.NewRequest( "POST", - &url.URL{ - Path: "compile-config-with-defaults", - }, + &url.URL{Path: compilePath}, compileRequest, ) if err != nil { From 188282bbd2fcf7681ee468a111b428db93ab6e39 Mon Sep 17 00:00:00 2001 From: JulesFaucherre Date: Tue, 8 Aug 2023 09:48:50 +0200 Subject: [PATCH 5/5] style: Addressed PR comments --- api/rest/client.go | 2 +- config/api_client.go | 19 +++++++++++++------ 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/api/rest/client.go b/api/rest/client.go index a64d14778..1e8e9e6bb 100644 --- a/api/rest/client.go +++ b/api/rest/client.go @@ -98,7 +98,7 @@ func (c *Client) DoRequest(req *http.Request, resp interface{}) (int, error) { }{} body, err := io.ReadAll(httpResp.Body) if err != nil { - return httpResp.StatusCode, err + return 0, err } err = json.Unmarshal(body, &httpError) if err != nil { diff --git a/config/api_client.go b/config/api_client.go index 6c62bef0b..22c791fcc 100644 --- a/config/api_client.go +++ b/config/api_client.go @@ -2,6 +2,7 @@ package config import ( "fmt" + "net/http" "net/url" "github.com/CircleCI-Public/circleci-cli/api/graphql" @@ -9,9 +10,7 @@ import ( "github.com/CircleCI-Public/circleci-cli/settings" ) -var ( - compilePath = "compile-config-with-defaults" -) +const compilePath = "compile-config-with-defaults" type apiClientVersion string @@ -38,17 +37,25 @@ func newAPIClient(config *settings.Config) (APIClient, error) { } } +// detectAPIClientVersion returns the highest available version of the config API. +// +// To do that it tries to request the `compilePath` API route. +// If the route returns a 404, this means the route does not exist on the requested host and the function returns +// `v1_string` indicating that the deprecated GraphQL endpoint should be used instead. +// Else if the route returns any other status, this means it is available for request and the function returns +// `v2_string` indicating that the route can be used func detectAPIClientVersion(restClient *rest.Client) (apiClientVersion, error) { req, err := restClient.NewRequest("POST", &url.URL{Path: compilePath}, nil) if err != nil { return "", err } - statusCode, err := restClient.DoRequest(req, nil) - if _, ok := err.(*rest.HTTPError); !ok { + _, err = restClient.DoRequest(req, nil) + httpErr, ok := err.(*rest.HTTPError) + if !ok { return "", err } - if statusCode == 404 { + if httpErr.Code == http.StatusNotFound { return v1_string, nil } return v2_string, nil