diff --git a/apps/provisioning/pkg/repository/tester.go b/apps/provisioning/pkg/repository/tester.go index 6bd65443d92..14290d0514b 100644 --- a/apps/provisioning/pkg/repository/tester.go +++ b/apps/provisioning/pkg/repository/tester.go @@ -23,7 +23,13 @@ func NewSimpleRepositoryTester(validator RepositoryValidator) SimpleRepositoryTe // TestRepository validates the repository and then runs a health check func (t *SimpleRepositoryTester) TestRepository(ctx context.Context, repo Repository) (*provisioning.TestResults, error) { - errors := t.validator.ValidateRepository(repo) + // Determine if this is a CREATE or UPDATE operation + // If the repository has been observed by the controller (ObservedGeneration > 0), + // it's an existing repository and we should treat it as UPDATE + cfg := repo.Config() + isCreate := cfg.Status.ObservedGeneration == 0 + + errors := t.validator.ValidateRepository(repo, isCreate) if len(errors) > 0 { rsp := &provisioning.TestResults{ Code: http.StatusUnprocessableEntity, // Invalid diff --git a/apps/provisioning/pkg/repository/validator.go b/apps/provisioning/pkg/repository/validator.go index 73198f0d7ee..46ba81f8cbd 100644 --- a/apps/provisioning/pkg/repository/validator.go +++ b/apps/provisioning/pkg/repository/validator.go @@ -32,7 +32,9 @@ func NewValidator(minSyncInterval time.Duration, allowedTargets []provisioning.S } // ValidateRepository solely does configuration checks on the repository object. It does not run a health check or compare against existing repositories. -func (v *RepositoryValidator) ValidateRepository(repo Repository) field.ErrorList { +// isCreate indicates whether this is a CREATE operation (true) or UPDATE operation (false). +// When isCreate is false, allowedTargets validation is skipped to allow existing repositories to continue working. +func (v *RepositoryValidator) ValidateRepository(repo Repository, isCreate bool) field.ErrorList { list := repo.Validate() cfg := repo.Config() @@ -44,7 +46,7 @@ func (v *RepositoryValidator) ValidateRepository(repo Repository) field.ErrorLis if cfg.Spec.Sync.Target == "" { list = append(list, field.Required(field.NewPath("spec", "sync", "target"), "The target type is required when sync is enabled")) - } else if !slices.Contains(v.allowedTargets, cfg.Spec.Sync.Target) { + } else if isCreate && !slices.Contains(v.allowedTargets, cfg.Spec.Sync.Target) { list = append(list, field.Invalid( field.NewPath("spec", "target"), diff --git a/apps/provisioning/pkg/repository/validator_test.go b/apps/provisioning/pkg/repository/validator_test.go index cb8b726a9bf..75980733e17 100644 --- a/apps/provisioning/pkg/repository/validator_test.go +++ b/apps/provisioning/pkg/repository/validator_test.go @@ -303,7 +303,8 @@ func TestValidateRepository(t *testing.T) { validator := NewValidator(10*time.Second, []provisioning.SyncTargetType{provisioning.SyncTargetTypeFolder}, false) for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - errors := validator.ValidateRepository(tt.repository) + // Tests validate new configurations, so always pass isCreate=true + errors := validator.ValidateRepository(tt.repository, true) require.Len(t, errors, tt.expectedErrs) if tt.validateError != nil { tt.validateError(t, errors) diff --git a/conf/defaults.ini b/conf/defaults.ini index 8ee03e6a34c..28cd0420eb2 100644 --- a/conf/defaults.ini +++ b/conf/defaults.ini @@ -2264,7 +2264,7 @@ fail_tests_on_console = true # List of targets that can be controlled by a repository, separated by |. # Instance means the whole grafana instance will be controlled by a repository. # Folder limits it to a folder within the grafana instance. -allowed_targets = instance|folder +allowed_targets = folder # Whether image rendering is allowed for dashboard previews. # Requires image rendering service to be configured. diff --git a/pkg/registry/apis/provisioning/register.go b/pkg/registry/apis/provisioning/register.go index 80c22338ae0..f5376cb20ab 100644 --- a/pkg/registry/apis/provisioning/register.go +++ b/pkg/registry/apis/provisioning/register.go @@ -673,7 +673,8 @@ func (b *APIBuilder) Validate(ctx context.Context, a admission.Attributes, o adm // // the only time to add configuration checks here is if you need to compare // the incoming change to the current configuration - list := b.validator.ValidateRepository(repo) + isCreate := a.GetOperation() == admission.Create + list := b.validator.ValidateRepository(repo, isCreate) cfg := repo.Config() if a.GetOperation() == admission.Update { diff --git a/pkg/setting/setting.go b/pkg/setting/setting.go index 00bcdabd88d..995a01ad825 100644 --- a/pkg/setting/setting.go +++ b/pkg/setting/setting.go @@ -2167,7 +2167,7 @@ func (cfg *Cfg) readProvisioningSettings(iniFile *ini.File) error { } cfg.ProvisioningAllowedTargets = iniFile.Section("provisioning").Key("allowed_targets").Strings("|") if len(cfg.ProvisioningAllowedTargets) == 0 { - cfg.ProvisioningAllowedTargets = []string{"instance", "folder"} + cfg.ProvisioningAllowedTargets = []string{"folder"} } cfg.ProvisioningAllowImageRendering = iniFile.Section("provisioning").Key("allow_image_rendering").MustBool(true) cfg.ProvisioningMinSyncInterval = iniFile.Section("provisioning").Key("min_sync_interval").MustDuration(10 * time.Second) diff --git a/pkg/tests/apis/provisioning/exportjob_test.go b/pkg/tests/apis/provisioning/exportjob_test.go index 2218e87ef77..5d4348d23cb 100644 --- a/pkg/tests/apis/provisioning/exportjob_test.go +++ b/pkg/tests/apis/provisioning/exportjob_test.go @@ -44,6 +44,7 @@ func TestIntegrationProvisioning_ExportUnifiedToRepository(t *testing.T) { const repo = "local-repository" testRepo := TestRepo{ Name: repo, + Target: "instance", // Export is only supported for instance sync Copies: map[string]string{}, // No initial files needed for export test ExpectedDashboards: 4, // 4 dashboards created above (v0, v1, v2alpha1, v2beta1) ExpectedFolders: 0, // No folders expected after sync @@ -177,6 +178,7 @@ func TestIntegrationProvisioning_ExportDashboardsWithStoredVersions(t *testing.T const repo = "version-test-repository" testRepo := TestRepo{ Name: repo, + Target: "instance", // Export is only supported for instance sync Copies: map[string]string{}, ExpectedDashboards: len(tests), ExpectedFolders: 0, diff --git a/pkg/tests/apis/provisioning/helper_test.go b/pkg/tests/apis/provisioning/helper_test.go index 0b6a5e3c806..791ac4b8a20 100644 --- a/pkg/tests/apis/provisioning/helper_test.go +++ b/pkg/tests/apis/provisioning/helper_test.go @@ -695,6 +695,9 @@ func runGrafana(t *testing.T, options ...grafanaOption) *provisioningTestHelper }, }, PermittedProvisioningPaths: ".|" + provisioningPath, + // Allow both folder and instance sync targets for tests + // (instance is needed for export jobs, folder for most operations) + ProvisioningAllowedTargets: []string{"folder", "instance"}, } for _, o := range options { o(&opts) diff --git a/pkg/tests/apis/provisioning/job_validation_test.go b/pkg/tests/apis/provisioning/job_validation_test.go index c54ad58500c..4c1947693f1 100644 --- a/pkg/tests/apis/provisioning/job_validation_test.go +++ b/pkg/tests/apis/provisioning/job_validation_test.go @@ -24,10 +24,10 @@ func TestIntegrationProvisioning_JobValidation(t *testing.T) { const repo = "job-validation-test-repo" testRepo := TestRepo{ Name: repo, - Target: "instance", + Target: "folder", Copies: map[string]string{}, ExpectedDashboards: 0, - ExpectedFolders: 0, + ExpectedFolders: 1, // folder sync creates a folder } helper.CreateRepo(t, testRepo) diff --git a/pkg/tests/apis/provisioning/movejob_test.go b/pkg/tests/apis/provisioning/movejob_test.go index 85d227996fd..9cd298711a9 100644 --- a/pkg/tests/apis/provisioning/movejob_test.go +++ b/pkg/tests/apis/provisioning/movejob_test.go @@ -24,14 +24,15 @@ func TestIntegrationProvisioning_MoveJob(t *testing.T) { ctx := context.Background() const repo = "move-test-repo" testRepo := TestRepo{ - Name: repo, + Name: repo, + Target: "folder", Copies: map[string]string{ "testdata/all-panels.json": "dashboard1.json", "testdata/text-options.json": "dashboard2.json", "testdata/timeline-demo.json": "folder/dashboard3.json", }, ExpectedDashboards: 3, - ExpectedFolders: 1, + ExpectedFolders: 2, // folder sync creates a folder for the repo + one nested folder } helper.CreateRepo(t, testRepo) @@ -236,6 +237,7 @@ func TestIntegrationProvisioning_MoveJob(t *testing.T) { const refRepo = "move-ref-test-repo" helper.CreateRepo(t, TestRepo{ Name: refRepo, + Target: "folder", SkipResourceAssertions: true, // HACK: I am not sure why sometimes it's 6 or 3 dashbaords. }) diff --git a/pkg/tests/apis/provisioning/repository_test.go b/pkg/tests/apis/provisioning/repository_test.go index 877ed0d5f98..3cee3cf4aba 100644 --- a/pkg/tests/apis/provisioning/repository_test.go +++ b/pkg/tests/apis/provisioning/repository_test.go @@ -578,7 +578,13 @@ func TestIntegrationProvisioning_RunLocalRepository(t *testing.T) { const targetPath = "all-panels.json" // Set up the repository. - helper.CreateRepo(t, TestRepo{Name: repo}) + helper.CreateRepo(t, TestRepo{ + Name: repo, + Target: "folder", + ExpectedDashboards: 0, + ExpectedFolders: 1, // folder sync creates a folder for the repo + SkipResourceAssertions: false, + }) // Write a file -- this will create it *both* in the local file system, and in grafana t.Run("write all panels", func(t *testing.T) { @@ -744,10 +750,10 @@ func TestIntegrationProvisioning_ImportAllPanelsFromLocalRepository(t *testing.T // Set up the repository and the file to import. testRepo := TestRepo{ Name: repo, - Target: "instance", + Target: "folder", Copies: map[string]string{"testdata/all-panels.json": "all-panels.json"}, ExpectedDashboards: 1, - ExpectedFolders: 0, + ExpectedFolders: 1, // folder sync creates a folder } // We create the repository helper.CreateRepo(t, testRepo) diff --git a/pkg/tests/apis/provisioning/stats_test.go b/pkg/tests/apis/provisioning/stats_test.go index eae3cbf7531..25ecd5c8f7c 100644 --- a/pkg/tests/apis/provisioning/stats_test.go +++ b/pkg/tests/apis/provisioning/stats_test.go @@ -21,13 +21,14 @@ func TestIntegrationProvisioning_Stats(t *testing.T) { const repo = "stats-test-repo1" testRepo := TestRepo{ - Name: repo, + Name: repo, + Target: "folder", Copies: map[string]string{ "testdata/all-panels.json": "dashboard1.json", "testdata/text-options.json": "folder/dashboard2.json", }, ExpectedDashboards: 2, - ExpectedFolders: 1, + ExpectedFolders: 2, // folder sync creates a folder for the repo + one nested folder } helper.CreateRepo(t, testRepo) @@ -94,7 +95,7 @@ func TestIntegrationProvisioning_Stats(t *testing.T) { require.Equal(t, int64(2), count, "repo should manage 2 dashboards") } else if group == "folder.grafana.app" && resource == "folders" { count, _, _ := unstructured.NestedInt64(stat, "count") - require.Equal(t, int64(1), count, "repo should manage 1 folder") + require.Equal(t, int64(2), count, "repo should manage 2 folders (repo folder + nested folder)") } } } diff --git a/pkg/tests/testinfra/testinfra.go b/pkg/tests/testinfra/testinfra.go index c84ddd6fb40..6e2f0660c32 100644 --- a/pkg/tests/testinfra/testinfra.go +++ b/pkg/tests/testinfra/testinfra.go @@ -580,6 +580,12 @@ func CreateGrafDir(t *testing.T, opts GrafanaOpts) (string, string) { _, err = pathsSect.NewKey("permitted_provisioning_paths", opts.PermittedProvisioningPaths) require.NoError(t, err) } + if len(opts.ProvisioningAllowedTargets) > 0 { + provisioningSect, err := getOrCreateSection("provisioning") + require.NoError(t, err) + _, err = provisioningSect.NewKey("allowed_targets", strings.Join(opts.ProvisioningAllowedTargets, "|")) + require.NoError(t, err) + } if opts.EnableSCIM { scimSection, err := getOrCreateSection("auth.scim") require.NoError(t, err) @@ -669,6 +675,7 @@ type GrafanaOpts struct { UnifiedStorageEnableSearch bool UnifiedStorageMaxPageSizeBytes int PermittedProvisioningPaths string + ProvisioningAllowedTargets []string GrafanaComSSOAPIToken string LicensePath string EnableRecordingRules bool diff --git a/public/app/features/provisioning/Config/ConfigForm.tsx b/public/app/features/provisioning/Config/ConfigForm.tsx index a29bc68329e..9c86403fee6 100644 --- a/public/app/features/provisioning/Config/ConfigForm.tsx +++ b/public/app/features/provisioning/Config/ConfigForm.tsx @@ -80,10 +80,7 @@ export function ConfigForm({ data }: ConfigFormProps) { const [isLoading, setIsLoading] = useState(false); const navigate = useNavigate(); const [type, readOnly] = watch(['type', 'readOnly']); - const targetOptions = useMemo( - () => getTargetOptions(settings.data?.allowedTargets || ['instance', 'folder']), - [settings.data] - ); + const targetOptions = useMemo(() => getTargetOptions(settings.data?.allowedTargets || ['folder']), [settings.data]); const isGitBased = isGitProvider(type); const { diff --git a/public/app/features/provisioning/Config/defaults.ts b/public/app/features/provisioning/Config/defaults.ts index 7a8f27ae456..19d17fb4da7 100644 --- a/public/app/features/provisioning/Config/defaults.ts +++ b/public/app/features/provisioning/Config/defaults.ts @@ -11,7 +11,7 @@ export interface GetDefaultValuesOptions { export function getDefaultValues({ repository, - allowedTargets = ['instance', 'folder'], + allowedTargets = ['folder'], }: GetDefaultValuesOptions = {}): RepositoryFormData { if (!repository) { const defaultTarget = allowedTargets.includes('folder') ? 'folder' : 'instance'; diff --git a/public/app/features/provisioning/Shared/RepositoryList.tsx b/public/app/features/provisioning/Shared/RepositoryList.tsx index 038359c6e8b..76dab485a74 100644 --- a/public/app/features/provisioning/Shared/RepositoryList.tsx +++ b/public/app/features/provisioning/Shared/RepositoryList.tsx @@ -22,6 +22,7 @@ export function RepositoryList({ items }: Props) { const filteredItems = items.filter((item) => item.metadata?.name?.includes(query)); const { instanceConnected } = checkSyncSettings(items); + const hasInstanceSyncRepo = items.some((item) => item.spec?.sync?.target === 'instance'); const getResourceCountSection = () => { if (isProvisionedInstance) { @@ -77,6 +78,17 @@ export function RepositoryList({ items }: Props) { return ( <> {getResourceCountSection()} + {hasInstanceSyncRepo && ( + + + Instance sync is currently not fully supported and breaks library panels and alerts. To use library panels + and alerts, disconnect your repository and reconnect it using folder sync instead. + + + )} {!instanceConnected && ( diff --git a/public/app/features/provisioning/Wizard/BootstrapStep.test.tsx b/public/app/features/provisioning/Wizard/BootstrapStep.test.tsx index 90d566415b9..24d7e322ac3 100644 --- a/public/app/features/provisioning/Wizard/BootstrapStep.test.tsx +++ b/public/app/features/provisioning/Wizard/BootstrapStep.test.tsx @@ -32,7 +32,7 @@ function FormWrapper({ children, defaultValues }: { children: ReactNode; default url: 'https://github.com/test/repo', title: '', sync: { - target: 'instance', + target: 'folder', enabled: true, }, branch: 'main', @@ -101,12 +101,6 @@ describe('BootstrapStep', () => { (useModeOptions as jest.Mock).mockReturnValue({ enabledOptions: [ - { - target: 'instance', - label: 'Sync all resources with external storage', - description: 'Resources will be synced with external storage', - subtitle: 'Use this option if you want to sync your entire instance', - }, { target: 'folder', label: 'Sync external storage to a new Grafana folder', @@ -142,8 +136,8 @@ describe('BootstrapStep', () => { it('should render correct info for GitHub repository type', async () => { setup(); - expect(screen.getAllByText('External storage')).toHaveLength(2); - expect(screen.getAllByText('Empty')).toHaveLength(4); // Four elements should show "Empty" (2 external + 2 unmanaged, one per card) + expect(screen.getAllByText('External storage')).toHaveLength(1); // Only folder sync is shown by default + expect(screen.getAllByText('Empty')).toHaveLength(2); // Two elements should have the role "Empty" (1 external + 1 unmanaged) }); it('should render correct info for local file repository type', async () => { @@ -171,10 +165,12 @@ describe('BootstrapStep', () => { setup(); - expect(await screen.getAllByText('2 files')).toHaveLength(2); + expect(await screen.getAllByText('2 files')).toHaveLength(1); // Only folder sync is shown by default }); it('should display resource counts when resources exist', async () => { + // Note: Resource counts are only shown for instance sync, but instance sync is not available by default + // This test is kept for when instance sync is explicitly enabled via settings (useGetResourceStatsQuery as jest.Mock).mockReturnValue({ data: { instance: [ @@ -196,10 +192,29 @@ describe('BootstrapStep', () => { shouldSkipSync: false, }); - setup(); + // Mock settings to allow instance sync for this test + (useModeOptions as jest.Mock).mockReturnValue({ + enabledOptions: [ + { + target: 'instance', + label: 'Sync all resources with external storage', + description: 'Resources will be synced with external storage', + subtitle: 'Use this option if you want to sync your entire instance', + }, + ], + disabledOptions: [], + }); - // Two elements display "7 resources": one in the external storage card and one in unmanaged resources card - expect(await screen.findAllByText('7 resources')).toHaveLength(2); + setup({ + settingsData: { + allowedTargets: ['instance', 'folder'], + allowImageRendering: true, + items: [], + availableRepositoryTypes: [], + }, + }); + + expect(await screen.findByText('7 resources')).toBeInTheDocument(); }); }); @@ -208,16 +223,30 @@ describe('BootstrapStep', () => { setup(); const mockUseResourceStats = require('./hooks/useResourceStats').useResourceStats; - expect(mockUseResourceStats).toHaveBeenCalledWith('test-repo', 'instance'); + expect(mockUseResourceStats).toHaveBeenCalledWith('test-repo', 'folder'); + }); + + it('should use useResourceStats hook with settings data', async () => { + setup({ + settingsData: { + allowedTargets: ['folder'], + allowImageRendering: true, + items: [], + availableRepositoryTypes: [], + }, + }); + + const mockUseResourceStats = require('./hooks/useResourceStats').useResourceStats; + expect(mockUseResourceStats).toHaveBeenCalledWith('test-repo', 'folder'); }); }); describe('sync target options', () => { - it('should display both instance and folder options by default', async () => { + it('should display only folder option by default', async () => { setup(); - expect(await screen.findByText('Sync all resources with external storage')).toBeInTheDocument(); expect(await screen.findByText('Sync external storage to a new Grafana folder')).toBeInTheDocument(); + expect(screen.queryByText('Sync all resources with external storage')).not.toBeInTheDocument(); }); it('should only display instance option when legacy storage exists', async () => { @@ -242,6 +271,7 @@ describe('BootstrapStep', () => { setup({ settingsData: { + allowedTargets: ['instance', 'folder'], allowImageRendering: true, items: [], availableRepositoryTypes: [], @@ -264,15 +294,10 @@ describe('BootstrapStep', () => { }); describe('title field visibility', () => { - it('should show title field only for folder sync target', async () => { - const { user } = setup(); - - // Initially should not show title field (default is instance) - expect(screen.queryByRole('textbox', { name: /display name/i })).not.toBeInTheDocument(); - - const folderOption = await screen.findByText('Sync external storage to a new Grafana folder'); - await user.click(folderOption); + it('should show title field for folder sync target', async () => { + setup(); + // Default is folder, so title field should be visible expect(await screen.findByRole('textbox', { name: /display name/i })).toBeInTheDocument(); }); }); diff --git a/public/app/features/provisioning/Wizard/ProvisioningWizard.test.tsx b/public/app/features/provisioning/Wizard/ProvisioningWizard.test.tsx index 24d23acc607..db49c3bb86e 100644 --- a/public/app/features/provisioning/Wizard/ProvisioningWizard.test.tsx +++ b/public/app/features/provisioning/Wizard/ProvisioningWizard.test.tsx @@ -227,6 +227,15 @@ describe('ProvisioningWizard', () => { error: null, refetch: jest.fn(), }); + // Mock files to ensure sync step is not skipped for folder sync + mockUseGetRepositoryFilesQuery.mockReturnValue({ + data: { + items: [{ name: 'test.json', path: 'test.json' }], + }, + isLoading: false, + error: null, + refetch: jest.fn(), + }); const { user } = setup(); await fillConnectionForm(user, 'github', { @@ -500,10 +509,10 @@ describe('ProvisioningWizard', () => { }); it('should show button text changes based on current step', async () => { - // Mock resources to ensure sync step is not skipped - mockUseGetResourceStatsQuery.mockReturnValue({ + // Mock files to ensure sync step is not skipped for folder sync + mockUseGetRepositoryFilesQuery.mockReturnValue({ data: { - instance: [{ group: 'dashboard.grafana.app', count: 1 }], + items: [{ name: 'test.json', path: 'test.json' }], }, isLoading: false, error: null, diff --git a/public/app/features/provisioning/Wizard/hooks/useModeOptions.ts b/public/app/features/provisioning/Wizard/hooks/useModeOptions.ts index 4bf817b091e..3d288ab487d 100644 --- a/public/app/features/provisioning/Wizard/hooks/useModeOptions.ts +++ b/public/app/features/provisioning/Wizard/hooks/useModeOptions.ts @@ -10,7 +10,7 @@ import { ModeOption } from '../types'; */ function filterModeOptions(modeOptions: ModeOption[], repoName: string, settings?: RepositoryViewList): ModeOption[] { const folderConnected = settings?.items?.some((item) => item.target === 'folder' && item.name !== repoName); - const allowedTargets = settings?.allowedTargets || ['instance', 'folder']; + const allowedTargets = settings?.allowedTargets || ['folder']; return modeOptions.map((option) => { if (option.disabled) { diff --git a/public/locales/en-US/grafana.json b/public/locales/en-US/grafana.json index febd4c0442e..00757c4251d 100644 --- a/public/locales/en-US/grafana.json +++ b/public/locales/en-US/grafana.json @@ -11951,6 +11951,10 @@ "unsupported-repository-type": "Unsupported repository type: {{repositoryType}}" }, "inline-secure-values-warning": "You need to save your access tokens again due to a system update", + "instance-sync-deprecation": { + "message": "Instance sync is currently not fully supported and breaks library panels and alerts. To use library panels and alerts, disconnect your repository and reconnect it using folder sync instead.", + "title": "Instance sync is not fully supported" + }, "job-status": { "label-view-details": "View details", "loading-finished-job": "Loading finished job...",