From d7b0025efd029b3395841ab3f5ccffc6a3bf2ba8 Mon Sep 17 00:00:00 2001 From: Kemal <223029+disq@users.noreply.github.com> Date: Wed, 22 Nov 2023 07:50:54 -0800 Subject: [PATCH] fix: Quality of life improvements (#172) Extracted from https://github.com/cloudquery/plugin-pb-go/pull/170 --- .github/workflows/lint_golang.yml | 2 +- .golangci.yml | 4 ++++ go.mod | 3 +++ managedplugin/download.go | 13 +++++++------ managedplugin/plugin.go | 8 ++++---- managedplugin/registry_test.go | 6 +++--- specs/destination.go | 5 +---- specs/destination_test.go | 10 +++------- specs/registry.go | 4 ++++ specs/registry_test.go | 6 +++--- specs/source.go | 7 ++----- specs/source_test.go | 10 +++------- 12 files changed, 38 insertions(+), 40 deletions(-) diff --git a/.github/workflows/lint_golang.yml b/.github/workflows/lint_golang.yml index 541682d..261af37 100644 --- a/.github/workflows/lint_golang.yml +++ b/.github/workflows/lint_golang.yml @@ -31,5 +31,5 @@ jobs: - name: golangci-lint uses: golangci/golangci-lint-action@v3 with: - version: v1.54.2 + version: v1.55.2 args: --verbose diff --git a/.golangci.yml b/.golangci.yml index 816724f..ea79ce0 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -51,6 +51,10 @@ linters-settings: disabled: true - name: nested-structs disabled: true + - name: import-alias-naming + disabled: true + - name: unchecked-type-assertion + disabled: true gofmt: rewrite-rules: diff --git a/go.mod b/go.mod index bc96df5..7d790d1 100644 --- a/go.mod +++ b/go.mod @@ -14,6 +14,7 @@ require ( github.com/opencontainers/image-spec v1.0.2 github.com/rs/zerolog v1.29.1 github.com/schollz/progressbar/v3 v3.13.1 + github.com/stretchr/testify v1.8.4 github.com/thoas/go-funk v0.9.3 golang.org/x/exp v0.0.0-20231006140011-7918f672742d google.golang.org/grpc v1.58.3 @@ -36,6 +37,7 @@ require ( github.com/bytedance/sonic v1.10.2 // indirect github.com/chenzhuoyu/base64x v0.0.0-20230717121745-296ad89f973d // indirect github.com/chenzhuoyu/iasm v0.9.0 // indirect + github.com/davecgh/go-spew v1.1.1 // indirect github.com/deepmap/oapi-codegen v1.15.0 // indirect github.com/distribution/reference v0.5.0 // indirect github.com/docker/distribution v2.8.3+incompatible // indirect @@ -84,6 +86,7 @@ require ( github.com/pelletier/go-toml/v2 v2.0.8 // indirect github.com/pierrec/lz4/v4 v4.1.18 // indirect github.com/pkg/errors v0.9.1 // indirect + github.com/pmezard/go-difflib v1.0.0 // indirect github.com/rivo/uniseg v0.2.0 // indirect github.com/russross/blackfriday/v2 v2.1.0 // indirect github.com/schollz/closestmatch v2.1.0+incompatible // indirect diff --git a/managedplugin/download.go b/managedplugin/download.go index e74ae23..7fde1f0 100644 --- a/managedplugin/download.go +++ b/managedplugin/download.go @@ -85,20 +85,21 @@ func getURLLocation(ctx context.Context, org string, name string, version string } resp.Body.Close() // Check server response - switch { - case resp.StatusCode == http.StatusNotFound: + switch resp.StatusCode { + case http.StatusOK: + return nil + case http.StatusNotFound: return err404 - case resp.StatusCode == http.StatusUnauthorized: + case http.StatusUnauthorized: fmt.Printf("Failed downloading %s with status code %d. Retrying\n", downloadURL, resp.StatusCode) return err401 - case resp.StatusCode == http.StatusTooManyRequests: + case http.StatusTooManyRequests: fmt.Printf("Failed downloading %s with status code %d. Retrying\n", downloadURL, resp.StatusCode) return err429 - case resp.StatusCode >= http.StatusBadRequest: // anything that's not 200 or 30* + default: fmt.Printf("Failed downloading %s with status code %d\n", downloadURL, resp.StatusCode) return fmt.Errorf("statusCode %d", resp.StatusCode) } - return nil }, retry.RetryIf(func(err error) bool { return err == err401 || err == err429 }), diff --git a/managedplugin/plugin.go b/managedplugin/plugin.go index a16f804..79056a5 100644 --- a/managedplugin/plugin.go +++ b/managedplugin/plugin.go @@ -96,13 +96,13 @@ type Client struct { // typ will be deprecated soon but now required for a transition period func NewClients(ctx context.Context, typ PluginType, specs []Config, opts ...Option) (Clients, error) { - clients := make(Clients, len(specs)) - for i, spec := range specs { + clients := make(Clients, 0, len(specs)) + for _, spec := range specs { client, err := NewClient(ctx, typ, spec, opts...) if err != nil { - return nil, err + return clients, err // previous entries in clients determine which plugins were successfully created } - clients[i] = client + clients = append(clients, client) } return clients, nil } diff --git a/managedplugin/registry_test.go b/managedplugin/registry_test.go index 198c81a..c48cf89 100644 --- a/managedplugin/registry_test.go +++ b/managedplugin/registry_test.go @@ -17,11 +17,11 @@ func TestRegistryJsonMarshalUnmarshal(t *testing.T) { t.Fatal("failed to unmarshal registry:", err) } if registry != RegistryGrpc { - t.Fatal("expected registry to be github, but got:", registry) + t.Fatal("expected registry to be grpc, but got:", registry) } } -func TestRegistryYamlMarshalUnmarsahl(t *testing.T) { +func TestRegistryYamlMarshalUnmarshal(t *testing.T) { b, err := yaml.Marshal(RegistryGrpc) if err != nil { t.Fatal("failed to marshal registry:", err) @@ -31,6 +31,6 @@ func TestRegistryYamlMarshalUnmarsahl(t *testing.T) { t.Fatal("failed to unmarshal registry:", err) } if registry != RegistryGrpc { - t.Fatal("expected registry to be github, but got:", registry) + t.Fatal("expected registry to be grpc, but got:", registry) } } diff --git a/specs/destination.go b/specs/destination.go index 1360b48..8d7a110 100644 --- a/specs/destination.go +++ b/specs/destination.go @@ -23,9 +23,6 @@ type Destination struct { } func (d *Destination) SetDefaults(defaultBatchSize, defaultBatchSizeBytes int) { - if d.Registry.String() == "" { - d.Registry = RegistryGithub - } if d.BatchSize == 0 { d.BatchSize = defaultBatchSize } @@ -59,7 +56,7 @@ func (d *Destination) Validate() error { return fmt.Errorf(msg) } - if d.Registry == RegistryGithub { + if d.Registry.NeedVersion() { if d.Version == "" { return fmt.Errorf("version is required") } diff --git a/specs/destination_test.go b/specs/destination_test.go index 7514ef7..b6c9671 100644 --- a/specs/destination_test.go +++ b/specs/destination_test.go @@ -3,7 +3,7 @@ package specs import ( "testing" - "github.com/google/go-cmp/cmp" + "github.com/stretchr/testify/require" ) type testDestinationSpec struct { @@ -77,9 +77,7 @@ func TestDestinationUnmarshalSpec(t *testing.T) { } source := spec.Spec.(*Source) - if cmp.Diff(source, tc.source) != "" { - t.Fatalf("expected:%v got:%v", tc.source, source) - } + require.Equal(t, tc.source, source) }) } } @@ -191,9 +189,7 @@ func TestDestinationUnmarshalSpecValidate(t *testing.T) { return } - if cmp.Diff(destination, tc.destination) != "" { - t.Fatalf("expected:\n%v\ngot:\n%v\n", tc.destination, destination) - } + require.Equal(t, tc.destination, destination) }) } } diff --git a/specs/registry.go b/specs/registry.go index ab6511b..8fafd46 100644 --- a/specs/registry.go +++ b/specs/registry.go @@ -37,6 +37,10 @@ func (r *Registry) UnmarshalJSON(data []byte) (err error) { return nil } +func (r Registry) NeedVersion() bool { + return r == RegistryGithub || r == RegistryCloudQuery +} + func RegistryFromString(s string) (Registry, error) { switch s { case "github": diff --git a/specs/registry_test.go b/specs/registry_test.go index d7751a2..e3e4153 100644 --- a/specs/registry_test.go +++ b/specs/registry_test.go @@ -17,11 +17,11 @@ func TestRegistryJsonMarshalUnmarshal(t *testing.T) { t.Fatal("failed to unmarshal registry:", err) } if registry != RegistryGrpc { - t.Fatal("expected registry to be github, but got:", registry) + t.Fatal("expected registry to be grpc, but got:", registry) } } -func TestRegistryYamlMarshalUnmarsahl(t *testing.T) { +func TestRegistryYamlMarshalUnmarshal(t *testing.T) { b, err := yaml.Marshal(RegistryGrpc) if err != nil { t.Fatal("failed to marshal registry:", err) @@ -31,6 +31,6 @@ func TestRegistryYamlMarshalUnmarsahl(t *testing.T) { t.Fatal("failed to unmarshal registry:", err) } if registry != RegistryGrpc { - t.Fatal("expected registry to be github, but got:", registry) + t.Fatal("expected registry to be grpc, but got:", registry) } } diff --git a/specs/source.go b/specs/source.go index b69839b..dd82187 100644 --- a/specs/source.go +++ b/specs/source.go @@ -25,7 +25,7 @@ type Source struct { // For the local registry the path will be the path to the binary: ./path/to/binary // For the gRPC registry the path will be the address of the gRPC server: host:port Path string `json:"path,omitempty"` - // Registry can be github,local,grpc. + // Registry can be github,local,grpc,cloudquery Registry Registry `json:"registry,omitempty"` Concurrency uint64 `json:"concurrency,omitempty"` TableConcurrency uint64 `json:"table_concurrency,omitempty"` // deprecated: use Concurrency instead @@ -55,9 +55,6 @@ type Source struct { } func (s *Source) SetDefaults() { - if s.Registry.String() == "" { - s.Registry = RegistryGithub - } if s.Backend.String() == "" { s.Backend = BackendNone } @@ -117,7 +114,7 @@ func (s *Source) Validate() error { return fmt.Errorf("tables configuration is required. Hint: set the tables you want to sync by adding `tables: [...]` or use `cloudquery tables` to list available tables") } - if s.Registry == RegistryGithub { + if s.Registry.NeedVersion() { if s.Version == "" { return fmt.Errorf("version is required") } diff --git a/specs/source_test.go b/specs/source_test.go index 3d0ec12..3c69bc0 100644 --- a/specs/source_test.go +++ b/specs/source_test.go @@ -3,7 +3,7 @@ package specs import ( "testing" - "github.com/google/go-cmp/cmp" + "github.com/stretchr/testify/require" ) var sourceUnmarshalSpecTestCases = []struct { @@ -58,9 +58,7 @@ func TestSourceUnmarshalSpec(t *testing.T) { } source := spec.Spec.(*Source) - if cmp.Diff(source, tc.source) != "" { - t.Fatalf("expected:%v got:%v", tc.source, source) - } + require.Equal(t, tc.source, source) }) } } @@ -199,9 +197,7 @@ func TestSourceUnmarshalSpecValidate(t *testing.T) { return } - if cmp.Diff(source, tc.source) != "" { - t.Fatalf("expected:%v got:%v", tc.source, source) - } + require.Equal(t, tc.source, source) }) } }