Skip to content

Commit 4202c52

Browse files
authored
fix(cli): Fix admin cli parsing multiple config values (#7726)
<!-- 1-2 line summary of WHAT changed technically: - Always link the relevant projects GitHub issue, unless it is a minor bugfix - Good: "Modified FailoverDomain mapper to allow null ActiveClusterName #320" - Bad: "added nil check" --> **What changed?** Fixed the bug in the CLI's admin config update command, it can not parse multiple values with comma separated <!-- Your goal is to provide all the required context for a future maintainer to understand the reasons for making this change (see https://cbea.ms/git-commit/#why-not-how). How did this work previously (and what was wrong with it)? What has changed, and why did you solve it this way? - Good: "Active-active domains have independent cluster attributes per region. Previously, modifying cluster attributes required spedifying the default ActiveClusterName which updates the global domain default. This prevents operators from updating regional configurations without affecting the primary cluster designation. This change allows attribute updates to be independent of active cluster selection." - Bad: "Improves domain handling" --> **Why?** The command cadence admin config update --name frontend.validSearchAttributes --value '{"Value":{"DomainID":1,"WorkflowID":1}}' was failing with the error "unexpected end of JSON input". Root Cause: The urfave/cli v2 library's StringSliceFlag splits values on commas by default. When you passed JSON containing commas like {"Value":{"DomainID":1,"WorkflowID":1}}, it was being split into multiple pieces: "{\"Value\":{\"DomainID\":1" and "\"WorkflowID\":1}}", which broke the JSON parsing. <!-- Include specific test commands and setup. Please include the exact commands such that another maintainer or contributor can reproduce the test steps taken. - e.g Unit test commands with exact invocation `go test -v ./common/types/mapper/proto -run TestFailoverDomainRequest` - For integration tests include setup steps and test commands Example: "Started local server with `./cadence start`, then ran `make test_e2e`" - For local simulation testing include setup steps for the server and how you ran the tests - Good: Full commands that reviewers can copy-paste to verify - Bad: "Tested locally" or "Added tests" --> **How did you test it?** go test -v ./tools/cli -run TestAdminUpdateDynamicConfig <!-- If there are risks that the release engineer should know about document them here. For example: - Has an API/IDL been modified? Is it backwards/forwards compatible? If not, what are the repecussions? - Has a schema change been introduced? Is it possible to roll back? - Has a feature flag been re-used for a new purpose? - Is there a potential performance concern? Is the change modifying core task processing logic? - If truly N/A, you can mark it as such --> **Potential risks** Only affects admin cli tool to update configs, no impact to file based configs <!-- If this PR completes a user facing feature or changes functionality add release notes here. Your release notes should allow a user and the release engineer to understand the changes with little context. Always ensure that the description contains a link to the relevant GitHub issue. --> **Release notes** <!-- Consider whether this change requires documentation updates in the Cadence-Docs repo - If yes: mention what needs updating (or link to docs PR in cadence-docs repo) - If in doubt, add a note about potential doc needs - Only mark N/A if you're certain no docs are affected --> **Documentation Changes** --- ## Reviewer Validation **PR Description Quality** (check these before reviewing code): - [x] **"What changed"** provides a clear 1-2 line summary - [ ] Project Issue is linked - [x] **"Why"** explains the full motivation with sufficient context - [x] **Testing is documented:** - [x] Unit test commands are included (with exact `go test` invocation) - [ ] Integration test setup/commands included (if integration tests were run) - [ ] Canary testing details included (if canary was mentioned) - [x] **Potential risks** section is thoughtfully filled out (or legitimately N/A) - [x] **Release notes** included if this completes a user-facing feature - [x] **Documentation** needs are addressed (or noted if uncertain) --------- Signed-off-by: Neil Xie <neil.xie@uber.com>
1 parent cfa5b2d commit 4202c52

File tree

2 files changed

+103
-1
lines changed

2 files changed

+103
-1
lines changed

tools/cli/admin_config_store_commands.go

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import (
2424
"encoding/json"
2525
"fmt"
2626
"sort"
27+
"strings"
2728

2829
"github.com/olekukonko/tablewriter"
2930
"github.com/urfave/cli/v2"
@@ -110,7 +111,22 @@ func AdminUpdateDynamicConfig(c *cli.Context) error {
110111
if err != nil {
111112
return commoncli.Problem("Required flag not found", err)
112113
}
113-
dcValues := c.StringSlice(FlagDynamicConfigValue)
114+
dcValuesRaw := c.StringSlice(FlagDynamicConfigValue)
115+
116+
// WORKAROUND: urfave/cli v2 StringSliceFlag splits on commas by default.
117+
// This breaks JSON values. Try reassembling the split pieces.
118+
var dcValues []string
119+
if len(dcValuesRaw) > 1 && strings.HasPrefix(dcValuesRaw[0], "{") {
120+
assembled := strings.Join(dcValuesRaw, ",")
121+
var test interface{}
122+
if json.Unmarshal([]byte(assembled), &test) == nil {
123+
dcValues = []string{assembled}
124+
} else {
125+
dcValues = dcValuesRaw
126+
}
127+
} else {
128+
dcValues = dcValuesRaw
129+
}
114130

115131
ctx, cancel, err := newContext(c)
116132
defer cancel()

tools/cli/admin_config_store_commands_test.go

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ package cli
2424

2525
import (
2626
"context"
27+
"encoding/json"
2728
"testing"
2829

2930
"github.com/stretchr/testify/assert"
@@ -166,6 +167,91 @@ func TestAdminUpdateDynamicConfig(t *testing.T) {
166167
},
167168
errContains: "Failed to update dynamic config value",
168169
},
170+
{
171+
name: "update with simple boolean value",
172+
cmdline: `cadence admin config update --name test.config --value ` + `'{"Value":true,"Filters":[]}'`,
173+
setupMocks: func(td *cliTestData) {
174+
td.mockAdminClient.EXPECT().UpdateDynamicConfig(gomock.Any(), gomock.Any()).
175+
DoAndReturn(func(_ context.Context, request *types.UpdateDynamicConfigRequest, _ ...yarpc.CallOption) error {
176+
assert.Equal(t, "test.config", request.ConfigName)
177+
assert.Len(t, request.ConfigValues, 1)
178+
179+
// Verify the value is correctly parsed
180+
var actualValue interface{}
181+
err := json.Unmarshal(request.ConfigValues[0].Value.Data, &actualValue)
182+
assert.NoError(t, err)
183+
assert.Equal(t, true, actualValue)
184+
185+
// Verify no filters
186+
assert.Empty(t, request.ConfigValues[0].Filters)
187+
188+
return nil
189+
})
190+
},
191+
errContains: "",
192+
},
193+
{
194+
name: "update with map value and no filters",
195+
cmdline: `cadence admin config update --name frontend.validSearchAttributes --value ` + `'{"Value":{"DomainID":1,"WorkflowID":1},"Filters":[]}'`,
196+
setupMocks: func(td *cliTestData) {
197+
td.mockAdminClient.EXPECT().UpdateDynamicConfig(gomock.Any(), gomock.Any()).
198+
DoAndReturn(func(_ context.Context, request *types.UpdateDynamicConfigRequest, _ ...yarpc.CallOption) error {
199+
assert.Equal(t, "frontend.validSearchAttributes", request.ConfigName)
200+
assert.Len(t, request.ConfigValues, 1)
201+
202+
// Verify the value is correctly parsed
203+
var actualValue interface{}
204+
err := json.Unmarshal(request.ConfigValues[0].Value.Data, &actualValue)
205+
assert.NoError(t, err)
206+
207+
expectedValue := map[string]interface{}{
208+
"DomainID": float64(1), // JSON numbers unmarshal to float64
209+
"WorkflowID": float64(1),
210+
}
211+
assert.Equal(t, expectedValue, actualValue)
212+
213+
// Verify no filters
214+
assert.Empty(t, request.ConfigValues[0].Filters)
215+
216+
return nil
217+
})
218+
},
219+
errContains: "",
220+
},
221+
{
222+
name: "update with map value and filters",
223+
cmdline: `cadence admin config update --name frontend.validSearchAttributes --value ` + `'{"Value":{"DomainID":1,"WorkflowID":1},"Filters":[{"Name":"domainName","Value":"test-domain"}]}'`,
224+
setupMocks: func(td *cliTestData) {
225+
td.mockAdminClient.EXPECT().UpdateDynamicConfig(gomock.Any(), gomock.Any()).
226+
DoAndReturn(func(_ context.Context, request *types.UpdateDynamicConfigRequest, _ ...yarpc.CallOption) error {
227+
assert.Equal(t, "frontend.validSearchAttributes", request.ConfigName)
228+
assert.Len(t, request.ConfigValues, 1)
229+
230+
// Verify the value is correctly parsed
231+
var actualValue interface{}
232+
err := json.Unmarshal(request.ConfigValues[0].Value.Data, &actualValue)
233+
assert.NoError(t, err)
234+
235+
expectedValue := map[string]interface{}{
236+
"DomainID": float64(1),
237+
"WorkflowID": float64(1),
238+
}
239+
assert.Equal(t, expectedValue, actualValue)
240+
241+
// Verify filters
242+
assert.Len(t, request.ConfigValues[0].Filters, 1)
243+
assert.Equal(t, "domainName", request.ConfigValues[0].Filters[0].Name)
244+
245+
var filterValue interface{}
246+
err = json.Unmarshal(request.ConfigValues[0].Filters[0].Value.Data, &filterValue)
247+
assert.NoError(t, err)
248+
assert.Equal(t, "test-domain", filterValue)
249+
250+
return nil
251+
})
252+
},
253+
errContains: "",
254+
},
169255
}
170256

171257
for _, tt := range tests {

0 commit comments

Comments
 (0)