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: azurerm_service_fabric_cluster #4

Merged

Conversation

lstolyarov
Copy link
Contributor

@lstolyarov lstolyarov commented Jun 11, 2017

Fixes #541

leostolyarov and others added 8 commits July 29, 2018 09:25
Tests pass:

```
$ acctests azurerm TestAccAzureRMServiceFabricCluster_
=== RUN   TestAccAzureRMServiceFabricCluster_basic
--- PASS: TestAccAzureRMServiceFabricCluster_basic (65.66s)
=== RUN   TestAccAzureRMServiceFabricCluster_addOnFeatures
--- PASS: TestAccAzureRMServiceFabricCluster_addOnFeatures (54.47s)
=== RUN   TestAccAzureRMServiceFabricCluster_certificate
--- PASS: TestAccAzureRMServiceFabricCluster_certificate (68.11s)
=== RUN   TestAccAzureRMServiceFabricCluster_clientCertificateThumbprint
--- PASS: TestAccAzureRMServiceFabricCluster_clientCertificateThumbprint (59.19s)
=== RUN   TestAccAzureRMServiceFabricCluster_diagnosticsConfig
--- PASS: TestAccAzureRMServiceFabricCluster_diagnosticsConfig (82.73s)
=== RUN   TestAccAzureRMServiceFabricCluster_fabricSettings
--- PASS: TestAccAzureRMServiceFabricCluster_fabricSettings (61.09s)
=== RUN   TestAccAzureRMServiceFabricCluster_fabricSettingsRemove
--- PASS: TestAccAzureRMServiceFabricCluster_fabricSettingsRemove (76.76s)
=== RUN   TestAccAzureRMServiceFabricCluster_nodeTypeCustomPorts
--- PASS: TestAccAzureRMServiceFabricCluster_nodeTypeCustomPorts (67.23s)
=== RUN   TestAccAzureRMServiceFabricCluster_nodeTypesMultiple
--- PASS: TestAccAzureRMServiceFabricCluster_nodeTypesMultiple (65.39s)
=== RUN   TestAccAzureRMServiceFabricCluster_nodeTypesUpdate
--- PASS: TestAccAzureRMServiceFabricCluster_nodeTypesUpdate (65.42s)
=== RUN   TestAccAzureRMServiceFabricCluster_tags
--- PASS: TestAccAzureRMServiceFabricCluster_tags (62.97s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	729.355s
```
@tombuildsstuff
Copy link
Contributor

hi @lstolyarov

Thanks for this PR :)

Firstly, apologies for the delay with this PR - it's been sat at the bottom of the PR Review list for far too long (which is totally my fault!).

In order to see how we could move forward with this, I spent a little time yesterday looking into what this would entail. I've amended your original commit (although your name's still on the commit) to remove the old SDK and the config.go/provider.go files (since these would cause merge conflicts), and then rebased this on top of master. Once that was done, I then took a look into what was needed to get this merged: I ended up altering the schema to better match the API/ARM Templates that I could find (in particular the fabric_settings and node_type blocks, since there can be multiple of each); and have since added some acceptance tests and documentation.

Whilst I appreciate this PR has been sitting here awkwardly for far too long - I hope you don't mind that I've made these changes and pushed them to your fork. This now LGTM (although I'm going to ask @katbyte to review it) - and I believe we should be able to get this shipped into the next release of the AzureRM Provider.

Thanks!

@tombuildsstuff tombuildsstuff added this to the 1.12.0 milestone Jul 30, 2018
@tombuildsstuff tombuildsstuff changed the title Creating azure service fabric resource New Resource: azurerm_service_fabric_cluster Jul 30, 2018
@tombuildsstuff
Copy link
Contributor

Tests pass:

screenshot 2018-07-30 at 15 34 49

@tombuildsstuff tombuildsstuff merged commit c4a37df into hashicorp:master Jul 30, 2018
tombuildsstuff added a commit that referenced this pull request Jul 30, 2018
@lstolyarov
Copy link
Contributor Author

Hey @tombuildsstuff, no problems at all. Nice to see this in terraform!

@pixelicous
Copy link

pixelicous commented Aug 14, 2018

Thank you!!!
Is it possible to also update service fabric configuration using terraform now?
I specifically wonder would it work like ARM template, as for configuring certificates on the nodes you usally redeploy the ARM template..

@tombuildsstuff
Copy link
Contributor

@pixelicous only the node count of a cluster can be changed via TF, due to limitations within the API - please see the docs for more info 😄

@pixelicous
Copy link

pixelicous commented Aug 15, 2018

@tombuildsstuff Only that? Damn, that makes this very unusable because one of the major things you need to continuously change in the cluster is pushing rolled certificaes. We provide certificates to the nodes so that the applications and their processes can use it and authenticate with keyvault, today we are doing it with ARM template, isn't it working with API as well??
Can find more details of what i mean here

I read the documentation for this object on terraform docs, it seems the "properties" block is missing, this also is a problem because you cannot use the RBAC block and this forces users to use client certificates which is not a best practice, the RBAC block is created using Microsoft's AADTOOL and looks like this:

"azureActiveDirectory": { 
     "tenantId":"xxxxxx-xxxxx-xxxxx-xxxxxx-xxxxxx", 
     "clusterApplication":"xxxxxx-xxxx-xxxx-xxxxx-xxxxxx", 
     "clientApplication":"xxxxx-xxxx-xxxx-xxxx-xxxxxx" 
},

Details can be found here under "Set up Azure Active Directory for client authentication"

@ghost
Copy link

ghost commented Mar 30, 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 👉 hashibot-feedback@hashicorp.com. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 30, 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.

Feature Request: Service Fabric
5 participants