new resource `azurerm_graph_account` #22334

teowa · 2023-06-30T04:43:43Z

reference:

doc
swagger

teowa · 2023-07-03T02:59:15Z

internal/services/graph/graph_account_resource_test.go

+
+resource "azurerm_graph_account" "test" {
+  name                = "acctesta-%[2]d"
+  app_id              = azuread_application.test.application_id


this resource is depend on an azuread_application, however through my local test the resource creation may fail occationally with error even after the ad app is created: performing AccountsCreateAndUpdate: unexpected status 400 with error: BadRequest: The application was not found in this tenant.. The service team told that this is hard to change and advices to use a predefined azure ad app, can we do this?

This is a common issue particularly when crossing service boundaries such as this. We can use the calling principal to work around this for now - you can grab this from the environment variable ARM_CLIENT_ID in the test function, see the following suggestion.

manicminer · 2023-07-03T12:57:54Z

internal/services/graph/graph_account_resource.go

+			Type:     pluginsdk.TypeString,
+		},
+		"resource_group_name": commonschema.ResourceGroupName(),
+		"app_id": {


Can we rename this to application_id to align with AzureAD resources

manicminer · 2023-07-03T12:59:32Z

internal/services/graph/graph_account_resource_test.go

+
+resource "azurerm_graph_account" "test" {
+  name                = "acctesta-%[2]d"
+  app_id              = azuread_application.test.application_id


This is a common issue particularly when crossing service boundaries such as this. We can use the calling principal to work around this for now - you can grab this from the environment variable ARM_CLIENT_ID in the test function, see the following suggestion.

internal/services/graph/graph_account_resource_test.go

manicminer · 2023-07-03T13:02:32Z

internal/services/graph/graph_account_resource_test.go

+
+resource "azurerm_graph_account" "test" {
+  name                = "acctesta-%[2]d"
+  app_id              = azuread_application.test.application_id


Suggested change

app_id = azuread_application.test.application_id

application_id = "%[3]s"

manicminer · 2023-07-03T13:02:41Z

internal/services/graph/graph_account_resource_test.go

+  app_id              = azuread_application.test.application_id
+  resource_group_name = azurerm_resource_group.test.name
+}
+`, r.template(data), data.RandomInteger)


Suggested change

`, r.template(data), data.RandomInteger)

`, r.template(data), data.RandomInteger, clientId)

…zurerm into graph_account

teowa · 2023-07-04T15:22:40Z

Hi @manicminer , thanks for reviewing this. I have modified the testcases, but we cannot create two graph account for single ad application, so I have made the test running in serial.

TF_ACC=1 go test -v ./internal/services/graph -parallel 50 -test.run=TestAccGraph -timeout 1440m -ldflags="-X=github.com/hashicorp/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccGraphAccount
=== RUN   TestAccGraphAccount/account
=== RUN   TestAccGraphAccount/account/requiresImport
=== RUN   TestAccGraphAccount/account/basic
=== RUN   TestAccGraphAccount/account/update
=== RUN   TestAccGraphAccount/account/complete
--- PASS: TestAccGraphAccount (586.13s)
    --- PASS: TestAccGraphAccount/account (586.13s)
        --- PASS: TestAccGraphAccount/account/requiresImport (150.75s)
        --- PASS: TestAccGraphAccount/account/basic (144.59s)
        --- PASS: TestAccGraphAccount/account/update (150.11s)
        --- PASS: TestAccGraphAccount/account/complete (140.68s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/graph 587.394s

teowa · 2023-07-12T13:54:17Z

Hi @manicminer, kindly ping. The service team is asking for this.

manicminer · 2023-07-12T15:12:14Z

Apologies @teowa, will look at this today

manicminer

Thanks @teowa, this mostly LGTM. If you can take a look at the comments below, then this should be good to merge.

Tests look good for me:

manicminer · 2023-07-13T11:31:47Z

internal/services/graph/graph_account_resource.go

+		"name": {
+			ForceNew: true,
+			Required: true,
+			Type:     pluginsdk.TypeString,


We should validate this property

manicminer · 2023-07-13T11:35:32Z

internal/services/graph/graph_account_resource.go

+				Location: pointer.To(location.Normalize("global")),
+				Properties: graphservicesprods.AccountResourceProperties{
+					AppId: config.ApplicationId,
+				},
+				Tags: tags.Expand(config.Tags),


Can we tidy this up? i.e. move Properties to the end and align the others

manicminer · 2023-07-13T11:36:06Z

internal/services/graph/graph_account_resource.go

+				ApplicationId: model.Properties.AppId,
+				Name:          id.AccountName, ResourceGroupName: id.ResourceGroupName,
+				Tags: tags.Flatten(model.Tags),


As above, can this be tidied up (one field per line, all aligned for readability)

…m into graph_account

manicminer

Thanks for making the changes, this LGTM 👍

teowa added 4 commits June 16, 2023 17:43

new resource azurerm_graph_account

a32e1fd

complement

3e586c3

new resource azurerm_graph_account

0d247ef

complement

e7ad3d3

github-actions bot added dependencies documentation size/XL labels Jun 30, 2023

teowa added 2 commits June 30, 2023 13:53

fix doc

5811d74

add empty line

8800b20

teowa commented Jul 3, 2023

View reviewed changes

manicminer reviewed Jul 3, 2023

View reviewed changes

github-actions bot added the waiting-response label Jul 3, 2023

teowa added 2 commits July 4, 2023 23:14

use principal for test

2b98897

Merge branch 'graph_account' of github.com:teowa/terraform-provider-a…

a9a2a57

…zurerm into graph_account

github-actions bot removed the waiting-response label Jul 4, 2023

manicminer requested changes Jul 13, 2023

View reviewed changes

github-actions bot added the waiting-response label Jul 13, 2023

refine

7d1737f

github-actions bot added waiting-response and removed waiting-response labels Jul 13, 2023

Merge branch 'main' of github.com:hashicorp/terraform-provider-azurer…

5840fc7

…m into graph_account

github-actions bot added waiting-response and removed waiting-response labels Jul 14, 2023

generate

46a5128

github-actions bot removed the waiting-response label Jul 17, 2023

manicminer approved these changes Jul 18, 2023

View reviewed changes

manicminer added the new-resource label Jul 18, 2023

manicminer added the service/graph label Jul 18, 2023

manicminer added this to the v3.66.0 milestone Jul 18, 2023

manicminer merged commit 08c72f7 into hashicorp:main Jul 18, 2023
24 checks passed

manicminer added a commit that referenced this pull request Jul 18, 2023

Changelog for #22334

882744c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new resource `azurerm_graph_account` #22334

new resource `azurerm_graph_account` #22334

teowa commented Jun 30, 2023

teowa Jul 3, 2023

manicminer Jul 3, 2023

manicminer Jul 3, 2023

manicminer Jul 3, 2023

manicminer Jul 3, 2023

manicminer Jul 3, 2023

teowa commented Jul 4, 2023

teowa commented Jul 12, 2023

manicminer commented Jul 12, 2023

manicminer left a comment •

edited

manicminer Jul 13, 2023

manicminer Jul 13, 2023

manicminer Jul 13, 2023

manicminer left a comment

	app_id = azuread_application.test.application_id
	application_id = "%[3]s"

	`, r.template(data), data.RandomInteger)
	`, r.template(data), data.RandomInteger, clientId)

new resource azurerm_graph_account #22334

new resource azurerm_graph_account #22334

Conversation

teowa commented Jun 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teowa commented Jul 4, 2023

teowa commented Jul 12, 2023

manicminer commented Jul 12, 2023

manicminer left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manicminer left a comment

Choose a reason for hiding this comment

new resource `azurerm_graph_account` #22334

new resource `azurerm_graph_account` #22334

manicminer left a comment •

edited