New data source: azurerm_subscription #285
Conversation
|
The resources/subscriptions package was added via: |
|
Acceptance tests: |
There was a problem hiding this comment.
Hey @ewbankkit
Thanks for this PR - I've taken a look and left a few minor comments but this generally looks good to me :)
Out of interest - would it be possible to give an example of when this Data Source would be used? Presumably this is to access information about multiple subscriptions within the same Tenant?
Thanks!
| "display_name": { | ||
| Type: schema.TypeString, | ||
| Computed: true, | ||
| }, |
There was a problem hiding this comment.
are there any other fields (e.g. Tenant ID?) that would make sense to be exposed here?
There was a problem hiding this comment.
I have added the subscription state and possible subscription policies.
I'm not adding the tenant as the tenant API only works for the "current" account.
azurerm/data_source_subscription.go
Outdated
|
|
||
| resp, err := groupClient.Get(subscriptionId) | ||
| if err != nil { | ||
| return fmt.Errorf("Error reading subscription: %s", err) |
There was a problem hiding this comment.
given the SDK's currently masking some error messages - can we change this the formatted string to %+v to return the full error message?
| Config: testAccDataSourceAzureRMSubscription_specificConfig(os.Getenv("ARM_SUBSCRIPTION_ID")), | ||
| Check: resource.ComposeTestCheckFunc( | ||
| resource.TestCheckResourceAttrSet(resourceName, "subscription_id"), | ||
| testCheckAzureRMSubscriptionId(resourceName), |
There was a problem hiding this comment.
I think we can remove this in favour of checking via:
resource.TestCheckResourceAttr(resourceName, "subscription_id", "{Subscription ID Value}")
There was a problem hiding this comment.
The reason we can't do as suggested is that {Subscription ID Value} is obtained from the ArmClient for the running test step and this can't be done when the steps are declared (testAccProvider.Meta().(*ArmClient) is nil at this point).
There was a problem hiding this comment.
true, but we could instead check this against the ARM_SUBSCRIPTION_ID environment variable? It's not a blocker by any means and this otherwise LGTM :)
|
I'll work on the changes you suggested. |
Changes after review. Correct schema and add test steps.
|
Made changes after review and rebased. |
There was a problem hiding this comment.
Hey @ewbankkit
Thanks for pushing those updates - I've taken another look and this LGTM :)
Tests Pass:
$ acctests azurerm TestAccDataSourceAzureRMSubscription
=== RUN TestAccDataSourceAzureRMSubscription_current
--- PASS: TestAccDataSourceAzureRMSubscription_current (8.29s)
=== RUN TestAccDataSourceAzureRMSubscription_specific
--- PASS: TestAccDataSourceAzureRMSubscription_specific (7.36s)
PASS
ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 15.681s
Thanks!
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks! |
ghost
locked and limited conversation to collaborators
New data source to return information about a specific Azure subscription.
If not subscription ID is specified, the subscription ID of the current Azure Resource Manager provider is used.
Fixes #284.