-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
azurerm_eventhub
- deprecate namespace_name
and resource_group_name
in favour of namespace_id
#28055
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @catriona-m. Do the tests need to be updated as well?
### `azurerm_eventhub` | ||
|
||
* The deprecated `namespace_name` property has been removed in favour of the `namespace_id` property. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be listed in alphabetical order, so should be moved further up
* `namespace_name` - (Optional) Specifies the name of the EventHub Namespace. Changing this forces a new resource to be created. This property is deprecated in favour of `namespace_id`. | ||
|
||
* `namespace_id` - (Optional) Specifies the id of the EventHub Namespace. Changing this forces a new resource to be created. | ||
|
||
~> **NOTE:** One of `namespace_name` or `namespace_id` must be specified. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We remove deprecated properties entirely from the documentation to discourage use of them
* `namespace_name` - (Optional) Specifies the name of the EventHub Namespace. Changing this forces a new resource to be created. This property is deprecated in favour of `namespace_id`. | |
* `namespace_id` - (Optional) Specifies the id of the EventHub Namespace. Changing this forces a new resource to be created. | |
~> **NOTE:** One of `namespace_name` or `namespace_id` must be specified. | |
* `namespace_id` - (Optional) Specifies the id of the EventHub Namespace. Changing this forces a new resource to be created. |
r.Schema["namespace_id"] = &pluginsdk.Schema{ | ||
Type: pluginsdk.TypeString, | ||
Optional: true, | ||
ForceNew: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably shouldn't be ForceNew
yet so that users can migrate over to using namespace_id
r.Schema["namespace_name"] = &pluginsdk.Schema{ | ||
Type: pluginsdk.TypeString, | ||
Optional: true, | ||
ForceNew: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise here
ForceNew: true, |
@@ -379,7 +379,7 @@ resource "azurerm_eventhub_namespace" "test" { | |||
|
|||
resource "azurerm_eventhub" "test" { | |||
name = "acctesteventhub-%d" | |||
namespace_name = azurerm_eventhub_namespace.test.name | |||
namespace_id = azurerm_eventhub_namespace.test.id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming all the test configs have been updated to use namespace_id
instead of namespace_name
now, provided the migration path for users switching from namespace_name
-> namespace_id
works this is fine but it might be worth keeping one test config with namespace_name
to ensure it's still functional for the time being e.g.
func (EventHubResource) basic(data acceptance.TestData, partitionCount int) string {
if !features.FivePointOhBeta() {
// return config using namespace_name here
}
// return config using namespace_id here
}
Dumb question: Shouldn't be also the resource group deprecated? |
@CorrenSoft not dumb at all, good catch 😄 |
Type: pluginsdk.TypeString, | ||
Required: true, | ||
ForceNew: true, | ||
ValidateFunc: validate.ValidateEventHubNamespaceName(), | ||
ValidateFunc: namespaces.ValidateNamespaceID, | ||
}, | ||
|
||
"resource_group_name": commonschema.ResourceGroupName(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@catriona-m as pointed out by @CorrenSoft this probably wants to be deprecated as well
azurerm_eventhub
- deprecate namespace_name
in favour of namespace_id
azurerm_eventhub
- deprecate namespace_name
and resource_group_name
in favour of namespace_id
Thanks @CorrenSoft and @stephybun - I've added that change now and re-run the tests, which look good (apart from 2 flaky tests) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @catriona-m. Once the deprecation messages and upgrade guide have been updated this LGTM ⛄
Computed: true, | ||
ValidateFunc: validate.ValidateEventHubNamespaceName(), | ||
ExactlyOneOf: []string{"namespace_id", "namespace_name"}, | ||
Deprecated: "`namespace_name` has been deprecated in favour of `namespace_id`", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't believe I didn't catch this in the previous review, but could we update this so it's consistent with other deprecation messages in the provider and also clear in that it will be removed in the next major release
Deprecated: "`namespace_name` has been deprecated in favour of `namespace_id`", | |
Deprecated: "`namespace_name` has been deprecated in favour of `namespace_id` and will be removed in v5.0 of the AzureRM Provider", |
Computed: true, | ||
ExactlyOneOf: []string{"namespace_id", "resource_group_name"}, | ||
ValidateFunc: resourcegroups.ValidateName, | ||
Deprecated: "`resource_group_name` has been deprecated in favour of `namespace_id` ", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deprecated: "`resource_group_name` has been deprecated in favour of `namespace_id` ", | |
Deprecated: "`resource_group_name` has been deprecated in favour of `namespace_id` and will be removed in v5.0 of the AzureRM Provider ", |
|
||
* The deprecated `name` property has been removed. | ||
* The deprecated `namespace_name` property has been removed in favour of the `namespace_id` property. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to add a line for the resource_group_name
property here as well
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Community Note
Description
PR Checklist
For example: “
resource_name_here
- description of change e.g. adding propertynew_property_name_here
”Changes to existing Resource / Data Source
Testing
Change Log
Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.
azurerm_eventhub
- deprecatenamespace_name
andresource_group_name
in favour ofnamespace_id
[GH-00000]This is a (please select all that apply):
Related Issue(s)
Fixes #27008
Note
If this PR changes meaningfully during the course of review please update the title and description as required.