New Resource: azurerm_policy_set_definition #2535

olohmann · 2018-12-18T13:53:57Z

This PR adds support for a new resource: Policy Set Definitions (aka Policy Initiatives).

Addresses issue #2193

Test Results:

> $ make testacc TESTARGS='-run=TestAccAzureRMPolicySetDefinition'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestAccAzureRMPolicySetDefinition -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   TestAccAzureRMPolicySetDefinition_basic
=== PAUSE TestAccAzureRMPolicySetDefinition_basic
=== CONT  TestAccAzureRMPolicySetDefinition_basic
--- PASS: TestAccAzureRMPolicySetDefinition_basic (113.77s)

(fixed #2193)

katbyte

Hi @olohmann,

Thank you for the new resource! Aside from a few (mostly minor) comments i've left inline this is looking pretty good. Once those issues have been addressed look forward to getting this merged for you 🙂

azurerm/resource_arm_policy_set_definition.go

website/docs/r/policy_set_definition.html.markdown

Co-Authored-By: olohmann <[email protected]>

olohmann · 2018-12-18T19:02:39Z

@katbyte - thanks so much for that quick and helpful review!

Applied requested changes and ran acceptance tests again.

> $ make testacc TESTARGS='-run=TestAccAzureRMPolicySetDefinition'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestAccAzureRMPolicySetDefinition -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   TestAccAzureRMPolicySetDefinition_basic
=== PAUSE TestAccAzureRMPolicySetDefinition_basic
=== CONT  TestAccAzureRMPolicySetDefinition_basic
--- PASS: TestAccAzureRMPolicySetDefinition_basic (111.78s)

tombuildsstuff

hey @olohmann

Thanks for pushing those changes - I've taken a look through and left some minor comments in line but this is otherwise looking good; if we can fix those up (and add an extra test case) we should be able to get this merged 👍

Thanks!

azurerm/resource_arm_policy_set_definition.go

website/docs/r/policy_set_definition.html.markdown

azurerm/resource_arm_policy_set_definition.go

azurerm/resource_arm_policy_set_definition_test.go

Co-Authored-By: olohmann <[email protected]>

…erraform-provider-azurerm into azurerm_policy_set_definition

olohmann · 2018-12-19T20:09:38Z

@tombuildsstuff - cheers for the review! Added requested changes.

Acceptance test results:

> $ make testacc TESTARGS='-run=TestAccAzureRMPolicySetDefinition'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestAccAzureRMPolicySetDefinition -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   TestAccAzureRMPolicySetDefinition_built_in_policy
=== PAUSE TestAccAzureRMPolicySetDefinition_built_in_policy
=== RUN   TestAccAzureRMPolicySetDefinition_custom_policy
=== PAUSE TestAccAzureRMPolicySetDefinition_custom_policy
=== CONT  TestAccAzureRMPolicySetDefinition_built_in_policy
=== CONT  TestAccAzureRMPolicySetDefinition_custom_policy
--- PASS: TestAccAzureRMPolicySetDefinition_built_in_policy (111.13s)
--- PASS: TestAccAzureRMPolicySetDefinition_custom_policy (202.65s)

tombuildsstuff

hey @olohmann

Thanks for pushing those changes - aside from a few minor comments (which I'll push a commit to fix) this otherwise LGTM 👍

Thanks!

tombuildsstuff · 2018-12-20T08:56:21Z

azurerm/resource_arm_policy_set_definition_test.go

+	"github.com/hashicorp/terraform/helper/resource"
+)
+
+func TestAccAzureRMPolicySetDefinition_built_in_policy(t *testing.T) {


Suggested change

func TestAccAzureRMPolicySetDefinition_built_in_policy(t *testing.T) {

func TestAccAzureRMPolicySetDefinition_builtIn(t *testing.T) {

tombuildsstuff · 2018-12-20T08:56:28Z

azurerm/resource_arm_policy_set_definition_test.go

+	})
+}
+
+func TestAccAzureRMPolicySetDefinition_custom_policy(t *testing.T) {


Suggested change

func TestAccAzureRMPolicySetDefinition_custom_policy(t *testing.T) {

func TestAccAzureRMPolicySetDefinition_custom(t *testing.T) {

tombuildsstuff · 2018-12-20T08:56:36Z

azurerm/resource_arm_policy_set_definition_test.go

+		CheckDestroy: testCheckAzureRMPolicySetDefinitionDestroy,
+		Steps: []resource.TestStep{
+			{
+				Config: testAzureRMPolicySetDefinition_built_in_policy(ri),


Suggested change

Config: testAzureRMPolicySetDefinition_built_in_policy(ri),

Config: testAzureRMPolicySetDefinition_builtIn(ri),

tombuildsstuff · 2018-12-20T08:56:47Z

azurerm/resource_arm_policy_set_definition_test.go

+		CheckDestroy: testCheckAzureRMPolicySetDefinitionDestroy,
+		Steps: []resource.TestStep{
+			{
+				Config: testAzureRMPolicySetDefinition_custom_policy(ri),


Suggested change

Config: testAzureRMPolicySetDefinition_custom_policy(ri),

Config: testAzureRMPolicySetDefinition_custom(ri),

tombuildsstuff · 2018-12-20T08:56:57Z

azurerm/resource_arm_policy_set_definition_test.go

+	})
+}
+
+func testAzureRMPolicySetDefinition_built_in_policy(ri int) string {


Suggested change

func testAzureRMPolicySetDefinition_built_in_policy(ri int) string {

func testAzureRMPolicySetDefinition_builtIn(ri int) string {

tombuildsstuff · 2018-12-20T08:57:03Z

azurerm/resource_arm_policy_set_definition_test.go

+`, ri, ri)
+}
+
+func testAzureRMPolicySetDefinition_custom_policy(ri int) string {


Suggested change

func testAzureRMPolicySetDefinition_custom_policy(ri int) string {

func testAzureRMPolicySetDefinition_custom(ri int) string {

tombuildsstuff · 2018-12-20T09:15:41Z

azurerm/resource_arm_policy_set_definition.go

+				ValidateFunc: validation.StringInSlice([]string{
+					string(policy.TypeBuiltIn),
+					string(policy.TypeCustom),
+					string(policy.TypeNotSpecified),


I think we should remove NotSpecified here

Suggested change

string(policy.TypeNotSpecified),

tombuildsstuff · 2018-12-20T09:24:58Z

azurerm/resource_arm_policy_set_definition.go

+	d.Set("name", resp.Name)
+
+	if props := resp.SetDefinitionProperties; props != nil {
+		d.Set("policy_type", props.PolicyType)


since this is returned as an Enum rather than a string, it's best to cast this to a string explicitly here:

Suggested change

d.Set("policy_type", props.PolicyType)

d.Set("policy_type", string(props.PolicyType))

tombuildsstuff · 2018-12-20T09:26:18Z

website/docs/r/policy_set_definition.html.markdown

+
+* `name` - (Required) The name of the policy set definition. Changing this forces a new resource to be created.
+
+* `policy_type` - (Required) The policy set type. The value can be `BuiltIn`, `Custom` or `NotSpecified`


We tend to phrase this as:

Suggested change

* `policy_type` - (Required) The policy set type. The value can be `BuiltIn`, `Custom` or `NotSpecified`

* `policy_type` - (Required) The policy set type. Possible values are `BuiltIn` or `Custom`. Changing this forces a new resource to be created.

(note I've removed NotSpecified since I don't believe we need to support it)

changes have been pushed

tombuildsstuff · 2018-12-20T09:48:24Z

Tests pass:

ghost · 2019-03-05T14:18:09Z

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 👉 [email protected]. Thanks!

olohmann added 2 commits December 18, 2018 14:00

Added new resource: azurerm_policy_set_definition

b03a304

Added documentation for azurerm_policy_set_definition.

1264f5a

ghost added size/XL documentation labels Dec 18, 2018

Fixed goimports.

e35cb85

katbyte previously requested changes Dec 18, 2018

View reviewed changes

katbyte added the waiting-response label Dec 18, 2018

katbyte and others added 11 commits December 18, 2018 18:40

Update azurerm/resource_arm_policy_set_definition.go

9565158

Co-Authored-By: olohmann <[email protected]>

Update azurerm/resource_arm_policy_set_definition.go

fe23ffb

Co-Authored-By: olohmann <[email protected]>

Update website/docs/r/policy_set_definition.html.markdown

a79d556

Co-Authored-By: olohmann <[email protected]>

Update website/docs/r/policy_set_definition.html.markdown

71599cc

Co-Authored-By: olohmann <[email protected]>

Update website/docs/r/policy_set_definition.html.markdown

7bed6de

Co-Authored-By: olohmann <[email protected]>

Update azurerm/resource_arm_policy_set_definition.go

4538cf2

Co-Authored-By: olohmann <[email protected]>

Update azurerm/resource_arm_policy_set_definition.go

a3a6066

Co-Authored-By: olohmann <[email protected]>

Update azurerm/resource_arm_policy_set_definition_test.go

2994601

Co-Authored-By: olohmann <[email protected]>

Added parameter validation.

14c7a11

Clean up formatting and import sample.

5e29dae

Fixed whitespace.

ef48757

ghost removed the waiting-response label Dec 18, 2018

tombuildsstuff added new-resource service/policy labels Dec 19, 2018

tombuildsstuff suggested changes Dec 19, 2018

View reviewed changes

tombuildsstuff added the waiting-response label Dec 19, 2018

tombuildsstuff and others added 6 commits December 19, 2018 19:23

Update website/docs/r/policy_set_definition.html.markdown

67bd3e9

Co-Authored-By: olohmann <[email protected]>

Update azurerm/resource_arm_policy_set_definition.go

5f25b44

Co-Authored-By: olohmann <[email protected]>

Update azurerm/resource_arm_policy_set_definition.go

a3f42a0

Co-Authored-By: olohmann <[email protected]>

Update azurerm/resource_arm_policy_set_definition.go

551a7a7

Co-Authored-By: olohmann <[email protected]>

Update azurerm/resource_arm_policy_set_definition.go

32da510

Co-Authored-By: olohmann <[email protected]>

Update azurerm/resource_arm_policy_set_definition.go

faabcf3

Co-Authored-By: olohmann <[email protected]>

tombuildsstuff and others added 7 commits December 19, 2018 19:27

Update azurerm/resource_arm_policy_set_definition.go

78372b8

Co-Authored-By: olohmann <[email protected]>

Update azurerm/resource_arm_policy_set_definition.go

73720f3

Co-Authored-By: olohmann <[email protected]>

Update website/docs/r/policy_set_definition.html.markdown

f277daf

Co-Authored-By: olohmann <[email protected]>

Update azurerm/resource_arm_policy_set_definition.go

c11d20d

Co-Authored-By: olohmann <[email protected]>

Update azurerm/resource_arm_policy_set_definition.go

e105206

Co-Authored-By: olohmann <[email protected]>

Additional acceptance test case for custom policies.

2d4cd01

Merge branch 'azurerm_policy_set_definition' of github.com:olohmann/t…

f20e2a1

…erraform-provider-azurerm into azurerm_policy_set_definition

ghost removed the waiting-response label Dec 19, 2018

tombuildsstuff approved these changes Dec 20, 2018

View reviewed changes

Fixing issues from code review

ea98e6d

tombuildsstuff merged commit 09831b6 into hashicorp:master Dec 20, 2018

tombuildsstuff added a commit that referenced this pull request Dec 20, 2018

Updating to include #2535

8bc3626

ghost locked and limited conversation to collaborators Mar 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Resource: azurerm_policy_set_definition #2535

New Resource: azurerm_policy_set_definition #2535

olohmann commented Dec 18, 2018

katbyte left a comment

olohmann commented Dec 18, 2018

tombuildsstuff left a comment

olohmann commented Dec 19, 2018

tombuildsstuff left a comment

tombuildsstuff Dec 20, 2018

tombuildsstuff Dec 20, 2018

tombuildsstuff Dec 20, 2018

tombuildsstuff Dec 20, 2018

tombuildsstuff Dec 20, 2018

tombuildsstuff Dec 20, 2018

tombuildsstuff Dec 20, 2018

tombuildsstuff Dec 20, 2018

tombuildsstuff Dec 20, 2018

tombuildsstuff commented Dec 20, 2018

ghost commented Mar 5, 2019

	func TestAccAzureRMPolicySetDefinition_built_in_policy(t *testing.T) {
	func TestAccAzureRMPolicySetDefinition_builtIn(t *testing.T) {

	func TestAccAzureRMPolicySetDefinition_custom_policy(t *testing.T) {
	func TestAccAzureRMPolicySetDefinition_custom(t *testing.T) {

	Config: testAzureRMPolicySetDefinition_built_in_policy(ri),
	Config: testAzureRMPolicySetDefinition_builtIn(ri),

	Config: testAzureRMPolicySetDefinition_custom_policy(ri),
	Config: testAzureRMPolicySetDefinition_custom(ri),

	func testAzureRMPolicySetDefinition_built_in_policy(ri int) string {
	func testAzureRMPolicySetDefinition_builtIn(ri int) string {

	func testAzureRMPolicySetDefinition_custom_policy(ri int) string {
	func testAzureRMPolicySetDefinition_custom(ri int) string {

	d.Set("policy_type", props.PolicyType)
	d.Set("policy_type", string(props.PolicyType))


		* `name` - (Required) The name of the policy set definition. Changing this forces a new resource to be created.

		* `policy_type` - (Required) The policy set type. The value can be `BuiltIn`, `Custom` or `NotSpecified`

New Resource: azurerm_policy_set_definition #2535

New Resource: azurerm_policy_set_definition #2535

Conversation

olohmann commented Dec 18, 2018

katbyte left a comment

Choose a reason for hiding this comment

olohmann commented Dec 18, 2018

tombuildsstuff left a comment

Choose a reason for hiding this comment

olohmann commented Dec 19, 2018

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tombuildsstuff commented Dec 20, 2018

ghost commented Mar 5, 2019