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

New resource & data source 'azurerm_azuread_group' #1839

Closed
wants to merge 7 commits into from

Conversation

tiwood
Copy link
Contributor

@tiwood tiwood commented Aug 29, 2018

This replaces PR #1585.

This PR adds a new resource and data source for Azure Active Directory groups.

  • New data source 'azurerm_azuread_group'
  • New resource 'azurerm_azuread_group'
  • Add markdown documentation
  • Add tests

Example Usage

// This creates a new Azure AD group with the display name "my_group"
resource "azurerm_azuread_group" "my_group" {
 name = "my_group"
}

// This queries an Azure AD group by ID
data "azurerm_azuread_group" "my_group_by_id" {
  object_id = "78722cfc-8946-11e8-95f1-2200ec79ad01"
}

// This queries an Azure AD group by Name
data "azurerm_azuread_group" "my_group_by_name" {
  name = "my_group"
}

The following attributes are exported:

  • id - The Object ID for the Azure AD Group.
  • object_id - The Object ID for the Azure AD Group.
  • name - The Display Name for the Azure AD Group.

@ghost ghost added the size/L label Aug 29, 2018
@tiwood
Copy link
Contributor Author

tiwood commented Sep 5, 2018

Related: #1272

@ghost ghost added the size/L label Sep 7, 2018
@tiwood
Copy link
Contributor Author

tiwood commented Sep 11, 2018

@jeffreyCline, @tombuildsstuff, any plans to get this merged into 1.15?

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.

Hello @tiwood,

Thank you for this contribution, I have reviewed it and left some comments inline. The main blockers are:

  • I don't believe object_id is required for this resource, .id serves the same purpose
  • the 3 other properties used when creating the group should be exposed instead of silently defaulted: (MailEnabled, MailNickname & SecurityEnabled)
  • Import check steps should be added to the resource tests
  • The requirement for either object ID or name on the datasource should be enforced by a CustomizeDiff function. An example of the function can be seen here

azurerm/data_source_azuread_group.go Show resolved Hide resolved
azurerm/data_source_azuread_group.go Outdated Show resolved Hide resolved
azurerm/resource_arm_azuread_group.go Show resolved Hide resolved

properties := graphrbac.GroupCreateParameters{
DisplayName: &name,
MailEnabled: utils.Bool(false),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we expose these three properties in the schema instead of silently defaulting them?

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 could do that, but at the moment the Graph API only supports the creation of security enabled groups (MailEnabled: false, SecurityEnabled: true). See here.

So what is the protocol here, adding these to the schema as computed and default to them for now, or keep it as is?

azurerm/resource_arm_azuread_group.go Outdated Show resolved Hide resolved
azurerm/resource_arm_azuread_group_test.go Show resolved Hide resolved
azurerm/resource_arm_azuread_group_test.go Show resolved Hide resolved
azurerm/data_source_azuread_group.go Show resolved Hide resolved
azurerm/provider.go Show resolved Hide resolved
azurerm/provider.go Outdated Show resolved Hide resolved
@X-Guardian
Copy link

Hi @tiwood, are you able to make the requested changes to this PR? I would love to be able to use this data source instead of having to hard-code AD Group Ids in my Azure Terraform configurations.

@tiwood
Copy link
Contributor Author

tiwood commented Oct 15, 2018

@X-Guardian, I'm going to make the required changes in the next few days.

@X-Guardian
Copy link

Thanks @tiwood , that's excellent news!

@ghost ghost added size/XL and removed size/L labels Oct 17, 2018
@tiwood
Copy link
Contributor Author

tiwood commented Oct 21, 2018

Hi @katbyte, I think this is ready for another review, please let me know if further changes are required.

@tombuildsstuff tombuildsstuff added this to the 1.20.0 milestone Oct 25, 2018
@katbyte katbyte removed this from the 1.20.0 milestone Oct 25, 2018
@katbyte katbyte removed their assignment Oct 25, 2018
@Leon99
Copy link

Leon99 commented Nov 6, 2018

Huge thanks to @tiwood for contributing this. @katbyte any ideas if this can be reviewed/merged any time soon?

@tiwood
Copy link
Contributor Author

tiwood commented Nov 8, 2018

maybe @tombuildsstuff can help reviewing this since @katbyte removed her assignment?

@tombuildsstuff tombuildsstuff removed the request for review from WodansSon November 8, 2018 12:20
@tombuildsstuff
Copy link
Contributor

tombuildsstuff commented Nov 15, 2018

hey @tiwood @Leon99 @X-Guardian

Thanks for this PR - apologies for the delayed review of this!

Just to give some context behind the delayed review of this: as a part of the upcoming v2.0 release of the AzureRM Provider we're thinking through some of the longer-term changes that we'd like to make to the Provider.

At this point we're trying to determine where the Azure Active Directory resources belong, since there's now enough edge-cases/feature enhancements that this may make sense in it's own Provider; which would allow us to more easily ship support for the AzureAD resources which are harder to test.

The proposal to split the AzureAD resources out into its own provider is being tracked in #2322 - would you be able to take a look and let us know your thoughts?

Thanks!

@tombuildsstuff tombuildsstuff added this to the 2.0.0 milestone Nov 15, 2018
@tombuildsstuff tombuildsstuff modified the milestones: 2.0.0, Blocked Nov 22, 2018
@tombuildsstuff
Copy link
Contributor

👋

As mentioned in #2322 - we're planning on splitting the AzureAD resources out into their own provider. Since this PR is waiting on that change (which is planned in the not-too-distant future) rather than leaving this open in a holding pattern I'm going to temporarily close this PR for the moment - however we should be able to migrate this PR over once the split-out's done (I hope you don't mind @tiwood).

Thanks!

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

5 participants