Skip to content

Commit 2259cc6

Browse files
bahrmichaelbahrmichael
committed
review updates
1 parent 819935a commit 2259cc6

5 files changed

Lines changed: 125 additions & 31 deletions

File tree

‎cmd/src/abc_variables_delete.go‎

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -44,11 +44,11 @@ Examples:
4444
client := cfg.apiClient(clicompat.APIFlagsFromCmd(cmd), cmd.Writer)
4545
variableNames := append(cmd.Args().Tail(), cmd.StringSlice("var")...)
4646

47-
return runABCVariablesDelete(ctx, client, instanceID, variableNames, cmd.Writer, cmd.Bool("get-curl"))
47+
return runABCVariablesDelete(ctx, client, instanceID, variableNames, cmd.Writer)
4848
},
4949
})
5050

51-
func runABCVariablesDelete(ctx context.Context, client api.Client, instanceID string, variableNames []string, output io.Writer, getCurl bool) error {
51+
func runABCVariablesDelete(ctx context.Context, client api.Client, instanceID string, variableNames []string, output io.Writer) error {
5252
if len(variableNames) == 0 {
5353
return cmderrors.Usage("must provide at least one variable name")
5454
}
@@ -65,14 +65,11 @@ func runABCVariablesDelete(ctx context.Context, client api.Client, instanceID st
6565
})
6666
}
6767

68-
if err := updateABCWorkflowInstanceVariables(ctx, client, instanceID, variables); err != nil {
68+
ok, err := updateABCWorkflowInstanceVariables(ctx, client, instanceID, variables)
69+
if err != nil || !ok {
6970
return err
7071
}
7172

72-
if getCurl {
73-
return nil
74-
}
75-
76-
_, err := fmt.Fprintf(output, "Removed variables %q from workflow instance %q.\n", variableNames, instanceID)
73+
_, err = fmt.Fprintf(output, "Removed variables %q from workflow instance %q.\n", variableNames, instanceID)
7774
return err
7875
}

‎cmd/src/abc_variables_delete_test.go‎

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ func TestRunABCVariablesDelete(t *testing.T) {
2929
}).Return(request).Once()
3030
request.On("Do", context.Background(), mock.Anything).Return(true, nil).Once()
3131

32-
err := runABCVariablesDelete(context.Background(), client, "QWdlbnRpY1dvcmtmbG93SW5zdGFuY2U6MQ==", variableNames, output, false)
32+
err := runABCVariablesDelete(context.Background(), client, "QWdlbnRpY1dvcmtmbG93SW5zdGFuY2U6MQ==", variableNames, output)
3333
require.NoError(t, err)
3434
require.Equal(t, "Removed variables [\"approval\" \"checkpoints\" \"prompt\"] from workflow instance \"QWdlbnRpY1dvcmtmbG93SW5zdGFuY2U6MQ==\".\n", output.String())
3535

@@ -40,6 +40,32 @@ func TestRunABCVariablesDelete(t *testing.T) {
4040
func TestRunABCVariablesDeleteRejectsEmptyVariableName(t *testing.T) {
4141
t.Parallel()
4242

43-
err := runABCVariablesDelete(context.Background(), nil, "QWdlbnRpY1dvcmtmbG93SW5zdGFuY2U6MQ==", []string{"approval", ""}, io.Discard, false)
43+
err := runABCVariablesDelete(context.Background(), nil, "QWdlbnRpY1dvcmtmbG93SW5zdGFuY2U6MQ==", []string{"approval", ""}, io.Discard)
4444
require.ErrorContains(t, err, "variable names must not be empty")
4545
}
46+
47+
func TestRunABCVariablesDeleteSuppressesSuccessMessageWhenRequestDoesNotExecute(t *testing.T) {
48+
t.Parallel()
49+
50+
client := new(mockapi.Client)
51+
request := &mockapi.Request{}
52+
output := &bytes.Buffer{}
53+
variableNames := []string{"approval", "checkpoints", "prompt"}
54+
55+
client.On("NewRequest", updateABCWorkflowInstanceVariablesMutation, map[string]any{
56+
"instanceID": "QWdlbnRpY1dvcmtmbG93SW5zdGFuY2U6MQ==",
57+
"variables": []map[string]string{
58+
{"key": "approval", "value": "null"},
59+
{"key": "checkpoints", "value": "null"},
60+
{"key": "prompt", "value": "null"},
61+
},
62+
}).Return(request).Once()
63+
request.On("Do", context.Background(), mock.Anything).Return(false, nil).Once()
64+
65+
err := runABCVariablesDelete(context.Background(), client, "QWdlbnRpY1dvcmtmbG93SW5zdGFuY2U6MQ==", variableNames, output)
66+
require.NoError(t, err)
67+
require.Empty(t, output.String())
68+
69+
client.AssertExpectations(t)
70+
request.AssertExpectations(t)
71+
}

‎cmd/src/abc_variables_set.go‎

Lines changed: 17 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"encoding/json"
77
"fmt"
88
"io"
9+
"slices"
910
"strings"
1011

1112
"github.com/sourcegraph/src-cli/internal/api"
@@ -63,7 +64,7 @@ NOTE: Values are interpreted as JSON literals when valid. Otherwise they are sen
6364
if err != nil {
6465
return err
6566
}
66-
return runABCVariablesSet(ctx, client, instanceID, abcVariables, cmd.Writer, cmd.Bool("get-curl"))
67+
return runABCVariablesSet(ctx, client, instanceID, abcVariables, cmd.Writer)
6768
},
6869
})
6970

