`azurerm_service_fabric_cluster` - support for specifying the cluster code version #1945

steve-hawkins · 2018-09-19T17:09:49Z

this should resolve #1906

merge from origin

katbyte

Hi @steve-hawkins,

Thank you for adding this property, it looks great outside aside from a couple minor comments I've left inline.

Look forward to getting this merged soon

katbyte · 2018-09-19T17:46:06Z

.gitattributes

@@ -0,0 +1,2 @@
+# make sure all .go files stay LF, this will help developers on Windows


This file is outside of the scope of this PR.

azurerm/resource_arm_service_fabric_cluster.go

katbyte · 2018-09-19T17:48:24Z

azurerm/resource_arm_service_fabric_cluster_test.go

+func testAccAzureRMServiceFabricCluster_manualClusterCodeVersion(rInt int, location, clusterCodeVersion string) string {
+	return fmt.Sprintf(`
+resource "azurerm_resource_group" "test" {
+  name = "acctestRG-%[1]d"


Very minor but could we align these assignments?

sorry copy and paste job

steve-hawkins · 2018-09-19T18:12:09Z

I must be doing something wrong in the acceptance test as I get the following:-

$ make fmt; make testacc TEST=./azurerm TESTARGS=-test.run=TestAccAzureRMServiceFabricCluster_manualClusterCodeVersion
gofmt -w $(find . -name '*.go' |grep -v vendor)
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./azurerm -v -test.run=TestAccAzureRMServiceFabricCluster_manualClusterCodeVersion -timeout 180m
=== RUN   TestAccAzureRMServiceFabricCluster_manualClusterCodeVersion
--- FAIL: TestAccAzureRMServiceFabricCluster_manualClusterCodeVersion (57.59s)
    testing.go:513: Step 1 error: ImportStateVerify attributes not equivalent. Difference is shown below. Top is actual, bottom is expected.

        (map[string]string) {
        }


        (map[string]string) (len=1) {
         (string) (len=20) "cluster_code_version": (string) (len=12) "6.3.176.9494"
        }

FAIL
FAIL    github.com/terraform-providers/terraform-provider-azurerm/azurerm       59.535s
make: *** [testacc] Error 1

forgive me as I have only just started learning GoLang and contributing to this project

I get that it appears to not find the attribute key cluster_code_version or a value, but I can't figure out how the TestCheckResourceAttr function in testing.go actually checks the resource?

I know my change works as I have tested it manually, but I cannot figure out how to get the acceptance test working

any help appreciated

katbyte · 2018-09-19T18:23:46Z

@steve-hawkins,

What is happening is on import, the cluster_code_version is not being saved into state so the verification step fails. Which is a bit weird because at first glance it the code looks correct.

tombuildsstuff · 2018-09-19T19:32:33Z

azurerm/resource_arm_service_fabric_cluster.go

@@ -423,6 +433,7 @@ func resourceArmServiceFabricClusterRead(d *schema.ResourceData, meta interface{
 		d.Set("management_endpoint", props.ManagementEndpoint)
 		d.Set("reliability_level", string(props.ReliabilityLevel))
 		d.Set("upgrade_mode", string(props.UpgradeMode))
+		d.Set("cluster_code_version ", props.ClusterCodeVersion)


I believe the test failure here is because there's a space at the end of this string (so the d.Set can't find a matching property in the schema called cluster_code_version [note the space] - so the Set fails)?

@steve-hawkins since this PR otherwise LGTM - I hope you don't mind but I've pushed a commit to remove the extra space (and will kick off the tests now)

tombuildsstuff · 2018-09-19T19:36:36Z

azurerm/resource_arm_service_fabric_cluster.go

@@ -372,6 +381,7 @@ func resourceArmServiceFabricClusterUpdate(d *schema.ResourceData, meta interfac
 			NodeTypes:                    nodeTypes,
 			ReliabilityLevel:             servicefabric.ReliabilityLevel1(reliabilityLevel),
 			UpgradeMode:                  servicefabric.UpgradeMode1(upgradeMode),
+			ClusterCodeVersion:           utils.String(clusterCodeVersion),


we've run the test suite here and this fails because the ClusterCodeVersion is an empty string when it's not specified (so this fails on the API Validation):

* azurerm_service_fabric_cluster.test: Error creating Service Fabric Cluster "acctest-9043244035322992792" (Resource Group "acctestRG-9043244035322992792"): servicefabric.ClustersClient#Create: Failure sending request: StatusCode=0 -- Original Error: autorest/azure: Service returned an error. Status=400 Code="InvalidRequestFormat" Message="Cannot parse the request." Details=[{"code":"InvalidRequestFormat","message":"Error parsing version string:. Path 'properties.clusterCodeVersion', line 1, position 115."}]

could we update this to only set the field if it's got a value? e.g.

if clusterCodeVersion != "" { parameters.ClusterPropertiesUpdateParameters.ClusterCodeVersion = utils.String(clusterCodeVersion) }

(we also need to update the Create above to do the same thing)

…ve-hawkins/terraform-provider-azurerm into feature/cluster_code_version

steve-hawkins · 2018-09-21T12:14:38Z

Hi @tombuildsstuff and @katbyte , I'm having some trouble with the conditional logic on ClusterCodeVersion in the resourceArmServiceFabricClusterRead function. I have all the current acceptance tests working with the following:-

upgradeMode := string(props.UpgradeMode)
d.Set("upgrade_mode", upgradeMode)

if upgradeMode == "Manual" {
  d.Set("cluster_code_version", props.ClusterCodeVersion)
}

This however will fail if UpgradeMode is Manual and no version is specified:-

$ make fmt; make testacc TEST=./azurerm TESTARGS=-test.run=TestAccAzureRMServiceFabricCluster_manualLatest
gofmt -w $(find . -name '*.go' |grep -v vendor)
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./azurerm -v -test.run=TestAccAzureRMServiceFabricCluster_manualLatest -timeout 180m
=== RUN   TestAccAzureRMServiceFabricCluster_manualLatest
--- FAIL: TestAccAzureRMServiceFabricCluster_manualLatest (53.41s)
    testing.go:513: Step 0 error: After applying this step, the plan was not empty:

        DIFF:

        UPDATE: azurerm_service_fabric_cluster.test
          cluster_code_version: "6.3.176.9494" => ""

        STATE:

        azurerm_resource_group.test:
          ID = /subscriptions/[REDACTED]/resourceGroups/acctestRG-8531727941990102137
          provider = provider.azurerm
          location = uksouth
          name = acctestRG-8531727941990102137
          tags.% = 0
        azurerm_service_fabric_cluster.test:
          ID = /subscriptions/[REDACTED]/resourcegroups/acctestRG-8531727941990102137/providers/Microsoft.ServiceFabric/clusters/acctest-8531727941990102137
          provider = provider.azurerm
          add_on_features.# = 0
          certificate.# = 0
          client_certificate_thumbprint.# = 0
          cluster_code_version = 6.3.176.9494
          cluster_endpoint = https://uksouth.servicefabric.azure.com/runtime/clusters/64d87576-e429-4e0e-ac9e-db8b20b4f546
          diagnostics_config.# = 0
          fabric_settings.# = 0
          location = uksouth
          management_endpoint = http://example:80
          name = acctest-8531727941990102137
          node_type.# = 1
          node_type.0.application_ports.# = 1
          node_type.0.application_ports.0.end_port = 30000
          node_type.0.application_ports.0.start_port = 20000
          node_type.0.client_endpoint_port = 2020
          node_type.0.durability_level = Bronze
          node_type.0.ephemeral_ports.# = 1
          node_type.0.ephemeral_ports.0.end_port = 65534
          node_type.0.ephemeral_ports.0.start_port = 49152
          node_type.0.http_endpoint_port = 80
          node_type.0.instance_count = 3
          node_type.0.is_primary = true
          node_type.0.name = first
          reliability_level = Bronze
          resource_group_name = acctestRG-8531727941990102137
          tags.% = 0
          upgrade_mode = Manual
          vm_image = Windows

          Dependencies:
            azurerm_resource_group.test
FAIL
FAIL    github.com/terraform-providers/terraform-provider-azurerm/azurerm       54.873s
make: *** [testacc] Error 1

I could make it so the cluster_code_version has to be specified if the upgrade_mode is set to Manual, would that be acceptable?

I might need some help if we want this to work with upgrade_mode to Manual and no cluster_code_version specified

katbyte · 2018-09-21T17:10:32Z

@steve-hawkins,

Might i suggest always reading in cluster_code_version:

  d.Set("cluster_code_version", v) //d.Set will handle nil values

and then adding computed to the cluster_code_version property so terraform expects it to be read in

``` $ acctests azurerm TestAccAzureRMServiceFabricCluster_manualLatest === RUN TestAccAzureRMServiceFabricCluster_manualLatest --- PASS: TestAccAzureRMServiceFabricCluster_manualLatest (96.07s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 97.192s ```

tombuildsstuff

hey @steve-hawkins

This however will fail if UpgradeMode is Manual and no version is specified:-

So in order to fix this we can make the field Computed, which means if a value isn't specified then it'll use the default value from the API. Since this PR otherwise LGTM I'm going to push a commit to fix this and get this merged as a part of v1.16 (I hope you don't mind!)

Thanks!

tombuildsstuff · 2018-09-23T22:39:36Z

azurerm/resource_arm_service_fabric_cluster.go

+
+		if upgradeMode == "Manual" {
+			d.Set("cluster_code_version", props.ClusterCodeVersion)
+		}


we can remove the upgradeMode == "Manual" check here - and ensure this value is always set (since this information is always useful) - instead we can make this field Computed in the schema (above), which will allow this value to be optional & have whatever default value is specified by the Azure API

changes have been pushed

tombuildsstuff · 2018-09-24T01:59:19Z

Tests pass:

steve-hawkins · 2018-09-24T10:29:11Z

Thanks @katbyte @tombuildsstuff I knew I would have been missing something obvious

Much appreciated and looking forward to contributing more

steve-hawkins · 2018-09-27T13:47:18Z

@tombuildsstuff any chance this could make it into 1.16.0?

tombuildsstuff · 2018-09-27T21:22:28Z

@steve-hawkins it's been merged so it'll go out as a part of that release - we're looking to ship v1.16 once #1987 has been merged :)

ghost · 2019-03-06T14:30:39Z

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!

Steven Hawkins and others added 7 commits September 18, 2018 11:59

Merge pull request #1 from terraform-providers/master

5dab9e0

merge from origin

allowed two client certificates to be specified

c7c080a

added cluster_code_version atribute

16c9c64

corrected docs

762f6e4

added acceptance tests

71672ce

keep LF on checkout to Windows

695c118

new line added

e5991f4

ghost added the size/M label Sep 19, 2018

katbyte previously requested changes Sep 19, 2018

View reviewed changes

removed from this PR

26a2db6

added validation

11f2ba0

ghost added the size/M label Sep 19, 2018

alignment based on PR comment

b20430b

tombuildsstuff reviewed Sep 19, 2018

View reviewed changes

Removing a blank space

3f1f375

tombuildsstuff added enhancement service/service-fabric labels Sep 19, 2018

tombuildsstuff reviewed Sep 19, 2018

View reviewed changes

Steven Hawkins added 2 commits September 19, 2018 20:50

fmt

a15e2f2

Merge branch 'feature/cluster_code_version' of https://github.com/ste…

2b6309a

…ve-hawkins/terraform-provider-azurerm into feature/cluster_code_version

ghost added the size/M label Sep 19, 2018

Steven Hawkins added 2 commits September 20, 2018 17:25

added empty string condition

6cdb272

read ClusterCodeVersion when UpgradeMode is Manual

6d13c16

tombuildsstuff added 2 commits September 24, 2018 11:40

Making the cluster_code_version field computed

6f1d1f2

tombuildsstuff approved these changes Sep 24, 2018

View reviewed changes

ghost added the size/M label Sep 24, 2018

tombuildsstuff changed the title ~~manual upgrade cluster code version~~ azurerm_service_fabric_cluster - support for specifying the cluster code version Sep 24, 2018

tombuildsstuff merged commit f276997 into hashicorp:master Sep 24, 2018

tombuildsstuff added a commit that referenced this pull request Sep 24, 2018

Updating to include #1945

5129c7a

steve-hawkins deleted the feature/cluster_code_version branch September 24, 2018 09:59

tombuildsstuff added this to the 1.16.0 milestone Sep 27, 2018

ghost locked and limited conversation to collaborators Mar 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`azurerm_service_fabric_cluster` - support for specifying the cluster code version #1945

`azurerm_service_fabric_cluster` - support for specifying the cluster code version #1945

steve-hawkins commented Sep 19, 2018

katbyte left a comment

katbyte Sep 19, 2018

katbyte Sep 19, 2018

steve-hawkins Sep 19, 2018

steve-hawkins commented Sep 19, 2018

katbyte commented Sep 19, 2018

tombuildsstuff Sep 19, 2018

tombuildsstuff Sep 19, 2018

tombuildsstuff Sep 19, 2018

steve-hawkins commented Sep 21, 2018

katbyte commented Sep 21, 2018

tombuildsstuff left a comment

tombuildsstuff Sep 23, 2018

tombuildsstuff commented Sep 24, 2018

steve-hawkins commented Sep 24, 2018

steve-hawkins commented Sep 27, 2018

tombuildsstuff commented Sep 27, 2018

ghost commented Mar 6, 2019

		@@ -0,0 +1,2 @@
		# make sure all .go files stay LF, this will help developers on Windows

azurerm_service_fabric_cluster - support for specifying the cluster code version #1945

azurerm_service_fabric_cluster - support for specifying the cluster code version #1945

Conversation

steve-hawkins commented Sep 19, 2018

katbyte left a comment

Choose a reason for hiding this comment

katbyte Sep 19, 2018

Choose a reason for hiding this comment

katbyte Sep 19, 2018

Choose a reason for hiding this comment

steve-hawkins Sep 19, 2018

Choose a reason for hiding this comment

steve-hawkins commented Sep 19, 2018

katbyte commented Sep 19, 2018

tombuildsstuff Sep 19, 2018

Choose a reason for hiding this comment

tombuildsstuff Sep 19, 2018

Choose a reason for hiding this comment

tombuildsstuff Sep 19, 2018

Choose a reason for hiding this comment

steve-hawkins commented Sep 21, 2018

katbyte commented Sep 21, 2018

tombuildsstuff left a comment

Choose a reason for hiding this comment

tombuildsstuff Sep 23, 2018

Choose a reason for hiding this comment

tombuildsstuff commented Sep 24, 2018

steve-hawkins commented Sep 24, 2018

steve-hawkins commented Sep 27, 2018

tombuildsstuff commented Sep 27, 2018

ghost commented Mar 6, 2019

`azurerm_service_fabric_cluster` - support for specifying the cluster code version #1945

`azurerm_service_fabric_cluster` - support for specifying the cluster code version #1945