New Resource: `azurerm_sql_active_directory_administrator` #765

simongh · 2018-01-29T00:16:10Z

I've added the ability to set the AD administrator on SQL servers.

This is my first attempt at using GO and writing anything for Terraform, so I've probably made egregious mistakes.

It appeared to work when I tried it out on my Azure.

tombuildsstuff

Thanks for this PR - apologies for the delay in reviewing this!

I've taken a look through and left some comments inline, but this is off to a good start. If we can fix up the issues raised in the comments and some documentation - this otherwise looks good to kick off the tests. Regarding documentation, this consists of a link in the sidebar and a documentation page similar to this one.

Thanks!

tombuildsstuff · 2018-02-06T18:06:06Z

azurerm/resource_arm_sql_administrator.go

+			"tenant_id": {
+				Type:     schema.TypeString,
+				Required: true,
+			},


can we add some validation for this to ensure it's a valid UUID? we can use:

ValidateFunc: validateUUID,

Added suggested validator. This value can be updated.

tombuildsstuff · 2018-02-06T18:06:13Z

azurerm/resource_arm_sql_administrator.go

+
+			"object_id": {
+				Type:     schema.TypeString,
+				Required: true,


can we add some validation for this to ensure it's a valid UUID? we can use:

ValidateFunc: validateUUID,

also, can this value be updated? if not, this would be better as ForceNew: true

Added validator. This value can be updated

tombuildsstuff · 2018-02-06T18:06:26Z

azurerm/resource_arm_sql_administrator.go

+
+			"resource_group_name": resourceGroupNameSchema(),
+
+			"login": {


would this be better as username? also, can this value be updated? if not, this would be better as ForceNew: true

login matches up with the Azure API. They don't tend to use username very much, as this could also be a group name.

I'm not precious about it either way though

👍 that's fair - let's leave this as-is

tombuildsstuff · 2018-02-06T18:06:58Z

azurerm/resource_arm_sql_administrator.go

+	resp, error := client.Get(ctx, resGroup, serverName, administratorName)
+	if error != nil {
+		return error
+	}


minor can we change error -> err to match the convention here?

updated to match conventions

tombuildsstuff · 2018-02-19T20:46:11Z

azurerm/resource_arm_sql_administrator.go

+	uuid "github.com/satori/go.uuid"
+	"github.com/terraform-providers/terraform-provider-azurerm/azurerm/utils"
+	//"github.com/hashicorp/terraform/helper/validation"
+	//"github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/response"


minor can we remove these unused imports?

tombuildsstuff · 2018-02-19T21:05:43Z

azurerm/resource_arm_sql_administrator.go

+		Create: resourceArmSqlAdministratorCreateUpdate,
+		Read:   resourceArmSqlAdministratorRead,
+		Update: resourceArmSqlAdministratorCreateUpdate,
+		Delete: resourceArmSqlAdministratorDelete,


could we update the function name + these prefixes to resourceArmSqlActiveDirectoryAdministrator

renamed as requested

tombuildsstuff · 2018-02-19T21:08:51Z

azurerm/resource_arm_sql_administrator.go

+
+	serverName := d.Get("server_name").(string)
+	resGroup := d.Get("resource_group_name").(string)
+	administratorName := "activeDirectory"


note when #825 is resolved we shouldn't need this field

presuming I don't need to do anything here?

indeed - we'll take care of those changes once it's merged in (it's more a future TODO for us 😄)

tombuildsstuff · 2018-02-19T21:15:57Z

azurerm/resource_arm_sql_administrator_test.go

+	login = "sqladmin"
+    tenant_id = "${data.azurerm_client_config.current.tenant_id}"
+    object_id = "${data.azurerm_client_config.current.client_id}"
+}


minor could we make the spacing consistent?

tombuildsstuff · 2018-02-19T21:16:11Z

azurerm/resource_arm_sql_administrator_test.go

+	resource_group_name = "${azurerm_resource_group.test.name}"
+	login = "sqladmin2"
+    tenant_id = "${data.azurerm_client_config.current.tenant_id}"
+    object_id = "${data.azurerm_client_config.current.client_id}"


minor could we fix the spacing here?

tombuildsstuff · 2018-02-19T21:23:38Z

azurerm/resource_arm_sql_administrator.go

+		Delete: resourceArmSqlAdministratorDelete,
+		Importer: &schema.ResourceImporter{
+			State: schema.ImportStatePassthrough,
+		},


given we've supported import - can we add an import test for this resource? here's an example of how we do that https://github.com/terraform-providers/terraform-provider-azurerm/blob/master/azurerm/import_arm_mysql_server_test.go

I think I've implemented this. i'm getting a "filename too long" error, which makes it tricky to test this is working at all

no worries, I can kick off the tests and take a look. Out of interest (I'm assuming you're running on Windows) - do you have the unlimited filename length Windows feature enabled?

I hadn't & I have now, but it seems VS Code still complains.

simongh · 2018-02-22T00:00:04Z

I do this normally at work, but just checking here before I do it. If I rebase my branch with these changes onto master, it's not going to mess the pull request up is it?

tombuildsstuff · 2018-02-28T00:20:44Z

I do this normally at work, but just checking here before I do it. If I rebase my branch with these changes onto master, it's not going to mess the pull request up is it?

@simongh nope, it should be fine :)

tombuildsstuff

hey @simongh

Thanks for pushing those changes - I've taken a look through and this mostly LGTM. I've picked out a few minor issues which I'll push a commit to fix so that we can get this merged (I hope you don't mind) and then kick off the tests - but otherwise we should be good to merge this :)

