[WIP] - Add HDInsight provider #814
Conversation
There was a problem hiding this comment.
Thanks for this PR - I've taken a look through and left some comments in-line, but this is off to a good start :)
Most of the comments I've made are around matching the design of the Azure API to both ease migration and to be able to handle future changes; also since this resource is quite large, I’ve made a few suggestions to focus on just the required attributes for the time being - we can add the optional attributes in the future
Thanks!
azurerm/provider.go
Outdated
| "azurerm_snapshot": dataSourceArmSnapshot(), | ||
| "azurerm_subnet": dataSourceArmSubnet(), | ||
| "azurerm_subscription": dataSourceArmSubscription(), | ||
| "azurerm_virtual_network": dataSourceArmVirtualNetwork(), | ||
| "azurerm_virtual_network_gateway": dataSourceArmVirtualNetworkGateway(), |
There was a problem hiding this comment.
this looks like a merge conflict - can we add these back in?
azurerm/config.go
Outdated
|
|
||
| return &client, nil | ||
| } | ||
|
|
||
| func (c *ArmClient) registerHDIInsightClients(endpoint, subscriptionID string, auth autorest.Authorizer, sender autorest.Sender) { |
There was a problem hiding this comment.
minor - can we update the method name to registerHDInsightsClients to match the other clients?
Also - since HDInsights is in a new Resource Provider (e.g. Microsoft.Compute/Microsoft.HDInsight) - we need to add Microsoft.HDInsight to the list of Resource Providers which gets automatically gets registered (note: this is case-sensitive). This means that Terraform users don't have to navigate to the Portal and manually activate the HDInsights Resource Provider (or, attempt to deploy an HDInsights Cluster via the Portal) - unless they opt-out of this behaviour.
azurerm/config.go
Outdated
| hde.Authorizer = auth | ||
| hde.Sender = sender | ||
| hde.SkipResourceProviderRegistration = c.skipProviderRegistration | ||
| c.hdiExtentionClient = hde |
There was a problem hiding this comment.
since we're not using the extensions client at this time - could we remove the registration (and the field?)
azurerm/config.go
Outdated
| hda.Authorizer = auth | ||
| hda.Sender = sender | ||
| hda.SkipResourceProviderRegistration = c.skipProviderRegistration | ||
| c.hdiApplicationClient = hda |
There was a problem hiding this comment.
since we're not using the applications client at this time - could we remove the registration (and the field?)
azurerm/config.go
Outdated
| hdc.Authorizer = auth | ||
| hdc.Sender = sender | ||
| hdc.SkipResourceProviderRegistration = c.skipProviderRegistration | ||
| c.hdiClusterClient = hdc |
There was a problem hiding this comment.
minor can we update this to the newer format:
clustersClient := hdinsight.NewClustersClientWithBaseURI(endpoint, subscriptionID)
c.configureClient(&clustersClient.Client, auth)
c.hdiClusterClient = clustersClient
| }, | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
can we split the three of these out into their own sub-list to match the Azure API? e.g.
"os_profile": {
Type: schema.TypeSet,
Optional: true,
MaxItems: 1,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"username": {
Type: schema.TypeString,
Required: true,
},
"password": {
Type: schema.TypeString,
Optional: true,
Sensitive: true,
},
"ssh_public_key": {
Type: schema.TypeString,
Optional: true,
},
},
},
},
azurerm/resource_arm_hdinsight.go
Outdated
| "subnet_name": { | ||
| Type: schema.TypeString, | ||
| Required: true, | ||
| }, |
There was a problem hiding this comment.
can we split these two out into their own network_profile block to match the Azure API? e.g.
"network_profile": {
Type: schema.TypeList,
Optional: true,
MaxItems: 1,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"virtual_network_id": {
Type: schema.TypeString,
Required: true,
},
"subnet_name": {
Type: schema.TypeString,
Required: true,
},
},
},
},
| Type: schema.TypeInt, | ||
| Optional: true, | ||
| }, | ||
| "scripts": { |
There was a problem hiding this comment.
Given Custom Scripts are optional (and this is a large resource) - I think it might be worth removing these from the initial PR. We can add support for Custom Scripts back in a subsequent PR - however I think that they're going to require a few tests, would be easier to review separately - what do you think?
There was a problem hiding this comment.
Yes we could move Custom Scripts on next PR
azurerm/resource_arm_hdinsight.go
Outdated
| } | ||
|
|
||
| /* | ||
| func resourceArmHDInsightUpdate(d *schema.ResourceData, meta interface{}) error { |
There was a problem hiding this comment.
it looks like it's possible to update the size/count of the machines; so can we add this back in at some point?
azurerm/resource_arm_hdinsight.go
Outdated
| // return fmt.Errorf() | ||
| } | ||
| } | ||
| // not implemented in azure-sdk |
There was a problem hiding this comment.
There was a problem hiding this comment.
Yes
o Azure Datalake store is configure throught configuration section with core-site and cluster_identity properties
|
You can close this pull request in favor of PR #924 that is most advanced |
|
@jeanfrancoislelezec sorry I didn't see your PR before I made mine! I'm not sure mine is more 'advanced' - it's certainly not as clean as yours. I'll review yours next week and see if I can blend the 2 and we can combine our efforts. Thanks. |
|
@jjcollinge definitely your version is more advanced. I've still an issue on read method :(; I can give a hand on yours :) |
|
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
locked and limited conversation to collaborators
Please find a first commit HDIsnsight
For the moment create operation isn't working due to an issue on configuration property. On the go api the field "Configuration'" is type of *map[string]interface{} and on azure side ( rest api) the structure is
configurations": {
"gateway": {
"restAuthCredential.isEnabled": true,
"restAuthCredential.username": "http-user",
"restAuthCredential.password": "password"
},
I've tried to deal with json structure but desrialization occured on azure side
And I can not add hdinsinght go api properly in the project
Thanks for your help
#37