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

INTMDB-183: Migrate to TF SDK 2 #489

Merged
merged 23 commits into from
Jul 27, 2021
Merged

INTMDB-183: Migrate to TF SDK 2 #489

merged 23 commits into from
Jul 27, 2021

Conversation

coderGo93
Copy link
Contributor

@coderGo93 coderGo93 commented Jul 1, 2021

Description

  • Migrated to SDK Version 2
  • Because of bad practice and is not supported for sdk v2, using TypeMap as object will show error with make test, so the fix was adding another attribute and deprecate the old attribute, and for the new attributes was changed to TypeList with MaxItem 1, resources/datasources affected are alert_configuration, cloud_provider_snapshot_restore_job, mongodbatlas_encryption_at_rest

Link to any related issue(s):

Type of change:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Documentation fix/enhancement

Required Checklist:

  • I have signed the MongoDB CLA
  • I have read the Terraform contribution guidelines
  • I have added tests that prove my fix is effective or that my feature works per HashiCorp requirements
  • I have added any necessary documentation (if appropriate)
  • I have run make fmt and formatted my code

Further comments

@coderGo93 coderGo93 requested a review from abner-dou July 1, 2021 00:38
@coderGo93 coderGo93 marked this pull request as draft July 1, 2021 00:38
@coderGo93 coderGo93 added run-testacc To run acceptance tests and removed run-testacc To run acceptance tests labels Jul 13, 2021
@coderGo93 coderGo93 marked this pull request as ready for review July 14, 2021 17:07
@coderGo93 coderGo93 changed the title WIP: INTMDB-183: Migrate to TF SDK 2 INTMDB-183: Migrate to TF SDK 2 Jul 14, 2021
@coderGo93 coderGo93 requested a review from thetonymaster July 14, 2021 17:07
@coderGo93 coderGo93 self-assigned this Jul 14, 2021
@coderGo93 coderGo93 requested a review from themantissa July 14, 2021 17:12
@coderGo93 coderGo93 added run-testacc To run acceptance tests and removed run-testacc To run acceptance tests labels Jul 16, 2021
Copy link
Collaborator

@themantissa themantissa left a comment

Choose a reason for hiding this comment

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

Just a few docs edits, otherwise looks good. I'll create a ticket to update search and backup after they are merged. Just a note for the future - let's break these huge PRs up. For example this could have been a PR for the examples, a PR for docs, and a PR for the work (which we could maybe break up?) Will just help with review speed and accuracy.


```

Actual usage:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Replace "Actual usage" with "New usage" for all please.

@@ -79,6 +79,155 @@ so no changes are needed.
**NOTE** Doc links for [mongodbatlas_privatelink_endpoint](https://registry.terraform.io/providers/mongodb/mongodbatlas/latest/docs/resources/privatelink_endpoint)
**NOTE** Doc links for [mongodbatlas_privatelink_endpoint_service](https://registry.terraform.io/providers/mongodb/mongodbatlas/latest/docs/resources/privatelink_endpoint_service)

## Migration to Terraform SDK v2

Because of migration to terraform sdk v2, in various resources and datasource(s) are not supported using TypeMap as object, instead it should use TypeSet or TypeList with MaxItems:1, here the list of resources affected by migration
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just a rewrite to make a bit more clear, please replace this with:
Due to the migration to Terraform SDK v2 various resources and datasource(s) have been impacted and will require a change within your configuration. These resources and datasources used the now unsupported TypeMap object. In SDK v2 these must be TypeSet or TypeList with MaxItems set to 1. The following is the list of resources and datasources impacted by the migration.

@themantissa themantissa requested a review from a team July 20, 2021 23:16
},
},
"aws_config": {
Type: schema.TypeList,

Choose a reason for hiding this comment

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

[Question] For my own understanding and learning. Why use TypeList with maxlen 1 instead of TypeStr?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because TypeList with MaxItems 1 ideally it's a object from the API, and TypeSet is an unordered list

Choose a reason for hiding this comment

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

ah, I miss typed it. I meant why not use TypeString?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:O, I misread it then haha, well, because aws_config is a object with two fields

Copy link

@threebee threebee left a comment

Choose a reason for hiding this comment

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

LGTM, if test pass.

This PR was too big to review for me. I'm afraid that I might have missed something.

@coderGo93 coderGo93 added run-testacc To run acceptance tests and removed run-testacc To run acceptance tests labels Jul 27, 2021
@coderGo93 coderGo93 merged commit 1fcff25 into master Jul 27, 2021
@coderGo93 coderGo93 deleted the INTMDB-183 branch July 27, 2021 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-testacc To run acceptance tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants