azurerm_lighthouse_definition - support the eligible_authorization property
#19569
Conversation
There was a problem hiding this comment.
Thanks @neil-yechenwei
This looks off to a good start, some questions below on the new schema if you could take a look.
Thanks!
| ValidateFunc: validation.IsUUID, | ||
| }, | ||
|
|
||
| "principal_display_name": { |
There was a problem hiding this comment.
Does this provide any additional benefit to just using the principal_id? What happens if the id and display name do not match?
There was a problem hiding this comment.
The resource still can be created successfully if the id and display name do not match.
Service team confirmed they don't have any strong validation enforced for PrincipalIdDisplayName with Azure Active directory to cross check the name correctness.
The display name in AzureLighthouse resource is that customers can be able to have some mapping to this principalId given and not has any tight dependency with actual name of the principal itself.
To align with the previous style and official sample, so I added it here.
| "eligible_authorization": { | ||
| Type: pluginsdk.TypeSet, | ||
| Optional: true, | ||
| Elem: &pluginsdk.Resource{ |
There was a problem hiding this comment.
Are there a maximum number of elements this Set allows in the API?
There was a problem hiding this comment.
I didn't find the maximum number from anywhere. The autorization previously added also didn't mention the maximum number.
| }, | ||
| }, | ||
|
|
||
| "principal_display_name": { |
There was a problem hiding this comment.
as above, what happens if this does not match the display name of the principal_id?
There was a problem hiding this comment.
Same as above
| principal_display_name = "Reader" | ||
| } | ||
|
|
||
| eligible_authorization { |
There was a problem hiding this comment.
Should we have a test with multiple entries for this new block?
There was a problem hiding this comment.
Given test data limitation, there is no more principal for testing. So we cannot add one more block. As you can see, we only can test them on local with limited test data. And per my understanding, I assume it's not a required thing to test multiple entries for new block.
|
@jackofallops , thanks for the comment. I've updated PR and left some suggestion. |
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
This PR is to support new property
eligible_authorizationforazurerm_lighthouse_definition.Note: As the TCs related with Lighthouse Definition are skipped in Teamcity due to test data issue, so I ran them on my local and they passed.
--- PASS: TestAccLighthouseDefinition_plan (134.77s)
--- PASS: TestAccLighthouseDefinition_basic (135.99s)
--- PASS: TestAccLighthouseDefinition_emptyID (138.56s)
--- PASS: TestAccLighthouseDefinition_eligibleAuthorization (157.81s)
--- PASS: TestAccLighthouseDefinition_complete (160.86s)
--- PASS: TestAccLighthouseDefinition_requiresImport (184.04s)
--- PASS: TestAccLighthouseDefinition_update (527.25s)