New Resource `azurerm_maintenance_configuration` #6038

njuCZ · 2020-03-09T09:40:38Z

partially address #6037
Once it is merged, I'll go on implementing azurerm_maintenance_assignment

After tested, I found the resource_group_name from the response of read api is always small case, so I use azure.SchemaResourceGroupNameDiffSuppress()

Another point is the key of tags from the response of read api is always small case

katbyte

Thanks for the new resource @njuCZ,

Aside from a couple minor comments iv'e left inline this is off to a great start!

azurerm/internal/services/maintenance/data_source_maintenance_configuration.go

azurerm/internal/services/maintenance/resource_arm_maintenance_configuration.go

azurerm/internal/services/maintenance/tests/resource_arm_maintenance_configuration_test.go

katbyte · 2020-03-10T05:14:20Z

azurerm/internal/services/maintenance/tests/resource_arm_maintenance_configuration_test.go

+  scope               = "Host"
+
+  tags = {
+    env = "test"


could we ensure uppercase works?

Suggested change

env = "test"

EnV = "TesT"

I have changed the value to TesT
as for the key, this service will return small case key, regardless of the input case.
If we set EnV = "TesT", the service will return env = "TesT"

njuCZ · 2020-03-10T07:44:14Z

@katbyte Thanks for the suggestion, I have refined the codes accordingly. Please have a look again

jackofallops

Hi @njuCZ
Thanks for this PR for the preview resource.
The code looks largely OK, but there are aspects/issues with the API that are going to make this problematic for Terraform, in particular the non-standard behaviour around Tag keys. I think we should investigate getting that behaviour corrected before trying to compensate in the provider, or progressing the PR towards merging.

jackofallops · 2020-04-01T14:34:54Z

azurerm/internal/services/maintenance/resource_arm_maintenance_configuration.go

+	if _, err := client.CreateOrUpdate(ctx, resGroup, name, configuration); err != nil {
+		return fmt.Errorf("Error creating/updating MaintenanceConfiguration %q (Resource Group %q): %+v", name, resGroup, err)
+	}
+
+	resp, err := client.Get(ctx, resGroup, name)
+	if err != nil {
+		return fmt.Errorf("Error retrieving MaintenanceConfiguration %q (Resource Group %q): %+v", name, resGroup, err)
+	}


I have concerns around this as the ID in the return from CreateOrUpdate and Get are the same values, but the latter is returned entirely in lower case. This feels like a bug in the API and I'm concerned that when (if?) it is fixed, it will break for us here (since the parsing of the ID elements is case sensitive). Are you aware of any issue raised against this?

jackofallops · 2020-04-07T09:17:15Z

azurerm/internal/services/maintenance/resource_arm_maintenance_configuration.go

+			},
+
+			// can not contain upper case key
+			"tags": tags.Schema(),


I believe this behaviour to be a problem. Whilst Azure does not treat the Keys as case sensitive, normal expected behaviour is to return the casing provided by the user. Terraform also expects the keys to be returned as originally defined. Can we open an issue on this?

jackofallops · 2020-04-07T09:52:40Z

website/docs/r/maintenance_configuration.html.markdown

+
+* `scope` - (Optional) The scope of the Maintenance Configuration. Possible values are `All`, `Host`, `Resource` or `InResource`. Default to `All`.
+
+* `tags` - (Optional) A mapping of tags to assign to the resource.


If the tag key bug cannot be addressed, this needs to be highlighted here. Users specifying Tags that are not all lower will experience problems using this resource.

njuCZ · 2020-04-13T02:16:05Z

@jackofallops I have added some support for case insensitive resource ID, and added some notes in the doc, do you think it's ok? The service team says it's by design to make the case insensitive

katbyte

@njuCZ,

given the API has an issue could we open a ticket on the SDK & link to it in the code as well as add validation to prevent any tags from being added with upper case chars in the key?

katbyte · 2020-04-15T03:23:38Z

azurerm/internal/services/maintenance/resource_arm_maintenance_configuration.go

+			},
+
+			// can not contain upper case key
+			"tags": tags.Schema(),


njuCZ · 2020-04-15T06:27:59Z

@katbyte I have added custom validation for the tags and opened an issue, Could you please have a look again?

katbyte

Aside from one comment LGTM @njuCZ! I'm gonna approve this and merge to get it in but i'd be great if you could open an issue on the SDK for the comment i've left inline (not a big deal to not link in code but that would be ideal). Thanks!

