diff --git a/pkg/registry/apis/provisioning/register.go b/pkg/registry/apis/provisioning/register.go index 8a55fb7ec9c..92f7483110d 100644 --- a/pkg/registry/apis/provisioning/register.go +++ b/pkg/registry/apis/provisioning/register.go @@ -476,36 +476,26 @@ func (b *APIBuilder) Validate(ctx context.Context, a admission.Attributes, o adm } } + // Early exit to avoid more expensive checks if we have already found errors + if len(list) > 0 { + return invalidRepositoryError(a.GetName(), list) + } + + // Exit early if we have already found errors targetError := b.verifyAgaintsExistingRepositories(cfg) if targetError != nil { - list = append(list, targetError) + return invalidRepositoryError(a.GetName(), field.ErrorList{targetError}) } - // For *create* we do a synchronous test... this can be expensive! - // it is the same as a full healthcheck, so should not be run on every update - if len(list) == 0 && a.GetOperation() == admission.Create { - testResults, err := repository.TestRepository(ctx, repo) - if err != nil { - list = append(list, field.Invalid(field.NewPath("spec"), - "Repository test failed", "Unable to verify repository: "+err.Error())) - } - - if !testResults.Success { - for _, err := range testResults.Errors { - list = append(list, field.Invalid(field.NewPath("spec"), - "Repository test failed", err)) - } - } - } - - if len(list) > 0 { - return apierrors.NewInvalid( - provisioning.RepositoryResourceInfo.GroupVersionKind().GroupKind(), - a.GetName(), list) - } return nil } +func invalidRepositoryError(name string, list field.ErrorList) error { + return apierrors.NewInvalid( + provisioning.RepositoryResourceInfo.GroupVersionKind().GroupKind(), + name, list) +} + // TODO: move this to a more appropriate place. Probably controller/validation.go func (b *APIBuilder) verifyAgaintsExistingRepositories(cfg *provisioning.Repository) *field.Error { all, err := b.repositoryLister.Repositories(cfg.Namespace).List(labels.Everything()) diff --git a/public/app/api/clients/provisioning/index.ts b/public/app/api/clients/provisioning/index.ts index 109b3847102..3aeb73d552f 100644 --- a/public/app/api/clients/provisioning/index.ts +++ b/public/app/api/clients/provisioning/index.ts @@ -1,3 +1,4 @@ +import { isFetchError } from '@grafana/runtime'; import { notifyApp } from 'app/core/actions'; import { createSuccessNotification, createErrorNotification } from 'app/core/copy/appNotification'; import { t } from 'app/core/internationalization'; @@ -93,8 +94,16 @@ export const provisioningAPI = generatedAPI.enhanceEndpoints({ try { await queryFulfilled; } catch (e) { - if (e instanceof Error) { - dispatch(notifyApp(createErrorNotification('Error testing repository', e))); + if (!e) { + dispatch(notifyApp(createErrorNotification('Error validating repository', new Error('Unknown error')))); + } else if (e instanceof Error) { + dispatch(notifyApp(createErrorNotification('Error validating repository', e))); + } else if (typeof e === 'object' && 'error' in e && isFetchError(e.error)) { + if (Array.isArray(e.error.data.errors) && e.error.data.errors.length) { + dispatch( + notifyApp(createErrorNotification('Error validating repository', e.error.data.errors.join('\n'))) + ); + } } } }, diff --git a/public/app/features/provisioning/Wizard/ConnectStep.tsx b/public/app/features/provisioning/Wizard/ConnectStep.tsx index 878314f201e..025b728001a 100644 --- a/public/app/features/provisioning/Wizard/ConnectStep.tsx +++ b/public/app/features/provisioning/Wizard/ConnectStep.tsx @@ -41,15 +41,22 @@ export function ConnectStep() { 'Choose the type of storage for your resources' )} > - { - const repoType = value?.value; - setValue('repository.type', repoType); - setValue( - 'repository.workflows', - getWorkflowOptions(repoType).map((v) => v.value) + { + return ( + { + const repoType = value?.value; + onChange(repoType); + setValue( + 'repository.workflows', + getWorkflowOptions(repoType).map((v) => v.value) + ); + }} + {...field} + /> ); }} /> diff --git a/public/app/features/provisioning/hooks/useCreateOrUpdateRepository.ts b/public/app/features/provisioning/hooks/useCreateOrUpdateRepository.ts index f91c4f0cdca..a7d78c1385d 100644 --- a/public/app/features/provisioning/hooks/useCreateOrUpdateRepository.ts +++ b/public/app/features/provisioning/hooks/useCreateOrUpdateRepository.ts @@ -3,15 +3,28 @@ import { useCallback } from 'react'; import { RepositorySpec, useCreateRepositoryMutation, + useCreateRepositoryTestMutation, useReplaceRepositoryMutation, } from 'app/api/clients/provisioning'; export function useCreateOrUpdateRepository(name?: string) { const [create, createRequest] = useCreateRepositoryMutation(); const [update, updateRequest] = useReplaceRepositoryMutation(); + const [testConfig, testRequest] = useCreateRepositoryTestMutation(); const updateOrCreate = useCallback( - (data: RepositorySpec) => { + async (data: RepositorySpec) => { + // First test the config and wait for the result + // unwrap will throw an error if the test fails + await testConfig({ + // HACK: we need to provide a name to the test configuration + name: name || 'new', + body: { + spec: data, + }, + }).unwrap(); + + // If test passes, proceed with create/update if (name) { return update({ name, @@ -28,10 +41,10 @@ export function useCreateOrUpdateRepository(name?: string) { } return create({ repository: { metadata: generateRepositoryMetadata(data), spec: data } }); }, - [create, name, update] + [create, name, update, testConfig] ); - return [updateOrCreate, name ? updateRequest : createRequest] as const; + return [updateOrCreate, name ? updateRequest : createRequest, testRequest] as const; } const generateRepositoryMetadata = (data: RepositorySpec) => { diff --git a/public/app/features/provisioning/utils/data.ts b/public/app/features/provisioning/utils/data.ts index 1abd1f879e9..e3017d7446e 100644 --- a/public/app/features/provisioning/utils/data.ts +++ b/public/app/features/provisioning/utils/data.ts @@ -27,16 +27,17 @@ export const dataToSpec = (data: RepositoryFormData): RepositorySpec => { break; } - return spec; + // We need to deep clone the data, so it doesn't become immutable + return structuredClone(spec); }; export const specToData = (spec: RepositorySpec): RepositoryFormData => { - return { + return structuredClone({ ...spec, ...spec.github, ...spec.local, branch: spec.github?.branch || '', url: spec.github?.url || '', generateDashboardPreviews: spec.github?.generateDashboardPreviews || false, - }; + }); };