-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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: azurerm_data_factory
#3159
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.
hey @mbfrahry
Thanks for this PR :)
Taking a look through this looks pretty good to me - if we can fix up the minor comments this should otherwise be good to merge 👍
Thanks!
Thanks for the review @tombuildsstuff! I believe I've addressed all your comments |
Any idea when this will make it into master? I'd like to take it to prod :-) |
dismissing since changes have been pushed
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.
Aside from a few comments i've left inline LGTM @mbfrahry
azurerm/resource_arm_data_factory.go
Outdated
"tenant_id": { | ||
Type: schema.TypeString, | ||
Required: true, | ||
ValidateFunc: validate.NoEmptyStrings, |
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.
Could this be validated as an ID?
azurerm/resource_arm_data_factory.go
Outdated
return fmt.Errorf("Error setting `vsts_configuration`: %+v", err) | ||
} | ||
} else { | ||
d.Set("vsts_configuration", []interface{}{}) |
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.
Should we move this and the
d.Set("github_configuration", []interface{}{})
up above the two checks and then remove the else statements?
azurerm/resource_arm_data_factory.go
Outdated
repositoryName := vsts["repository_name"].(string) | ||
rootFolder := vsts["root_folder"].(string) | ||
tenantID := vsts["tenant_id"].(string) | ||
// https://github.com/Azure/go-autorest/issues/307 |
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.
minor expanding on this comment as to whats going on beyond a link could be useful
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.
Hbuckle found a weird case where you can pass the struct or the address of the struct into the sdk. If you didn't pass the address though, the sdk broke. I don't think we need to track that inside Terraform though since we can just pass the address like we normally do.
resource "azurerm_data_factory" "test" { | ||
name = "acctestdf%d" | ||
location = "${azurerm_resource_group.test.location}" | ||
resource_group_name = "${azurerm_resource_group.test.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.
Could we fix the alignment 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.
Pesky tabs
resource "azurerm_data_factory" "test" { | ||
name = "acctestdf%d" | ||
location = "${azurerm_resource_group.test.location}" | ||
resource_group_name = "${azurerm_resource_group.test.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.
And here?
Hey @archmangler, this will make it into the next release 1.25.0 |
This has been released in version 1.25.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 = "~> 1.25.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! |
Adding a new resource
azurerm_data_factory
This is taking the majority of @hbuckle's work from #1720 and modifying it/updating tests.
Fixes #1720