public/grafana
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Provisioning: Default to folder sync only and block new instance sync repositories (#115569)

Browse Source 
* Default to folder sync only and block new instance sync repositories

- Change default allowed_targets to folder-only in backend configuration
- Modify validation to only enforce allowedTargets on CREATE operations
- Add deprecation warning for existing instance sync repositories
- Update frontend defaults and tests to reflect new behavior

Fixes #619

* Update warning message: change 'deprecated' to 'not fully supported'

* Fix health check: don't validate allowedTargets for existing repositories

Health checks for existing repositories should treat them as UPDATE operations,
not CREATE operations, so they don't fail validation for instance sync target.

* Fix tests and update i18n translations

- Update BootstrapStep tests to reflect folder-only default behavior
- Run i18n-extract to update translation file structure

* Fix integration tests

* Fix tests

* Fix provisioning test wizard

* Fix fronted test
This commit is contained in:
Roberto Jiménez Sánchez
2025-12-19 11:44:15 +00:00
committed by GitHub
parent b5793a5f73
commit a0751b6e71
20 changed files with 128 additions and 50 deletions
Download Patch File Download Diff File Expand all files Collapse all files
apps/provisioning/pkg/repository/tester.go
+7 -1
View File
@@ -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
apps/provisioning/pkg/repository/validator.go
+4 -2
View File
@@ -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"),
apps/provisioning/pkg/repository/validator_test.go
+2 -1
View File
@@ -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)
conf/defaults.ini
+1 -1
View File
@@ -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.
pkg/registry/apis/provisioning/register.go
+2 -1
View File
@@ -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 {
pkg/setting/setting.go
+1 -1
View File
@@ -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)
pkg/tests/apis/provisioning/exportjob_test.go
+2
View File
@@ -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,
pkg/tests/apis/provisioning/helper_test.go
+3
View File
@@ -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)
pkg/tests/apis/provisioning/job_validation_test.go
+2 -2
View File
@@ -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)
pkg/tests/apis/provisioning/movejob_test.go
+4 -2
View File
@@ -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.
})
pkg/tests/apis/provisioning/repository_test.go
+9 -3
View File
@@ -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)
pkg/tests/apis/provisioning/stats_test.go
+4 -3
View File
@@ -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)")
}
}
}
pkg/tests/testinfra/testinfra.go
+7
View File
@@ -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
public/app/features/provisioning/Config/ConfigForm.tsx
+1 -4
View File
@@ -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 {
public/app/features/provisioning/Config/defaults.ts
+1 -1
View File
@@ -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';
public/app/features/provisioning/Shared/RepositoryList.tsx
+12
View File
@@ -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 && (
<Alert
title={t('provisioning.instance-sync-deprecation.title', 'Instance sync is not fully supported')}
severity="warning"
>
<Trans i18nKey="provisioning.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.
</Trans>
</Alert>
)}
<Stack direction={'column'} gap={3}>
{!instanceConnected && (
<Stack gap={2}>
public/app/features/provisioning/Wizard/BootstrapStep.test.tsx
+49 -24
View File
@@ -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();
});
});
public/app/features/provisioning/Wizard/ProvisioningWizard.test.tsx
+12 -3
View File
@@ -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(<ProvisioningWizard type="github" />);
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,
public/app/features/provisioning/Wizard/hooks/useModeOptions.ts
+1 -1
View File
@@ -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) {
public/locales/en-US/grafana.json
+4
View File
@@ -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...",