-
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
New Resource: azurerm_eventgrid_namespace
#27682
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, could you take a look at the review comments left in-line?
PublicNetworkAccess string `tfschema:"public_network_access"` | ||
Sku string `tfschema:"sku"` | ||
TopicSpacesConfiguration []TopicSpacesConfigurationModel `tfschema:"topic_spaces_configuration"` | ||
ZoneRedundant bool `tfschema:"zone_redundant"` |
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 field was deprecated and removed in 4.0 not too long ago for the service bus and event hub namespace resources. We may want to check with MSFT whether this is also the case for event grid before exposing this.
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 @stephybun - I tested this and confirmed the api handles setting this based on location so it can be removed here 👍
"inbound_ip_rule": { | ||
Type: pluginsdk.TypeList, | ||
Optional: true, | ||
ConfigMode: pluginsdk.SchemaConfigModeAttr, |
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 isn't Computed
so I don't think we need to be setting this on the block
ConfigMode: pluginsdk.SchemaConfigModeAttr, |
"key": { | ||
Type: pluginsdk.TypeString, | ||
Required: true, | ||
}, | ||
|
||
"value": { | ||
Type: pluginsdk.TypeString, | ||
Required: 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.
Validation here?
|
||
|
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.
|
||
data.ResourceTest(t, r, []acceptance.TestStep{ | ||
{ | ||
Config: r.basicWithTags(data), |
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.
Tags can be updated but it looks like we don't cover this in the tests
|
||
* `location` - (Required) Specifies the supported Azure location where the resource should exist. Changing this forces a new resource to be created. | ||
|
||
* `capacity` - (Optional) (Optional) Specifies the Capacity / Throughput Units for an Eventgrid Namespace. Valid values can be between `1` and `40`. |
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.
* `capacity` - (Optional) (Optional) Specifies the Capacity / Throughput Units for an Eventgrid Namespace. Valid values can be between `1` and `40`. | |
* `capacity` - (Optional) Specifies the Capacity / Throughput Units for an Eventgrid Namespace. Valid values can be between `1` and `40`. |
|
||
* `alternative_authentication_name_source` - (Optional) Specifies a list of alternative sources for the client authentication name from the client certificate. Possible values are `ClientCertificateDns`, `ClientCertificateEmail`, `ClientCertificateIp`, `ClientCertificateSubject` and `ClientCertificateUri`. | ||
|
||
* `maximum_client_sessions_per_authentication_name` - (Optional) Specifies the maximum number of client sessions epr authentication name. Valid values can be between `1` and `100`. |
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.
* `maximum_client_sessions_per_authentication_name` - (Optional) Specifies the maximum number of client sessions epr authentication name. Valid values can be between `1` and `100`. | |
* `maximum_client_sessions_per_authentication_name` - (Optional) Specifies the maximum number of client sessions per authentication name. Valid values can be between `1` and `100`. |
|
||
* `maximum_session_expiry_in_hours` - (Optional) Specifies the maximum session expiry interval allowed for all MQTT clients connecting to the Event Grid namespace. Valid values can be between `1` and `8`. | ||
|
||
* `route_topic_id` - (Optional) Specifies the Event Grid topic resource id to route messages to. |
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.
* `route_topic_id` - (Optional) Specifies the Event Grid topic resource id to route messages to. | |
* `route_topic_id` - (Optional) Specifies the Event Grid topic resource ID to route messages to. |
Many thanks for the work. Any update on this please? |
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. A few more minor comments, but other than that this LGTM provided the tests are passing!
} | ||
|
||
func flattenInboundIPRules(ipRules *[]namespaces.InboundIPRule) []InboundIpRuleModel { | ||
var output []InboundIpRuleModel |
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.
Forgot to flag this in my previous review, there is a difference between declaring a slice like this and like
output := make([]InboundIpRuleModel, 0)
Which is that the declaration var output []InboundIpRuleModel
evaluates to nil
if nothing has been appended to it, whereas initialising it with make
will result in an empty slice []
which is what we want when setting values into state.
Can you please update this, as well as all the other flatten functions to
var output []InboundIpRuleModel | |
output := make([]InboundIpRuleModel, 0) |
`, data.RandomInteger, data.Locations.Primary) | ||
} | ||
|
||
func (r EventGridNamespaceResource) basicWithTags(data acceptance.TestData) string { |
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.
Minor but we could just call this
func (r EventGridNamespaceResource) basicWithTags(data acceptance.TestData) string { | |
func (r EventGridNamespaceResource) tags(data acceptance.TestData) string { |
`, data.RandomInteger, data.Locations.Primary) | ||
} | ||
|
||
func (r EventGridNamespaceResource) basicWithTagsUpdated(data acceptance.TestData) string { |
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
func (r EventGridNamespaceResource) basicWithTagsUpdated(data acceptance.TestData) string { | |
func (r EventGridNamespaceResource) tagsUpdated(data acceptance.TestData) string { |
Thanks @stephybun - the tests are passing so going to merge this now 👍 |
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
Adding
azurerm_eventgrid_namespace
as a new resource.PR Checklist
For example: “
resource_name_here
- description of change e.g. adding propertynew_property_name_here
”Testing
Change Log
Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.
azurerm_eventgrid_namespace
[New Resource:azurerm_eventgrid_namespace
#27682]This is a (please select all that apply):
Related Issue(s)
Fixes #24079
Note
If this PR changes meaningfully during the course of review please update the title and description as required.