Provisioning: Run validation on config updates (#103265)

* Run validation on config updates

* Refactor code

* Add debug lines

* Remove test check on admission

* Organize imports

* Delegate events to the API client

* Extend error notification

* Deep copy default data

---------

Co-authored-by: Ryan McKinley <ryantxu@gmail.com>
Co-authored-by: Clarity-89 <homes89@ukr.net>

pkg/registry/apis/provisioning/register.go

@@ -476,36 +476,26 @@ func (b *APIBuilder) Validate(ctx context.Context, a admission.Attributes, o adm
 		}
 	}
 
-	targetError := b.verifyAgaintsExistingRepositories(cfg)
-	if targetError != nil {
-		list = append(list, targetError)
+	// Early exit to avoid more expensive checks if we have already found errors
+	if len(list) > 0 {
+		return invalidRepositoryError(a.GetName(), list)
 	}
 
-	// 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))
-			}
-		}
+	// Exit early if we have already found errors
+	targetError := b.verifyAgaintsExistingRepositories(cfg)
+	if targetError != nil {
+		return invalidRepositoryError(a.GetName(), field.ErrorList{targetError})
 	}
 
-	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())

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')))
+              );
+            }
           }
         }
       },

public/app/features/provisioning/Wizard/ConnectStep.tsx

@@ -41,15 +41,22 @@ export function ConnectStep() {
           'Choose the type of storage for your resources'
         )}
       >
-        <Combobox
-          options={typeOptions}
-          value={type}
-          onChange={(value) => {
-            const repoType = value?.value;
-            setValue('repository.type', repoType);
-            setValue(
-              'repository.workflows',
-              getWorkflowOptions(repoType).map((v) => v.value)
+        <Controller
+          name={'repository.type'}
+          render={({ field: { ref, onChange, ...field } }) => {
+            return (
+              <Combobox
+                options={typeOptions}
+                onChange={(value) => {
+                  const repoType = value?.value;
+                  onChange(repoType);
+                  setValue(
+                    'repository.workflows',
+                    getWorkflowOptions(repoType).map((v) => v.value)
+                  );
+                }}
+                {...field}
+              />
             );
           }}
         />

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) => {

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,
-  };
+  });
 };