New Resource: `azurerm_eventhub_cluster` #7306

mbfrahry · 2020-06-12T04:12:09Z

jackofallops

Thanks @mbfrahry
Couple questions and comments below, but otherwise LGTM 👍
Thanks!

jackofallops · 2020-06-15T08:26:39Z

azurerm/internal/services/eventhub/eventhub_cluster_resource.go

+		}
+
+		if err := future.WaitForCompletionRef(ctx, client.Client); err != nil {
+			return resource.NonRetryableError(fmt.Errorf("creating eventhub cluster: %+v", err))


typo here?

Suggested change

return resource.NonRetryableError(fmt.Errorf("creating eventhub cluster: %+v", err))

return resource.NonRetryableError(fmt.Errorf("deleting eventhub cluster: %+v", err))

website/docs/r/eventhub_cluster.html.markdown

jackofallops · 2020-06-15T09:08:24Z

azurerm/internal/services/eventhub/eventhub_cluster_resource.go

+	id, err := azure.ParseAzureResourceID(d.Id())
+	if err != nil {
+		return err
+	}
+
+	resourceGroup := id.ResourceGroup
+	name := id.Path["clusters"]


Worth a custom ID parser here?

jackofallops · 2020-06-15T09:19:02Z

azurerm/internal/services/eventhub/eventhub_cluster_resource.go

+			if response.WasNotFound(future.Response()) {
+				return nil
+			}
+			if strings.Contains(err.Error(), "Cluster cannot be deleted until four hours after its creation time") {


Is it worth a 429 check here too given the length of time this operation could be running for?

Creating/deleting clusters is actually quite fast. It's just that once created you can't delete it for the first 4 hours. I don't think a case where TF would wait for 4 hours makes sense?

Hi @favoretti
In this case, if the delete operation is requested before the 4 hour minimum lifetime (say as part of a daily build, or acceptance test), the provider is capable of performing the operation without error or intervention (whilst honouring any custom timeouts). If the request is sent after that minimum interval this section of code won't trigger and will, as you point out, complete quickly.

Co-authored-by: Steve <11830746+jackofallops@users.noreply.github.com>

katbyte

Aside from one comment about upgrading all the resources to the new api looks good @mbfrahry

katbyte · 2020-06-14T06:31:26Z

azurerm/internal/services/eventhub/client/client.go

@@ -2,17 +2,22 @@ package client

 import (
 	"github.com/Azure/azure-sdk-for-go/services/eventhub/mgmt/2017-04-01/eventhub"
+	previewEventhub "github.com/Azure/azure-sdk-for-go/services/preview/eventhub/mgmt/2018-01-01-preview/eventhub"


Is there a reason we can't upgrade the rest of the resources to this api version

Good call! The preview version was a little wonky when I started but it seems to have evened out with this latest version. It's been taken care of and tests look good

ArcturusZhang

Thanks @mbfrahry I have left some question and minor comments, please have a look

ArcturusZhang · 2020-06-16T03:10:43Z

azurerm/internal/services/eventhub/eventhub_cluster_resource.go

+		Importer: &schema.ResourceImporter{
+			State: schema.ImportStatePassthrough,
+		},


Could we also check the ID before the import?

ArcturusZhang · 2020-06-16T03:13:14Z

azurerm/internal/services/eventhub/eventhub_cluster_resource.go

+
+	future, err := client.Put(ctx, resourceGroup, name, cluster)
+	if err != nil {
+		return err


Maybe we should put some custom error messages here?

ArcturusZhang · 2020-06-16T03:13:21Z

azurerm/internal/services/eventhub/eventhub_cluster_resource.go

+
+	read, err := client.Get(ctx, resourceGroup, name)
+	if err != nil {
+		return err


Maybe we should put some custom error messages here?

ArcturusZhang · 2020-06-16T03:14:26Z

azurerm/internal/services/eventhub/eventhub_cluster_resource.go

+	client := meta.(*clients.Client).Eventhub.ClusterClient
+	ctx, cancel := timeouts.ForDelete(meta.(*clients.Client).StopContext, d)
+	defer cancel()
+	id, err := parse.NamespaceAuthorizationRuleID(d.Id())


Why here we are parsing this ID as a namespace authorization rule instead of cluster?

katbyte

aside from a few minor remarks LGTM 👍

katbyte · 2020-06-16T15:30:37Z

azurerm/internal/services/eventhub/eventhub_cluster_resource.go

+
+	read, err := client.Get(ctx, resourceGroup, name)
+	if err != nil {
+		return err


katbyte · 2020-06-16T15:31:11Z

azurerm/internal/services/eventhub/tests/eventhub_cluster_resource_test.go

+		name := rs.Primary.Attributes["name"]
+		resourceGroup := rs.Primary.Attributes["resource_group_name"]


we should use the parse id function here on the id

katbyte · 2020-06-16T15:31:25Z

azurerm/internal/services/eventhub/tests/eventhub_cluster_resource_test.go

+		name := rs.Primary.Attributes["name"]
+		resourceGroup, hasResourceGroup := rs.Primary.Attributes["resource_group_name"]


and parse id again here

katbyte · 2020-06-16T15:32:22Z

azurerm/internal/services/eventhub/tests/eventhub_cluster_resource_test.go

+}
+
+resource "azurerm_eventhub_cluster" "test" {
+  name                = "acctesteventhubcluster-%d"


can capitals be used here?

ghost · 2020-06-19T02:32:00Z

This has been released in version 2.15.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.15.0"
}
# ... other configuration ...

djsly · 2020-06-19T03:44:04Z

@mbfrahry how can I assign eventhub_namespaces to eventhub_cluster ?

favoretti · 2020-06-20T11:20:42Z

@djsly Yeah... That's what we're discussing here: #7347

ghost · 2020-07-17T14:50:13Z

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!

mbfrahry added 3 commits April 22, 2020 12:44

Start eventhub cluster support

c319c7a

Merge with master

0cd8d06

Finalizing eventhub cluster

9b6dba6

ghost added size/XXL dependencies documentation labels Jun 12, 2020

mbfrahry changed the title ~~New Resource: azurerm_eventhub_cluster~~ [WIP] New Resource: azurerm_eventhub_cluster Jun 12, 2020

mbfrahry added the service/event-hubs EventHubs label Jun 12, 2020

mbfrahry added this to the v2.15.0 milestone Jun 12, 2020

mbfrahry added 2 commits June 12, 2020 08:44

lint + wait for future

020bf3a

fmt

da20d62

mbfrahry changed the title ~~[WIP] New Resource: azurerm_eventhub_cluster~~ New Resource: azurerm_eventhub_cluster Jun 12, 2020

mbfrahry requested a review from a team June 12, 2020 20:20

jackofallops approved these changes Jun 15, 2020

View reviewed changes

mbfrahry and others added 2 commits June 15, 2020 09:03

Update website/docs/r/eventhub_cluster.html.markdown

4a246ba

Co-authored-by: Steve <11830746+jackofallops@users.noreply.github.com>

Address review

3fb05c6

katbyte requested changes Jun 15, 2020

View reviewed changes

ArcturusZhang reviewed Jun 16, 2020

View reviewed changes

mbfrahry added 2 commits June 15, 2020 20:44

Address reviews

7e7931d

vendor

d2f46fc

katbyte approved these changes Jun 16, 2020

View reviewed changes

mbfrahry added 2 commits June 16, 2020 08:51

Review

a3e49c8

lint

be10780

mbfrahry merged commit f0e1d1c into master Jun 16, 2020

mbfrahry deleted the f-eventhub-dedicated branch June 16, 2020 17:40

mbfrahry added a commit that referenced this pull request Jun 16, 2020

Update changelog for #7306

5c30e7d

mybayern1974 mentioned this pull request Jun 18, 2020

Support for Event Hub Cluster #7370

Closed

tombuildsstuff added the preview label Jun 30, 2020

hashicorp locked and limited conversation to collaborators Jul 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Resource: `azurerm_eventhub_cluster` #7306

New Resource: `azurerm_eventhub_cluster` #7306

mbfrahry commented Jun 12, 2020

jackofallops left a comment

jackofallops Jun 15, 2020

jackofallops Jun 15, 2020

jackofallops Jun 15, 2020

favoretti Jun 15, 2020

jackofallops Jun 15, 2020

katbyte left a comment

katbyte Jun 14, 2020

mbfrahry Jun 16, 2020

ArcturusZhang left a comment

ArcturusZhang Jun 16, 2020

ArcturusZhang Jun 16, 2020

ArcturusZhang Jun 16, 2020

katbyte Jun 16, 2020

ArcturusZhang Jun 16, 2020

katbyte left a comment

katbyte Jun 16, 2020

katbyte Jun 16, 2020

katbyte Jun 16, 2020

katbyte Jun 16, 2020

ghost commented Jun 19, 2020

djsly commented Jun 19, 2020

favoretti commented Jun 20, 2020

ghost commented Jul 17, 2020

	return resource.NonRetryableError(fmt.Errorf("creating eventhub cluster: %+v", err))
	return resource.NonRetryableError(fmt.Errorf("deleting eventhub cluster: %+v", err))

		name := rs.Primary.Attributes["name"]
		resourceGroup := rs.Primary.Attributes["resource_group_name"]

		name := rs.Primary.Attributes["name"]
		resourceGroup, hasResourceGroup := rs.Primary.Attributes["resource_group_name"]

New Resource: azurerm_eventhub_cluster #7306

New Resource: azurerm_eventhub_cluster #7306

Conversation

mbfrahry commented Jun 12, 2020

jackofallops left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katbyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArcturusZhang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katbyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jun 19, 2020

djsly commented Jun 19, 2020

favoretti commented Jun 20, 2020

ghost commented Jul 17, 2020

New Resource: `azurerm_eventhub_cluster` #7306

New Resource: `azurerm_eventhub_cluster` #7306