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

Support for Managed Identity on azurerm_policy_assignment #2549

Conversation

olohmann
Copy link
Contributor

This PR adds support for Managed Identity on azurerm_policy_assignment.

Addresses issue #2194

Test Results:

==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestAccAzureRMPolicyAssignment -timeout 180m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
?   	github.com/terraform-providers/terraform-provider-azurerm	[no test files]
=== RUN   TestAccAzureRMPolicyAssignment_basic
=== PAUSE TestAccAzureRMPolicyAssignment_basic
=== RUN   TestAccAzureRMPolicyAssignment_deployIfNotExists_policy
=== PAUSE TestAccAzureRMPolicyAssignment_deployIfNotExists_policy
=== RUN   TestAccAzureRMPolicyAssignment_complete
=== PAUSE TestAccAzureRMPolicyAssignment_complete
=== CONT  TestAccAzureRMPolicyAssignment_basic
=== CONT  TestAccAzureRMPolicyAssignment_complete
=== CONT  TestAccAzureRMPolicyAssignment_deployIfNotExists_policy
--- PASS: TestAccAzureRMPolicyAssignment_complete (247.55s)
--- PASS: TestAccAzureRMPolicyAssignment_basic (247.76s)
--- PASS: TestAccAzureRMPolicyAssignment_deployIfNotExists_policy (251.44s)

(fixes #2194)

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 @olohmann

Thanks for this PR - I've taken a look and left some comments inline, but this is mostly looking good. The main question I've got is around the design of the schema, which IMO would be better to match the approach used by the Virtual Machine (and other) resources - but most of the other comments are fairly minor 👍

Thanks!

azurerm/resource_arm_policy_assignment.go Outdated Show resolved Hide resolved
azurerm/resource_arm_policy_assignment.go Outdated Show resolved Hide resolved
azurerm/resource_arm_policy_assignment.go Show resolved Hide resolved
azurerm/resource_arm_policy_assignment.go Outdated Show resolved Hide resolved
azurerm/resource_arm_policy_assignment.go Outdated Show resolved Hide resolved
website/docs/r/policy_assignment.html.markdown Outdated Show resolved Hide resolved
website/docs/r/policy_assignment.html.markdown Outdated Show resolved Hide resolved
azurerm/resource_arm_policy_assignment.go Outdated Show resolved Hide resolved
website/docs/r/policy_assignment.html.markdown Outdated Show resolved Hide resolved
@olohmann
Copy link
Contributor Author

Again, thanks a lot for the great feedback! Applied requested changes.

Test results:

> $ make testacc TESTARGS='-run=TestAccAzureRMPolicyAssignment'       
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestAccAzureRMPolicyAssignment -timeout 180m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
?   	github.com/terraform-providers/terraform-provider-azurerm	[no test files]
=== RUN   TestAccAzureRMPolicyAssignment_basic
=== PAUSE TestAccAzureRMPolicyAssignment_basic
=== RUN   TestAccAzureRMPolicyAssignment_deployIfNotExists_policy
=== PAUSE TestAccAzureRMPolicyAssignment_deployIfNotExists_policy
=== RUN   TestAccAzureRMPolicyAssignment_complete
=== PAUSE TestAccAzureRMPolicyAssignment_complete
=== CONT  TestAccAzureRMPolicyAssignment_basic
=== CONT  TestAccAzureRMPolicyAssignment_complete
=== CONT  TestAccAzureRMPolicyAssignment_deployIfNotExists_policy
--- PASS: TestAccAzureRMPolicyAssignment_basic (247.32s)
--- PASS: TestAccAzureRMPolicyAssignment_complete (247.38s)
--- PASS: TestAccAzureRMPolicyAssignment_deployIfNotExists_policy (250.14s)

@ghost ghost removed the waiting-response label Dec 22, 2018
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Hi @olohmann,

I've taken a quick look at it seems you mention UserAssinged in the docs but its not supported in the schema?

website/docs/r/policy_assignment.html.markdown 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 @olohmann

Thanks for pushing those changes - this now LGTM 👍 - I'll kick off the tests shortly

Thanks!

@tombuildsstuff tombuildsstuff dismissed katbyte’s stale review January 3, 2019 11:16

dismissing since changes have been pushed

@tombuildsstuff tombuildsstuff added this to the 1.21.0 milestone Jan 3, 2019
@tombuildsstuff
Copy link
Contributor

Tests pass - thanks for this @olohmann :)

screenshot 2019-01-03 at 11 26 37

@tombuildsstuff tombuildsstuff merged commit 3d63906 into hashicorp:master Jan 3, 2019
tombuildsstuff added a commit that referenced this pull request Jan 3, 2019
@ghost
Copy link

ghost commented Mar 5, 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 5, 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.

Managed Identity on azurerm_policy_assignment
3 participants