diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index c8dde5e..f3d26a6 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -28,6 +28,10 @@ jobs: build ci refactor + revert + design + style + perf subjectPattern: ^(?![A-Z]).+$ subjectPatternError: | The subject "{subject}" found in the pull request title "{title}" diff --git a/internal/telemetry/client.go b/internal/telemetry/client.go index 288b2ab..37d393c 100644 --- a/internal/telemetry/client.go +++ b/internal/telemetry/client.go @@ -96,10 +96,11 @@ func Get(opts ...GetOption) Client { getOrCreateConfigFile := func(getCfg getConfig) (Config, error) { configPath := filepath.Join(getCfg.userHome, ConfigFile) + // if no file exists, create a new one analyticsCfg, err := loadConfigFromFile(configPath) if errors.Is(err, os.ErrNotExist) { // file not found, create a new one - analyticsCfg = Config{UserID: NewULID()} + analyticsCfg = Config{UserUUID: NewUUID()} if err := writeConfigToFile(configPath, analyticsCfg); err != nil { return analyticsCfg, fmt.Errorf("could not write file to %s: %w", configPath, err) } @@ -108,6 +109,14 @@ func Get(opts ...GetOption) Client { return Config{}, fmt.Errorf("could not load config from %s: %w", configPath, err) } + // if a file exists but doesn't have a uuid, create a new uuid + if analyticsCfg.UserUUID.IsZero() { + analyticsCfg.UserUUID = NewUUID() + if err := writeConfigToFile(configPath, analyticsCfg); err != nil { + return analyticsCfg, fmt.Errorf("could not write file to %s: %w", configPath, err) + } + } + return analyticsCfg, nil } diff --git a/internal/telemetry/client_test.go b/internal/telemetry/client_test.go index 67f6eeb..2c0c7d0 100644 --- a/internal/telemetry/client_test.go +++ b/internal/telemetry/client_test.go @@ -28,6 +28,46 @@ func TestGet(t *testing.T) { if !strings.Contains(string(data), "Airbyte") { t.Error("expected config file to contain 'Airbyte'") } + + if !strings.Contains(string(data), "anonymous_user_uuid") { + t.Error("expected config file to contain 'anonymous_user_uuid'") + } + + if strings.Contains(string(data), "anonymous_user_id") { + t.Error("config file should not contain 'anonymous_user_id'") + } +} + +func TestGet_WithExistingULID(t *testing.T) { + instance = nil + home := t.TempDir() + + // write a config with a ulid only + cfg := Config{UserID: NewULID()} + if err := writeConfigToFile(filepath.Join(home, ConfigFile), cfg); err != nil { + t.Fatal("failed writing config", err) + } + + cli := Get(WithUserHome(home)) + if _, ok := cli.(*SegmentClient); !ok { + t.Error(fmt.Sprintf("expected SegmentClient; received: %T", cli)) + } + + // verify configuration file was created + data, err := os.ReadFile(filepath.Join(home, ConfigFile)) + if err != nil { + t.Error("reading config file", err) + } + + // and has some data + if !strings.Contains(string(data), "Airbyte") { + t.Error("expected config file to contain 'Airbyte'") + } + + if !strings.Contains(string(data), "anonymous_user_uuid") { + t.Error("expected config file to contain 'anonymous_user_uuid'") + } + if !strings.Contains(string(data), "anonymous_user_id") { t.Error("expected config file to contain 'anonymous_user_id'") } diff --git a/internal/telemetry/config.go b/internal/telemetry/config.go index 27f8d76..adeb530 100644 --- a/internal/telemetry/config.go +++ b/internal/telemetry/config.go @@ -2,6 +2,7 @@ package telemetry import ( "fmt" + "github.com/google/uuid" "github.com/oklog/ulid/v2" "gopkg.in/yaml.v3" "os" @@ -16,7 +17,45 @@ Anonymous usage reporting is currently enabled. For more information, please see var ConfigFile = filepath.Join(".airbyte", "analytics.yml") +// UUID is a wrapper around uuid.UUID so that we can implement the yaml interfaces. +type UUID uuid.UUID + +// NewUUID returns a new randomized UUID. +func NewUUID() UUID { + return UUID(uuid.New()) +} + +// String returns a string representation of this UUID. +func (u UUID) String() string { + return uuid.UUID(u).String() +} + +func (u *UUID) UnmarshalYAML(node *yaml.Node) error { + var s string + if err := node.Decode(&s); err != nil { + return fmt.Errorf("could not unmarshal yaml: %w", err) + } + + parsed, err := uuid.Parse(s) + if err != nil { + return fmt.Errorf("could not parse uuid (%s): %w", s, err) + } + + *u = UUID(parsed) + return nil +} + +func (u UUID) MarshalYAML() (any, error) { + return uuid.UUID(u).String(), nil +} + +// IsZero implements the yaml interface, used to treat a uuid.Nil as empty for yaml purposes +func (u UUID) IsZero() bool { + return u.String() == uuid.Nil.String() +} + // ULID is a wrapper around ulid.ULID so that we can implement the yaml interfaces. +// Deprecated: use UUID instead type ULID ulid.ULID // NewULID returns a new randomized ULID. @@ -50,16 +89,18 @@ func (u *ULID) UnmarshalYAML(node *yaml.Node) error { } // MarshalYAML allows for converting a ULID into a yaml field. -// -//goland:noinspection GoMixedReceiverTypes func (u ULID) MarshalYAML() (any, error) { - //panic("test") return ulid.ULID(u).String(), nil } +func (u ULID) IsZero() bool { + return u.String() == "00000000000000000000000000" +} + // Config represents the analytics.yaml file. type Config struct { - UserID ULID `yaml:"anonymous_user_id"` + UserID ULID `yaml:"anonymous_user_id,omitempty"` + UserUUID UUID `yaml:"anonymous_user_uuid,omitempty"` } // permissions sets the file and directory permission level for the telemetry files that may be created. diff --git a/internal/telemetry/config_test.go b/internal/telemetry/config_test.go index 1f50d83..7d1ee6d 100644 --- a/internal/telemetry/config_test.go +++ b/internal/telemetry/config_test.go @@ -4,100 +4,269 @@ import ( "errors" "fmt" "github.com/google/go-cmp/cmp" + "github.com/google/uuid" "github.com/oklog/ulid/v2" + "gopkg.in/yaml.v3" "os" "path/filepath" + "strings" "testing" ) -var id = ulid.Make() +var ulidID = ulid.Make() +var uuidID = uuid.New() -func TestLoadConfigFromFile(t *testing.T) { - f, err := os.CreateTemp(t.TempDir(), "analytics-") - if err != nil { - t.Fatal("could not create temp file", err) - } - defer f.Close() +func TestLoadConfigWithULID(t *testing.T) { + t.Run("happy path", func(t *testing.T) { + f, err := os.CreateTemp(t.TempDir(), "analytics-") + if err != nil { + t.Fatal("could not create temp file", err) + } + defer f.Close() - if _, err := f.WriteString(`# comments -anonymous_user_id: ` + id.String()); err != nil { - t.Fatal("could not write to temp file", err) - } + if _, err := f.WriteString(`# comments +anonymous_user_id: ` + ulidID.String()); err != nil { + t.Fatal("could not write to temp file", err) + } - cnf, err := loadConfigFromFile(f.Name()) - if d := cmp.Diff(nil, err); d != "" { - t.Error("failed to load file", d) - } + cnf, err := loadConfigFromFile(f.Name()) + if d := cmp.Diff(nil, err); d != "" { + t.Error("failed to load file", d) + } - if d := cmp.Diff(id.String(), cnf.UserID.String()); d != "" { - t.Error("id is incorrect", d) - } -} + if d := cmp.Diff(ulidID.String(), cnf.UserID.String()); d != "" { + t.Error("id is incorrect", d) + } + }) + + t.Run("no file returns err", func(t *testing.T) { + _, err := loadConfigFromFile(filepath.Join(t.TempDir(), "dne.yml")) + if err == nil { + t.Error("expected an error to be returned") + } + // should return a os.ErrNotExist if no file was found + if d := cmp.Diff(true, errors.Is(err, os.ErrNotExist)); d != "" { + t.Error("expected os.ErrNotExist", err) + } + }) + + t.Run("incorrect format returns err", func(t *testing.T) { + f, err := os.CreateTemp(t.TempDir(), "analytics-") + if err != nil { + t.Fatal("could not create temp file", err) + } + defer f.Close() + + if _, err := f.WriteString(`This is a regular file, not a configuration file!`); err != nil { + t.Fatal("could not write to temp file", err) + } + + _, err = loadConfigFromFile(f.Name()) + if err == nil { + t.Error("expected an error to be returned") + } + }) -func TestLoadConfigFromFile_NoFileReturnsErrNotExist(t *testing.T) { - _, err := loadConfigFromFile(filepath.Join(t.TempDir(), "dne.yml")) - if err == nil { - t.Error("expected an error to be returned") - } - // should return a os.ErrNotExist if no file was found - if d := cmp.Diff(true, errors.Is(err, os.ErrNotExist)); d != "" { - t.Error("expected os.ErrNotExist", err) - } + t.Run("unreadable file returns err", func(t *testing.T) { + f, err := os.CreateTemp(t.TempDir(), "analytics-") + if err != nil { + t.Fatal("could not create temp file", err) + } + defer f.Close() + + // remove read permissions from file + if err := f.Chmod(0222); err != nil { + t.Fatal("could not chmod temp file", err) + } + + _, err = loadConfigFromFile(f.Name()) + if err == nil { + t.Error("expected an error to be returned") + } + }) } -func TestLoadConfigFromFile_IncorrectFormatReturnsErr(t *testing.T) { - f, err := os.CreateTemp(t.TempDir(), "analytics-") - if err != nil { - t.Fatal("could not create temp file", err) - } - defer f.Close() - - if _, err := f.WriteString(`This is a regular file, not a configuration file!`); err != nil { - t.Fatal("could not write to temp file", err) - } - - _, err = loadConfigFromFile(f.Name()) - if err == nil { - t.Error("expected an error to be returned") - } +func TestLoadConfigWithUUID(t *testing.T) { + t.Run("happy path", func(t *testing.T) { + f, err := os.CreateTemp(t.TempDir(), "analytics-") + if err != nil { + t.Fatal("could not create temp file", err) + } + defer f.Close() + + if _, err := f.WriteString(`# comments +anonymous_user_uuid: ` + uuidID.String()); err != nil { + t.Fatal("could not write to temp file", err) + } + + cnf, err := loadConfigFromFile(f.Name()) + if d := cmp.Diff(nil, err); d != "" { + t.Error("failed to load file", d) + } + + if d := cmp.Diff(uuidID.String(), cnf.UserUUID.String()); d != "" { + t.Error("id is incorrect", d) + } + }) + + t.Run("no file returns err", func(t *testing.T) { + _, err := loadConfigFromFile(filepath.Join(t.TempDir(), "dne.yml")) + if err == nil { + t.Error("expected an error to be returned") + } + // should return a os.ErrNotExist if no file was found + if d := cmp.Diff(true, errors.Is(err, os.ErrNotExist)); d != "" { + t.Error("expected os.ErrNotExist", err) + } + }) + + t.Run("incorrect format returns err", func(t *testing.T) { + f, err := os.CreateTemp(t.TempDir(), "analytics-") + if err != nil { + t.Fatal("could not create temp file", err) + } + defer f.Close() + + if _, err := f.WriteString(`This is a regular file, not a configuration file!`); err != nil { + t.Fatal("could not write to temp file", err) + } + + _, err = loadConfigFromFile(f.Name()) + if err == nil { + t.Error("expected an error to be returned") + } + }) + + t.Run("unreadable file returns err", func(t *testing.T) { + f, err := os.CreateTemp(t.TempDir(), "analytics-") + if err != nil { + t.Fatal("could not create temp file", err) + } + defer f.Close() + + // remove read permissions from file + if err := f.Chmod(0222); err != nil { + t.Fatal("could not chmod temp file", err) + } + + _, err = loadConfigFromFile(f.Name()) + if err == nil { + t.Error("expected an error to be returned") + } + }) } -func TestLoadConfigFromFile_UnreadableFileReturnsErr(t *testing.T) { - f, err := os.CreateTemp(t.TempDir(), "analytics-") - if err != nil { - t.Fatal("could not create temp file", err) - } - defer f.Close() - - // remove read permissions from file - if err := f.Chmod(0222); err != nil { - t.Fatal("could not chmod temp file", err) - } - - _, err = loadConfigFromFile(f.Name()) - if err == nil { - t.Error("expected an error to be returned") - } +func TestWriteConfig(t *testing.T) { + t.Run("ulid", func(t *testing.T) { + path := filepath.Join(t.TempDir(), "nested", "deeply", ConfigFile) + + cfg := Config{UserID: ULID(ulidID)} + + if err := writeConfigToFile(path, cfg); err != nil { + t.Error("failed to create file", err) + } + + contents, err := os.ReadFile(path) + if err != nil { + t.Error("failed to read file", err) + } + + exp := fmt.Sprintf(`%sanonymous_user_id: %s +`, header, ulidID.String()) + + if d := cmp.Diff(exp, string(contents)); d != "" { + t.Error("contents do not match", d) + } + }) + + t.Run("uuid", func(t *testing.T) { + path := filepath.Join(t.TempDir(), "nested", "deeply", ConfigFile) + + cfg := Config{UserUUID: UUID(uuidID)} + + if err := writeConfigToFile(path, cfg); err != nil { + t.Error("failed to create file", err) + } + + contents, err := os.ReadFile(path) + if err != nil { + t.Error("failed to read file", err) + } + + exp := fmt.Sprintf(`%sanonymous_user_uuid: %s +`, header, uuidID.String()) + + if d := cmp.Diff(exp, string(contents)); d != "" { + t.Error("contents do not match", d) + } + }) + + t.Run("ulid and uuid", func(t *testing.T) { + path := filepath.Join(t.TempDir(), "nested", "deeply", ConfigFile) + + cfg := Config{ + UserID: ULID(ulidID), + UserUUID: UUID(uuidID), + } + + if err := writeConfigToFile(path, cfg); err != nil { + t.Error("failed to create file", err) + } + + contents, err := os.ReadFile(path) + if err != nil { + t.Error("failed to read file", err) + } + + exp := fmt.Sprintf(`%sanonymous_user_id: %s +anonymous_user_uuid: %s +`, header, ulidID.String(), uuidID.String()) + + if d := cmp.Diff(exp, string(contents)); d != "" { + t.Error("contents do not match", d) + } + }) } -func TestWriteConfigToFile(t *testing.T) { - path := filepath.Join(t.TempDir(), "nested", "deeply", ConfigFile) +func TestUUID(t *testing.T) { + t.Run("string", func(t *testing.T) { + uuid := uuid.New() + if d := cmp.Diff(36, len(uuid.String())); d != "" { + t.Error("uuid length mismatch", d) + } + }) - cfg := Config{UserID: ULID(id)} + t.Run("yaml marshal", func(t *testing.T) { + uuid := NewUUID() + s, err := yaml.Marshal(uuid) + if err != nil { + t.Error("failed to marshal uuid", err) + } + if d := cmp.Diff(uuid.String(), strings.TrimSpace(string(s))); d != "" { + t.Error("uuid values do not match", d) + } + }) - if err := writeConfigToFile(path, cfg); err != nil { - t.Error("failed to create file", err) - } + t.Run("yaml unmarshal", func(t *testing.T) { + var uuid UUID + if err := yaml.Unmarshal([]byte(NewUUID().String()), &uuid); err != nil { + t.Error("failed to unmarshal uuid", err) + } - contents, err := os.ReadFile(path) - if err != nil { - t.Error("failed to read file", err) - } + if d := cmp.Diff(36, len(uuid.String())); d != "" { + t.Error("uuid length mismatch", d) + } + }) - exp := fmt.Sprintf(`%sanonymous_user_id: %s -`, header, id.String()) + t.Run("isZero", func(t *testing.T) { + uuid := UUID(uuid.Nil) + if d := cmp.Diff(true, uuid.IsZero()); d != "" { + t.Error("uuid should zero", d) + } - if d := cmp.Diff(exp, string(contents)); d != "" { - t.Error("contents do not match", d) - } + uuid = NewUUID() + if d := cmp.Diff(false, uuid.IsZero()); d != "" { + t.Error("uuid should zero", d) + } + }) } diff --git a/internal/telemetry/segment.go b/internal/telemetry/segment.go index 5cc13e4..7f1aab5 100644 --- a/internal/telemetry/segment.go +++ b/internal/telemetry/segment.go @@ -5,7 +5,7 @@ import ( "context" "fmt" "github.com/airbytehq/abctl/internal/build" - "github.com/oklog/ulid/v2" + "github.com/google/uuid" "github.com/pbnjay/memory" "k8s.io/apimachinery/pkg/util/json" "maps" @@ -33,7 +33,7 @@ func WithHTTPClient(d Doer) Option { } // WithSessionID overrides the default ulid session, primarily for testing purposes. -func WithSessionID(sessionID ulid.ULID) Option { +func WithSessionID(sessionID uuid.UUID) Option { return func(client *SegmentClient) { client.sessionID = sessionID } @@ -42,7 +42,7 @@ func WithSessionID(sessionID ulid.ULID) Option { // SegmentClient client, all methods communicate with segment. type SegmentClient struct { doer Doer - sessionID ulid.ULID + sessionID uuid.UUID cfg Config attrs map[string]string } @@ -51,7 +51,7 @@ func NewSegmentClient(cfg Config, opts ...Option) *SegmentClient { cli := &SegmentClient{ doer: &http.Client{Timeout: 10 * time.Second}, cfg: cfg, - sessionID: ulid.Make(), + sessionID: uuid.New(), attrs: map[string]string{}, } @@ -103,7 +103,7 @@ func (s *SegmentClient) send(ctx context.Context, es EventState, et EventType, e } body := body{ - ID: s.cfg.UserID.String(), + ID: s.cfg.UserUUID.String(), Event: string(et), Properties: properties, Timestamp: time.Now().UTC().Format(time.RFC3339), diff --git a/internal/telemetry/segment_test.go b/internal/telemetry/segment_test.go index d9aade2..0c40fa2 100644 --- a/internal/telemetry/segment_test.go +++ b/internal/telemetry/segment_test.go @@ -7,7 +7,7 @@ import ( "github.com/airbytehq/abctl/internal/build" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" - "github.com/oklog/ulid/v2" + "github.com/google/uuid" "github.com/pbnjay/memory" "io" "net/http" @@ -18,8 +18,8 @@ import ( "time" ) -var userID = ulid.Make() -var sessionID = ulid.Make() +var userID = uuid.New() +var sessionID = uuid.New() func TestSegmentClient_Options(t *testing.T) { mDoer := &mockDoer{} @@ -53,7 +53,7 @@ func TestSegmentClient_Start(t *testing.T) { WithHTTPClient(mDoer), } - cli := NewSegmentClient(Config{UserID: ULID(userID)}, opts...) + cli := NewSegmentClient(Config{UserUUID: UUID(userID)}, opts...) ctx := context.Background() @@ -155,7 +155,7 @@ func TestSegmentClient_StartWithAttr(t *testing.T) { WithHTTPClient(mDoer), } - cli := NewSegmentClient(Config{UserID: ULID(userID)}, opts...) + cli := NewSegmentClient(Config{UserUUID: UUID(userID)}, opts...) cli.Attr("key1", "val1") cli.Attr("key2", "val2") @@ -264,7 +264,7 @@ func TestSegmentClient_StartErr(t *testing.T) { WithHTTPClient(mDoer), } - cli := NewSegmentClient(Config{UserID: ULID(userID)}, opts...) + cli := NewSegmentClient(Config{UserUUID: UUID(userID)}, opts...) ctx := context.Background() @@ -289,7 +289,7 @@ func TestSegmentClient_Success(t *testing.T) { WithHTTPClient(mDoer), } - cli := NewSegmentClient(Config{UserID: ULID(userID)}, opts...) + cli := NewSegmentClient(Config{UserUUID: UUID(userID)}, opts...) ctx := context.Background() @@ -391,7 +391,7 @@ func TestSegmentClient_SuccessWithAttr(t *testing.T) { WithHTTPClient(mDoer), } - cli := NewSegmentClient(Config{UserID: ULID(userID)}, opts...) + cli := NewSegmentClient(Config{UserUUID: UUID(userID)}, opts...) cli.Attr("key1", "val1") cli.Attr("key2", "val2") @@ -500,7 +500,7 @@ func TestSegmentClient_SuccessErr(t *testing.T) { WithHTTPClient(mDoer), } - cli := NewSegmentClient(Config{UserID: ULID(userID)}, opts...) + cli := NewSegmentClient(Config{UserUUID: UUID(userID)}, opts...) ctx := context.Background() @@ -525,7 +525,7 @@ func TestSegmentClient_Failure(t *testing.T) { WithHTTPClient(mDoer), } - cli := NewSegmentClient(Config{UserID: ULID(userID)}, opts...) + cli := NewSegmentClient(Config{UserUUID: UUID(userID)}, opts...) ctx := context.Background() failure := errors.New("failure reason") @@ -628,7 +628,7 @@ func TestSegmentClient_FailureWithAttr(t *testing.T) { WithHTTPClient(mDoer), } - cli := NewSegmentClient(Config{UserID: ULID(userID)}, opts...) + cli := NewSegmentClient(Config{UserUUID: UUID(userID)}, opts...) cli.Attr("key1", "val1") cli.Attr("key2", "val2") @@ -738,7 +738,7 @@ func TestSegmentClient_FailureErr(t *testing.T) { WithHTTPClient(mDoer), } - cli := NewSegmentClient(Config{UserID: ULID(userID)}, opts...) + cli := NewSegmentClient(Config{UserUUID: UUID(userID)}, opts...) ctx := context.Background() failure := errors.New("failure reason")