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.

Sign up for GitHub

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

Jump to bottom

Upgrade to TF SDKv2 #14432

Merged
merged 15 commits into from
Aug 11, 2020
Merged

Upgrade to TF SDKv2 #14432

merged 15 commits into from
Aug 11, 2020

Conversation

appilon
Copy link
Contributor

@appilon appilon commented Jul 31, 2020 •
edited
Loading

At long at last. Please note importing V1 of the SDK is still present for some helper packages in V1 (encrypt, hashcode, etc). The official recommendation is to internalize these packages or find better maintained third party libraries. I will follow up with internalizing these dependencies.

The two go modules can be imported side by side cleanly because the helper packages are "leaf" packages and don't intermix with the types throughout the SDK codebase. HOWEVER, be wary that your editor may choose to import V1 of the SDK when creating a new resource. This will result in an odd compilation error with something along the lines of cannot use type *schema.Provider as *schema.Provider, the reason of course is the provider.go file has been migrated to V2 SDK and is expecting V2 SDK namespaced types.

This annoyance will go away once the V1 dependency is removed

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Closed #12087

Release note for CHANGELOG:

NONE

Output from acceptance testing:

No significant error rate increase or runtime increase!

@appilon appilon requested a review from a team July 31, 2020 15:19
@ghost ghost added the size/XXL Managed by automation to categorize the size of a PR. label Jul 31, 2020
@bflad bflad mentioned this pull request Jul 31, 2020
Closed
18 tasks
@appilon
Copy link
Contributor Author

appilon commented Jul 31, 2020

@bflad I seem to have upset the linter, any chance I can get an assist on making that happy? I thought upgrading the import path to V2 SDK would be all I need.

@bflad
Copy link
Contributor

bflad commented Jul 31, 2020

@appilon my presumption is that tfproviderlint (used by awsproviderlint) will need to be updated to support the new module paths (github.com/hashicorp/terraform-plugin-sdk/... versus github.com/hashicorp/terraform-plugin-sdk/v2/...). Created a tracking issue here: bflad/tfproviderlint#178

@bflad
Copy link
Contributor

bflad commented Aug 5, 2020

tfproviderlint v0.15.0 has been merged with SDK v2 support -- hopefully a rebase will resolve that particular issue. 🤞

@appilon appilon force-pushed the sdkv2 branch 3 times, most recently from c6e0960 to 1c18c4c Compare August 7, 2020 20:11
bflad
bflad reviewed Aug 10, 2020
View reviewed changes
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Going through the file diffs now, but dropping this here so it can be done before I'm done.

.hashibot.hcl Outdated Show resolved Hide resolved
.hashibot.hcl
@@ -35,6 +35,30 @@ behavior "deprecated_import_commenter" "hashicorp_terraform" {
EOF
}

behavior "deprecated_import_commenter" "sdkv1" {
import_regexp = "github.com/hashicorp/terraform-plugin-sdk/helper/(schema|resource|acctest)"
marker_label = "terraform-plugin-sdk-v1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please add this new label in infrastructure/repository/labels-workflow.tf (using same color as the other label)?

.hashibot.hcl Outdated Show resolved Hide resolved
@appilon @bflad
Apply suggestions from code review
cd74ace 
Co-authored-by: Brian Flad <bflad417@gmail.com>
bflad
bflad approved these changes Aug 11, 2020
View reviewed changes
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Let's do it. 🚀

@appilon appilon merged commit 3b002df into master Aug 11, 2020
@appilon appilon deleted the sdkv2 branch August 11, 2020 00:09
@bflad bflad mentioned this pull request Aug 11, 2020
Closed
@appilon appilon mentioned this pull request Aug 11, 2020
Merged
@robbruce robbruce mentioned this pull request Aug 28, 2020
Merged
@ghost
Copy link

ghost commented Sep 10, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators Sep 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bflad bflad bflad approved these changes

Assignees
No one assigned
Labels
size/XXL Managed by automation to categorize the size of a PR.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Upgrade to Terraform Plugin SDK v2
2 participants
@appilon @bflad