Skip to content

Conversation

sinbai
Copy link
Contributor

@sinbai sinbai commented Sep 19, 2025

Community Note

  • Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
  • Please do not leave comments along the lines of "+1", "me too" or "any updates", they generate extra noise for PR followers and do not help prioritize for review

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

  • I have followed the guidelines in our Contributing Documentation.
  • I have checked to ensure there aren't other open Pull Requests for the same update/change.
  • I have checked if my changes close any open issues. If so please include appropriate closing keywords below.
  • I have updated/added Documentation as required written in a helpful and kind way to assist users that may be unfamiliar with the resource / data source.
  • I have used a meaningful PR title to help maintainers and other users understand this change and help prevent duplicate work.
    For example: “resource_name_here - description of change e.g. adding property new_property_name_here

Changes to existing Resource / Data Source

  • I have added an explanation of what my changes do and why I'd like you to include them (This may be covered by linking to an issue above, but may benefit from additional explanation).
  • I have written new tests for my resource or datasource changes & updated any relevant documentation.
  • I have successfully run tests with my changes locally. If not, please provide details on testing challenges that prevented you running the tests.
  • (For changes that include a state migration only). I have manually tested the migration path between relevant versions of the provider.

Testing

  • My submission includes Test coverage as described in the Contribution Guide and the tests pass. (if this is not possible for any reason, please include details of why you did or could not add test coverage)
image

Change Log

Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.

This is a (please select all that apply):

  • Bug Fix
  • New Feature (ie adding a service, resource, or data source)
  • Enhancement
  • Breaking Change

Related Issue(s)

Fixes #30522

@sinbai sinbai force-pushed the apim/new_resource_workspace_logger branch from be7401f to ba3ea75 Compare September 19, 2025 05:34
@sinbai sinbai force-pushed the apim/new_resource_workspace_logger branch from ba3ea75 to 340d310 Compare September 19, 2025 05:56
@sinbai sinbai force-pushed the apim/new_resource_workspace_logger branch from 340d310 to 85a9606 Compare September 19, 2025 06:48
@sinbai sinbai force-pushed the apim/new_resource_workspace_logger branch from 85a9606 to 09731b2 Compare September 23, 2025 06:48
@sinbai sinbai force-pushed the apim/new_resource_workspace_logger branch from 09731b2 to 9ae0273 Compare September 23, 2025 07:37
@sinbai sinbai force-pushed the apim/new_resource_workspace_logger branch from 9ae0273 to 59d7898 Compare September 23, 2025 07:49
@sinbai sinbai force-pushed the apim/new_resource_workspace_logger branch from 59d7898 to 65fbc33 Compare September 23, 2025 09:39
@sinbai sinbai force-pushed the apim/new_resource_workspace_logger branch from 65fbc33 to d05c2f2 Compare September 24, 2025 08:42
Copy link
Collaborator

@ms-zhenhua ms-zhenhua left a 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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* `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.

Copy link
Contributor Author

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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* `name` - (Required) Specifies the name of the eventhub.
* `name` - (Required) Specifies the name of the Event Hub.

Copy link
Contributor Author

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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* `connection_string` - (Optional) Specifies the connection string of the eventhub namespace.
* `connection_string` - (Optional) Specifies the connection string of the Event Hub namespace.

Copy link
Contributor Author

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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* `endpoint_uri` - (Optional) Specifies the endpoint address of an eventhub namespace.
* `endpoint_uri` - (Optional) Specifies the endpoint address of an Event Hub namespace.

Copy link
Contributor Author

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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* `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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

},
},

"eventhub": {
Copy link
Collaborator

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

Copy link
Contributor Author

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{}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
var _ sdk.Resource = ApiManagementWorkspaceLoggerResource{}
var _ sdk.ResourceWithUpdate = ApiManagementWorkspaceLoggerResource{}

Copy link
Contributor Author

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") {
Copy link
Collaborator

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Comment on lines 424 to 430
output := EventHubModel{
Name: (*input.Credentials)["name"],
}

if endpoint, ok := (*input.Credentials)["endpointAddress"]; ok {
output.EndpointUri = endpoint
}
Copy link
Collaborator

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?

Copy link
Contributor Author

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 {
Copy link
Collaborator

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@sinbai sinbai force-pushed the apim/new_resource_workspace_logger branch from e638215 to 33b5f36 Compare September 29, 2025 09:02
@sinbai
Copy link
Contributor Author

sinbai commented Sep 29, 2025

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 👍

Hi @ms-zhenhua thanks for your feedback. I've addressed the comments, and test results included below. Could you kindly review it again?
image

"eventhub.0.user_assigned_identity_client_id",
},
ValidateFunc: validation.StringIsNotEmpty,
},
Copy link
Collaborator

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

@ziyeqf
Copy link
Collaborator

ziyeqf commented Oct 6, 2025

mostly LGTM but one small comment

@sinbai
Copy link
Contributor Author

sinbai commented Oct 9, 2025

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.
Copy link
Collaborator

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

Copy link
Contributor Author

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?

@ziyeqf
Copy link
Collaborator

ziyeqf commented Oct 13, 2025

LGTM now please address Zhenhua's comment.

@sinbai sinbai force-pushed the apim/new_resource_workspace_logger branch from ca15edd to 1cfec52 Compare October 13, 2025 05:22
Copy link
Collaborator

@ms-zhenhua ms-zhenhua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM~

@sinbai sinbai marked this pull request as ready for review October 13, 2025 06:29
@sinbai sinbai requested review from a team, WodansSon and magodo as code owners October 13, 2025 06:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support for API Management Workspaces feature set

4 participants