Thanks!

tombuildsstuff · 2018-03-02T20:57:25Z

azurerm/import_arm_sql_administrator_test.go

+				ResourceName:            resourceName,
+				ImportState:             true,
+				ImportStateVerify:       true,
+				ImportStateVerifyIgnore: []string{},


minor we can actually remove this, since it's empty

tombuildsstuff · 2018-03-02T21:05:05Z

website/docs/r/sql_active_directory_administrator.markdown

+
+## Import
+
+A SQL Active Directory Administratir can be imported using the `resource id`, e.g.


minor Administratir -> Administrator

tombuildsstuff · 2018-03-02T21:05:40Z

website/docs/r/sql_active_directory_administrator.markdown

+
+* `server_name` - (Required) The name of the SQL Server on which to set the administrator
+
+* `resource_group_name` - (Required) The name of the resource group for the SQL server.


can we append Changing this forces a new resource to be created. to this?

tombuildsstuff · 2018-03-02T21:05:45Z

website/docs/r/sql_active_directory_administrator.markdown

+
+The following arguments are supported:
+
+* `server_name` - (Required) The name of the SQL Server on which to set the administrator


can we append Changing this forces a new resource to be created. to this?

tombuildsstuff · 2018-03-02T21:05:58Z

website/docs/r/sql_active_directory_administrator.markdown

+  Add a Active Directory administrator to a SQL server
+---
+
+# azurerm\_sql\_active\_directory\_administrator


minor we don't need the backslashes here

tombuildsstuff · 2018-03-02T22:04:27Z

Tests pass:

$ acctests azurerm TestAccAzureRMSqlAdministrator_
=== RUN   TestAccAzureRMSqlAdministrator_importBasic
--- PASS: TestAccAzureRMSqlAdministrator_importBasic (176.72s)
=== RUN   TestAccAzureRMSqlAdministrator_basic
--- PASS: TestAccAzureRMSqlAdministrator_basic (172.36s)
=== RUN   TestAccAzureRMSqlAdministrator_disappears
--- PASS: TestAccAzureRMSqlAdministrator_disappears (174.91s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	524.033s

simongh · 2018-03-03T11:30:28Z

That's all good. Happy to have contributed.

ghost · 2020-03-31T15:26:16Z

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!

tombuildsstuff added this to the 1.1.3 milestone Feb 16, 2018

tombuildsstuff added service/mssql Microsoft SQL Server new-resource labels Feb 19, 2018

tombuildsstuff suggested changes Feb 19, 2018

View reviewed changes

tombuildsstuff added the waiting-response label Feb 19, 2018

tombuildsstuff modified the milestones: 1.1.3, 1.1.4 Feb 28, 2018

simongh added 5 commits March 2, 2018 17:13

Added AD administrator schema

12f372d

Added sql AD administrators

bf8645b

Added tests

fd5b00a

implemented PR comments

230b15b

Added documentation

831238b

tombuildsstuff removed the waiting-response label Mar 2, 2018

tombuildsstuff approved these changes Mar 2, 2018

View reviewed changes

tombuildsstuff added 2 commits March 2, 2018 13:10

Minor tweaks from the PR review

4504706

Fixing the resource name

b3870ae

tombuildsstuff changed the title ~~Sql AD Administrator~~ New Resource: azurerm_sql_active_directory_administrator Mar 2, 2018

tombuildsstuff added 2 commits March 2, 2018 13:41

Updating the resource name

dc1a2da

Handling the resource group being deleted

a02d35a

tombuildsstuff merged commit f6429a1 into hashicorp:master Mar 2, 2018

tombuildsstuff added a commit that referenced this pull request Mar 2, 2018

Updating to include #765

d73f925

simongh deleted the sql-admin branch March 3, 2018 11:30

ghost locked and limited conversation to collaborators Mar 31, 2020


		## Import

		A SQL Active Directory Administratir can be imported using the `resource id`, e.g.


		* `server_name` - (Required) The name of the SQL Server on which to set the administrator

		* `resource_group_name` - (Required) The name of the resource group for the SQL server.


		The following arguments are supported:

		* `server_name` - (Required) The name of the SQL Server on which to set the administrator

New Resource: azurerm_sql_active_directory_administrator #765

New Resource: azurerm_sql_active_directory_administrator #765

Conversation

simongh commented Jan 29, 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

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 Feb 28, 2018 • edited Loading

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

simongh commented Feb 22, 2018

tombuildsstuff commented Feb 28, 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

tombuildsstuff commented Mar 2, 2018

simongh commented Mar 3, 2018

ghost commented Mar 31, 2020

New Resource: `azurerm_sql_active_directory_administrator` #765

New Resource: `azurerm_sql_active_directory_administrator` #765

tombuildsstuff Feb 28, 2018 •

edited

Loading