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 data source: azurerm_subscriptions #940

Merged
merged 3 commits into from
Mar 6, 2018
Merged

Conversation

katbyte
Copy link
Collaborator

@katbyte katbyte commented Mar 6, 2018

No description provided.

@katbyte katbyte added this to the 1.3.0 milestone Mar 6, 2018
@katbyte katbyte requested a review from tombuildsstuff March 6, 2018 16:43
@katbyte katbyte force-pushed the f-ds-subscriptions branch from 1a8ebc7 to 87229fb Compare March 6, 2018 16:44
@katbyte katbyte force-pushed the f-ds-subscriptions branch from 87229fb to 3633361 Compare March 6, 2018 16:45
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

a few minor points - otherwise LGTM assuming the tests pass 👍

"spending_limit": {
Type: schema.TypeString,
Computed: true,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

do you think it's also worth exposing the TenantID here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The tenant will always be what the provider is using and accessible from client_config data source.

if policies := val.SubscriptionPolicies; policies != nil {
s["location_placement_id"] = *policies.LocationPlacementID
s["quota_id"] = *policies.QuotaID
s["spending_limit"] = policies.SpendingLimit
Copy link
Contributor

Choose a reason for hiding this comment

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

we should add nil checks around these, otherwise there's a potential crash

## Example Usage

```hcl
data "azurerm_subscriptions" "availible" {}
Copy link
Contributor

Choose a reason for hiding this comment

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

minor availible -> available

```hcl
data "azurerm_subscriptions" "availible" {}

output "availible_subscriptions" {
Copy link
Contributor

Choose a reason for hiding this comment

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

(same here)

value = "${data.azurerm_subscriptions.current.subscriptions}"
}

output "first_availible_subscription_display_name" {
Copy link
Contributor

Choose a reason for hiding this comment

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

(same here)

return map[string]*schema.Schema{
"subscription_id": {
Type: schema.TypeString,
Optional: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Given this is only optional for the Subscription Data Source - I believe we don't want this set for the Subscriptions Data Source? We can override the schema for the Subscription data source to add this, there's examples in the AWS Provider

@@ -4,11 +4,11 @@ import (
"github.com/hashicorp/terraform/helper/schema"
)

func SubscriptionSchema() map[string]*schema.Schema {
return map[string]*schema.Schema{
func SubscriptionSchema(subscriptionIdOptional bool) map[string]*schema.Schema {
Copy link
Contributor

Choose a reason for hiding this comment

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

@katbyte
Copy link
Collaborator Author

katbyte commented Mar 6, 2018

Updated with requested changes.

@katbyte
Copy link
Collaborator Author

katbyte commented Mar 6, 2018

Tests pass:
screen shot 2018-03-06 at 11 25 54

@katbyte katbyte merged commit ecafad0 into master Mar 6, 2018
@katbyte katbyte deleted the f-ds-subscriptions branch March 6, 2018 19:26
katbyte added a commit that referenced this pull request Mar 9, 2018
tombuildsstuff pushed a commit that referenced this pull request Mar 13, 2018
@ghost
Copy link

ghost commented Mar 31, 2020

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 👉 hashibot-feedback@hashicorp.com. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 31, 2020
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.

3 participants