Skip to content

Commit

Permalink
Fix multi-cluster domain register
Browse files Browse the repository at this point in the history
The commands which used cluster names (domain register/update) were
broken. Previously with urfave v1 we did a hack - we collected posional
arguments and implicitely took all of them as clusters.
Thus, these crazy invocations worked:
```
$ cadence domain register --clusters clusterA clusterB --other_opt ...
$ cadence domain register --clusters clusterA --other_opt ... clusterB
...
```
urfave v2 requires posional arguments to be at the end (makes sense!),
that's why the above invocations are not working. One could be very
confused with error-message cadence-cli gives in this case.

The solution is to make it explicit:
```
$ cadence domain register --clusters clusterA,clusterB
$ cadence domain register --clusters clusterA --clusters clusterB # a
bit weird IMO, I'd recommend the next example
$ cadence domain register --cl clusterA --cl clusterB
```

I don't think we mention this anywhere in documentation, but I think the
new approach is more obvious and, after all, it works!
In anyway, the command' --help explains it nicely:
"Clusters (example: --clusters clusterA,clusterB or --cl clusterA --cl clusterB)"
  • Loading branch information
dkrotx committed Dec 10, 2024
1 parent 4f05e8a commit 05606e9
Show file tree
Hide file tree
Showing 4 changed files with 31 additions and 23 deletions.
12 changes: 2 additions & 10 deletions tools/cli/domain_commands.go
Original file line number Diff line number Diff line change
Expand Up @@ -121,11 +121,7 @@ func (d *domainCLIImpl) RegisterDomain(c *cli.Context) error {

var clusters []*types.ClusterReplicationConfiguration
if c.IsSet(FlagClusters) {
clusterStr := c.String(FlagClusters)
clusters = append(clusters, &types.ClusterReplicationConfiguration{
ClusterName: clusterStr,
})
for _, clusterStr := range c.Args().Slice() {
for _, clusterStr := range c.StringSlice(FlagClusters) {
clusters = append(clusters, &types.ClusterReplicationConfiguration{
ClusterName: clusterStr,
})
Expand Down Expand Up @@ -231,11 +227,7 @@ func (d *domainCLIImpl) UpdateDomain(c *cli.Context) error {
retentionDays = int32(c.Int(FlagRetentionDays))
}
if c.IsSet(FlagClusters) {
clusterStr := c.String(FlagClusters)
clusters = append(clusters, &types.ClusterReplicationConfiguration{
ClusterName: clusterStr,
})
for _, clusterStr := range c.Args().Slice() {
for _, clusterStr := range c.StringSlice(FlagClusters) {
clusters = append(clusters, &types.ClusterReplicationConfiguration{
ClusterName: clusterStr,
})
Expand Down
30 changes: 23 additions & 7 deletions tools/cli/domain_commands_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ func (s *cliAppSuite) TestDomainRegister() {
},
{
"domain with other options",
"cadence --do test-domain domain register --global_domain true --retention 5 --desc description --domain_data key1=value1,key2=value2 --active_cluster c1 --clusters c1 c2",
"cadence --do test-domain domain register --global_domain true --retention 5 --desc description --active_cluster c1 --clusters c1,c2 --domain_data key1=value1,key2=value2",
"",
func() {
s.serverFrontendClient.EXPECT().RegisterDomain(gomock.Any(), &types.RegisterDomainRequest{
Expand All @@ -73,12 +73,28 @@ func (s *cliAppSuite) TestDomainRegister() {
},
ActiveClusterName: "c1",
Clusters: []*types.ClusterReplicationConfiguration{
{
ClusterName: "c1",
},
{
ClusterName: "c2",
},
{ClusterName: "c1"}, {ClusterName: "c2"},
},
}).Return(nil)
},
},
{
"domain with other options and clusters mentioned as multiple options",
"cadence --do test-domain domain register --global_domain true --retention 5 --desc description --active_cluster c1 --cl c1 --cl c2 --domain_data key1=value1,key2=value2 --cl c3",
"",
func() {
s.serverFrontendClient.EXPECT().RegisterDomain(gomock.Any(), &types.RegisterDomainRequest{
Name: "test-domain",
WorkflowExecutionRetentionPeriodInDays: 5,
IsGlobalDomain: true,
Description: "description",
Data: map[string]string{
"key1": "value1",
"key2": "value2",
},
ActiveClusterName: "c1",
Clusters: []*types.ClusterReplicationConfiguration{
{ClusterName: "c1"}, {ClusterName: "c2"}, {ClusterName: "c3"},
},
}).Return(nil)
},
Expand Down
10 changes: 4 additions & 6 deletions tools/cli/domain_utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,11 +72,10 @@ var (
Aliases: []string{"ac"},
Usage: "Active cluster name",
},
&cli.StringFlag{
// TODO use StringSliceFlag instead
&cli.StringSliceFlag{
Name: FlagClusters,
Aliases: []string{"cl"},
Usage: "Clusters",
Usage: FlagClustersUsage,
},
&cli.StringFlag{
Name: FlagIsGlobalDomain,
Expand Down Expand Up @@ -138,11 +137,10 @@ var (
Aliases: []string{"ac"},
Usage: "Active cluster name",
},
&cli.StringFlag{
// TODO use StringSliceFlag instead
&cli.StringSliceFlag{
Name: FlagClusters,
Aliases: []string{"cl"},
Usage: "Clusters",
Usage: FlagClustersUsage,
},
&cli.GenericFlag{
Name: FlagDomainData,
Expand Down
2 changes: 2 additions & 0 deletions tools/cli/flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,8 @@ const (
FlagSearchAttribute = "search_attr"
FlagNumReadPartitions = "num_read_partitions"
FlagNumWritePartitions = "num_write_partitions"

FlagClustersUsage = "Clusters (example: --clusters clusterA,clusterB or --cl clusterA --cl clusterB)"
)

var flagsForExecution = []cli.Flag{
Expand Down

0 comments on commit 05606e9

Please sign in to comment.