`azurerm_role_definition` - swap to typed sdk #23679

ziyeqf · 2023-10-25T05:56:50Z

To make the PR clear, only swap to typed sdk in this PR. Then there will be another 3 PRs, to swap to go-azure-sdk, to upgrade sdk version, and adding feature.

test

❯ tftest authorization TestAccRoleDefinition
=== RUN   TestAccRoleDefinitionDataSource_basic
=== PAUSE TestAccRoleDefinitionDataSource_basic
=== RUN   TestAccRoleDefinitionDataSource_basicByName
=== PAUSE TestAccRoleDefinitionDataSource_basicByName
=== RUN   TestAccRoleDefinitionDataSource_builtIn_contributor
=== PAUSE TestAccRoleDefinitionDataSource_builtIn_contributor
=== RUN   TestAccRoleDefinitionDataSource_builtIn_owner
=== PAUSE TestAccRoleDefinitionDataSource_builtIn_owner
=== RUN   TestAccRoleDefinitionDataSource_builtIn_reader
=== PAUSE TestAccRoleDefinitionDataSource_builtIn_reader
=== RUN   TestAccRoleDefinitionDataSource_builtIn_virtualMachineContributor
=== PAUSE TestAccRoleDefinitionDataSource_builtIn_virtualMachineContributor
=== RUN   TestAccRoleDefinition_basic
=== PAUSE TestAccRoleDefinition_basic
=== RUN   TestAccRoleDefinition_requiresImport
=== PAUSE TestAccRoleDefinition_requiresImport
=== RUN   TestAccRoleDefinition_complete
=== PAUSE TestAccRoleDefinition_complete
=== RUN   TestAccRoleDefinition_update
=== PAUSE TestAccRoleDefinition_update
=== RUN   TestAccRoleDefinition_updateEmptyId
=== PAUSE TestAccRoleDefinition_updateEmptyId
=== RUN   TestAccRoleDefinition_emptyName
=== PAUSE TestAccRoleDefinition_emptyName
=== RUN   TestAccRoleDefinition_emptyPermissions
=== PAUSE TestAccRoleDefinition_emptyPermissions
=== RUN   TestAccRoleDefinition_managementGroup
=== PAUSE TestAccRoleDefinition_managementGroup
=== RUN   TestAccRoleDefinition_assignToSmallerScope
=== PAUSE TestAccRoleDefinition_assignToSmallerScope
=== RUN   TestAccRoleDefinition_noAssignableScope
=== PAUSE TestAccRoleDefinition_noAssignableScope
=== CONT  TestAccRoleDefinitionDataSource_basic
=== CONT  TestAccRoleDefinition_complete
=== CONT  TestAccRoleDefinitionDataSource_builtIn_reader
=== CONT  TestAccRoleDefinition_emptyPermissions
=== CONT  TestAccRoleDefinition_noAssignableScope
=== CONT  TestAccRoleDefinition_basic
=== CONT  TestAccRoleDefinition_requiresImport
=== CONT  TestAccRoleDefinition_managementGroup
--- PASS: TestAccRoleDefinitionDataSource_builtIn_reader (43.24s)
=== CONT  TestAccRoleDefinition_assignToSmallerScope
--- PASS: TestAccRoleDefinition_managementGroup (333.77s)
=== CONT  TestAccRoleDefinitionDataSource_builtIn_virtualMachineContributor
=== CONT  TestAccRoleDefinition_updateEmptyId
--- PASS: TestAccRoleDefinitionDataSource_builtIn_virtualMachineContributor (18.15s)
--- PASS: TestAccRoleDefinition_assignToSmallerScope (386.88s)
=== CONT  TestAccRoleDefinition_emptyName
--- PASS: TestAccRoleDefinition_requiresImport (576.82s)
=== CONT  TestAccRoleDefinitionDataSource_builtIn_contributor
--- PASS: TestAccRoleDefinition_basic (577.24s)
=== CONT  TestAccRoleDefinitionDataSource_builtIn_owner
--- PASS: TestAccRoleDefinitionDataSource_builtIn_contributor (24.53s)
=== CONT  TestAccRoleDefinitionDataSource_basicByName
--- PASS: TestAccRoleDefinitionDataSource_builtIn_owner (28.08s)
=== CONT  TestAccRoleDefinition_update
--- PASS: TestAccRoleDefinition_noAssignableScope (609.31s)
--- PASS: TestAccRoleDefinition_complete (621.25s)
--- PASS: TestAccRoleDefinitionDataSource_basic (651.67s)
--- PASS: TestAccRoleDefinition_emptyPermissions (686.30s)
--- PASS: TestAccRoleDefinition_emptyName (610.44s)
--- PASS: TestAccRoleDefinition_updateEmptyId (727.10s)
--- PASS: TestAccRoleDefinitionDataSource_basicByName (819.05s)
--- PASS: TestAccRoleDefinition_update (999.10s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/authorization 1604.514s

tombuildsstuff · 2023-10-25T06:52:35Z

@ziyeqf why are we changing the Resource ID for this resource?

ziyeqf · 2023-10-25T06:57:38Z

@ziyeqf why are we changing the Resource ID for this resource?

I'm trying to swap it to use go-azure-sdk and typed sdk, and the go-azure-sdk could manage the resource id correctly now.
Without changing the resource id there will be more workaround to let the previous patched resource id to fit typed sdk.
So I just update it to use the sdk one.

magodo

Regardless whether we want the resource id migration, I've reviewed the PR and left some comments. Once these are resolved, we can take another look!

internal/services/authorization/migration/role_definition_migration_test.go

internal/services/authorization/role_assignment_resource_base.go

internal/services/authorization/role_definition_data_source.go

internal/services/authorization/role_definition_resource.go

internal/services/authorization/role_definition_data_source.go

internal/services/authorization/role_definition_resource.go

magodo · 2023-10-31T01:49:47Z

@ziyeqf One more comment is that we might want to split this PR to 3 PRs: one to migrate it to the typed sdk and go-azure-sdk, one to upgrade the API version, one to add the new features. WDYT?

ziyeqf · 2023-10-31T06:53:33Z

code updated, I will append the new test result here

migration test

❯ tftest authorization/migration Test       
=== RUN   TestRoleDefinitionMigrateStateV1
2023/10/31 14:53:57 [DEBUG] Migrating ID from v0 to v1 format
2023/10/31 14:53:57 [DEBUG] Migrating ID from v0 to v1 format
--- PASS: TestRoleDefinitionMigrateStateV1 (0.00s)
=== RUN   TestRoleDefinitionMigrateStateV2
2023/10/31 14:53:57 [DEBUG] Migrating ID from v0 to v1 format
2023/10/31 14:53:57 [DEBUG] Migrating ID from v1 to v2 format
2023/10/31 14:53:57 [DEBUG] Migrating ID from v1 to v2 format
2023/10/31 14:53:57 [DEBUG] Migrating ID from v1 to v2 format
2023/10/31 14:53:57 [DEBUG] Migrating ID from v1 to v2 format
2023/10/31 14:53:57 [DEBUG] Migrating ID from v1 to v2 format
2023/10/31 14:53:57 [DEBUG] Migrating ID from v0 to v1 format
2023/10/31 14:53:57 [DEBUG] Migrating ID from v1 to v2 format
2023/10/31 14:53:57 [DEBUG] Migrating ID from v0 to v1 format
2023/10/31 14:53:57 [DEBUG] Migrating ID from v1 to v2 format
2023/10/31 14:53:57 [DEBUG] Migrating ID from v0 to v1 format
2023/10/31 14:53:57 [DEBUG] Migrating ID from v1 to v2 format
--- PASS: TestRoleDefinitionMigrateStateV2 (0.00s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/authorization/migration       (cached)

terraform-provider-azurerm [ tengzh/rbac/role_definition][!] [🐹 v1.21.3]
❯ go clean -cache

terraform-provider-azurerm [ tengzh/rbac/role_definition][!] [🐹 v1.21.3][⏱ 31s]
❯ tftest authorization/migration Test       
=== RUN   TestRoleDefinitionMigrateStateV1
2023/10/31 15:58:42 [DEBUG] Migrating ID from v0 to v1 format
2023/10/31 15:58:42 [DEBUG] Migrating ID from v0 to v1 format
--- PASS: TestRoleDefinitionMigrateStateV1 (0.00s)
=== RUN   TestRoleDefinitionMigrateStateV2
2023/10/31 15:58:42 [DEBUG] Migrating ID from v1 to v2 format
2023/10/31 15:58:42 [DEBUG] Migrating ID from v1 to v2 format
2023/10/31 15:58:42 [DEBUG] Migrating ID from v0 to v1 format
2023/10/31 15:58:42 [DEBUG] Migrating ID from v1 to v2 format
2023/10/31 15:58:42 [DEBUG] Migrating ID from v0 to v1 format
2023/10/31 15:58:42 [DEBUG] Migrating ID from v1 to v2 format
2023/10/31 15:58:42 [DEBUG] Migrating ID from v0 to v1 format
2023/10/31 15:58:42 [DEBUG] Migrating ID from v1 to v2 format
2023/10/31 15:58:42 [DEBUG] Migrating ID from v0 to v1 format
2023/10/31 15:58:42 [DEBUG] Migrating ID from v1 to v2 format
2023/10/31 15:58:42 [DEBUG] Migrating ID from v1 to v2 format
2023/10/31 15:58:42 [DEBUG] Migrating ID from v1 to v2 format
--- PASS: TestRoleDefinitionMigrateStateV2 (0.00s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/authorization/migration       0.017s

role definition

❯ tftest authorization TestAccRoleDefinition         
=== RUN   TestAccRoleDefinitionDataSource_basic
=== PAUSE TestAccRoleDefinitionDataSource_basic
=== RUN   TestAccRoleDefinitionDataSource_basicByName
=== PAUSE TestAccRoleDefinitionDataSource_basicByName
=== RUN   TestAccRoleDefinitionDataSource_builtIn_contributor
=== PAUSE TestAccRoleDefinitionDataSource_builtIn_contributor
=== RUN   TestAccRoleDefinitionDataSource_builtIn_owner
=== PAUSE TestAccRoleDefinitionDataSource_builtIn_owner
=== RUN   TestAccRoleDefinitionDataSource_builtIn_reader
=== PAUSE TestAccRoleDefinitionDataSource_builtIn_reader
=== RUN   TestAccRoleDefinitionDataSource_builtIn_virtualMachineContributor
=== PAUSE TestAccRoleDefinitionDataSource_builtIn_virtualMachineContributor
=== RUN   TestAccRoleDefinition_basic
=== PAUSE TestAccRoleDefinition_basic
=== RUN   TestAccRoleDefinition_requiresImport
=== PAUSE TestAccRoleDefinition_requiresImport
=== RUN   TestAccRoleDefinition_complete
=== PAUSE TestAccRoleDefinition_complete
=== RUN   TestAccRoleDefinition_update
=== PAUSE TestAccRoleDefinition_update
=== RUN   TestAccRoleDefinition_updateEmptyId
=== PAUSE TestAccRoleDefinition_updateEmptyId
=== RUN   TestAccRoleDefinition_emptyName
=== PAUSE TestAccRoleDefinition_emptyName
=== RUN   TestAccRoleDefinition_emptyPermissions
=== PAUSE TestAccRoleDefinition_emptyPermissions
=== RUN   TestAccRoleDefinition_managementGroup
=== PAUSE TestAccRoleDefinition_managementGroup
=== RUN   TestAccRoleDefinition_assignToSmallerScope
=== PAUSE TestAccRoleDefinition_assignToSmallerScope
=== RUN   TestAccRoleDefinition_noAssignableScope
=== PAUSE TestAccRoleDefinition_noAssignableScope
=== CONT  TestAccRoleDefinitionDataSource_basic
=== CONT  TestAccRoleDefinition_complete
=== CONT  TestAccRoleDefinition_emptyPermissions
=== CONT  TestAccRoleDefinition_updateEmptyId
=== CONT  TestAccRoleDefinition_assignToSmallerScope
=== CONT  TestAccRoleDefinition_managementGroup
=== CONT  TestAccRoleDefinition_update
=== CONT  TestAccRoleDefinition_noAssignableScope
--- PASS: TestAccRoleDefinition_managementGroup (527.30s)
=== CONT  TestAccRoleDefinition_emptyName
--- PASS: TestAccRoleDefinition_complete (569.17s)
=== CONT  TestAccRoleDefinitionDataSource_builtIn_reader
=== CONT  TestAccRoleDefinition_requiresImport
--- PASS: TestAccRoleDefinition_noAssignableScope (609.31s)
--- PASS: TestAccRoleDefinitionDataSource_builtIn_reader (55.84s)
=== CONT  TestAccRoleDefinition_basic
--- PASS: TestAccRoleDefinitionDataSource_basic (639.52s)
=== CONT  TestAccRoleDefinitionDataSource_builtIn_virtualMachineContributor
--- PASS: TestAccRoleDefinition_assignToSmallerScope (684.53s)
=== CONT  TestAccRoleDefinitionDataSource_builtIn_contributor
--- PASS: TestAccRoleDefinitionDataSource_builtIn_virtualMachineContributor (48.41s)
=== CONT  TestAccRoleDefinitionDataSource_builtIn_owner
=== CONT  TestAccRoleDefinitionDataSource_basicByName
--- PASS: TestAccRoleDefinitionDataSource_builtIn_contributor (57.03s)
--- PASS: TestAccRoleDefinitionDataSource_builtIn_owner (66.22s)
--- PASS: TestAccRoleDefinition_emptyPermissions (768.66s)
--- PASS: TestAccRoleDefinition_emptyName (354.20s)
--- PASS: TestAccRoleDefinition_requiresImport (272.23s)
--- PASS: TestAccRoleDefinition_updateEmptyId (1019.50s)
--- PASS: TestAccRoleDefinition_update (1147.57s)
--- PASS: TestAccRoleDefinition_basic (597.67s)
--- PASS: TestAccRoleDefinitionDataSource_basicByName (1022.48s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/authorization 1764.120s

internal/services/authorization/role_definition_data_source.go

internal/services/authorization/role_definition_resource.go

Signed-off-by: ziyeqf <[email protected]>

stephybun

Thanks for this PR @ziyeqf. I left a few very minor comments which would be helpful for us if you looked at.

Although the tests are passing, from memory role definition is problematic resource, so I'd like to have another member of the team review it too.

stephybun · 2023-11-01T12:01:52Z

internal/services/authorization/role_definition_data_source.go

-func dataSourceArmRoleDefinition() *pluginsdk.Resource {
-	return &pluginsdk.Resource{
-		Read: dataSourceArmRoleDefinitionRead,
+type ArmRoleDefinitionDataSource struct{}


Arm.. is a legacy naming pattern in the provider. This would be a good opportunity to remove it, can you please rename this to

Suggested change

type ArmRoleDefinitionDataSource struct{}

type RoleDefinitionDataSource struct{}

stephybun · 2023-11-02T13:09:08Z

internal/services/authorization/role_definition_resource.go

-		Read:   resourceArmRoleDefinitionRead,
-		Update: resourceArmRoleDefinitionUpdate,
-		Delete: resourceArmRoleDefinitionDelete,
+type ArmRoleDefinitionResource struct{}


Same rename here

Suggested change

type ArmRoleDefinitionResource struct{}

type RoleDefinitionResource struct{}

stephybun · 2023-11-02T13:14:18Z

internal/services/authorization/role_definition_resource.go

+				return err
+			}
+			if read.ID == nil || *read.ID == "" {
+				return fmt.Errorf("Cannot read Role Definition ID for %q (Scope %q)", config.Name, config.Scope)


Beginning of error messages should be lowercased

Suggested change

return fmt.Errorf("Cannot read Role Definition ID for %q (Scope %q)", config.Name, config.Scope)

return fmt.Errorf("cannot read Role Definition ID for %q (Scope %q)", config.Name, config.Scope)

ziyeqf · 2023-12-05T05:15:45Z

Just finished the rename which I should do earlier...

jackofallops

Thanks for finishing up the changes @ziyeqf - This LGTM 👍

Outdated

github-actions · 2024-05-05T02:10:25Z

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 bot added documentation dependencies service/authorization state-migration size/XL waiting-response and removed waiting-response labels Oct 25, 2023

tombuildsstuff added the waiting-response label Oct 25, 2023

github-actions bot removed the waiting-response label Oct 25, 2023

magodo requested changes Oct 26, 2023

View reviewed changes

magodo previously requested changes Oct 31, 2023

View reviewed changes

internal/services/authorization/role_definition_data_source.go Outdated Show resolved Hide resolved

internal/services/authorization/role_definition_resource.go Show resolved Hide resolved

internal/services/authorization/role_definition_resource.go Outdated Show resolved Hide resolved

github-actions bot added waiting-response and removed dependencies waiting-response labels Oct 31, 2023

magodo reviewed Oct 31, 2023

View reviewed changes

internal/services/authorization/role_definition_data_source.go Outdated Show resolved Hide resolved

internal/services/authorization/role_definition_data_source.go Outdated Show resolved Hide resolved

internal/services/authorization/role_definition_resource.go Outdated Show resolved Hide resolved

github-actions bot added waiting-response and removed waiting-response labels Oct 31, 2023

magodo marked this pull request as ready for review November 1, 2023 02:54

swap to typed sdk

01c8e59

Signed-off-by: ziyeqf <[email protected]>

ziyeqf changed the title ~~azurerm_role_definition: swap to typed sdk & swap to go-azure-sdk & migrate resource id~~ azurerm_role_definition: swap to typed sdk Nov 1, 2023

ziyeqf force-pushed the tengzh/rbac/role_definition branch from 03e4442 to 01c8e59 Compare November 1, 2023 07:50

github-actions bot added waiting-response and removed documentation state-migration labels Nov 1, 2023

golint

ce1d32a

Signed-off-by: ziyeqf <[email protected]>

github-actions bot removed the waiting-response label Nov 1, 2023

stephybun reviewed Nov 2, 2023

View reviewed changes

github-actions bot added the waiting-response label Nov 2, 2023

stephybun added refactor and removed waiting-response labels Nov 2, 2023

stephybun assigned jackofallops Nov 2, 2023

remove unused comments

529790f

ziyeqf changed the title ~~azurerm_role_definition: swap to typed sdk~~ azurerm_role_definition - swap to typed sdk Nov 13, 2023

Merge branch 'main' into tengzh/rbac/role_definition

631e800

github-actions bot added the waiting-response label Nov 27, 2023

refresh vendor

1a9473c

github-actions bot removed the waiting-response label Nov 27, 2023

update per comment

5296ead

jackofallops approved these changes Dec 6, 2023

View reviewed changes

jackofallops merged commit 7c1a305 into hashicorp:main Dec 6, 2023
22 checks passed

github-actions bot added this to the v3.84.0 milestone Dec 6, 2023

jackofallops added a commit that referenced this pull request Dec 6, 2023

Update CHANGELOG.md for #23679

9cf3306

ziyeqf mentioned this pull request Dec 18, 2023

azurerm_role_definition - swap to go-azure-sdk #24266

Merged

github-actions bot locked as resolved and limited conversation to collaborators May 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`azurerm_role_definition` - swap to typed sdk #23679

`azurerm_role_definition` - swap to typed sdk #23679

ziyeqf commented Oct 25, 2023 •

edited

Loading

tombuildsstuff commented Oct 25, 2023

ziyeqf commented Oct 25, 2023 •

edited

Loading

magodo left a comment

magodo commented Oct 31, 2023

ziyeqf commented Oct 31, 2023 •

edited

Loading

stephybun left a comment

stephybun Nov 1, 2023

stephybun Nov 2, 2023

stephybun Nov 2, 2023

ziyeqf commented Dec 5, 2023

jackofallops left a comment

github-actions bot commented May 5, 2024

	type ArmRoleDefinitionDataSource struct{}
	type RoleDefinitionDataSource struct{}

	type ArmRoleDefinitionResource struct{}
	type RoleDefinitionResource struct{}

	return fmt.Errorf("Cannot read Role Definition ID for %q (Scope %q)", config.Name, config.Scope)
	return fmt.Errorf("cannot read Role Definition ID for %q (Scope %q)", config.Name, config.Scope)

azurerm_role_definition - swap to typed sdk #23679

azurerm_role_definition - swap to typed sdk #23679

Conversation

ziyeqf commented Oct 25, 2023 • edited Loading

test

tombuildsstuff commented Oct 25, 2023

ziyeqf commented Oct 25, 2023 • edited Loading

magodo left a comment

Choose a reason for hiding this comment

magodo commented Oct 31, 2023

ziyeqf commented Oct 31, 2023 • edited Loading

migration test

role definition

stephybun left a comment

Choose a reason for hiding this comment

stephybun Nov 1, 2023

Choose a reason for hiding this comment

stephybun Nov 2, 2023

Choose a reason for hiding this comment

stephybun Nov 2, 2023

Choose a reason for hiding this comment

ziyeqf commented Dec 5, 2023

jackofallops left a comment

Choose a reason for hiding this comment

github-actions bot commented May 5, 2024

`azurerm_role_definition` - swap to typed sdk #23679

`azurerm_role_definition` - swap to typed sdk #23679

ziyeqf commented Oct 25, 2023 •

edited

Loading

ziyeqf commented Oct 25, 2023 •

edited

Loading

ziyeqf commented Oct 31, 2023 •

edited

Loading