From 7a96cce93fc93bb83688f1335ee6d6882796348e Mon Sep 17 00:00:00 2001 From: Gary Holtz Date: Wed, 14 May 2025 18:00:24 -0500 Subject: [PATCH 01/10] chore: Add telemetry data for commands/projects/namespaces --- cmd/glab/main.go | 9 ++ commands/hooks/hooks.go | 88 ++++++++++++++ commands/hooks/hooks_test.go | 226 +++++++++++++++++++++++++++++++++++ 3 files changed, 323 insertions(+) create mode 100644 commands/hooks/hooks.go create mode 100644 commands/hooks/hooks_test.go diff --git a/cmd/glab/main.go b/cmd/glab/main.go index 15fc8b140..756b8eff6 100644 --- a/cmd/glab/main.go +++ b/cmd/glab/main.go @@ -20,6 +20,7 @@ import ( "gitlab.com/gitlab-org/cli/commands/alias/expand" "gitlab.com/gitlab-org/cli/commands/cmdutils" "gitlab.com/gitlab-org/cli/commands/help" + "gitlab.com/gitlab-org/cli/commands/hooks" "gitlab.com/gitlab-org/cli/commands/update" "gitlab.com/gitlab-org/cli/internal/config" "gitlab.com/gitlab-org/cli/internal/run" @@ -152,6 +153,8 @@ func main() { rootCmd.SetArgs(expandedArgs) + checkForTelemetryHook(cfg, cmdFactory, expandedArgs) + if cmd, err := rootCmd.ExecuteC(); err != nil { if !errors.Is(err, cmdutils.SilentError) { printError(cmdFactory.IO, err, cmd, debug) @@ -248,3 +251,9 @@ func maybeOverrideDefaultHost(f *cmdutils.Factory, cfg config.Config) { glinstance.OverrideDefault(customGLHost) } } + +func checkForTelemetryHook(cfg config.Config, f *cmdutils.Factory, args []string) { + if hooks.IsTelemetryEnabled(cfg) { + cobra.OnFinalize(hooks.AddTelemetryHook(f, args)) + } +} diff --git a/commands/hooks/hooks.go b/commands/hooks/hooks.go new file mode 100644 index 000000000..51b18941e --- /dev/null +++ b/commands/hooks/hooks.go @@ -0,0 +1,88 @@ +package hooks + +import ( + "strconv" + "strings" + + gitlab "gitlab.com/gitlab-org/api/client-go" + "gitlab.com/gitlab-org/cli/commands/cmdutils" + "gitlab.com/gitlab-org/cli/internal/config" +) + +func AddTelemetryHook(f *cmdutils.Factory, args []string) func() { + return func() { + var projectID string + var namespaceID string + + command, subcommand, fullCommand, _ := parseCommand(args) + + client, _ := f.HttpClient() + + repo, _ := f.BaseRepo() + + project, err := repo.Project(client) + if err == nil { + projectID = strconv.Itoa(project.ID) + namespaceID = strconv.Itoa(project.Namespace.ID) + } + + if client != nil { + _, _ = client.UsageData.TrackEvent(&gitlab.TrackEventOptions{ + Event: "gitlab_cli_command_used", + AdditionalProperties: map[string]string{ + "label": command, + "property": subcommand, + "command_and_subcommand": fullCommand, + "NamespaceID": projectID, + "ProjectID": namespaceID, + "SendToSnowplow": "true", + }, + }) + } + } +} + +// IsTelemetryEnabled checks if usage data is disabled via config or env var +func IsTelemetryEnabled(cfg config.Config) bool { + disableTelemetry, _ := cfg.Get("", "telemetry") + if disableTelemetry == "false" || disableTelemetry == "0" { + return false + } + + return true +} + +// parseCommand parses a command string and returns components +func parseCommand(parts []string) (command, subcommand, fullCommand, flags string) { + if len(parts) < 1 { + return "", "", "", "" + } + + command = parts[0] // command is the first part + + // where do the flags start? + flagStartIndex := len(parts) + for i := 1; i < len(parts); i++ { + if strings.HasPrefix(parts[i], "-") { + flagStartIndex = i + break + } + } + + if flagStartIndex > 1 { + subcommandParts := parts[1:flagStartIndex] + subcommand = strings.Join(subcommandParts, " ") + // everything after this is presumably flags/parameters + } + + fullCommand = command + if subcommand != "" { + fullCommand += " " + subcommand + } + + if flagStartIndex < len(parts) { + flags = strings.Join(parts[flagStartIndex:], " ") + } + + return command, subcommand, fullCommand, flags +} diff --git a/commands/hooks/hooks_test.go b/commands/hooks/hooks_test.go new file mode 100644 index 000000000..69cd2790e --- /dev/null +++ b/commands/hooks/hooks_test.go @@ -0,0 +1,226 @@ +package hooks + +import ( + "testing" + + "github.com/stretchr/testify/require" + gitlab "gitlab.com/gitlab-org/api/client-go" + gitlab_testing "gitlab.com/gitlab-org/api/client-go/testing" + "gitlab.com/gitlab-org/cli/commands/cmdtest" + "gitlab.com/gitlab-org/cli/commands/cmdutils" + "gitlab.com/gitlab-org/cli/internal/config" +) + +func TestSendCommandUsage(t *testing.T) { + tests := []struct { + name string + args []string + command string + subcommand string + fullCommand string + }{ + { + name: "command with args", + args: []string{"hey", "subcmd", "-p", "blah"}, + command: "hey", + subcommand: "subcmd", + fullCommand: "hey subcmd", + }, + { + name: "command with no subcommand", + args: []string{"issue", "-l", "bug"}, + command: "issue", + subcommand: "", + fullCommand: "issue", + }, + { + name: "command with multiple subcommands", + args: []string{"mr", "create", "new", "--title", "Test"}, + command: "mr", + subcommand: "create new", + fullCommand: "mr create new", + }, + { + name: "command with no flags", + args: []string{"ci", "status"}, + command: "ci", + subcommand: "status", + fullCommand: "ci status", + }, + { + name: "single command only", + args: []string{"version"}, + command: "version", + subcommand: "", + fullCommand: "version", + }, + { + name: "command with flags having values", + args: []string{"repo", "view", "--web", "--token", "abc123"}, + command: "repo", + subcommand: "view", + fullCommand: "repo view", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tc := gitlab_testing.NewTestClient(t) + ios, _, _, _ := cmdtest.InitIOStreams(true, "") + + f := &cmdutils.Factory{ + IO: ios, + HttpClient: func() (*gitlab.Client, error) { + return tc.Client, nil + }, + } + + hook := AddTelemetryHook(f, tt.args) + + tc.MockUsageData.EXPECT(). + TrackEvent(&gitlab.TrackEventOptions{ + Event: "gitlab_cli_command_used", + AdditionalProperties: map[string]string{ + "label": tt.command, + "property": tt.subcommand, + "command_and_subcommand": tt.fullCommand, + }, + }) + + hook() + }) + } +} + +func Test_parseCommand(t *testing.T) { + tests := []struct { + name string + cmdString []string + command string + subcommand string + fullCommand string + flags string + }{ + { + name: "basic command", + cmdString: []string{"mr", "list", "--web", "-R", "blah", "-p", "3"}, + command: "mr", + subcommand: "list", + fullCommand: "mr list", + flags: "--web -R blah -p 3", + }, + { + name: "multiple subcommands", + cmdString: []string{"mr", "list", "thing", "--web", "-R", "blah", "-p", "3"}, + command: "mr", + subcommand: "list thing", + fullCommand: "mr list thing", + flags: "--web -R blah -p 3", + }, + { + name: "no subcommand", + cmdString: []string{"mr", "--web", "-R", "blah", "-p", "3"}, + command: "mr", + subcommand: "", + fullCommand: "mr", + flags: "--web -R blah -p 3", + }, + { + name: "no flags", + cmdString: []string{"mr", "list", "thing"}, + command: "mr", + subcommand: "list thing", + fullCommand: "mr list thing", + flags: "", + }, + { + name: "too short of a command", + cmdString: []string{}, + command: "", + subcommand: "", + fullCommand: "", + flags: "", + }, + { + name: "many subcommands", + cmdString: []string{"mr", "list", "thing", "a", "b", "c", "--flag"}, + command: "mr", + subcommand: "list thing a b c", + fullCommand: "mr list thing a b c", + flags: "--flag", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + command, subcommand, fullCommand, flags := parseCommand(tt.cmdString) + + require := require.New(t) + require.Equal(tt.command, command) + require.Equal(tt.subcommand, subcommand) + require.Equal(tt.fullCommand, fullCommand) + require.Equal(tt.flags, flags) + }) + } +} + +func TestIsTelemetryEnabled(t *testing.T) { + tests := []struct { + name string + configYaml string + expectedResult bool + }{ + { + name: "enabled with 'true' value", + configYaml: "telemetry: true", + expectedResult: true, + }, + { + name: "enabled with '1' value", + configYaml: "telemetry: '1'", + expectedResult: true, + }, + { + name: "disabled with 'false' value", + configYaml: "telemetry: false", + expectedResult: false, + }, + { + name: "disabled with '0' value", + configYaml: "telemetry: '0'", + expectedResult: false, + }, + { + name: "enabled with empty value", + configYaml: "telemetry: ''", + expectedResult: true, + }, + { + name: "enabled with other value", + configYaml: "telemetry: something", + expectedResult: true, + }, + { + name: "no config value set", + expectedResult: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + restore := config.StubConfig(tt.configYaml, "") + defer restore() + + cfg, err := config.ParseConfig("config.yml") + if tt.configYaml == "" { + cfg = config.NewBlankConfig() + } else { + require.NoError(t, err) + } + + result := IsTelemetryEnabled(cfg) + + require.Equal(t, tt.expectedResult, result) + }) + } +} -- GitLab From d6da57125c51e769cd35cc735ee7bd9d6afabdbe Mon Sep 17 00:00:00 2001 From: Gary Holtz Date: Wed, 14 May 2025 20:38:03 -0500 Subject: [PATCH 02/10] Swap namespace and project --- commands/hooks/hooks.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/commands/hooks/hooks.go b/commands/hooks/hooks.go index 51b18941e..90b7ab097 100644 --- a/commands/hooks/hooks.go +++ b/commands/hooks/hooks.go @@ -33,8 +33,8 @@ func AddTelemetryHook(f *cmdutils.Factory, args []string) func() { "label": command, "property": subcommand, "command_and_subcommand": fullCommand, - "NamespaceID": projectID, - "ProjectID": namespaceID, + "NamespaceID": namespaceID, + "ProjectID": projectID, "SendToSnowplow": "true", }, }) -- GitLab From 2878a6073978e750e620af0f612ee25119c956f0 Mon Sep 17 00:00:00 2001 From: Gary Holtz Date: Thu, 15 May 2025 14:08:41 -0500 Subject: [PATCH 03/10] Fixing up issues --- commands/hooks/hooks.go | 17 ++++++++--------- commands/hooks/hooks_test.go | 20 +++++++++++++++++++- 2 files changed, 27 insertions(+), 10 deletions(-) diff --git a/commands/hooks/hooks.go b/commands/hooks/hooks.go index 90b7ab097..200115d56 100644 --- a/commands/hooks/hooks.go +++ b/commands/hooks/hooks.go @@ -1,7 +1,6 @@ package hooks import ( - "strconv" "strings" gitlab "gitlab.com/gitlab-org/api/client-go" @@ -11,8 +10,8 @@ import ( func AddTelemetryHook(f *cmdutils.Factory, args []string) func() { return func() { - var projectID string - var namespaceID string + var projectID int + var namespaceID int command, subcommand, fullCommand, _ := parseCommand(args) @@ -22,20 +21,20 @@ func AddTelemetryHook(f *cmdutils.Factory, args []string) func() { project, err := repo.Project(client) if err == nil { - projectID = strconv.Itoa(project.ID) - namespaceID = strconv.Itoa(project.Namespace.ID) + projectID = project.ID + namespaceID = project.Namespace.ID } if client != nil { _, _ = client.UsageData.TrackEvent(&gitlab.TrackEventOptions{ - Event: "gitlab_cli_command_used", + Event: "gitlab_cli_command_used", + NamespaceID: gitlab.Ptr(namespaceID), + ProjectID: gitlab.Ptr(projectID), + SendToSnowplow: gitlab.Ptr(true), AdditionalProperties: map[string]string{ "label": command, "property": subcommand, "command_and_subcommand": fullCommand, - "NamespaceID": namespaceID, - "ProjectID": projectID, - "SendToSnowplow": "true", }, }) } diff --git a/commands/hooks/hooks_test.go b/commands/hooks/hooks_test.go index 69cd2790e..8e5c15cb1 100644 --- a/commands/hooks/hooks_test.go +++ b/commands/hooks/hooks_test.go @@ -6,9 +6,12 @@ import ( "github.com/stretchr/testify/require" gitlab "gitlab.com/gitlab-org/api/client-go" gitlab_testing "gitlab.com/gitlab-org/api/client-go/testing" + "go.uber.org/mock/gomock" + "gitlab.com/gitlab-org/cli/commands/cmdtest" "gitlab.com/gitlab-org/cli/commands/cmdutils" "gitlab.com/gitlab-org/cli/internal/config" + "gitlab.com/gitlab-org/cli/internal/glrepo" ) func TestSendCommandUsage(t *testing.T) { @@ -73,13 +76,28 @@ func TestSendCommandUsage(t *testing.T) { HttpClient: func() (*gitlab.Client, error) { return tc.Client, nil }, + BaseRepo: func() (glrepo.Interface, error) { + return glrepo.New("OWNER", "REPO"), nil + }, } hook := AddTelemetryHook(f, tt.args) + project := gitlab.Project{ + ID: 123, + Namespace: &gitlab.ProjectNamespace{ID: 123}, + } + + tc.MockProjects.EXPECT(). + GetProject(gomock.Any(), gomock.Any(), gomock.Any()). + Return(&project, &gitlab.Response{}, nil) + tc.MockUsageData.EXPECT(). TrackEvent(&gitlab.TrackEventOptions{ - Event: "gitlab_cli_command_used", + Event: "gitlab_cli_command_used", + NamespaceID: gitlab.Ptr(project.Namespace.ID), + ProjectID: gitlab.Ptr(project.ID), + SendToSnowplow: gitlab.Ptr(true), AdditionalProperties: map[string]string{ "label": tt.command, "property": tt.subcommand, -- GitLab From a54af67641dcc42d4404329440d2f860f9b81ef5 Mon Sep 17 00:00:00 2001 From: Gary Holtz Date: Thu, 15 May 2025 15:31:32 -0500 Subject: [PATCH 04/10] Updating stub test --- internal/config/config_stub.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/internal/config/config_stub.go b/internal/config/config_stub.go index a430df68e..97351254c 100644 --- a/internal/config/config_stub.go +++ b/internal/config/config_stub.go @@ -95,6 +95,15 @@ func rootConfig() *yaml.Node { Kind: yaml.ScalarNode, Value: "false", }, + { + HeadComment: "# Set to false (0) to disable sending usage data to GitLab or true (1) to enable.\n# See https://docs.gitlab.com/administration/settings/usage_statistics/\n# for more information", + Kind: yaml.ScalarNode, + Value: "telemetry", + }, + { + Kind: yaml.ScalarNode, + Value: "true", + }, { HeadComment: "# Configuration specific for GitLab instances.", Kind: yaml.ScalarNode, -- GitLab From ceaac0c8ab272542ea23ce63077c9be15a372986 Mon Sep 17 00:00:00 2001 From: Gary Holtz Date: Thu, 15 May 2025 23:13:26 -0500 Subject: [PATCH 05/10] Updates from review --- commands/hooks/hooks.go | 63 +++++++++++++++++++----------------- commands/hooks/hooks_test.go | 8 +++++ 2 files changed, 41 insertions(+), 30 deletions(-) diff --git a/commands/hooks/hooks.go b/commands/hooks/hooks.go index 200115d56..40af822a6 100644 --- a/commands/hooks/hooks.go +++ b/commands/hooks/hooks.go @@ -10,34 +10,36 @@ import ( func AddTelemetryHook(f *cmdutils.Factory, args []string) func() { return func() { - var projectID int - var namespaceID int - - command, subcommand, fullCommand, _ := parseCommand(args) - - client, _ := f.HttpClient() - - repo, _ := f.BaseRepo() - - project, err := repo.Project(client) - if err == nil { - projectID = project.ID - namespaceID = project.Namespace.ID - } - - if client != nil { - _, _ = client.UsageData.TrackEvent(&gitlab.TrackEventOptions{ - Event: "gitlab_cli_command_used", - NamespaceID: gitlab.Ptr(namespaceID), - ProjectID: gitlab.Ptr(projectID), - SendToSnowplow: gitlab.Ptr(true), - AdditionalProperties: map[string]string{ - "label": command, - "property": subcommand, - "command_and_subcommand": fullCommand, - }, - }) - } + go func() { + var projectID int + var namespaceID int + + command, subcommand, fullCommand, _ := parseCommand(args) + + client, _ := f.HttpClient() + + repo, _ := f.BaseRepo() + + project, err := repo.Project(client) + if err == nil { + projectID = project.ID + namespaceID = project.Namespace.ID + } + + if client != nil { + _, _ = client.UsageData.TrackEvent(&gitlab.TrackEventOptions{ + Event: "gitlab_cli_command_used", + NamespaceID: gitlab.Ptr(namespaceID), + ProjectID: gitlab.Ptr(projectID), + SendToSnowplow: gitlab.Ptr(true), + AdditionalProperties: map[string]string{ + "label": command, + "property": subcommand, + "command_and_subcommand": fullCommand, + }, + }) + } + }() } } @@ -59,10 +61,11 @@ func parseCommand(parts []string) (command, subcommand, fullCommand, flags strin command = parts[0] // command is the first part - // where do the flags start? + // where do the flags/parameters start? flagStartIndex := len(parts) for i := 1; i < len(parts); i++ { - if strings.HasPrefix(parts[i], "-") { + // check for things in "quotes" or -flags + if strings.HasPrefix(parts[i], "-") || strings.Contains(parts[i], "\"") { flagStartIndex = i break } diff --git a/commands/hooks/hooks_test.go b/commands/hooks/hooks_test.go index 8e5c15cb1..a55e60d3e 100644 --- a/commands/hooks/hooks_test.go +++ b/commands/hooks/hooks_test.go @@ -135,6 +135,14 @@ func Test_parseCommand(t *testing.T) { fullCommand: "mr list thing", flags: "--web -R blah -p 3", }, + { + name: "strip out parameters", + cmdString: []string{"mr", "list", "thing", "\"also\"", "--web", "-R", "blah", "-p", "3"}, + command: "mr", + subcommand: "list thing", + fullCommand: "mr list thing", + flags: "\"also\" --web -R blah -p 3", + }, { name: "no subcommand", cmdString: []string{"mr", "--web", "-R", "blah", "-p", "3"}, -- GitLab From 0cf43a8d9e8fd0ffbb16866339949504a1a53a34 Mon Sep 17 00:00:00 2001 From: Gary Holtz Date: Fri, 16 May 2025 00:30:29 -0500 Subject: [PATCH 06/10] Refactoring to make goroutine function testable --- commands/hooks/hooks.go | 62 +++++++++++++++++++----------------- commands/hooks/hooks_test.go | 6 ++-- 2 files changed, 34 insertions(+), 34 deletions(-) diff --git a/commands/hooks/hooks.go b/commands/hooks/hooks.go index 40af822a6..ee1189913 100644 --- a/commands/hooks/hooks.go +++ b/commands/hooks/hooks.go @@ -10,36 +10,7 @@ import ( func AddTelemetryHook(f *cmdutils.Factory, args []string) func() { return func() { - go func() { - var projectID int - var namespaceID int - - command, subcommand, fullCommand, _ := parseCommand(args) - - client, _ := f.HttpClient() - - repo, _ := f.BaseRepo() - - project, err := repo.Project(client) - if err == nil { - projectID = project.ID - namespaceID = project.Namespace.ID - } - - if client != nil { - _, _ = client.UsageData.TrackEvent(&gitlab.TrackEventOptions{ - Event: "gitlab_cli_command_used", - NamespaceID: gitlab.Ptr(namespaceID), - ProjectID: gitlab.Ptr(projectID), - SendToSnowplow: gitlab.Ptr(true), - AdditionalProperties: map[string]string{ - "label": command, - "property": subcommand, - "command_and_subcommand": fullCommand, - }, - }) - } - }() + go sendTelemetryData(f, args) } } @@ -88,3 +59,34 @@ func parseCommand(parts []string) (command, subcommand, fullCommand, flags strin return command, subcommand, fullCommand, flags } + +func sendTelemetryData(f *cmdutils.Factory, args []string) { + var projectID int + var namespaceID int + + command, subcommand, fullCommand, _ := parseCommand(args) + + client, _ := f.HttpClient() + + repo, _ := f.BaseRepo() + + project, err := repo.Project(client) + if err == nil { + projectID = project.ID + namespaceID = project.Namespace.ID + } + + if client != nil { + _, _ = client.UsageData.TrackEvent(&gitlab.TrackEventOptions{ + Event: "gitlab_cli_command_used", + NamespaceID: gitlab.Ptr(namespaceID), + ProjectID: gitlab.Ptr(projectID), + SendToSnowplow: gitlab.Ptr(true), + AdditionalProperties: map[string]string{ + "label": command, + "property": subcommand, + "command_and_subcommand": fullCommand, + }, + }) + } +} diff --git a/commands/hooks/hooks_test.go b/commands/hooks/hooks_test.go index a55e60d3e..f60efba2b 100644 --- a/commands/hooks/hooks_test.go +++ b/commands/hooks/hooks_test.go @@ -14,7 +14,7 @@ import ( "gitlab.com/gitlab-org/cli/internal/glrepo" ) -func TestSendCommandUsage(t *testing.T) { +func Test_sendTelemetryData(t *testing.T) { tests := []struct { name string args []string @@ -81,8 +81,6 @@ func TestSendCommandUsage(t *testing.T) { }, } - hook := AddTelemetryHook(f, tt.args) - project := gitlab.Project{ ID: 123, Namespace: &gitlab.ProjectNamespace{ID: 123}, @@ -105,7 +103,7 @@ func TestSendCommandUsage(t *testing.T) { }, }) - hook() + sendTelemetryData(f, tt.args) }) } } -- GitLab From 014a1262f2c7d031ed3926253263d438b3f64f04 Mon Sep 17 00:00:00 2001 From: Gary Holtz Date: Wed, 21 May 2025 15:34:56 -0500 Subject: [PATCH 07/10] Strip out singles quote too --- commands/hooks/hooks.go | 2 +- commands/hooks/hooks_test.go | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/commands/hooks/hooks.go b/commands/hooks/hooks.go index ee1189913..51f50454e 100644 --- a/commands/hooks/hooks.go +++ b/commands/hooks/hooks.go @@ -36,7 +36,7 @@ func parseCommand(parts []string) (command, subcommand, fullCommand, flags strin flagStartIndex := len(parts) for i := 1; i < len(parts); i++ { // check for things in "quotes" or -flags - if strings.HasPrefix(parts[i], "-") || strings.Contains(parts[i], "\"") { + if strings.HasPrefix(parts[i], "-") || strings.Contains(parts[i], "\"") || strings.Contains(parts[i], "'") { flagStartIndex = i break } diff --git a/commands/hooks/hooks_test.go b/commands/hooks/hooks_test.go index f60efba2b..cfe2aef71 100644 --- a/commands/hooks/hooks_test.go +++ b/commands/hooks/hooks_test.go @@ -141,6 +141,14 @@ func Test_parseCommand(t *testing.T) { fullCommand: "mr list thing", flags: "\"also\" --web -R blah -p 3", }, + { + name: "strip out single quote parameters", + cmdString: []string{"mr", "list", "thing", "'also'"}, + command: "mr", + subcommand: "list thing", + fullCommand: "mr list thing", + flags: "'also'", + }, { name: "no subcommand", cmdString: []string{"mr", "--web", "-R", "blah", "-p", "3"}, -- GitLab From fc98967898377f389051b82d7e2a3cc566089120 Mon Sep 17 00:00:00 2001 From: Gary Holtz Date: Thu, 29 May 2025 01:11:43 -0500 Subject: [PATCH 08/10] Refactoring to simplify command parsing --- cmd/glab/main.go | 9 +-- commands/hooks/hooks.go | 42 +++++-------- commands/hooks/hooks_test.go | 119 +++++++++++++---------------------- 3 files changed, 62 insertions(+), 108 deletions(-) diff --git a/cmd/glab/main.go b/cmd/glab/main.go index 756b8eff6..e7b461a58 100644 --- a/cmd/glab/main.go +++ b/cmd/glab/main.go @@ -110,6 +110,9 @@ func main() { } cmd, _, err := rootCmd.Traverse(expandedArgs) + + checkForTelemetryHook(cfg, cmdFactory, cmd) + if err != nil || cmd == rootCmd { originalArgs := expandedArgs isShell := false @@ -153,8 +156,6 @@ func main() { rootCmd.SetArgs(expandedArgs) - checkForTelemetryHook(cfg, cmdFactory, expandedArgs) - if cmd, err := rootCmd.ExecuteC(); err != nil { if !errors.Is(err, cmdutils.SilentError) { printError(cmdFactory.IO, err, cmd, debug) @@ -252,8 +253,8 @@ func maybeOverrideDefaultHost(f *cmdutils.Factory, cfg config.Config) { } } -func checkForTelemetryHook(cfg config.Config, f *cmdutils.Factory, args []string) { +func checkForTelemetryHook(cfg config.Config, f *cmdutils.Factory, cmd *cobra.Command) { if hooks.IsTelemetryEnabled(cfg) { - cobra.OnFinalize(hooks.AddTelemetryHook(f, args)) + cobra.OnFinalize(hooks.AddTelemetryHook(f, cmd)) } } diff --git a/commands/hooks/hooks.go b/commands/hooks/hooks.go index 51f50454e..dd0031ad7 100644 --- a/commands/hooks/hooks.go +++ b/commands/hooks/hooks.go @@ -3,14 +3,15 @@ package hooks import ( "strings" + "github.com/spf13/cobra" gitlab "gitlab.com/gitlab-org/api/client-go" "gitlab.com/gitlab-org/cli/commands/cmdutils" "gitlab.com/gitlab-org/cli/internal/config" ) -func AddTelemetryHook(f *cmdutils.Factory, args []string) func() { +func AddTelemetryHook(f *cmdutils.Factory, cmd *cobra.Command) func() { return func() { - go sendTelemetryData(f, args) + go sendTelemetryData(f, cmd) } } @@ -25,46 +26,31 @@ func IsTelemetryEnabled(cfg config.Config) bool { } // parseCommand parses a command string and returns components -func parseCommand(parts []string) (command, subcommand, fullCommand, flags string) { - if len(parts) < 1 { - return "", "", "", "" +func parseCommand(parts []string) (command, subcommand, fullCommand string) { + if len(parts) < 2 { + return "", "", "" } - command = parts[0] // command is the first part + // glab is always the first value, command is the next + command = parts[1] - // where do the flags/parameters start? - flagStartIndex := len(parts) - for i := 1; i < len(parts); i++ { - // check for things in "quotes" or -flags - if strings.HasPrefix(parts[i], "-") || strings.Contains(parts[i], "\"") || strings.Contains(parts[i], "'") { - flagStartIndex = i - break - } - } - - if flagStartIndex > 1 { - subcommandParts := parts[1:flagStartIndex] - subcommand = strings.Join(subcommandParts, " ") - // everything after this is presumably flags/parameters - } + subcommandParts := parts[2:] + subcommand = strings.Join(subcommandParts, " ") fullCommand = command if subcommand != "" { fullCommand += " " + subcommand } - if flagStartIndex < len(parts) { - flags = strings.Join(parts[flagStartIndex:], " ") - } - - return command, subcommand, fullCommand, flags + return command, subcommand, fullCommand } -func sendTelemetryData(f *cmdutils.Factory, args []string) { +func sendTelemetryData(f *cmdutils.Factory, cmd *cobra.Command) { var projectID int var namespaceID int + unparsedCommand := strings.Split(cmd.CommandPath(), " ") - command, subcommand, fullCommand, _ := parseCommand(args) + command, subcommand, fullCommand := parseCommand(unparsedCommand) client, _ := f.HttpClient() diff --git a/commands/hooks/hooks_test.go b/commands/hooks/hooks_test.go index cfe2aef71..0b962bbc5 100644 --- a/commands/hooks/hooks_test.go +++ b/commands/hooks/hooks_test.go @@ -3,6 +3,7 @@ package hooks import ( "testing" + "github.com/spf13/cobra" "github.com/stretchr/testify/require" gitlab "gitlab.com/gitlab-org/api/client-go" gitlab_testing "gitlab.com/gitlab-org/api/client-go/testing" @@ -18,52 +19,45 @@ func Test_sendTelemetryData(t *testing.T) { tests := []struct { name string args []string + cobraMocks []*cobra.Command command string subcommand string fullCommand string }{ { - name: "command with args", - args: []string{"hey", "subcmd", "-p", "blah"}, - command: "hey", - subcommand: "subcmd", - fullCommand: "hey subcmd", - }, - { - name: "command with no subcommand", - args: []string{"issue", "-l", "bug"}, - command: "issue", - subcommand: "", - fullCommand: "issue", + name: "command with subcommand", + cobraMocks: []*cobra.Command{ + {Use: "glab"}, + {Use: "mr"}, + {Use: "view"}, + }, + command: "mr", + subcommand: "view", + fullCommand: "mr view", }, { - name: "command with multiple subcommands", - args: []string{"mr", "create", "new", "--title", "Test"}, + name: "command with multiple subcommands", + cobraMocks: []*cobra.Command{ + {Use: "glab"}, + {Use: "mr"}, + {Use: "create"}, + {Use: "new"}, + }, + args: []string{"glab", "mr", "create", "new"}, command: "mr", subcommand: "create new", fullCommand: "mr create new", }, { - name: "command with no flags", - args: []string{"ci", "status"}, - command: "ci", - subcommand: "status", - fullCommand: "ci status", - }, - { - name: "single command only", - args: []string{"version"}, + name: "single command only", + cobraMocks: []*cobra.Command{ + {Use: "glab"}, + {Use: "version"}, + }, command: "version", subcommand: "", fullCommand: "version", }, - { - name: "command with flags having values", - args: []string{"repo", "view", "--web", "--token", "abc123"}, - command: "repo", - subcommand: "view", - fullCommand: "repo view", - }, } for _, tt := range tests { @@ -103,7 +97,18 @@ func Test_sendTelemetryData(t *testing.T) { }, }) - sendTelemetryData(f, tt.args) + passedCommand := tt.cobraMocks[0] + numberOfCommands := len(tt.cobraMocks) + + for i, cmd := range tt.cobraMocks { + if i < numberOfCommands && i > 0 { + tt.cobraMocks[i-1].AddCommand(cmd) + + passedCommand = cmd + } + } + + sendTelemetryData(f, passedCommand) }) } } @@ -115,83 +120,45 @@ func Test_parseCommand(t *testing.T) { command string subcommand string fullCommand string - flags string }{ { name: "basic command", - cmdString: []string{"mr", "list", "--web", "-R", "blah", "-p", "3"}, + cmdString: []string{"glab", "mr", "list"}, command: "mr", subcommand: "list", fullCommand: "mr list", - flags: "--web -R blah -p 3", }, { name: "multiple subcommands", - cmdString: []string{"mr", "list", "thing", "--web", "-R", "blah", "-p", "3"}, - command: "mr", - subcommand: "list thing", - fullCommand: "mr list thing", - flags: "--web -R blah -p 3", - }, - { - name: "strip out parameters", - cmdString: []string{"mr", "list", "thing", "\"also\"", "--web", "-R", "blah", "-p", "3"}, - command: "mr", - subcommand: "list thing", - fullCommand: "mr list thing", - flags: "\"also\" --web -R blah -p 3", - }, - { - name: "strip out single quote parameters", - cmdString: []string{"mr", "list", "thing", "'also'"}, + cmdString: []string{"glab", "mr", "list", "thing", "blah"}, command: "mr", - subcommand: "list thing", - fullCommand: "mr list thing", - flags: "'also'", + subcommand: "list thing blah", + fullCommand: "mr list thing blah", }, { name: "no subcommand", - cmdString: []string{"mr", "--web", "-R", "blah", "-p", "3"}, + cmdString: []string{"glab", "mr"}, command: "mr", subcommand: "", fullCommand: "mr", - flags: "--web -R blah -p 3", - }, - { - name: "no flags", - cmdString: []string{"mr", "list", "thing"}, - command: "mr", - subcommand: "list thing", - fullCommand: "mr list thing", - flags: "", }, { name: "too short of a command", - cmdString: []string{}, + cmdString: []string{"glab"}, command: "", subcommand: "", fullCommand: "", - flags: "", - }, - { - name: "many subcommands", - cmdString: []string{"mr", "list", "thing", "a", "b", "c", "--flag"}, - command: "mr", - subcommand: "list thing a b c", - fullCommand: "mr list thing a b c", - flags: "--flag", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - command, subcommand, fullCommand, flags := parseCommand(tt.cmdString) + command, subcommand, fullCommand := parseCommand(tt.cmdString) require := require.New(t) require.Equal(tt.command, command) require.Equal(tt.subcommand, subcommand) require.Equal(tt.fullCommand, fullCommand) - require.Equal(tt.flags, flags) }) } } -- GitLab From 7a22d3554033898ff2783e493c3fdbbe38243527 Mon Sep 17 00:00:00 2001 From: Gary Holtz Date: Thu, 29 May 2025 01:12:41 -0500 Subject: [PATCH 09/10] Rename variable --- commands/hooks/hooks.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/commands/hooks/hooks.go b/commands/hooks/hooks.go index dd0031ad7..317e272c3 100644 --- a/commands/hooks/hooks.go +++ b/commands/hooks/hooks.go @@ -17,8 +17,8 @@ func AddTelemetryHook(f *cmdutils.Factory, cmd *cobra.Command) func() { // IsTelemetryEnabled checks if usage data is disabled via config or env var func IsTelemetryEnabled(cfg config.Config) bool { - disableTelemetry, _ := cfg.Get("", "telemetry") - if disableTelemetry == "false" || disableTelemetry == "0" { + telemetryEnabled, _ := cfg.Get("", "telemetry") + if telemetryEnabled == "false" || telemetryEnabled == "0" { return false } -- GitLab From a53042830090ea4d99c7bbcaebf35efe1738657d Mon Sep 17 00:00:00 2001 From: Gary Holtz Date: Thu, 29 May 2025 11:59:25 -0500 Subject: [PATCH 10/10] Clarify subcommands in tests --- commands/hooks/hooks_test.go | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/commands/hooks/hooks_test.go b/commands/hooks/hooks_test.go index 0b962bbc5..0e1847977 100644 --- a/commands/hooks/hooks_test.go +++ b/commands/hooks/hooks_test.go @@ -39,14 +39,14 @@ func Test_sendTelemetryData(t *testing.T) { name: "command with multiple subcommands", cobraMocks: []*cobra.Command{ {Use: "glab"}, - {Use: "mr"}, - {Use: "create"}, - {Use: "new"}, + {Use: "command"}, + {Use: "subcommand1"}, + {Use: "subcommand2"}, }, - args: []string{"glab", "mr", "create", "new"}, - command: "mr", - subcommand: "create new", - fullCommand: "mr create new", + args: []string{"glab", "command", "subcommand1", "subcommand2"}, + command: "command", + subcommand: "subcommand1 subcommand2", + fullCommand: "command subcommand1 subcommand2", }, { name: "single command only", @@ -130,10 +130,10 @@ func Test_parseCommand(t *testing.T) { }, { name: "multiple subcommands", - cmdString: []string{"glab", "mr", "list", "thing", "blah"}, - command: "mr", - subcommand: "list thing blah", - fullCommand: "mr list thing blah", + cmdString: []string{"glab", "command", "subcommand1", "subcommand2", "subcommand3"}, + command: "command", + subcommand: "subcommand1 subcommand2 subcommand3", + fullCommand: "command subcommand1 subcommand2 subcommand3", }, { name: "no subcommand", -- GitLab