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

azurerm_policy_assignment - added support for enforcement_mode #7331

Merged
merged 3 commits into from
Jun 16, 2020

Conversation

ccampo
Copy link
Contributor

@ccampo ccampo commented Jun 15, 2020

I added the parameter enforcement_mode to allow policies that are assigned to a scope to be set to "DoNotEnforce". Default behaviour is unchanged and still "Default".

Added the code of the implementation, update the documentation html and added a testcase. My first pull request for this project, so please if anything is missing.

Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

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

Hey @ccampo, thanks for taking the time to write this out. This change is nearly there. We're just missing a few things that I've called out below and we want to set this attribute in the read so we can confirm that the attribute is being set properly. We can do that here with d.Set("enforcement_mode", props.EnforcementMode)

Copy link
Contributor

@ArcturusZhang ArcturusZhang left a comment

Choose a reason for hiding this comment

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

Hi @ccampo based on the doc page here, it sounds to me that this enforcement_mode is a switch which corresponds to a Disable and Enable (see the chart in the doc page). Therefore I suppose maybe we could make this attribute a bool with true corresponded to Default and false corresponded to DoNotEnforce?

@mbfrahry what do you think?

…arameter and added a Default and adressed the other comments (read property)
@ccampo
Copy link
Contributor Author

ccampo commented Jun 16, 2020

Hi @ccampo based on the doc page here, it sounds to me that this enforcement_mode is a switch which corresponds to a Disable and Enable (see the chart in the doc page). Therefore I suppose maybe we could make this attribute a bool with true corresponded to Default and false corresponded to DoNotEnforce?

@mbfrahry what do you think?

that sounds fair. I updated the pull request to make the parameter a boolean

Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

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

Hey @ccampo, nearly there! Just a couple more things

@@ -251,6 +258,7 @@ func resourceArmPolicyAssignmentRead(d *schema.ResourceData, meta interface{}) e
d.Set("policy_definition_id", props.PolicyDefinitionID)
d.Set("description", props.Description)
d.Set("display_name", props.DisplayName)
d.Set("enforcement_mode", props.EnforcementMode)
Copy link
Member

Choose a reason for hiding this comment

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

We'll also want to convert this into a boolean with something like:

Suggested change
d.Set("enforcement_mode", props.EnforcementMode)
d.Set("enforcement_mode", props.EnforcementMode == policy.Default)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed it. Still not sure what this code actually does, or how this fix changes it. The tfstate was already correct and the code was correctly comparing state against definition.

@mbfrahry
Copy link
Member

Hi @ccampo based on the doc page here, it sounds to me that this enforcement_mode is a switch which corresponds to a Disable and Enable (see the chart in the doc page). Therefore I suppose maybe we could make this attribute a bool with true corresponded to Default and false corresponded to DoNotEnforce?

@mbfrahry what do you think?

Exactly right! Good catch @ArcturusZhang

@mbfrahry mbfrahry changed the title added parameter enforcement_mode to resource azurerm_policy_assignment azurerm_policy_assignment - added support for enforcement_mode Jun 16, 2020
@mbfrahry mbfrahry added this to the v2.15.0 milestone Jun 16, 2020
@mbfrahry
Copy link
Member

Fixes #5699

Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

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

LGTM!

@mbfrahry
Copy link
Member

Thanks for taking the time to write this out @ccampo. It'll make it into the next release!

@mbfrahry mbfrahry merged commit f1b22dd into hashicorp:master Jun 16, 2020
mbfrahry added a commit that referenced this pull request Jun 16, 2020
@ghost
Copy link

ghost commented Jun 19, 2020

This has been released in version 2.15.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.15.0"
}
# ... other configuration ...

@ghost
Copy link

ghost commented Jul 17, 2020

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!

@ghost ghost locked and limited conversation to collaborators Jul 17, 2020
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.

3 participants