@@ -94,28 +95,31 @@ func parseABCVariables(positional []string, flagged []string) (map[string]string
9495
return variables, nil
9596
}
9697

97-
func runABCVariablesSet(ctx context.Context, client api.Client, instanceID string, variables map[string]string, output io.Writer, getCurl bool) error {
98+
func runABCVariablesSet(ctx context.Context, client api.Client, instanceID string, variables map[string]string, output io.Writer) error {
9899
graphqlVariables := make([]map[string]string, 0, len(variables))
99-
for k, v := range variables {
100+
keys := make([]string, 0, len(variables))
101+
for k := range variables {
102+
keys = append(keys, k)
103+
}
104+
slices.Sort(keys)
105+
106+
for _, k := range keys {
100107
graphqlVariables = append(graphqlVariables, map[string]string{
101108
"key": k,
102-
"value": v,
109+
"value": variables[k],
103110
})
104111
}
105112

106-
if err := updateABCWorkflowInstanceVariables(ctx, client, instanceID, graphqlVariables); err != nil {
113+
ok, err := updateABCWorkflowInstanceVariables(ctx, client, instanceID, graphqlVariables)
114+
if err != nil || !ok {
107115
return err
108116
}
109117

110-
if getCurl {
111-
return nil
112-
}
113-
114-
_, err := fmt.Fprintf(output, "Updated %d variables on workflow instance %q.\n", len(variables), instanceID)
118+
_, err = fmt.Fprintf(output, "Updated %d variables on workflow instance %q.\n", len(variables), instanceID)
115119
return err
116120
}
117121

118-
func updateABCWorkflowInstanceVariables(ctx context.Context, client api.Client, instanceID string, variables []map[string]string) error {
122+
func updateABCWorkflowInstanceVariables(ctx context.Context, client api.Client, instanceID string, variables []map[string]string) (bool, error) {
119123
var result struct {
120124
UpdateAgenticWorkflowInstanceVariables struct {
121125
ID string `json:"id"`
@@ -125,10 +129,10 @@ func updateABCWorkflowInstanceVariables(ctx context.Context, client api.Client,
125129
"instanceID": instanceID,
126130
"variables": variables,
127131
}).Do(ctx, &result); err != nil || !ok {
128-
return err
132+
return ok, err
129133
}
130134

131-
return nil
135+
return true, nil
132136
}
133137

134138
func marshalABCVariableValue(raw string) (value string, remove bool, err error) {

‎cmd/src/abc_variables_set_test.go‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,14 @@
11
package main
22

33
import (
4+
"bytes"
5+
"context"
46
"testing"
57

68
"github.com/google/go-cmp/cmp"
9+
mockapi "github.com/sourcegraph/src-cli/internal/api/mock"
10+
"github.com/stretchr/testify/mock"
11+
"github.com/stretchr/testify/require"
712
)
813

914
// If we were to do a json marshalling roundtrip, it may break large integer literals.
@@ -42,3 +47,52 @@ func TestParseABCVariables(t *testing.T) {
4247
t.Errorf("err: %v", diff)
4348
}
4449
}
50+
51+
func TestRunABCVariablesSet(t *testing.T) {
52+
t.Parallel()
53+
54+
client := new(mockapi.Client)
55+
request := &mockapi.Request{Response: `{"data":{"updateAgenticWorkflowInstanceVariables":{"id":"workflow"}}}`}
56+
output := &bytes.Buffer{}
57+
variables := map[string]string{
58+
"prompt": `"tighten the review criteria"`,
59+
"checkpoints": "[1,2,3]",
60+
}
61+
62+
client.On("NewRequest", updateABCWorkflowInstanceVariablesMutation, map[string]any{
63+
"instanceID": "QWdlbnRpY1dvcmtmbG93SW5zdGFuY2U6MQ==",
64+
"variables": []map[string]string{
65+
{"key": "checkpoints", "value": "[1,2,3]"},
66+
{"key": "prompt", "value": `"tighten the review criteria"`},
67+
},
68+
}).Return(request).Once()
69+
request.On("Do", context.Background(), mock.Anything).Return(true, nil).Once()
70+
71+
err := runABCVariablesSet(context.Background(), client, "QWdlbnRpY1dvcmtmbG93SW5zdGFuY2U6MQ==", variables, output)
72+
require.NoError(t, err)
73+
require.Equal(t, "Updated 2 variables on workflow instance \"QWdlbnRpY1dvcmtmbG93SW5zdGFuY2U6MQ==\".\n", output.String())
74+
75+
client.AssertExpectations(t)
76+
request.AssertExpectations(t)
77+
}
78+
79+
func TestRunABCVariablesSetSuppressesSuccessMessageWhenRequestDoesNotExecute(t *testing.T) {
80+
t.Parallel()
81+
82+
client := new(mockapi.Client)
83+
request := &mockapi.Request{}
84+
output := &bytes.Buffer{}
85+
86+
client.On("NewRequest", updateABCWorkflowInstanceVariablesMutation, map[string]any{
87+
"instanceID": "QWdlbnRpY1dvcmtmbG93SW5zdGFuY2U6MQ==",
88+
"variables": []map[string]string{{"key": "prompt", "value": `"tighten the review criteria"`}},
89+
}).Return(request).Once()
90+
request.On("Do", context.Background(), mock.Anything).Return(false, nil).Once()
91+
92+
err := runABCVariablesSet(context.Background(), client, "QWdlbnRpY1dvcmtmbG93SW5zdGFuY2U6MQ==", map[string]string{"prompt": `"tighten the review criteria"`}, output)
93+
require.NoError(t, err)
94+
require.Empty(t, output.String())
95+
96+
client.AssertExpectations(t)
97+
request.AssertExpectations(t)
98+
}

‎internal/clicompat/help.go‎

Lines changed: 21 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,13 @@
11
package clicompat
22

3-
import "github.com/urfave/cli/v3"
3+
import (
4+
"context"
5+
"fmt"
6+
7+
"github.com/sourcegraph/sourcegraph/lib/errors"
8+
"github.com/sourcegraph/src-cli/internal/cmderrors"
9+
"github.com/urfave/cli/v3"
10+
)
411

512
// LegacyCommandHelpTemplate formats leaf command help in a style closer to the
613
// existing flag.FlagSet-based help output.
@@ -28,6 +35,7 @@ func Wrap(cmd *cli.Command) *cli.Command {
2835

2936
cmd.CustomHelpTemplate = LegacyCommandHelpTemplate
3037
cmd.OnUsageError = OnUsageError
38+
cmd.Action = wrapUsageAction(cmd.Action)
3139
return cmd
3240
}
3341

@@ -39,18 +47,23 @@ func WrapRoot(cmd *cli.Command) *cli.Command {
3947

4048
cmd.CustomRootCommandHelpTemplate = LegacyRootCommandHelpTemplate
4149
cmd.OnUsageError = OnUsageError
50+
cmd.Action = wrapUsageAction(cmd.Action)
4251
return cmd
4352
}
4453

45-
// WithLegacyHelp applies both root and leaf legacy help templates.
46-
func WithLegacyHelp(cmd *cli.Command) *cli.Command {
47-
if cmd == nil {
54+
func wrapUsageAction(action cli.ActionFunc) cli.ActionFunc {
55+
if action == nil {
4856
return nil
4957
}
5058

51-
cmd.CustomHelpTemplate = LegacyCommandHelpTemplate
52-
cmd.CustomRootCommandHelpTemplate = LegacyRootCommandHelpTemplate
53-
cmd.OnUsageError = OnUsageError
59+
return func(ctx context.Context, cmd *cli.Command) error {
60+
err := action(ctx, cmd)
61+
if err == nil || !errors.HasType[*cmderrors.UsageError](err) {
62+
return err
63+
}
5464

55-
return cmd
65+
_, _ = fmt.Fprintf(cmd.Root().ErrWriter, "error: %s\n", err)
66+
cli.DefaultPrintHelp(cmd.Root().ErrWriter, cmd.CustomHelpTemplate, cmd)
67+
return err
68+
}
5669
}

0 commit comments

Comments
 (0)