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

Making the azurerm_client_config data source work with AzureCLI auth #393

Merged
merged 1 commit into from Oct 5, 2017

Conversation

tombuildsstuff
Copy link
Member

Fixes #392

Unfortunately testing this is tricky, since we rely on a Service Principal to run the acceptance tests (and not AzureCLI auth) - however I've verified this works as expected in both cases

Tests pass:

$ acctests azurerm TestAccDataSourceAzureRMClientConfig_basic
=== RUN   TestAccDataSourceAzureRMClientConfig_basic
--- PASS: TestAccDataSourceAzureRMClientConfig_basic (12.65s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	12.673s

Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

LGTM, although I don't know much about service principles nor Azure, so take that with a pinch of salt.


servicePrincipal := (*listResult.Value)[0]
servicePrincipal = &(*listResult.Value)[0]
Copy link
Member

Choose a reason for hiding this comment

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

🙀

clientId string
tenantId string
subscriptionId string
usingServicePrincipal bool
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick, but using* to me sounds like a state that may be managed from somewhere outside, how do you feel about calling it useServicePrinciple instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

it will be managed from somewhere outside (it's determined in the Config class, and then cast to an ArmConfig once the authentication tokens have been obtained) - as such I think using makes more sense in this context than use? That said, I'm happy to change this - but I'm a little stumped for a better name ¯_(ツ)_/¯

Copy link
Member Author

Choose a reason for hiding this comment

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

(merging this for the moment - we can rename this later if needed)

@tombuildsstuff tombuildsstuff merged commit cf64101 into master Oct 5, 2017
@tombuildsstuff tombuildsstuff deleted the data-source-client-config branch October 5, 2017 12:39
tombuildsstuff added a commit that referenced this pull request Oct 5, 2017
@ghost
Copy link

ghost commented Apr 1, 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!

@hashicorp hashicorp locked and limited conversation to collaborators Apr 1, 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.

azurerm_client_config error listing Service Principals
2 participants