-
Notifications
You must be signed in to change notification settings - Fork 9.5k
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
provider/azurerm: add servicebus_topic resource #9151
Conversation
azure-sdk-for-go@5dbdd3e002c0c232938bf953a5e7fa9a58ee749e go-autorest@928711bfb9b6bc052ea85a8f4e1d8f4e1bf55f95
Hi @pmcatominey This LGTM! 1 small nit that isn't a limiting factor - just running the tests now P. |
ResourceName: resourceName, | ||
ImportState: true, | ||
ImportStateVerify: true, | ||
ImportStateVerifyIgnore: []string{"resource_group_name"}, |
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.
We shouldn't have to ignore this anymore - if you look at PR #9073 for details on how to handle this :)
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, fixed and noted :)
return err | ||
} | ||
|
||
read, err := client.Get(resGroup, namespaceName, name) |
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.
Is there any eventual consistency here or can we guarantee that this will be immediately available?
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.
Looking at the logs before, the topic appears to be returned in the body of the create request
51e0cb7
to
32e0bee
Compare
I've just updated with the resource group name importing changes, recommend you run the tests again to be sure :). |
Initial test run:
Will run the others when I am off this flight :) |
32e0bee
to
74533a8
Compare
TF_ACC=1 go test ./builtin/providers/azurerm -v -run TestAccAzureRMServiceBusTopic -timeout 120m === RUN TestAccAzureRMServiceBusTopic_importBasic --- PASS: TestAccAzureRMServiceBusTopic_importBasic (328.72s) === RUN TestAccAzureRMServiceBusTopic_basic --- PASS: TestAccAzureRMServiceBusTopic_basic (331.04s) === RUN TestAccAzureRMServiceBusTopic_update --- PASS: TestAccAzureRMServiceBusTopic_update (348.69s) PASS ok github.com/hashicorp/terraform/builtin/providers/azurerm 1008.588s
74533a8
to
cdbf0d7
Compare
Pushed again with the updated azurerm layout. |
Hi @pmcatominey Thanks for the work here - just ran the latest tests and all looks good to me!
Paul |
@pmcatominey I am going to revert this PR until the timestamp issue is fixed |
@stack72 no problem, you'll need to revert subscriptions too. On Wed, 5 Oct 2016, 00:49 Paul Stack, [email protected] wrote:
|
No need to revert now as the timestamp issue is fixed by bumping to go-autorest 7.2.1 :) |
Awesome! On Wed, 5 Oct 2016, 08:52 Paul Stack, [email protected] wrote:
|
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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
Includes a small bump of SDK version to pull in go-autorest time parsing fix.