Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not delete dashboard uid fields #136

Merged
merged 2 commits into from
Dec 4, 2020

Conversation

gw0
Copy link
Contributor

@gw0 gw0 commented Nov 4, 2020

The purpose of the uid field is to allow migrating dashboards between Grafana instances and provisioning Grafana from configuration without breaking references. From my understanding this field is not instance-specific (such as id), therefore it should be preserved.

@gw0
Copy link
Contributor Author

gw0 commented Nov 4, 2020

I believe this also causes weird folder issues (a duplicate gets placed into General) and consequently there are issues with multiple slugs (because the slug-based Grafana API is being used).

Copy link
Member

@trotttrotttrott trotttrotttrott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change makes sense to me, but it's causing some acceptance tests to fail (really need to add those to CI).

@gw0 gw0 force-pushed the gw0/dont-remove-uid branch from a58dc41 to 9cd1a09 Compare December 2, 2020 09:35
@gw0
Copy link
Contributor Author

gw0 commented Dec 2, 2020

You are right. I rebased and updated the acceptance tests to include the uid field. Its value should be retained if everything works correctly.

Now only the acceptance test that was failing before this PR is still failing:

$ GRAFANA_URL=http://172.17.0.1:3000 GRAFANA_AUTH=admin:admin make testacc
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v  -timeout 120m
?   	github.com/terraform-providers/terraform-provider-grafana	[no test files]
=== RUN   TestProvider
--- PASS: TestProvider (0.00s)
=== RUN   TestProvider_impl
--- PASS: TestProvider_impl (0.00s)
=== RUN   TestAccAlertNotification_basic
--- PASS: TestAccAlertNotification_basic (0.20s)
=== RUN   TestAccAlertNotification_invalid_frequence
    TestAccAlertNotification_invalid_frequence: testing.go:561: Step 0, expected error:
        
        errors during apply: time: invalid duration hi
        
        To match:
        
        invalid duration "hi"
        
        
--- FAIL: TestAccAlertNotification_invalid_frequence (0.04s)
=== RUN   TestAccAlertNotification_reminder_no_frequence
--- PASS: TestAccAlertNotification_reminder_no_frequence (0.04s)
=== RUN   TestAccDashboard_basic
--- PASS: TestAccDashboard_basic (0.31s)
=== RUN   TestAccDashboard_folder
--- PASS: TestAccDashboard_folder (0.34s)
=== RUN   TestAccDashboard_disappear
--- PASS: TestAccDashboard_disappear (0.15s)
=== RUN   TestAccDataSource_basic
--- PASS: TestAccDataSource_basic (2.14s)
=== RUN   TestAccFolderPermission_basic
--- PASS: TestAccFolderPermission_basic (0.54s)
=== RUN   TestAccFolder_basic
--- PASS: TestAccFolder_basic (0.23s)
=== RUN   TestAccOrganization_basic
--- PASS: TestAccOrganization_basic (0.46s)
=== RUN   TestAccOrganization_users
--- PASS: TestAccOrganization_users (0.71s)
=== RUN   TestAccOrganization_defaultAdmin
--- PASS: TestAccOrganization_defaultAdmin (0.34s)
=== RUN   TestAccTeamPreferences_basic
--- PASS: TestAccTeamPreferences_basic (0.50s)
=== RUN   TestAccTeam_basic
--- PASS: TestAccTeam_basic (0.34s)
=== RUN   TestAccTeam_Members
--- PASS: TestAccTeam_Members (0.40s)
=== RUN   TestAccUser_basic
--- PASS: TestAccUser_basic (0.39s)
FAIL
FAIL	github.com/terraform-providers/terraform-provider-grafana/grafana	7.151s
FAIL
GNUmakefile:16: recipe for target 'testacc' failed
make: *** [testacc] Error 1

Copy link
Member

@trotttrotttrott trotttrotttrott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@trotttrotttrott trotttrotttrott merged commit c8aa21a into grafana:master Dec 4, 2020
@ghmeier
Copy link

ghmeier commented Dec 23, 2020

Hi! Does this mean that the uid field is now required? This causes all resources to be recreated on every apply unless the uid is specified @trotttrotttrott.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants