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

Use filter for azuread_service_principal and azuread_application data sources #1862

Merged
merged 3 commits into from
Sep 7, 2018

Conversation

tiwood
Copy link
Contributor

@tiwood tiwood commented Sep 4, 2018

This fixes #1844

This adds a filter to the ListAll() method to limit the results that are returned by the API to only include objects that match the data source query.

@ghost ghost added the size/M label Sep 4, 2018
@tiwood tiwood changed the title Use filter for azuread_service_principal and azuread_application datasources Use filter for azuread_service_principal and azuread_application data sources Sep 4, 2018
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.

hey @tiwood

Thanks for this PR - I've taken a look through and this mostly LGTM, if we can fix up the (minor) comments we should be able to run the tests and get this merged :)

Thanks!

azurerm/data_source_azuread_application.go Outdated Show resolved Hide resolved
azurerm/data_source_azuread_service_principal.go Outdated Show resolved Hide resolved
azurerm/data_source_azuread_service_principal.go Outdated Show resolved Hide resolved
@ghost ghost added the size/M label Sep 7, 2018
Copy link
Contributor Author

@tiwood tiwood left a comment

Choose a reason for hiding this comment

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

Made changes as requested. Thank you for the review.

azurerm/data_source_azuread_application.go Outdated Show resolved Hide resolved
azurerm/data_source_azuread_service_principal.go Outdated Show resolved Hide resolved
azurerm/data_source_azuread_service_principal.go Outdated Show resolved Hide resolved
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.

hey @tiwood

Thanks for pushing those changes - this now LGTM 👍

Thanks!

@tombuildsstuff
Copy link
Contributor

Tests pass:

screenshot 2018-09-07 at 20 30 38

@tombuildsstuff tombuildsstuff merged commit 90ac0fc into hashicorp:master Sep 7, 2018
tombuildsstuff added a commit that referenced this pull request Sep 7, 2018
@ghost
Copy link

ghost commented Mar 6, 2019

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 6, 2019
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_azuread_service_principal does not always find a Service Principal
2 participants