Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/browser_pools.go
Original file line number Diff line number Diff line change
Expand Up @@ -742,7 +742,7 @@ func runBrowserPoolsAcquire(cmd *cobra.Command, args []string) error {
client := getKernelClient(cmd)
timeout, _ := cmd.Flags().GetInt64("timeout")
name, _ := cmd.Flags().GetString("name")
tags := tagsFromFlag(cmd, "tag")
tags, _ := tagsFromFlag(cmd, "tag")
output, _ := cmd.Flags().GetString("output")
c := BrowserPoolsCmd{client: &client.BrowserPools}
return c.Acquire(cmd.Context(), BrowserPoolsAcquireInput{
Expand Down
30 changes: 16 additions & 14 deletions cmd/browsers.go
Original file line number Diff line number Diff line change
Expand Up @@ -216,20 +216,22 @@ func parseKeyValueSpecs(specs []string) (map[string]string, []string) {
}

// tagsFromFlag reads a repeated KEY=VALUE flag and parses it into a map,
// warning about any malformed entries. Returns nil when no valid tags are set.
func tagsFromFlag(cmd *cobra.Command, flagName string) map[string]string {
// warning about any malformed entries. It returns the parsed tags (nil when no
// valid pairs are set) and whether the flag was provided at least once,
// independent of whether its values parsed.
func tagsFromFlag(cmd *cobra.Command, flagName string) (map[string]string, bool) {
specs, _ := cmd.Flags().GetStringArray(flagName)
if len(specs) == 0 {
return nil
return nil, false
Comment thread
cursor[bot] marked this conversation as resolved.
Outdated
}
tags, malformed := parseKeyValueSpecs(specs)
for _, invalid := range malformed {
pterm.Warning.Printf("Ignoring malformed tag: %s\n", invalid)
}
if len(tags) == 0 {
return nil
return nil, true // provided, but all values malformed
}
return tags
return tags, true
}

// formatTags renders tags as a deterministic "k=v, k2=v2" string with keys
Expand Down Expand Up @@ -302,7 +304,7 @@ type BrowsersUpdateInput struct {
SetName bool
ClearName bool
Tags map[string]string
SetTags bool
TagsProvided bool
ClearTags bool
Output string
}
Expand Down Expand Up @@ -708,16 +710,16 @@ func (b BrowsersCmd) Update(ctx context.Context, in BrowsersUpdateInput) error {
return fmt.Errorf("cannot specify both --name and --clear-name")
}

// Cannot specify both --tag and --clear-tags. Use SetTags (the raw flag
// signal) as well as the parsed map so the check still fires when every
// Cannot specify both --tag and --clear-tags. TagsProvided (whether the flag
// was passed) is the authoritative signal so the check still fires when every
// --tag value was malformed and dropped to an empty map.
if (in.SetTags || len(in.Tags) > 0) && in.ClearTags {
if (in.TagsProvided || len(in.Tags) > 0) && in.ClearTags {
return fmt.Errorf("cannot specify both --tag and --clear-tags")
}

// --tag was provided but parsed to zero valid pairs (every value malformed).
// Treat as a user error rather than silently leaving tags unchanged.
if in.SetTags && len(in.Tags) == 0 {
if in.TagsProvided && len(in.Tags) == 0 {
return fmt.Errorf("no valid --tag KEY=VALUE pairs provided")
}

Expand Down Expand Up @@ -2761,7 +2763,7 @@ func runBrowsersList(cmd *cobra.Command, args []string) error {
limit, _ := cmd.Flags().GetInt("limit")
offset, _ := cmd.Flags().GetInt("offset")
query, _ := cmd.Flags().GetString("query")
tags := tagsFromFlag(cmd, "tag")
tags, _ := tagsFromFlag(cmd, "tag")
return b.List(cmd.Context(), BrowsersListInput{
Output: out,
IncludeDeleted: includeDeleted,
Expand Down Expand Up @@ -2814,7 +2816,7 @@ func runBrowsersCreate(cmd *cobra.Command, args []string) error {
poolName, _ := cmd.Flags().GetString("pool-name")
telemetry, _ := cmd.Flags().GetString("telemetry")
name, _ := cmd.Flags().GetString("name")
tags := tagsFromFlag(cmd, "tag")
tags, _ := tagsFromFlag(cmd, "tag")
chromePolicy, _ := cmd.Flags().GetString("chrome-policy")
chromePolicyFile, _ := cmd.Flags().GetString("chrome-policy-file")
output, _ := cmd.Flags().GetString("output")
Expand Down Expand Up @@ -2986,7 +2988,7 @@ func runBrowsersUpdate(cmd *cobra.Command, args []string) error {
telemetry, _ := cmd.Flags().GetString("telemetry")
name, _ := cmd.Flags().GetString("name")
clearName, _ := cmd.Flags().GetBool("clear-name")
tags := tagsFromFlag(cmd, "tag")
tags, tagsProvided := tagsFromFlag(cmd, "tag")
clearTags, _ := cmd.Flags().GetBool("clear-tags")

svc := client.Browsers
Expand All @@ -3005,7 +3007,7 @@ func runBrowsersUpdate(cmd *cobra.Command, args []string) error {
SetName: cmd.Flags().Changed("name"),
ClearName: clearName,
Tags: tags,
SetTags: cmd.Flags().Changed("tag"),
TagsProvided: tagsProvided,
ClearTags: clearTags,
Output: out,
})
Expand Down
60 changes: 42 additions & 18 deletions cmd/browsers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -699,12 +699,36 @@ func TestTagsFromFlag_WarnsOnMalformed(t *testing.T) {
cmd := &cobra.Command{}
cmd.Flags().StringArray("tag", []string{"team=backend", "oops", "env=staging"}, "")

tags := tagsFromFlag(cmd, "tag")
tags, provided := tagsFromFlag(cmd, "tag")

assert.True(t, provided)
assert.Equal(t, map[string]string{"team": "backend", "env": "staging"}, tags)
assert.Contains(t, outBuf.String(), "Ignoring malformed tag: oops")
}

func TestTagsFromFlag_NotProvided_ReportsFalse(t *testing.T) {
cmd := &cobra.Command{}
cmd.Flags().StringArray("tag", nil, "")

tags, provided := tagsFromFlag(cmd, "tag")

assert.False(t, provided)
assert.Nil(t, tags)
}

func TestTagsFromFlag_AllMalformed_ReportsProvidedWithNoTags(t *testing.T) {
setupStdoutCapture(t)

cmd := &cobra.Command{}
cmd.Flags().StringArray("tag", []string{"foo"}, "")

tags, provided := tagsFromFlag(cmd, "tag")

assert.True(t, provided)
assert.Empty(t, tags)
assert.Contains(t, outBuf.String(), "Ignoring malformed tag: foo")
}

func TestBrowsersGet_JSONOutput_IncludesNameAndTags(t *testing.T) {
setupStdoutCapture(t)
oldStdout := os.Stdout
Expand Down Expand Up @@ -2239,12 +2263,12 @@ func TestBrowsersUpdate_NameAndTagsWithProxy_AllForwarded(t *testing.T) {
b := BrowsersCmd{browsers: fake}

err := b.Update(context.Background(), BrowsersUpdateInput{
Identifier: "session123",
ProxyID: "proxy-123",
Name: "combo",
SetName: true,
Tags: map[string]string{"k": "v"},
SetTags: true,
Identifier: "session123",
ProxyID: "proxy-123",
Name: "combo",
SetName: true,
Tags: map[string]string{"k": "v"},
TagsProvided: true,
})

assert.NoError(t, err)
Expand Down Expand Up @@ -2280,10 +2304,10 @@ func TestBrowsersUpdate_ClearNameWithSetTags_BothForwarded(t *testing.T) {
b := BrowsersCmd{browsers: fake}

err := b.Update(context.Background(), BrowsersUpdateInput{
Identifier: "session123",
ClearName: true,
Tags: map[string]string{"env": "prod"},
SetTags: true,
Identifier: "session123",
ClearName: true,
Tags: map[string]string{"env": "prod"},
TagsProvided: true,
})

assert.NoError(t, err)
Expand Down Expand Up @@ -2320,10 +2344,10 @@ func TestBrowsersUpdate_MalformedTagWithClearTags_Errors(t *testing.T) {
b := BrowsersCmd{browsers: fake}

err := b.Update(context.Background(), BrowsersUpdateInput{
Identifier: "session123",
SetTags: true, // --tag was provided...
Tags: nil, // ...but every value was malformed and dropped
ClearTags: true,
Identifier: "session123",
TagsProvided: true, // --tag was provided...
Tags: nil, // ...but every value was malformed and dropped
ClearTags: true,
})

assert.Error(t, err)
Expand All @@ -2338,9 +2362,9 @@ func TestBrowsersUpdate_AllMalformedTags_Errors(t *testing.T) {
b := BrowsersCmd{browsers: fake}

err := b.Update(context.Background(), BrowsersUpdateInput{
Identifier: "session123",
SetTags: true,
Tags: nil,
Identifier: "session123",
TagsProvided: true,
Tags: nil,
})

assert.Error(t, err)
Expand Down
Loading