New resources: azurerm_eventhub_namespace_dedicated and azurerm_eventhub_dedicated

[favoretti]

Support of 2 new resources - azurerm_eventhub_dedicated and azurerm_eventhub_namespace_dedicated. See discussion below.

[favoretti]

@mbfrahry Would you be so kind to have a look? This one stands in the way of creating larger hubs on EventHub Clusters.

[favoretti]

@jackofallops since you've reviewed Matthew's work on eventhub_cluster resource, can you have a look at this as well please?

[favoretti]

@jackofallops can you have a look please? If this is ok and merged, I’ll make another PR for data sources.

[jackofallops]

Hi @favoretti
Thanks for updating this to create the new resources, it's off to a really good start. I've left some comments and suggestions below, mostly around removing duplication and reusing code that's common with the original resources, with a few other tweaks.

One important thing, the changelog is only updated when a maintainer merges a PR when it's ready. I appreciate the thoroughness, but we'll that one backing out if you can.

Thanks again!

[favoretti]

after some internal discussion and that these belong to a cluster i think azurerm_eventhub_cluster_hub and azurerm_eventhub_cluster_[hub_]namespace would make the most sense?

@katbyte Naming here a bit awkward. Generally relation is Event Hub Cluster -> Namespace(s) -> EventHub(s). For the non-dedicated part it's just Namespace(s) -> EventHub(s).

azurerm_eventhub_cluster_namespace and azurerm_eventhub_cluster_eventhub? The latter would be quite a repetition, although can't do much about the fact that Event Hubs Cluster is called that way..

[tombuildsstuff]

hey @favoretti

Thanks for this PR :)

I've taken a look through here and we've had a chat about this internally - and ultimately we believe in this instance this'd make sense to reuse the existing azurerm_eventhub_namespace and azurerm_eventhub resources - rather than duplicating them for this single field - since based on what we can tell in the Azure API docs these should be complementary (ignoring validation/cluster specific configuration) rather than diverging. We've also discussed that the azurerm_eventhub_cluster resource probably wants to become azurerm_eventhub_dedicated_cluster - but that's a larger issue and probably a 3.0 thing.

Whilst I appreciate that's quite a substantial change from this PR - in practice these resources are essentially the same and so this likely makes the most sense until they substantially diverge - what do you think? :)

Thanks

[tombuildsstuff]

taking a look through this appears to be the same as the azurerm_eventhub resource, so I think we can remove this and reuse that instead?

[tombuildsstuff]

can we add a validation function to confirm this is the ID of EventHub Dedicated Cluster? there's examples in the ./azurerm/internal/services/{...}/validate folders

[tombuildsstuff]

after chatting internally - rather than splitting this out into it's own resource, since the only difference between these two resources is validation, could we add dedicated_cluster_id as an optional field to azurerm_eventhub_namespace

with regards to the validation differences - since we're already conditionally capturing this logic via the create call for network rules - we can catch this in the same manner for non-dedicated clusters during the create - so it should be possible to reuse these for the moment

[tombuildsstuff]

since we'll be inlining this within the main resource, this'll need to be:

Suggested change

ClusterArmID: &clusterID,

[tombuildsstuff]

.. and then:

Suggested change

	}
	}
	if v := d.Get("cluster_id").(string); v != "" {
	parameters.EHNamespaceProperties.ClusterArmID = utils.String(v)
	}

[tombuildsstuff]

since this is kind of ambiguous, it'd be worth probably calling this out as:

Suggested change

	"cluster_id": {
	"dedicated_cluster_id": {

(FWIW chatting internally we believe the azurerm_eventhub_cluster resource should probably be named azurerm_eventhub_dedicated_cluster, but that's a larger discussion)

[favoretti]

If we're going to ditch dedicated resources, this change will become obsolete then? I'm getting lost in diverging review opinions to be honest :)

[favoretti]

hey @favoretti

Thanks for this PR :)

I've taken a look through here and we've had a chat about this internally - and ultimately we believe in this instance this'd make sense to reuse the existing azurerm_eventhub_namespace and azurerm_eventhub resources - rather than duplicating them for this single field - since based on what we can tell in the Azure API docs these should be complementary (ignoring validation/cluster specific configuration) rather than diverging. We've also discussed that the azurerm_eventhub_cluster resource probably wants to become azurerm_eventhub_dedicated_cluster - but that's a larger issue and probably a 3.0 thing.

Whilst I appreciate that's quite a substantial change from this PR - in practice these resources are essentially the same and so this likely makes the most sense until they substantially diverge - what do you think? :)

Thanks

Right... :D @tombuildsstuff Back to where we started. Adding an optional clusterID field to existing namespace was my initial suggestion, @jackofallops said duping them would be a better option. I'm happy to rework this again, but can you folks agree on one way to go? :) See #7347 (comment)

[favoretti]

Also there's a challenge when we validate the limits for eventHub partition count and retention settings, where the whole discussion actually started. If I add optional field to namespace, this won't address eventhub validation challenge, since that one is unaware of where it's being created.

[jackofallops]

Right... :D @tombuildsstuff Back to where we started. Adding an optional clusterID field to existing namespace was my initial suggestion, @jackofallops said duping them would be a better option. I'm happy to rework this again, but can you folks agree on one way to go? :) See #7347 (comment)

Apologies, quite often we find ourselves part way down an implementation path before we find we've gone in the wrong direction. I've been there myself a lot over the years... Making the existing resource fit a preview API felt wrong at the time, but deeper inspection as this has progressed has brought us full circle.

On the validation point, we'll need to tackle it in the manner I would have preferred we could avoid. The schema validation will need to cover both sets of possibilities, and the code will need to react to those values based on the presence of the dedicated_cluster_id attribute.

[favoretti]

Right... :D @tombuildsstuff Back to where we started. Adding an optional clusterID field to existing namespace was my initial suggestion, @jackofallops said duping them would be a better option. I'm happy to rework this again, but can you folks agree on one way to go? :) See #7347 (comment)

Apologies, quite often we find ourselves part way down an implementation path before we find we've gone in the wrong direction. I've been there myself a lot over the years... Making the existing resource fit a preview API felt wrong at the time, but deeper inspection as this has progressed has brought us full circle.

On the validation point, we'll need to tackle it in the manner I would have preferred we could avoid. The schema validation will need to cover both sets of possibilities, and the code will need to react to those values based on the presence of the dedicated_cluster_id attribute.

No worries at all. I'll probably just abandon this and create a new PR with optional dedicated_cluster_id attribute. We'll tackle validation from there on?

[djsly]

Personally I think it makes more sense. Leveraging the existing namespace resources with an optional clusterID

…

On Jun 30, 2020, at 9:37 AM, Vladimir Lazarenko ***@***.***> wrote:  Right... :D @tombuildsstuff Back to where we started. Adding an optional clusterID field to existing namespace was my initial suggestion, @jackofallops said duping them would be a better option. I'm happy to rework this again, but can you folks agree on one way to go? :) See #7347 (comment) Apologies, quite often we find ourselves part way down an implementation path before we find we've gone in the wrong direction. I've been there myself a lot over the years... Making the existing resource fit a preview API felt wrong at the time, but deeper inspection as this has progressed has brought us full circle. On the validation point, we'll need to tackle it in the manner I would have preferred we could avoid. The schema validation will need to cover both sets of possibilities, and the code will need to react to those values based on the presence of the dedicated_cluster_id attribute. No worries at all. I'll probably just abandon this and create a new PR with optional dedicated_cluster_id attribute. We'll tackle validation from there on? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[favoretti]

Closing this one in favor of #7548.

[ghost]

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!