Skip to content
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_role_assignment_marketplace #22178

Closed

Conversation

ms-zhenhua
Copy link
Contributor

@ms-zhenhua ms-zhenhua commented Jun 15, 2023

Description

fix #18387
extract marketplace role assignment out from azurerm_role_assignment into azurerm_role_assignment_marketplace according to the comments in #18439

Test Result

image

--- PASS: TestAccRoleAssignmentMarketplace_roleName (67.14s)
--- PASS: TestAccRoleAssignmentMarketplace_requiresImport (59.19s)
--- PASS: TestAccRoleAssignmentMarketplace_emptyName (65.38s)
--- PASS: TestAccRoleAssignmentMarketplace_builtin (65.48s)
--- PASS: TestAccRoleAssignmentMarketplace_ServicePrincipalWithType (69.34s)
--- PASS: TestAccRoleAssignmentMarketplace_ServicePrincipalGroup (90.93s)
--- PASS: TestAccRoleAssignmentMarketplace_ServicePrincipal (93.24s)
PASS
ok github.com/hashicorp/terraform-provider-azurerm/internal/services/authorization 234.346s

Copy link
Collaborator

@magodo magodo left a comment

Choose a reason for hiding this comment

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

Thanks for this PR!
I've taken a look through and left some comments inline. The main concern is that other than Marketplace scope, does there any other un-regular scope exist need to be supported in the future?

@ms-zhenhua
Copy link
Contributor Author

Hi @magodo, thank you for your review. I have resolved the comments. Kindly take another review.
And your concern is reasonable. However, since the provider will validate the scope, it seems hard to support new similar resources without changing the provider.

@ms-zhenhua ms-zhenhua requested a review from magodo June 26, 2023 02:28
@magodo
Copy link
Collaborator

magodo commented Jun 27, 2023

Thank you for all the updates! This now LGTM 👍

Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

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

Thanks for splitting this out @ms-zhenhua.

Unfortunately I think some information was glossed over when we closed #18439. While it's correct to split this out - this should be refactored into a base resource (since the arguments are practically identical across the different role assignment types) - we reference the policy assignment resources in this comment as an example.

Does that make sense?

"fmt"
"time"

"github.com/Azure/azure-sdk-for-go/services/preview/authorization/mgmt/2020-04-01-preview/authorization" // nolint: staticcheck
Copy link
Member

Choose a reason for hiding this comment

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

Can we use go-azure-sdk here?

@ms-zhenhua
Copy link
Contributor Author

Thanks for splitting this out @ms-zhenhua.

Unfortunately I think some information was glossed over when we closed #18439. While it's correct to split this out - this should be refactored into a base resource (since the arguments are practically identical across the different role assignment types) - we reference the policy assignment resources in this comment as an example.

Does that make sense?

Hi @stephybun, thank you for clarifying. I think I misunderstood the meaning of this comment😭. For now, I think there are 2 ways to go on:

  1. Continue with PR 18439 to extract a new azurerm_role_assignment_marketplace, but to extract a common base resource, we need to keep using untyped format and azure-sdk as azurerm_role_assignment for this new resouce. After this issue is fixed, we can create a new PR to refactor both resources to typed resources and use go-azure-sdk later.
  2. First create a new PR to refactor azurerm_role_assignment to use typed resource and go-azure-sdk, then create a new azurerm_role_assignment_marketplace resource to fix this issue.

I prefer to the #1 way to first resolve this issue. WDYT?

@ms-zhenhua
Copy link
Contributor Author

Close this PR and will create a new PR using go-azure-sdk

Copy link

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to create role assignment at resource provider scopeof /providers/Microsoft.Marketplace
3 participants