-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Support for Azure Active Directory for Azure Service fabric #2553
Conversation
add _id to fields for clarity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @DenheenJ
Thanks for this PR :)
Taking a look through this mostly LGTM - if we can fix up the minor issues (and the tests pass 😄) then this should be good to merge 👍
Thanks!
Co-Authored-By: DenheenJ <39860623+DenheenJ@users.noreply.github.com>
Co-Authored-By: DenheenJ <39860623+DenheenJ@users.noreply.github.com>
Co-Authored-By: DenheenJ <39860623+DenheenJ@users.noreply.github.com>
Co-Authored-By: DenheenJ <39860623+DenheenJ@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @DenheenJ I've run the tests for this and whilst the existing tests pass the new one doesn't, from what appears to be a configuration failure - out of interest do we need another configuration option set in the tests for this?
Thanks! |
@tombuildsstuff i managed to reproduce the same failure when a certificate is not included. I have added the certificate to the test file. |
I have re-run the test and we are now getting this failure on our CI system:
|
sorry i added a holiday typo! should pass the test now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix, however i believe the test checks need a slight change:
------- Stdout: -------
=== RUN TestAccAzureRMServiceFabricCluster_azureActiveDirectory
=== PAUSE TestAccAzureRMServiceFabricCluster_azureActiveDirectory
=== CONT TestAccAzureRMServiceFabricCluster_azureActiveDirectory
--- FAIL: TestAccAzureRMServiceFabricCluster_azureActiveDirectory (72.87s)
testing.go:538: Step 0 error: Check failed: Check 6/11 error: azurerm_service_fabric_cluster.test: Attribute 'azure_active_directory.tenant_id' not found
FAIL
Co-Authored-By: DenheenJ <39860623+DenheenJ@users.noreply.github.com>
Co-Authored-By: DenheenJ <39860623+DenheenJ@users.noreply.github.com>
Co-Authored-By: DenheenJ <39860623+DenheenJ@users.noreply.github.com>
I've added additional resources to the test to use real resources however the client app still needs to be added manually as i don't think there is a way to do that in Terraform with this provider yet. |
hey @DenheenJ I hope you don't mind but I've pushed a commit to fix the failing test I mentioned above and the tests now pass: Thanks for this @DenheenJ! |
@tombuildsstuff no that's fantastic thank you. |
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! |
This PR adds support for Azure Active Directory (azure_active_directory) for Azure Service fabric (azurerm_service_fabric_cluster)
Addresses issue #2539
(fixes #2539 )