-
Notifications
You must be signed in to change notification settings - Fork 142
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
Terraform Plugin SDK Upgrade #379
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Initial upgrade to get everything compiling with the new version. Doesn't tackle any deprecations and doesn't have a working test suite yet. Changes are mostly a direct find/replace of the SDK imports, with some more significant changes: - The UserAgent generation was changed to account for an SDK change (fastly/config.go) - The hashcode package was removed from the SDK and it is needed in some of the data source code, so it has been copied back in locally (fastly/hashcode/hashcode.go) - Removal of d.SetPartial and d.Partial as it's been deprecated and doesn't seem to do anything - Addition of context to CustomizeDiffFunc as the signature has been updated in-place - A couple of lint errors that came up while trying to commit
A lot of the logging block tests for the compute service were panicking with the new SDK due to d.Set errors. These errors were ignored in the resource Read functions, which would previously not have caused an issue but now result in panics. The error was caused by trying to set VCL logging attributes on compute services when these fields aren't present in the schema. This commit avoids the error by pruning these fields on Compute services prior to calling d.Set.
Testing TypeSet elements has changed in acceptance tests. Looking up a preknown hash in the set no longer works, but there are new TestCheckTypeSetElemAttr and TestCheckTypeSetElemNestedAttrs functions for this instead which use "set.*" as the syntax. Also the test was working incorrectly with a "count" attribute. I'm guessing this worked before because a fake terraform implementation was being used, but I couldn't get this working now. I tried ....content[0] to try indexing the first instance of the "count"ed resource but it still didn't work. In the end, I noticed that it was a bit redundant anyway because there was always one thing being created, and the rest of the fields would all have been duplicated if there were multiple.
Subscription Validation resource not compiling as I need to change the retry mechanism
Used twice, once in the Service State Importer function, and once in the Subscription Validation Retry function. These are both context-aware but don't use diag.Diagnostics so need some conversion in order to work with the context-aware resource.*Read functions.
Quite a bit of coercion to get everyone happy because the signature has changed quite a bit from: func(interface{}, string) ([]string, []error) to: func(interface{}, cty.Path) diag.Diagnostics Which requires a diagsToWarnsAndErrs function and cty.GetAttrPath() in the unit tests for all of the validators. The validators themselves use the validation.ToDiagFunc() function to do this conversion which is quite simple, and also required for all of the builtin validation helper methods which have weirdly not yet been updated to SchemaValidateDiagFuncs.
…tion to diag Warning
…and remove logic for pre-v0.12.0 There was some logic for constructing the User Agent in TF versions earlier than v0.12.0 but those versions are now deprecated and unsupported by the plugin SDK anyway. Had to use the fmt string for the sweeper User Agent but it should be the same as it was before.
Doesn't fix all the errors but gets a decent bunch of them.
Integralist
approved these changes
Mar 11, 2021
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 @trentrosenbaum this LGTM.
I'll approve although I have a couple of small queries.
Integralist
reviewed
Mar 11, 2021
…port Terraform 0.11
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The focus of this PR is to update the Terraform Plugin SDK dependency from v1 to v2. The specific version at the time of this PR is v2.4.4.