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

add built-in policies for both subscriptions and management-groups #2788

Merged

Conversation

lawrenae
Copy link
Contributor

@lawrenae lawrenae commented Jan 28, 2019

this is meant as a fix for #2757

Feedback most welcome, of course

@ghost ghost added the size/M label Jan 28, 2019
@katbyte
Copy link
Collaborator

katbyte commented Jan 29, 2019

@lawrenae, should we expose more of the properties as the builtin role definition datasource does?

@lawrenae
Copy link
Contributor Author

@katbyte sure. I don't know how useful those extra properties would be to folks, but it should be easy to add them. Will see what I can do!

@ghost ghost added size/L documentation and removed size/M labels Jan 29, 2019
@lawrenae
Copy link
Contributor Author

@katbyte I added

  • display_name - (Required) Specifies the name of the built-in Policy Definition.
  • id - the ID of the built-in Policy Definition.
  • description - the Description of the built-in Policy.
  • type - the Type of Policy.
  • name - The Name of the Policy Definition

I see a few things in the portal that I couldnt find in the API -- effect and category that might also be useful.

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.

@lawrenae,

Thanks for the updates. So after some internal discussion we have combined azurerm_builtin_role_definition and azurerm_role_definition together in #2798 and I think we should follow the same pattern here and support both the built-in policies and custom ones in this data source.

As such we should rename this to azurerm_policy_definition and ensure that with a test we can retrieve custom policies. WDYT? I don't think it will require any code changes unlike role definitions.

azurerm/data_source_builtin_policy_definition.go Outdated Show resolved Hide resolved
azurerm/data_source_builtin_policy_definition.go Outdated Show resolved Hide resolved
website/docs/d/builtin_policy_definition.markdown Outdated Show resolved Hide resolved
azurerm/data_source_builtin_policy_definition.go Outdated Show resolved Hide resolved
@lawrenae
Copy link
Contributor Author

@katbyte I like the idea of combining them. I will get to working on that, hopefully very soon

@ghost ghost removed the waiting-response label Jan 30, 2019
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 @lawrenae,

Wasn't sure if this was ready for a review, but gave it a quick look over anyways 😅

Left a few comments inline and the remaining questions are if we should support looking up a policy by ID and support the rest of the possible properties:

  • policy_type
  • policy_rule
  • metadata
  • parameters

azurerm/data_source_policy_definition.go Show resolved Hide resolved
azurerm/data_source_policy_definition.go Show resolved Hide resolved
azurerm/data_source_policy_definition.go Outdated Show resolved Hide resolved
azurerm/data_source_policy_definition.go Outdated Show resolved Hide resolved
azurerm/data_source_policy_definition.go Show resolved Hide resolved
azurerm/data_source_policy_definition.go Outdated Show resolved Hide resolved
azurerm/data_source_policy_definition_test.go Show resolved Hide resolved
@ghost ghost added size/XL and removed size/L labels Jan 31, 2019
@lawrenae
Copy link
Contributor Author

@katbyte I appreciate the additional feedback, and think I've got this ready for review (again).

@ghost ghost removed the waiting-response label Jan 31, 2019
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.

thanks @lawrenae, aside from a few minor nits i've left inline and the build not passing (looks like a simple) this is looking pretty GTM 🙂

azurerm/data_source_policy_definition.go Show resolved Hide resolved
azurerm/provider.go Outdated Show resolved Hide resolved
website/docs/d/policy_definition.markdown Outdated Show resolved Hide resolved
website/docs/d/policy_definition.markdown Outdated Show resolved Hide resolved
website/docs/d/policy_definition.markdown Outdated Show resolved Hide resolved
website/docs/d/policy_definition.markdown Outdated Show resolved Hide resolved
website/docs/d/policy_definition.markdown Outdated Show resolved Hide resolved
website/docs/d/policy_definition.markdown Outdated Show resolved Hide resolved
website/docs/d/policy_definition.markdown Outdated Show resolved Hide resolved
website/docs/d/policy_definition.markdown Outdated Show resolved Hide resolved
@lawrenae
Copy link
Contributor Author

lawrenae commented Feb 4, 2019

@katbyte -- good catches on the docs. updated!

@ghost ghost removed the waiting-response label Feb 4, 2019
@katbyte
Copy link
Collaborator

katbyte commented Feb 5, 2019

Thanks @lawrenae, LGTM now 🙂

@katbyte katbyte added this to the 1.22.0 milestone Feb 5, 2019
@katbyte katbyte merged commit 815e3eb into hashicorp:master Feb 5, 2019
katbyte added a commit that referenced this pull request Feb 5, 2019
@ghost
Copy link

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

2 participants