-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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_iothub_digital_twins
#9212
Conversation
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.
@yupwei68 Thank you for this PR, it's looking really good so far but I left a few comments. If you can address those I think this will be good to go! 🚀
azurerm/internal/services/iothub/iothub_digital_twins_resource.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/iothub/iothub_digital_twins_resource.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/iothub/iothub_digital_twins_resource.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/iothub/iothub_digital_twins_resource.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/iothub/tests/iothub_digital_twins_resource_test.go
Outdated
Show resolved
Hide resolved
Thanks @WodansSon for your comments. Corresponding changes have been pushed. Please continue reviewing. |
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.
@yupwei68 Almost there, I think there was a search and replace error, but get those two issues fixed up and this resource LGTM! 🚀
azurerm/internal/services/iothub/iothub_digital_twins_resource.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/iothub/iothub_digital_twins_resource.go
Outdated
Show resolved
Hide resolved
@WodansSon Sorry for the mistakes. Please continue reviewing. |
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.
@yupwei68 Thanks for pushing those changes, this LGTM now! 🚀
} | ||
|
||
if !regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9-]+[A-Za-z0-9]$`).MatchString(v) { | ||
errors = append(errors, fmt.Errorf("expected value of %s does not match regular expression, got %v", k, v)) |
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.
this error message isn't helpful to users - can we explain the validation requirements here?
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.
Fixed in PR #9430
if err != nil { | ||
return err | ||
} | ||
d.SetId(id.ID(subscriptionId)) |
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 can remove all of this in favour of using an ID Formatter directly here?
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.
Fixed in PR #9430
} | ||
|
||
if err := future.WaitForCompletionRef(ctx, client.Client); err != nil { | ||
return fmt.Errorf("waiting on creating future for Digital Twins %q (Resource Group %q): %+v", name, resourceGroup, err) |
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.
this isn't meaningful to users - shouldn't this be "waiting for creation of Digital Twins.."?
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.
Fixed in PR #9430
} | ||
} | ||
if existing.ID != nil && *existing.ID != "" { | ||
return tf.ImportAsExistsError("azurerm_iothub_digital_twins", *existing.ID) |
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.
(as below) shouldn't this be using the ID Formatter to ensure it's consistent?
@@ -1,6 +1,7 @@ | |||
package client | |||
|
|||
import ( | |||
"github.com/Azure/azure-sdk-for-go/services/digitaltwins/mgmt/2020-10-31/digitaltwins" |
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.
Actually why was this added to the iothub service? this is a new package import so shouldn't it be a new service package??
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.
Fixed in PR #9430
|
||
# azurerm_iothub_digital_twins | ||
|
||
Manages a Digital Twins. |
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.
Digital Twin what? instance? something else?
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.
Fixed in PR #9430
@@ -21,6 +21,7 @@ func (r Registration) WebsiteCategories() []string { | |||
// SupportedDataSources returns the supported Data Sources supported by this Service | |||
func (r Registration) SupportedDataSources() map[string]*schema.Resource { | |||
return map[string]*schema.Resource{ | |||
"azurerm_iothub_digital_twins": dataSourceDigitalTwins(), |
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.
THis is a digital twin what? or will there not be any other resources for digital twins? azurerm_digital_twins_??????
instance?
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.
Fixed in PR #9430
This has been released in version 2.38.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.38.0"
}
# ... other configuration ... |
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! |
Fix #9211
=== RUN TestAccAzureRMDigitalTwins_basic
=== PAUSE TestAccAzureRMDigitalTwins_basic
=== CONT TestAccAzureRMDigitalTwins_basic
--- PASS: TestAccAzureRMDigitalTwins_basic (170.28s)
=== RUN TestAccAzureRMDigitalTwins_requiresImport
=== PAUSE TestAccAzureRMDigitalTwins_requiresImport
=== CONT TestAccAzureRMDigitalTwins_requiresImport
--- PASS: TestAccAzureRMDigitalTwins_requiresImport (147.93s)
=== RUN TestAccAzureRMDigitalTwins_complete
=== PAUSE TestAccAzureRMDigitalTwins_complete
=== CONT TestAccAzureRMDigitalTwins_complete
--- PASS: TestAccAzureRMDigitalTwins_complete (170.48s)
=== RUN TestAccAzureRMDigitalTwins_update
=== PAUSE TestAccAzureRMDigitalTwins_update
=== CONT TestAccAzureRMDigitalTwins_update
--- PASS: TestAccAzureRMDigitalTwins_update (284.05s)
Process finished with exit code 0