-
Notifications
You must be signed in to change notification settings - Fork 229
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
Upgrade to Terraform Plugin SDK v2 #176
Conversation
Intermediary step for migrating to standalone SDK.
Tool automatically migrates to standalone SDK.
Resolves, `panic: Invalid address to set: []string{"id"}`
Clearly bugs. Started making tests fail w/ SDK v2
Resolves, `panic: Invalid address to set: []string{"id"}`
Grafana will set these to 0. Defaulting to -1 creates drift after created in an initial apply.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed it commit by commit. Most changes are basically using the new imports and some minor fixes. Without having too much context about the provider, looks good to me 👍 Also neat that the upgrade already helped fixing some minor issues (drift 😄 ).
@@ -37,12 +37,12 @@ func ResourceDashboardPermission() *schema.Resource { | |||
"team_id": { | |||
Type: schema.TypeInt, | |||
Optional: true, | |||
Default: -1, | |||
Default: 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious about the reason behind this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With SDKv2, the following acceptance test failure was occurring for both dashboard and folder permissions:
=== RUN TestAccFolderPermission_basic
resource_folder_permission_test.go:16: Step 1/2 error: After applying this test step and performing a `terraform refresh`, the plan was not empty.
stdout
An execution plan has been generated and is shown below.
Resource actions are indicated with the following symbols:
~ update in-place
Terraform will perform the following actions:
# grafana_folder_permission.testPermission will be updated in-place
~ resource "grafana_folder_permission" "testPermission" {
id = "0XBNl58Gk"
# (1 unchanged attribute hidden)
+ permissions {
+ permission = "Admin"
+ team_id = -1
+ user_id = 3
}
- permissions {
- permission = "Admin" -> null
- team_id = 0 -> null
- user_id = 3 -> null
}
+ permissions {
+ permission = "Edit"
+ role = "Editor"
+ team_id = -1
+ user_id = -1
}
- permissions {
- permission = "Edit" -> null
- role = "Editor" -> null
- team_id = 0 -> null
- user_id = 0 -> null
}
+ permissions {
+ permission = "View"
+ team_id = 2
+ user_id = -1
}
- permissions {
- permission = "View" -> null
- team_id = 2 -> null
- user_id = 0 -> null
}
+ permissions {
+ permission = "View"
+ role = "Viewer"
+ team_id = -1
+ user_id = -1
}
- permissions {
- permission = "View" -> null
- role = "Viewer" -> null
- team_id = 0 -> null
- user_id = 0 -> null
}
}
Plan: 0 to add, 1 to change, 0 to destroy.
--- FAIL: TestAccFolderPermission_basic (22.75s)
Terraform was writing -1
for default values, but terraform refresh
showed that 0
is what Grafana uses for unset foreign key values. 0
makes sense given it's the default for int64
(DashboardAclUpdateItem).
It seems odd though that this did not cause drift without terraform refresh
. Typically resources are refreshed as part of creating an execution plan. I suspect the answer is in the resources' read functions, but I'd have to investigate further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for explaining. Definitely odd there wasn't drift. I wonder if this wouldn't be masquerading some obscure issue down the line 🤔
Minimal changes to migrate to SDK v2. Closes #159. Additional SDK v2 feature integrations will be done in a separate changeset (#175).
tf-sdk-migrator was primary used for code changes. Minor changes had to be made manually to fix acceptance tests.
Support for Terraform 0.11 and Below is being dropped. The readme now includes a note about a Terraform 0.12+ requirement.
This will not incur breaking changes for existing codebases (assuming use of Terraform 0.12+).