katbyte · 2020-04-21T03:20:53Z

azurerm/internal/services/maintenance/parse/maintenance_configuration.go

+	if name, err := id.PopSegment("maintenanceconfigurations"); err != nil {
+		if name, err = id.PopSegment("maintenanceConfigurations"); err != nil {


We should open an SKD issue here and link to it

thank you. I have added such bug about returned resource id:
Azure/azure-rest-api-specs#8653
I have noted in the resource_group_name schema defnition

ghost · 2020-04-23T23:30:25Z

This has been released in version 2.7.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.7.0"
}
# ... other configuration ...

ghost · 2020-05-21T14:50:43Z

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 added size/XXL dependencies documentation labels Mar 9, 2020

njuCZ changed the title ~~new Resource azurerm_maintenance_configuration~~ New Resource azurerm_maintenance_configuration Mar 9, 2020

katbyte requested changes Mar 10, 2020

View reviewed changes

katbyte added this to the v2.2.0 milestone Mar 10, 2020

katbyte added new-resource service/maintenance waiting-response labels Mar 10, 2020

ghost removed the waiting-response label Mar 10, 2020

njuCZ requested a review from katbyte March 10, 2020 07:44

katbyte modified the milestones: v2.2.0, v2.3.0 Mar 18, 2020

tombuildsstuff modified the milestones: v2.3.0, v2.5.0 Mar 25, 2020

njuCZ force-pushed the maintenance_config branch 4 times, most recently from 334e017 to 2c917ab Compare March 30, 2020 02:32

jackofallops self-assigned this Apr 1, 2020

jackofallops requested changes Apr 7, 2020

View reviewed changes

jackofallops modified the milestones: v2.5.0, v2.7.0 Apr 7, 2020

njuCZ added 2 commits April 8, 2020 11:07

new resource azurerm_maintenance_configuration

d262c83

rebase to new style and add support for case insensitive

56f4749

njuCZ force-pushed the maintenance_config branch from 2c917ab to 56f4749 Compare April 10, 2020 10:51

katbyte requested changes Apr 15, 2020

View reviewed changes

katbyte added the waiting-response label Apr 15, 2020

add tags validation that should not contain upper case letter

6685848

ghost removed the waiting-response label Apr 15, 2020

fix code style

8bfa8ac

njuCZ requested review from katbyte and jackofallops April 16, 2020 01:55

katbyte approved these changes Apr 21, 2020

View reviewed changes

katbyte merged commit 95efca9 into hashicorp:master Apr 21, 2020

katbyte added a commit that referenced this pull request Apr 21, 2020

update readme.MD to include #6038

352858d

ghost locked and limited conversation to collaborators May 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Resource `azurerm_maintenance_configuration` #6038

New Resource `azurerm_maintenance_configuration` #6038

njuCZ commented Mar 9, 2020 •

edited

Loading

katbyte left a comment

katbyte Mar 10, 2020

njuCZ Mar 10, 2020

njuCZ commented Mar 10, 2020

jackofallops left a comment

jackofallops Apr 1, 2020

jackofallops Apr 7, 2020

katbyte Apr 15, 2020

jackofallops Apr 7, 2020

njuCZ commented Apr 13, 2020

katbyte left a comment

katbyte Apr 15, 2020

njuCZ commented Apr 15, 2020

katbyte left a comment

katbyte Apr 21, 2020

njuCZ Apr 21, 2020

ghost commented Apr 23, 2020

ghost commented May 21, 2020


		* `scope` - (Optional) The scope of the Maintenance Configuration. Possible values are `All`, `Host`, `Resource` or `InResource`. Default to `All`.

		* `tags` - (Optional) A mapping of tags to assign to the resource.

		if name, err := id.PopSegment("maintenanceconfigurations"); err != nil {
		if name, err = id.PopSegment("maintenanceConfigurations"); err != nil {

New Resource azurerm_maintenance_configuration #6038

New Resource azurerm_maintenance_configuration #6038

Conversation

njuCZ commented Mar 9, 2020 • edited Loading

katbyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njuCZ commented Mar 10, 2020

jackofallops 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

njuCZ commented Apr 13, 2020

katbyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njuCZ commented Apr 15, 2020

katbyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Apr 23, 2020

ghost commented May 21, 2020

New Resource `azurerm_maintenance_configuration` #6038

New Resource `azurerm_maintenance_configuration` #6038

njuCZ commented Mar 9, 2020 •

edited

Loading