-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New Resource : azurerm_api_management_workspace_logger
#30652
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
base: main
Are you sure you want to change the base?
Conversation
be7401f
to
ba3ea75
Compare
ba3ea75
to
340d310
Compare
340d310
to
85a9606
Compare
85a9606
to
09731b2
Compare
09731b2
to
9ae0273
Compare
9ae0273
to
59d7898
Compare
59d7898
to
65fbc33
Compare
65fbc33
to
d05c2f2
Compare
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.
Hi @sinbai ,
Thanks for this PR - I've taken a look through and left some comments inline. If we can fix those up, this should be good to go 👍
|
||
* `description` - (Optional) Specifies a description of the API Management Workspace Logger. | ||
|
||
* `resource_id` - (Optional) Specifies the target resource ID of the API Management Workspace Logger, which can be either an azure event hub or an application insights resource. |
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.
* `resource_id` - (Optional) Specifies the target resource ID of the API Management Workspace Logger, which can be either an azure event hub or an application insights resource. | |
* `resource_id` - (Optional) Specifies the target resource ID of the API Management Workspace Logger, which can be either an Azure Event Hub or an application insights resource. |
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.
Fixed.
|
||
An `eventhub` block supports the following: | ||
|
||
* `name` - (Required) Specifies the name of the eventhub. |
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.
* `name` - (Required) Specifies the name of the eventhub. | |
* `name` - (Required) Specifies the name of the Event Hub. |
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.
Fixed.
|
||
* `name` - (Required) Specifies the name of the eventhub. | ||
|
||
* `connection_string` - (Optional) Specifies the connection string of the eventhub namespace. |
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.
* `connection_string` - (Optional) Specifies the connection string of the eventhub namespace. | |
* `connection_string` - (Optional) Specifies the connection string of the Event Hub namespace. |
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.
Fixed.
|
||
* `connection_string` - (Optional) Specifies the connection string of the eventhub namespace. | ||
|
||
* `endpoint_uri` - (Optional) Specifies the endpoint address of an eventhub namespace. |
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.
* `endpoint_uri` - (Optional) Specifies the endpoint address of an eventhub namespace. | |
* `endpoint_uri` - (Optional) Specifies the endpoint address of an Event Hub namespace. |
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.
Fixed.
|
||
-> **Note:** Exactly one of `connection_string` or `endpoint_uri` must be specified. | ||
|
||
* `user_assigned_identity_client_id` - (Optional) Specifies the client ID of user-assigned identity that has the "Azure Event Hubs Data Sender" role on the target eventhub namespace. |
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.
* `user_assigned_identity_client_id` - (Optional) Specifies the client ID of user-assigned identity that has the "Azure Event Hubs Data Sender" role on the target eventhub namespace. | |
* `user_assigned_identity_client_id` - (Optional) Specifies the client ID of user-assigned identity that has the "Azure Event Hubs Data Sender" role on the target Event Hub namespace. |
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.
Fixed.
}, | ||
}, | ||
|
||
"eventhub": { |
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.
eventhub
should be put after description
by alphabetical order
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.
Fixed.
|
||
type ApiManagementWorkspaceLoggerResource struct{} | ||
|
||
var _ sdk.Resource = ApiManagementWorkspaceLoggerResource{} |
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.
var _ sdk.Resource = ApiManagementWorkspaceLoggerResource{} | |
var _ sdk.ResourceWithUpdate = ApiManagementWorkspaceLoggerResource{} |
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.
Fixed.
credentials["identityClientId"] = eventHub.UserAssignedIdentityClientId | ||
} | ||
|
||
if metadata.ResourceData.HasChange("eventhub.0.name") { |
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.
why not use metadata.ResourceData.HasChange("eventhub")
and put them in a single expand
function?
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.
Fixed.
output := EventHubModel{ | ||
Name: (*input.Credentials)["name"], | ||
} | ||
|
||
if endpoint, ok := (*input.Credentials)["endpointAddress"]; ok { | ||
output.EndpointUri = endpoint | ||
} |
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.
could you use if
condition to confirm whether name
and endpointAddress
exist before assigning the value?
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.
Fixed.
output.EndpointUri = endpoint | ||
} | ||
|
||
if eventhub := expandApiManagementWorkspaceLoggerEventHub(model.EventHub); eventhub != nil { |
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.
it looks weird to use an expand
function in a flatten
function. Could you directly use value from model.EventHub
?
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.
Fixed.
e638215
to
33b5f36
Compare
Hi @ms-zhenhua thanks for your feedback. I've addressed the comments, and test results included below. Could you kindly review it again? |
"eventhub.0.user_assigned_identity_client_id", | ||
}, | ||
ValidateFunc: validation.StringIsNotEmpty, | ||
}, |
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.
nit, adding a new line may looks better
mostly LGTM but one small comment |
Hi @ziyeqf thanks for your feedback. I've fixed the comments, could you please take another look? |
|
||
* `application_insights` - (Optional) Specifies the application insights of the API Management Workspace Logger. Changing this forces a new resource to be created. | ||
|
||
* `eventhub` - (Optional) Specifies the eventhub of the API Management Workspace Logger. Changing this forces a new resource to be created. |
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.
eventhub
should be put after description
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.
Hi @ms-zhenhua , I've fixed the comment, could you please take another look?
LGTM now please address Zhenhua's comment. |
ca15edd
to
1cfec52
Compare
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.
LGTM~
Community Note
Description
Swigger: https://github.com/Azure/azure-rest-api-specs/blob/a61456ebadb3ad73cee898b8bdd2f14885663332/specification/apimanagement/resource-manager/Microsoft.ApiManagement/stable/2024-05-01/apimworkspaceloggers.json#L208
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_api_management_workspace_logger
[Support for API Management Workspaces feature set #30522]This is a (please select all that apply):
Related Issue(s)
Fixes #30522