update per comments

Signed-off-by: ziyeqf <51212351+ziyeqf@users.noreply.github.com>
hashicorp · Nov 1, 2023 · 03e4442 · 03e4442
1 parent 74d1b5d
commit 03e4442
Show file tree

Hide file tree

Showing 3 changed files with 13 additions and 8 deletions.
diff --git a/internal/services/authorization/role_definition_data_source.go b/internal/services/authorization/role_definition_data_source.go
@@ -199,6 +199,10 @@ func (a ArmRoleDefinitionDataSource) Read() sdk.ResourceFunc {
 				return fmt.Errorf("retirving %s: `Model` was nil", id)
 			}
 
+			if role.Id == nil {
+				return fmt.Errorf("retirving %s: `Id` was nil", id)
+			}
+
 			role = *resp.Model
 
 			state := ArmRoleDefinitionDataSourceModel{
@@ -214,8 +218,9 @@ func (a ArmRoleDefinitionDataSource) Read() sdk.ResourceFunc {
 				state.AssignableScopes = pointer.From(props.AssignableScopes)
 			}
 
-			// when `config.Scope` is "", the id starts with two "/"
-			// so we take the id from response to keep consistency
+			// The sdk managed id start with two "/" when scope is tenant level (empty).
+			// So we use the id from response without parsing and reformating it.
+			// Tracked on https://github.com/hashicorp/pandora/issues/3257
 			metadata.ResourceData.SetId(*role.Id)
 			return metadata.Encode(&state)
 		},

diff --git a/internal/services/authorization/role_definition_data_source_test.go b/internal/services/authorization/role_definition_data_source_test.go
@@ -67,7 +67,7 @@ func TestAccRoleDefinitionDataSource_builtIn_contributor(t *testing.T) {
 		{
 			Config: RoleDefinitionDataSource{}.builtIn("Contributor"),
 			Check: acceptance.ComposeTestCheckFunc(
-				check.That(data.ResourceName).Key("id").Exists(),
+				check.That(data.ResourceName).Key("id").HasValue("/providers/Microsoft.Authorization/roleDefinitions/b24988ac-6180-42a0-ab88-20f7382dd24c"),
 				check.That(data.ResourceName).Key("description").Exists(),
 				check.That(data.ResourceName).Key("type").Exists(),
 				check.That(data.ResourceName).Key("permissions.#").HasValue("1"),
@@ -94,7 +94,7 @@ func TestAccRoleDefinitionDataSource_builtIn_owner(t *testing.T) {
 		{
 			Config: RoleDefinitionDataSource{}.builtIn("Owner"),
 			Check: acceptance.ComposeTestCheckFunc(
-				check.That(data.ResourceName).Key("id").Exists(),
+				check.That(data.ResourceName).Key("id").HasValue("/providers/Microsoft.Authorization/roleDefinitions/8e3af657-a8ff-443c-a75c-2fe8c4bcb635"),
 				check.That(data.ResourceName).Key("description").Exists(),
 				check.That(data.ResourceName).Key("type").Exists(),
 				check.That(data.ResourceName).Key("permissions.#").HasValue("1"),
@@ -113,7 +113,7 @@ func TestAccRoleDefinitionDataSource_builtIn_reader(t *testing.T) {
 		{
 			Config: RoleDefinitionDataSource{}.builtIn("Reader"),
 			Check: acceptance.ComposeTestCheckFunc(
-				check.That(data.ResourceName).Key("id").Exists(),
+				check.That(data.ResourceName).Key("id").HasValue("/providers/Microsoft.Authorization/roleDefinitions/acdd72a7-3385-48ef-bd42-f606fba81ae7"),
 				check.That(data.ResourceName).Key("description").Exists(),
 				check.That(data.ResourceName).Key("type").Exists(),
 				check.That(data.ResourceName).Key("permissions.#").HasValue("1"),
@@ -132,7 +132,7 @@ func TestAccRoleDefinitionDataSource_builtIn_virtualMachineContributor(t *testin
 		{
 			Config: RoleDefinitionDataSource{}.builtIn("Virtual Machine Contributor"),
 			Check: acceptance.ComposeTestCheckFunc(
-				check.That(data.ResourceName).Key("id").Exists(),
+				check.That(data.ResourceName).Key("id").HasValue("/providers/Microsoft.Authorization/roleDefinitions/9980e02c-c2be-4d73-94e8-173b1dc7cf3c"),
 				check.That(data.ResourceName).Key("description").Exists(),
 				check.That(data.ResourceName).Key("type").Exists(),
 				check.That(data.ResourceName).Key("permissions.#").HasValue("1"),

diff --git a/internal/services/authorization/role_definition_resource.go b/internal/services/authorization/role_definition_resource.go
@@ -220,8 +220,8 @@ func (r ArmRoleDefinitionResource) Read() sdk.ResourceFunc {
 			}
 
 			if model := resp.Model; model != nil {
-				// for some scopes (management group), the resource id on Azure is different from the sdk generated one.
-				// so we read from the response.
+				// The Azure resource id of Role Definition is not as same as the one we used to create it.
+				// So we read from the response.
 				state.RoleDefinitionResourceId = pointer.From(model.Id)
 				if props := model.Properties; props != nil {
 					state.Name = pointer.From(props.RoleName)