feat: don't error on no changes, just log (#448)

captainbeardo · web-flow · commit 36446a7efaae · 2025-05-27T09:25:21.000-06:00
* feat: don't error on no changes, just log

* fix: use a flag for idempotentcy

* fix: tests

* feat: NS and user group idempotent flag
diff --git a/app/app.go b/app/app.go
@@ -23,6 +23,7 @@ func NewApp(params AppParams) (*cli.App, error) {
 			ServerFlag,
 			ConfigDirFlag,
 			AutoConfirmFlag,
+			IdempotentFlag,
 			APIKeyFlag,
 			InsecureConnectionFlag,
 			EnableDebugLogsFlag,
diff --git a/app/common.go b/app/common.go
@@ -7,6 +7,9 @@ import (
 	"strings"
 
 	apipayload "github.com/temporalio/tcld/protogen/temporal/api/common/v1"
+	"github.com/urfave/cli/v2"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 )
 
 const (
@@ -35,6 +38,19 @@ func IsFeatureEnabled(feature string) bool {
 	return false
 }
 
+func isNothingChangedErr(ctx *cli.Context, e error) bool {
+	// If we are not idempotent, we should error on nothing to change
+	if !ctx.Bool(IdempotentFlagName) {
+		return false
+	}
+
+	s, ok := status.FromError(e)
+	if !ok {
+		return false
+	}
+	return s.Code() == codes.InvalidArgument && strings.Contains(s.Message(), "nothing to change")
+}
+
 func parseAssumedRole(assumedRole string) (string, string, error) {
 	var accountID, roleName string
 	re := assumedRolePattern
diff --git a/app/flags.go b/app/flags.go
@@ -17,6 +17,7 @@ const (
 	APIKeyFlagName             = "api-key"
 	InsecureConnectionFlagName = "insecure"
 	EnableDebugLogsFlagName    = "enable-debug-logs"
+	IdempotentFlagName         = "idempotent"
 	AuthenticationFlagCategory = "Authentication:"
 
 	// APIKeyVersionTag indicates the state of API keys. This should be removed when fully released.
@@ -86,4 +87,9 @@ var (
 		Usage:   "A flag to enable debug logs",
 		EnvVars: []string{"TEMPORAL_CLOUD_ENABLE_DEBUG_LOGS"},
 	}
+	IdempotentFlag = &cli.BoolFlag{
+		Name:    IdempotentFlagName,
+		Usage:   "A flag to not error if there are no changes to the resource",
+		EnvVars: []string{"TEMPORAL_CLOUD_IDEMPOTENT"},
+	}
 )
diff --git a/app/namespace.go b/app/namespace.go
@@ -344,6 +344,9 @@ func (c *NamespaceClient) updateNamespace(ctx *cli.Context, n *namespace.Namespa
 		Spec:            n.Spec,
 	})
 	if err != nil {
+		if isNothingChangedErr(ctx, err) {
+			return nil
+		}
 		return err
 	}
 
@@ -802,13 +805,20 @@ func NewNamespaceCommand(getNamespaceClientFn GetNamespaceClientFn) (CommandOut,
 
 						if enable {
 							if n.Spec.Lifecycle != nil && n.Spec.Lifecycle.EnableDeleteProtection {
+								if ctx.Bool(IdempotentFlagName) {
+									return nil
+								}
+
 								return errors.New("delete protection is already enabled")
 							}
 							n.Spec.Lifecycle = &namespace.LifecycleSpec{
 								EnableDeleteProtection: true,
 							}
 						} else {
 							if n.Spec.Lifecycle == nil || !n.Spec.Lifecycle.EnableDeleteProtection {
+								if ctx.Bool(IdempotentFlagName) {
+									return nil
+								}
 								return errors.New("delete protection is already disabled")
 							}
 							y, err := ConfirmPrompt(ctx, "disabling namespace delete protection may be prone to accidental deletion. confirm?")
@@ -936,6 +946,9 @@ func NewNamespaceCommand(getNamespaceClientFn GetNamespaceClientFn) (CommandOut,
 							return err
 						}
 						if n.Spec.AcceptedClientCa == bundle {
+							if ctx.Bool(IdempotentFlagName) {
+								return nil
+							}
 							return errors.New("nothing to change")
 						}
 						n.Spec.AcceptedClientCa = bundle
@@ -983,6 +996,9 @@ func NewNamespaceCommand(getNamespaceClientFn GetNamespaceClientFn) (CommandOut,
 							return err
 						}
 						if n.Spec.AcceptedClientCa == bundle {
+							if ctx.Bool(IdempotentFlagName) {
+								return nil
+							}
 							return errors.New("nothing to change")
 						}
 						n.Spec.AcceptedClientCa = bundle
@@ -1014,6 +1030,9 @@ func NewNamespaceCommand(getNamespaceClientFn GetNamespaceClientFn) (CommandOut,
 							return err
 						}
 						if n.Spec.AcceptedClientCa == cert {
+							if ctx.Bool(IdempotentFlagName) {
+								return nil
+							}
 							return errors.New("nothing to change")
 						}
 						n.Spec.AcceptedClientCa = cert
@@ -1051,6 +1070,9 @@ func NewNamespaceCommand(getNamespaceClientFn GetNamespaceClientFn) (CommandOut,
 							return err
 						}
 						if n.Spec.AuthMethod == authMethod {
+							if ctx.Bool(IdempotentFlagName) {
+								return nil
+							}
 							return errors.New("nothing to change")
 						}
 						if disruptiveChange(n.Spec.AuthMethod, authMethod) {
diff --git a/app/namespace_test.go b/app/namespace_test.go
@@ -65,6 +65,7 @@ func (s *NamespaceTestSuite) SetupTest() {
 	}
 	flags := []cli.Flag{
 		AutoConfirmFlag,
+		IdempotentFlag,
 	}
 
 	s.cliApp, _ = NewTestApp(s.T(), cmds, flags)
@@ -224,6 +225,16 @@ func (s *NamespaceTestSuite) TestDeleteProtection() {
 			},
 			expectErr: true,
 		},
+		{
+			name: "no change already enabled, idempotent",
+			args: []string{"--idempotent", "n", "lc", "set", "-n", ns, "--edp", "true"},
+			expectGet: func(g *namespaceservice.GetNamespaceResponse) {
+				g.Namespace.Spec.Lifecycle = &namespace.LifecycleSpec{
+					EnableDeleteProtection: true,
+				}
+			},
+			expectErr: false,
+		},
 		{
 			name:      "no change already disabled",
 			args:      []string{"n", "lc", "set", "-n", ns, "--edp", "false"},
diff --git a/app/serviceaccount.go b/app/serviceaccount.go
@@ -183,6 +183,9 @@ func (c *ServiceAccountClient) performUpdate(
 
 	resp, err := c.client.UpdateServiceAccount(c.ctx, req)
 	if err != nil {
+		if isNothingChangedErr(ctx, err) {
+			return nil
+		}
 		return fmt.Errorf("unable to update service account: %w", err)
 	}
 	return PrintProto(resp.RequestStatus)
diff --git a/app/serviceaccount_test.go b/app/serviceaccount_test.go
@@ -13,6 +13,8 @@ import (
 	"github.com/temporalio/tcld/protogen/api/request/v1"
 	authservicemock "github.com/temporalio/tcld/protogen/apimock/authservice/v1"
 	"github.com/urfave/cli/v2"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 )
 
 func TestServiceAccount(t *testing.T) {
@@ -42,6 +44,7 @@ func (s *ServiceAccountTestSuite) SetupTest() {
 		Commands: []*cli.Command{out.Command},
 		Flags: []cli.Flag{
 			AutoConfirmFlag,
+			IdempotentFlag,
 		},
 	}
 }
@@ -354,3 +357,41 @@ func (s *ServiceAccountTestSuite) TestSetNamespacePermissionsEmpty() {
 	}, nil)
 	s.NoError(s.RunCmd("service-account", "set-namespace-permissions", "--service-account-id", "test-service-account-id"))
 }
+
+func (s *ServiceAccountTestSuite) TestSetNamespacePermissionsNoChange() {
+	s.mockAuthService.EXPECT().GetServiceAccount(gomock.Any(), gomock.Any()).Return(&authservice.GetServiceAccountResponse{
+		ServiceAccount: &auth.ServiceAccount{
+			Id: "test-service-account-id",
+			Spec: &auth.ServiceAccountSpec{
+				Name:        "test-service-account-name",
+				Description: "test-service-account-desc",
+				Access: &auth.Access{
+					AccountAccess: &auth.AccountAccess{
+						Role: auth.ACCOUNT_ACTION_GROUP_READ,
+					},
+					NamespaceAccesses: map[string]*auth.NamespaceAccess{},
+				},
+			},
+		},
+	}, nil).Times(1)
+	s.mockAuthService.EXPECT().UpdateServiceAccount(gomock.Any(), gomock.All(&updateServiceAccountRequestMatcher{
+		request: &authservice.UpdateServiceAccountRequest{
+			ServiceAccountId: "test-service-account-id",
+			Spec: &auth.ServiceAccountSpec{
+				Name:        "test-service-account-name",
+				Description: "test-service-account-desc",
+				Access: &auth.Access{
+					AccountAccess: &auth.AccountAccess{
+						Role: auth.ACCOUNT_ACTION_GROUP_READ,
+					},
+					NamespaceAccesses: map[string]*auth.NamespaceAccess{
+						"test-namespace-1": {
+							Permission: auth.NAMESPACE_ACTION_GROUP_READ,
+						},
+					},
+				},
+			},
+		},
+	})).Return(nil, status.Error(codes.InvalidArgument, "nothing to change")).Times(1)
+	s.NoError(s.RunCmd("--idempotent", "service-account", "set-namespace-permissions", "--service-account-id", "test-service-account-id", "-p", "test-namespace-1=Read"))
+}
diff --git a/app/user.go b/app/user.go
@@ -241,6 +241,9 @@ func (c *UserClient) performUpdate(ctx *cli.Context, user *auth.User) error {
 	}
 	resp, err := c.client.UpdateUser(c.ctx, req)
 	if err != nil {
+		if isNothingChangedErr(ctx, err) {
+			return nil
+		}
 		return fmt.Errorf("unable to update user: %w", err)
 	}
 	return PrintProto(resp.GetRequestStatus())
diff --git a/app/user_group.go b/app/user_group.go
@@ -127,7 +127,7 @@ func nsRoleToAccess(role string) (string, *identity.NamespaceAccess) {
 }
 
 // setAccess sets the access for a group using the cloud services API
-func (c *UserGroupClient) setAccess(_ *cli.Context, groupID string, accountRole string, nsRoles []string) error {
+func (c *UserGroupClient) setAccess(ctx *cli.Context, groupID string, accountRole string, nsRoles []string) error {
 	group, err := c.client.GetUserGroup(c.ctx, &cloudsvc.GetUserGroupRequest{
 		GroupId: groupID,
 	})
@@ -162,6 +162,9 @@ func (c *UserGroupClient) setAccess(_ *cli.Context, groupID string, accountRole
 
 	resp, err := c.client.UpdateUserGroup(c.ctx, req)
 	if err != nil {
+		if isNothingChangedErr(ctx, err) {
+			return nil
+		}
 		return err
 	}
 
diff --git a/app/user_test.go b/app/user_test.go
@@ -3,15 +3,18 @@ package app
 import (
 	"context"
 	"errors"
+	"reflect"
+	"testing"
+
 	"github.com/golang/mock/gomock"
 	"github.com/stretchr/testify/suite"
 	"github.com/temporalio/tcld/protogen/api/auth/v1"
 	"github.com/temporalio/tcld/protogen/api/authservice/v1"
 	"github.com/temporalio/tcld/protogen/api/request/v1"
 	authservicemock "github.com/temporalio/tcld/protogen/apimock/authservice/v1"
 	"github.com/urfave/cli/v2"
-	"reflect"
-	"testing"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 )
 
 func TestUser(t *testing.T) {
@@ -41,6 +44,7 @@ func (s *UserTestSuite) SetupTest() {
 	}
 	flags := []cli.Flag{
 		AutoConfirmFlag,
+		IdempotentFlag,
 	}
 	s.cliApp, _ = NewTestApp(s.T(), cmds, flags)
 }
@@ -543,3 +547,56 @@ func (s *UserTestSuite) TestSetNamespacePermissionsEmpty() {
 	}, nil)
 	s.NoError(s.RunCmd("user", "set-namespace-permissions", "--user-email", "test@example.com"))
 }
+
+func (s *UserTestSuite) TestSetNamespacePermissionsNoChanges() {
+	s.mockAuthService.EXPECT().GetUser(gomock.Any(), gomock.Any()).Return(&authservice.GetUserResponse{
+		User: &auth.User{
+			Id: "test-user-id",
+			Spec: &auth.UserSpec{
+				Email: "test@example.com",
+			},
+		},
+	}, nil).Times(1)
+	s.mockAuthService.EXPECT().GetRoles(gomock.Any(), gomock.Any()).Return(&authservice.GetRolesResponse{
+		Roles: []*auth.Role{
+			{
+				Id:   "test-account-developer-role",
+				Type: auth.ROLE_TYPE_PREDEFINED,
+				Spec: &auth.RoleSpec{
+					AccountRole: &auth.AccountRoleSpec{
+						ActionGroup: auth.ACCOUNT_ACTION_GROUP_DEVELOPER,
+					},
+				},
+			},
+		},
+	}, nil).Times(1)
+	s.mockAuthService.EXPECT().GetRolesByPermissions(gomock.Any(), gomock.Any()).Return(&authservice.GetRolesByPermissionsResponse{
+		Roles: []*auth.Role{
+			{
+				Id:   "test-ns1-admin-role",
+				Type: auth.ROLE_TYPE_PREDEFINED,
+				Spec: &auth.RoleSpec{
+					NamespaceRoles: []*auth.NamespaceRoleSpec{
+						{
+							Namespace:   "ns1",
+							ActionGroup: auth.NAMESPACE_ACTION_GROUP_ADMIN,
+						},
+					},
+				},
+			},
+		},
+	}, nil).Times(1)
+	s.mockAuthService.EXPECT().UpdateUser(gomock.Any(), gomock.All(&updateUserRequestMatcher{
+		request: &authservice.UpdateUserRequest{
+			UserId: "test-user-id",
+			Spec: &auth.UserSpec{
+				Email: "test@example.com",
+				Roles: []string{
+					"test-account-developer-role",
+					"test-ns1-admin-role",
+				},
+			},
+		},
+	})).Return(nil, status.Error(codes.InvalidArgument, "nothing to change")).Times(1)
+	s.NoError(s.RunCmd("--idempotent", "user", "set-namespace-permissions", "--user-email", "test@example.com", "-p", "ns1=Admin", "-p", "ns2=Write", "-p", "ns3=Read"))
+}

Original file line number	Diff line number	Diff line change
`@@ -344,6 +344,9 @@ func (c NamespaceClient) updateNamespace(ctx cli.Context, n *namespace.Namespa`
`344`	`344`	`Spec: n.Spec,`
`345`	`345`	`})`
`346`	`346`	`if err != nil {`
	`347`	`+ if isNothingChangedErr(ctx, err) {`
	`348`	`+ return nil`
	`349`	`+ }`
`347`	`350`	`return err`
`348`	`351`	`}`
`349`	`352`
`@@ -802,13 +805,20 @@ func NewNamespaceCommand(getNamespaceClientFn GetNamespaceClientFn) (CommandOut,`
`802`	`805`
`803`	`806`	`if enable {`
`804`	`807`	`if n.Spec.Lifecycle != nil && n.Spec.Lifecycle.EnableDeleteProtection {`
	`808`	`+ if ctx.Bool(IdempotentFlagName) {`
	`809`	`+ return nil`
	`810`	`+ }`
	`811`	`+`
`805`	`812`	`return errors.New("delete protection is already enabled")`
`806`	`813`	`}`
`807`	`814`	`n.Spec.Lifecycle = &namespace.LifecycleSpec{`
`808`	`815`	`EnableDeleteProtection: true,`
`809`	`816`	`}`
`810`	`817`	`} else {`
`811`	`818`	`if n.Spec.Lifecycle == nil \|\| !n.Spec.Lifecycle.EnableDeleteProtection {`
	`819`	`+ if ctx.Bool(IdempotentFlagName) {`
	`820`	`+ return nil`
	`821`	`+ }`
`812`	`822`	`return errors.New("delete protection is already disabled")`
`813`	`823`	`}`
`814`	`824`	`y, err := ConfirmPrompt(ctx, "disabling namespace delete protection may be prone to accidental deletion. confirm?")`
`@@ -936,6 +946,9 @@ func NewNamespaceCommand(getNamespaceClientFn GetNamespaceClientFn) (CommandOut,`
`936`	`946`	`return err`
`937`	`947`	`}`
`938`	`948`	`if n.Spec.AcceptedClientCa == bundle {`
	`949`	`+ if ctx.Bool(IdempotentFlagName) {`
	`950`	`+ return nil`
	`951`	`+ }`
`939`	`952`	`return errors.New("nothing to change")`
`940`	`953`	`}`
`941`	`954`	`n.Spec.AcceptedClientCa = bundle`
`@@ -983,6 +996,9 @@ func NewNamespaceCommand(getNamespaceClientFn GetNamespaceClientFn) (CommandOut,`
`983`	`996`	`return err`
`984`	`997`	`}`
`985`	`998`	`if n.Spec.AcceptedClientCa == bundle {`
	`999`	`+ if ctx.Bool(IdempotentFlagName) {`
	`1000`	`+ return nil`
	`1001`	`+ }`
`986`	`1002`	`return errors.New("nothing to change")`
`987`	`1003`	`}`
`988`	`1004`	`n.Spec.AcceptedClientCa = bundle`
`@@ -1014,6 +1030,9 @@ func NewNamespaceCommand(getNamespaceClientFn GetNamespaceClientFn) (CommandOut,`
`1014`	`1030`	`return err`
`1015`	`1031`	`}`
`1016`	`1032`	`if n.Spec.AcceptedClientCa == cert {`
	`1033`	`+ if ctx.Bool(IdempotentFlagName) {`
	`1034`	`+ return nil`
	`1035`	`+ }`
`1017`	`1036`	`return errors.New("nothing to change")`
`1018`	`1037`	`}`
`1019`	`1038`	`n.Spec.AcceptedClientCa = cert`
`@@ -1051,6 +1070,9 @@ func NewNamespaceCommand(getNamespaceClientFn GetNamespaceClientFn) (CommandOut,`
`1051`	`1070`	`return err`
`1052`	`1071`	`}`
`1053`	`1072`	`if n.Spec.AuthMethod == authMethod {`
	`1073`	`+ if ctx.Bool(IdempotentFlagName) {`
	`1074`	`+ return nil`
	`1075`	`+ }`
`1054`	`1076`	`return errors.New("nothing to change")`
`1055`	`1077`	`}`
`1056`	`1078`	`if disruptiveChange(n.Spec.AuthMethod, authMethod) {`
Original file line number	Diff line number	Diff line change
`@@ -183,6 +183,9 @@ func (c *ServiceAccountClient) performUpdate(`
`183`	`183`
`184`	`184`	`resp, err := c.client.UpdateServiceAccount(c.ctx, req)`
`185`	`185`	`if err != nil {`
	`186`	`+ if isNothingChangedErr(ctx, err) {`
	`187`	`+ return nil`
	`188`	`+ }`
`186`	`189`	`return fmt.Errorf("unable to update service account: %w", err)`
`187`	`190`	`}`
`188`	`191`	`return PrintProto(resp.RequestStatus)`
Original file line number	Diff line number	Diff line change
`@@ -241,6 +241,9 @@ func (c UserClient) performUpdate(ctx cli.Context, user *auth.User) error {`
`241`	`241`	`}`
`242`	`242`	`resp, err := c.client.UpdateUser(c.ctx, req)`
`243`	`243`	`if err != nil {`
	`244`	`+ if isNothingChangedErr(ctx, err) {`
	`245`	`+ return nil`
	`246`	`+ }`
`244`	`247`	`return fmt.Errorf("unable to update user: %w", err)`
`245`	`248`	`}`
`246`	`249`	`return PrintProto(resp.GetRequestStatus())`