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

New Resource & Data Source: azurerm_database_migration_project #5993

Merged
merged 4 commits into from
Mar 5, 2020

Conversation

magodo
Copy link
Collaborator

@magodo magodo commented Mar 5, 2020

Implement azurerm_database_migration_project.

See #5257 for more details.

@magodo
Copy link
Collaborator Author

magodo commented Mar 5, 2020

💤  terraform-provider-azurerm-data-migration [database_migration_project] ⚡  make testacc TEST="./azurerm/internal/services/databasemigration/tests" TESTARGS="-parallel=1 -run='TestAccAzureRMDatabaseMigrationProject_'"
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
TF_ACC=1 go test ./azurerm/internal/services/databasemigration/tests -v -parallel=1 -run='TestAccAzureRMDatabaseMigrationProject_' -timeout 180m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccAzureRMDatabaseMigrationProject_basic
=== PAUSE TestAccAzureRMDatabaseMigrationProject_basic
=== RUN   TestAccAzureRMDatabaseMigrationProject_complete
--- PASS: TestAccAzureRMDatabaseMigrationProject_complete (1353.02s)
=== RUN   TestAccAzureRMDatabaseMigrationProject_requiresImport
--- PASS: TestAccAzureRMDatabaseMigrationProject_requiresImport (1291.17s)
=== RUN   TestAccAzureRMDatabaseMigrationProject_update
--- PASS: TestAccAzureRMDatabaseMigrationProject_update (1402.37s)
=== CONT  TestAccAzureRMDatabaseMigrationProject_basic
--- PASS: TestAccAzureRMDatabaseMigrationProject_basic (1183.26s)
PASS
ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/databasemigration/tests     5229.835s

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @magodo, this is off to a great start. Left some comments inline that once addressed this should be good to merge.

@magodo
Copy link
Collaborator Author

magodo commented Mar 5, 2020

Hi @katbyte , really appreaciate your quick response. I have fixed the issue you pointed and also the ones in CI.

💤  terraform-provider-azurerm-data-migration [database_migration_project] ⚡  make testacc TEST="./azurerm/internal/services/databasemigration/tests" TESTARGS="-parallel=1 -run='TestAccAzureRMDatabaseMigrationProject_'"
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
TF_ACC=1 go test ./azurerm/internal/services/databasemigration/tests -v -parallel=1 -run='TestAccAzureRMDatabaseMigrationProject_' -timeout 180m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccAzureRMDatabaseMigrationProject_basic
=== PAUSE TestAccAzureRMDatabaseMigrationProject_basic
=== RUN   TestAccAzureRMDatabaseMigrationProject_complete
--- PASS: TestAccAzureRMDatabaseMigrationProject_complete (1175.76s)
=== RUN   TestAccAzureRMDatabaseMigrationProject_requiresImport
--- PASS: TestAccAzureRMDatabaseMigrationProject_requiresImport (1339.03s)
=== RUN   TestAccAzureRMDatabaseMigrationProject_update
--- PASS: TestAccAzureRMDatabaseMigrationProject_update (1435.03s)
=== CONT  TestAccAzureRMDatabaseMigrationProject_basic
--- PASS: TestAccAzureRMDatabaseMigrationProject_basic (2227.40s)
PASS
ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/databasemigration/tests     6177.248s

@ghost ghost removed the waiting-response label Mar 5, 2020
@magodo magodo requested a review from katbyte March 5, 2020 08:43
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Thanks @magodo, just a couple minor comments left and this should be good to merge

Comment on lines 146 to 148
name := id.Name
resourceGroup := id.ResourceGroup
serviceName := id.Service
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need to assign these to locals?

Comment on lines 188 to 190
name := id.Name
resourceGroup := id.ResourceGroup
serviceName := id.Service
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need to assign these to locals?

Comment on lines 124 to 126
name := id.Name
resourceGroup := id.ResourceGroup
serviceName := id.Service
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need to assign these to locals?

Comment on lines 155 to 157
name := id.Name
resourceGroup := id.ResourceGroup
serviceName := id.Service
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need to assign these to locals?

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Hope you don't mind but i pushed the changes required to get this merged 🙂 LGTM now 👍

@katbyte katbyte merged commit 55e456c into hashicorp:master Mar 5, 2020
katbyte added a commit that referenced this pull request Mar 5, 2020
@ghost
Copy link

ghost commented Mar 11, 2020

This has been released in version 2.1.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.1.0"
}
# ... other configuration ...

@ghost ghost removed the waiting-response label Mar 11, 2020
@ghost
Copy link

ghost commented Apr 5, 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. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

@ghost ghost locked and limited conversation to collaborators Apr 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants