From ccf9b203b7502a29074e3a811f5117154c077c84 Mon Sep 17 00:00:00 2001 From: Alex Ostrikov Date: Wed, 22 Mar 2023 17:03:21 +0400 Subject: [PATCH] permissions-center: make PermissionSyncCodeHostState's status typed. (#49834) This commit makes status typed instead of a raw string which will be further used in API and UI layers as well for increased stability and removal of possibility of unexpected string results being passed over API. API/UI changes will be made in a follow-up commit. Test plan: Unit tests updated. --- .../authz/resolvers/permissions_sync_jobs.go | 2 +- .../internal/authz/integration_test.go | 12 ++++----- .../internal/authz/perms_syncer_test.go | 22 ++++++++-------- .../internal/authz/perms_syncer_worker.go | 2 +- .../authz/perms_syncer_worker_test.go | 8 +++--- .../permission_sync_code_host_state.go | 25 +++++++++++-------- .../database/permission_sync_jobs_test.go | 4 +-- 7 files changed, 40 insertions(+), 35 deletions(-) diff --git a/enterprise/cmd/frontend/internal/authz/resolvers/permissions_sync_jobs.go b/enterprise/cmd/frontend/internal/authz/resolvers/permissions_sync_jobs.go index fc3558952c5d..188e4f732d4c 100644 --- a/enterprise/cmd/frontend/internal/authz/resolvers/permissions_sync_jobs.go +++ b/enterprise/cmd/frontend/internal/authz/resolvers/permissions_sync_jobs.go @@ -266,7 +266,7 @@ func (c codeHostStateResolver) ProviderType() string { } func (c codeHostStateResolver) Status() string { - return c.state.Status + return string(c.state.Status) } func (c codeHostStateResolver) Message() string { diff --git a/enterprise/cmd/repo-updater/internal/authz/integration_test.go b/enterprise/cmd/repo-updater/internal/authz/integration_test.go index b6c8ac0cf819..428934d8b548 100644 --- a/enterprise/cmd/repo-updater/internal/authz/integration_test.go +++ b/enterprise/cmd/repo-updater/internal/authz/integration_test.go @@ -145,7 +145,7 @@ func TestIntegration_GitHubPermissions(t *testing.T) { assert.Equal(t, database.CodeHostStatusesSet{{ ProviderID: "https://github.com/", ProviderType: "github", - Status: "SUCCESS", + Status: database.CodeHostStatusSuccess, Message: "FetchRepoPerms", }}, providerStates) @@ -230,7 +230,7 @@ func TestIntegration_GitHubPermissions(t *testing.T) { assert.Equal(t, database.CodeHostStatusesSet{{ ProviderID: "https://github.com/", ProviderType: "github", - Status: "SUCCESS", + Status: database.CodeHostStatusSuccess, Message: "FetchRepoPerms", }}, providerStates) @@ -256,7 +256,7 @@ func TestIntegration_GitHubPermissions(t *testing.T) { assert.Equal(t, database.CodeHostStatusesSet{{ ProviderID: "https://github.com/", ProviderType: "github", - Status: "SUCCESS", + Status: database.CodeHostStatusSuccess, Message: "FetchRepoPerms", }}, providerStates) @@ -353,7 +353,7 @@ func TestIntegration_GitHubPermissions(t *testing.T) { assert.Equal(t, database.CodeHostStatusesSet{{ ProviderID: "https://github.com/", ProviderType: "github", - Status: "SUCCESS", + Status: database.CodeHostStatusSuccess, Message: "FetchUserPerms", }}, providerStates) @@ -441,7 +441,7 @@ func TestIntegration_GitHubPermissions(t *testing.T) { assert.Equal(t, database.CodeHostStatusesSet{{ ProviderID: "https://github.com/", ProviderType: "github", - Status: "SUCCESS", + Status: database.CodeHostStatusSuccess, Message: "FetchUserPerms", }}, providerStates) @@ -467,7 +467,7 @@ func TestIntegration_GitHubPermissions(t *testing.T) { assert.Equal(t, database.CodeHostStatusesSet{{ ProviderID: "https://github.com/", ProviderType: "github", - Status: "SUCCESS", + Status: database.CodeHostStatusSuccess, Message: "FetchUserPerms", }}, providerStates) diff --git a/enterprise/cmd/repo-updater/internal/authz/perms_syncer_test.go b/enterprise/cmd/repo-updater/internal/authz/perms_syncer_test.go index 5942759d6f16..577a22417c7f 100644 --- a/enterprise/cmd/repo-updater/internal/authz/perms_syncer_test.go +++ b/enterprise/cmd/repo-updater/internal/authz/perms_syncer_test.go @@ -99,11 +99,11 @@ func (p *mockProvider) URN() string { return extsvc.URN(p.serviceType, p func (*mockProvider) ValidateConnection(context.Context) error { return nil } -func (p *mockProvider) FetchUserPerms(ctx context.Context, acct *extsvc.Account, opts authz.FetchPermsOptions) (*authz.ExternalUserPermissions, error) { +func (p *mockProvider) FetchUserPerms(ctx context.Context, acct *extsvc.Account, _ authz.FetchPermsOptions) (*authz.ExternalUserPermissions, error) { return p.fetchUserPerms(ctx, acct) } -func (p *mockProvider) FetchUserPermsByToken(ctx context.Context, token string, opts authz.FetchPermsOptions) (*authz.ExternalUserPermissions, error) { +func (p *mockProvider) FetchUserPermsByToken(ctx context.Context, token string, _ authz.FetchPermsOptions) (*authz.ExternalUserPermissions, error) { return p.fetchUserPermsByToken(ctx, token) } @@ -201,7 +201,7 @@ func TestPermsSyncer_syncUserPerms(t *testing.T) { assert.Equal(t, database.CodeHostStatusesSet{{ ProviderID: "https://gitlab.com/", ProviderType: "gitlab", - Status: "SUCCESS", + Status: database.CodeHostStatusSuccess, Message: "FetchUserPerms", }}, providers) } @@ -384,7 +384,7 @@ func TestPermsSyncer_syncUserPerms_fetchAccount(t *testing.T) { statuses: database.CodeHostStatusesSet{{ ProviderID: p1.serviceID, ProviderType: p1.serviceType, - Status: "SUCCESS", + Status: database.CodeHostStatusSuccess, Message: "FetchUserPerms", }}, }, @@ -393,12 +393,12 @@ func TestPermsSyncer_syncUserPerms_fetchAccount(t *testing.T) { statuses: database.CodeHostStatusesSet{{ ProviderID: p2.serviceID, ProviderType: p2.serviceType, - Status: "ERROR", + Status: database.CodeHostStatusError, Message: "FetchAccount: no account found for this user", }, { ProviderID: p1.serviceID, ProviderType: p1.serviceType, - Status: "SUCCESS", + Status: database.CodeHostStatusSuccess, Message: "FetchUserPerms", }}, fetchAccountError: errors.New("no account found for this user"), @@ -408,7 +408,7 @@ func TestPermsSyncer_syncUserPerms_fetchAccount(t *testing.T) { statuses: database.CodeHostStatusesSet{{ ProviderID: p1.serviceID, ProviderType: p1.serviceType, - Status: "ERROR", + Status: database.CodeHostStatusError, Message: "FetchUserPerms: horse error", }}, fetchUserPermsError: errors.New("horse error"), @@ -418,12 +418,12 @@ func TestPermsSyncer_syncUserPerms_fetchAccount(t *testing.T) { statuses: database.CodeHostStatusesSet{{ ProviderID: p2.serviceID, ProviderType: p2.serviceType, - Status: "ERROR", + Status: database.CodeHostStatusError, Message: "FetchAccount: no account found for this user", }, { ProviderID: p1.serviceID, ProviderType: p1.serviceType, - Status: "ERROR", + Status: database.CodeHostStatusError, Message: "FetchUserPerms: horse error", }}, fetchAccountError: errors.New("no account found for this user"), @@ -549,7 +549,7 @@ func TestPermsSyncer_syncUserPermsTemporaryProviderError(t *testing.T) { assert.Equal(t, database.CodeHostStatusesSet{{ ProviderID: "https://gitlab.com/", ProviderType: "gitlab", - Status: "ERROR", + Status: database.CodeHostStatusError, Message: "FetchUserPerms: context deadline exceeded", }}, providers) } @@ -1106,7 +1106,7 @@ func TestPermsSyncer_syncRepoPerms(t *testing.T) { assert.Greater(t, len(providerStates), 0) for _, ps := range providerStates { - if ps.Status == "ERROR" { + if ps.Status == database.CodeHostStatusError { t.Fatal("Did not expect provider status of ERROR") } } diff --git a/enterprise/cmd/repo-updater/internal/authz/perms_syncer_worker.go b/enterprise/cmd/repo-updater/internal/authz/perms_syncer_worker.go index 580afca43845..e4179a55e0b7 100644 --- a/enterprise/cmd/repo-updater/internal/authz/perms_syncer_worker.go +++ b/enterprise/cmd/repo-updater/internal/authz/perms_syncer_worker.go @@ -106,7 +106,7 @@ func (h *permsSyncerWorker) handlePermsSync(ctx context.Context, reqType request if err == nil { allProvidersFailedToSync := len(providerStates) > 0 for _, state := range providerStates { - if state.Status != "ERROR" { + if state.Status != database.CodeHostStatusError { allProvidersFailedToSync = false break } diff --git a/enterprise/cmd/repo-updater/internal/authz/perms_syncer_worker_test.go b/enterprise/cmd/repo-updater/internal/authz/perms_syncer_worker_test.go index 4eb243a207f6..94c33111619f 100644 --- a/enterprise/cmd/repo-updater/internal/authz/perms_syncer_worker_test.go +++ b/enterprise/cmd/repo-updater/internal/authz/perms_syncer_worker_test.go @@ -502,10 +502,10 @@ func (d *dummySyncerWithErrors) syncRepoPerms(_ context.Context, repoID api.Repo Options: options, } - codeHostStates := database.CodeHostStatusesSet{{ProviderID: "id1", Status: "SUCCESS"}, {ProviderID: "id2", Status: "SUCCESS"}} + codeHostStates := database.CodeHostStatusesSet{{ProviderID: "id1", Status: database.CodeHostStatusSuccess}, {ProviderID: "id2", Status: database.CodeHostStatusSuccess}} if typ, ok := d.repoIDErrors[repoID]; ok && typ == allProvidersFailed { for idx := range codeHostStates { - codeHostStates[idx].Status = "ERROR" + codeHostStates[idx].Status = database.CodeHostStatusError } } @@ -524,10 +524,10 @@ func (d *dummySyncerWithErrors) syncUserPerms(_ context.Context, userID int32, n Options: options, } - codeHostStates := database.CodeHostStatusesSet{{ProviderID: "id1", Status: "ERROR"}, {ProviderID: "id2", Status: "SUCCESS"}} + codeHostStates := database.CodeHostStatusesSet{{ProviderID: "id1", Status: database.CodeHostStatusError}, {ProviderID: "id2", Status: database.CodeHostStatusSuccess}} if typ, ok := d.userIDErrors[userID]; ok && typ == allProvidersFailed { for idx := range codeHostStates { - codeHostStates[idx].Status = "ERROR" + codeHostStates[idx].Status = database.CodeHostStatusError } } diff --git a/internal/database/permission_sync_code_host_state.go b/internal/database/permission_sync_code_host_state.go index 4910afabc7c3..9838b44546c6 100644 --- a/internal/database/permission_sync_code_host_state.go +++ b/internal/database/permission_sync_code_host_state.go @@ -12,14 +12,19 @@ import ( // PermissionSyncCodeHostState describes the state of a provider during an authz sync job. type PermissionSyncCodeHostState struct { - ProviderID string `json:"provider_id"` - ProviderType string `json:"provider_type"` - - // Status is one of "ERROR" or "SUCCESS". - Status string `json:"status"` - Message string `json:"message"` + ProviderID string `json:"provider_id"` + ProviderType string `json:"provider_type"` + Status CodeHostStatus `json:"status"` + Message string `json:"message"` } +type CodeHostStatus string + +const ( + CodeHostStatusSuccess CodeHostStatus = "SUCCESS" + CodeHostStatusError CodeHostStatus = "ERROR" +) + func (e *PermissionSyncCodeHostState) Scan(value any) error { b, ok := value.([]byte) if !ok { @@ -38,14 +43,14 @@ func NewProviderStatus(provider authz.Provider, err error, action string) Permis return PermissionSyncCodeHostState{ ProviderID: provider.ServiceID(), ProviderType: provider.ServiceType(), - Status: "ERROR", + Status: CodeHostStatusError, Message: fmt.Sprintf("%s: %s", action, err.Error()), } } else { return PermissionSyncCodeHostState{ ProviderID: provider.ServiceID(), ProviderType: provider.ServiceType(), - Status: "SUCCESS", + Status: CodeHostStatusSuccess, Message: action, } } @@ -62,12 +67,12 @@ func (ps CodeHostStatusesSet) SummaryField() log.Field { for _, p := range ps { key := fmt.Sprintf("%s:%s", p.ProviderType, p.ProviderID) switch p.Status { - case "ERROR": + case CodeHostStatusError: errored = append(errored, log.String( key, p.Message, )) - case "SUCCESS": + case CodeHostStatusSuccess: succeeded = append(errored, log.String( key, p.Message, diff --git a/internal/database/permission_sync_jobs_test.go b/internal/database/permission_sync_jobs_test.go index 3916b541d023..ac4951466456 100644 --- a/internal/database/permission_sync_jobs_test.go +++ b/internal/database/permission_sync_jobs_test.go @@ -853,13 +853,13 @@ func getSampleCodeHostStates() []PermissionSyncCodeHostState { { ProviderID: "ID", ProviderType: "Type", - Status: "SUCCESS", + Status: CodeHostStatusSuccess, Message: "successful success", }, { ProviderID: "ID", ProviderType: "Type", - Status: "ERROR", + Status: CodeHostStatusError, Message: "unsuccessful unsuccess :(", }, }