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

feat: allow data.azuread_application lookup using identifier_uri #1303

Merged
merged 3 commits into from
May 15, 2024

Conversation

JonasBak
Copy link
Contributor

@JonasBak JonasBak commented Feb 2, 2024

This PR introduces a small feature to the azuread_application data source that allows us to get the application based on an identifying URI. This is a feature we would really like to use. It looks like this:

data "azuread_application" "example" {
  identifier_uris  = [
    "api://example-app",
  ]
}

I've implemented it in a way where you can provide a list of one element to the identifier_uris field, instead of introducing another field called identifier_uri that is just a string, as I thought it might be easy to mix up the two fields, but would love some input on this. I added an error if more than one element is provided in identifier_uris.

Copy link
Contributor

@manicminer manicminer left a comment

Choose a reason for hiding this comment

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

Hi @JonasBak, thanks for this contribution, this would be a useful addition to the data source! Apologies for the delayed review.

If we are going to match any one identifier URI, then I think it would be better to add a new property identifier_uri and leave the existing identifier_uris as a computed attribute. My thinking here is that the returned application could have >1 identifier URI, which we would be saving to state, and this would create a discrepency between configuration and state. There is possibly no consequence to this at the moment, but this may throw an error in future versions of the provider RPC protocol (which is how the provider communicates with Terraform).

If you could rework this to add this additional property whilst keeping the existing attribute as-is, then I think this should be good to merge. The remainder of the implementation looks good to me.

Apologies again for the tardiness of this review. If you aren't in a position to make this change, just let me know and I'll happily pick this up whilst preserving your original commits.

Thanks!

@JonasBak
Copy link
Contributor Author

JonasBak commented May 13, 2024

Thanks for the review @manicminer, I rebased and added your suggestion 👍
It now looks like this:

data "azuread_application" "test" {
  identifier_uri = "api://example-app"
}

@JonasBak JonasBak changed the title feat: allow data.azuread_application lookup using identifier_uris feat: allow data.azuread_application lookup using identifier_uri May 13, 2024
Copy link
Contributor

@manicminer manicminer left a comment

Choose a reason for hiding this comment

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

Thanks for updating @JonasBak, this LGTM! 👍

docs/data-sources/application.md Outdated Show resolved Hide resolved
@manicminer
Copy link
Contributor

Test results

Screenshot 2024-05-15 at 23 57 37

@manicminer manicminer merged commit ec84fc7 into hashicorp:main May 15, 2024
27 checks passed
@github-actions github-actions bot added this to the v2.50.0 milestone May 15, 2024
manicminer added a commit that referenced this pull request May 15, 2024
BrendanThompson pushed a commit to BrendanThompson/terraform-provider-azuread that referenced this pull request Jun 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants