New Resource: `azurerm_app_configuration` #4859

brennerm · 2019-11-12T22:42:43Z

This PR adds the resource azurerm_app_configuration that allows creating, deleting and updating Azure App Configuration instances.

Related issue: #3514

brennerm · 2019-11-12T22:44:49Z

Tests and documentation are missing as I would like to agree on the implementation first.

brennerm · 2019-11-12T22:47:32Z

Has been validated with this Terraform config:

resource "azurerm_resource_group" "test" {
  name = "test"
  location = "West Europe"
}

resource "azurerm_app_configuration" "test" {
  name = "test"
  location = azurerm_resource_group.test.location
  resource_group_name = azurerm_resource_group.test.name
}

output "test" {
  value = azurerm_app_configuration.test
}

mbfrahry

Hey @brennerm. This is a good start and I'm glad you asked for a design check before fleshing the rest of it out. I think moving the keys from TypeMap to TypeList is a must and then we need to add quite a few nil checks

azurerm/resource_arm_app_configuration.go

brennerm · 2019-11-16T12:43:42Z

@mbfrahry I have another branch adding a new data source to get a value from an App Configuration. Are you OK with me adding these changes to this PR as both use the same App Configuration client code? Otherwise I'd create a separate PR.

mbfrahry · 2019-11-18T16:01:51Z

Hey @brennerm, adding it here should be fine

brennerm · 2019-11-18T18:45:52Z

Done

mbfrahry

Hey @brennerm, this is a good start but we're missing a test and doc for this. Also, do you updating with master to fix config.go

azurerm/data_source_app_configuration_value.go

- Terraform provider linters only allow string literals for ResourceData.Set() - switched to nested if-statements instead of string interpolation

Co-Authored-By: Matthew Frahry <[email protected]>

brennerm · 2019-11-20T16:53:01Z

@mbfrahry Apparently Azure allows upgrading from the free to the standard SKU but downgrading is not supported. The API responds with the following response:
StatusCode=400 -- Original Error: Code="SkuDowngradeNotAllowed" Message="ErrorSkuDowngradeNotAllowed"

How do you normally handle this? Make changing the SKU create a new resource by default or do we allow an upgrade in-place and tell the user that downgrading is not supported?

mbfrahry · 2019-11-20T17:58:51Z

Ohhh, that's a tricky one. We have a way to customize the diff of a resource depending on certain conditions but I don't think we should use that here.

The API leaves a clear enough message on what is going on when trying to downgrade a sku that a user could act on and deal with themselves. Force newing on our end might add more confusion since we wouldn't be able to leave as helpful of a message as the api about why we need to force new when downgrading.

I think leaving what to do in the hand of the user is fine here.

katbyte · 2019-11-28T20:39:12Z

@brennerm,

the tests are failing with:

------- Stdout: -------
=== RUN   TestAccAzureAppConfiguration_free
=== PAUSE TestAccAzureAppConfiguration_free
=== CONT  TestAccAzureAppConfiguration_free
--- FAIL: TestAccAzureAppConfiguration_free (135.92s)
    testing.go:569: Step 1 error: ImportStateVerify attributes not equivalent. Difference is shown below. Top is actual, bottom is expected.
        
        (map[string]string) {
        }
        
        
        (map[string]string) (len=1) {
         (string) (len=3) "sku": (string) (len=4) "free"
        }
        
FAIL

------- Stderr: -------
2019/11/28 19:40:38 [DEBUG] Registering Data Sources for "Compute"..
2019/11/28 19:40:38 [DEBUG] Registering Resources for "Compute"..

looks like you need to set the SKU on read

brennerm · 2019-11-28T20:53:10Z

Should be fixed. 👍

brennerm · 2019-11-28T21:35:39Z

The schema version could be set to 0 as this is the initial version of this resource, right?

…rennerm/terraform-provider-azurerm into brennerm-add-app-configuration-resource

mbfrahry

Hey @brennerm. I took another pass through and the tests are passing but the nil check aren't quite right. I left a comment but let me know if that doesn't make sense

mbfrahry · 2019-12-02T20:40:03Z

azurerm/resource_arm_app_configuration.go

+
+			"primary_read_key": {
+				Type:     schema.TypeList,
+				MaxItems: 1,


This can be removed as MaxItems is a validation check for what a user submits rather than a check that the api only returns 1 of this. This goes for MaxItems on the blocks down below as well

Just out of curiosity, I got this from resource_arm_kubernetes_cluster.go. Is there some reason why it's necessary there?

Not that I'm aware! It might just be a mistake. That validation is only checked against what a user submits not what the api returns.

mbfrahry · 2019-12-02T20:48:28Z

azurerm/resource_arm_app_configuration.go

+
+	values := resultPage.Values()
+	for _, value := range values {
+		if &value == nil {


Unfortunately, this isn't the right check for nil. value is actually fine in this case as it's just an APIKey but the individual attributes in an APIKey are pointers *. Those need to be nil checked before pointing to them

…rennerm/terraform-provider-azurerm into brennerm-add-app-configuration-resource

mbfrahry · 2019-12-09T22:29:42Z

Hey @brennerm. That nil check still isn't quite right. We want to still set whatever variables we have access too. I went ahead and wrote what it should look like up and merged with master since there were some tricky conflicts but for some reason I can't commit my changes to your branch. I opened a PR on your end with master changes and my three commits on top. Apologies for all the changes coming in! brennerm#1

brennerm · 2019-12-09T22:39:23Z

@mbfrahry Understand, thought we are going with the all or nothing approach and ignore incomplete access keys which probably shouldn't happen at all.

…guration-resource2

mbfrahry

LGTM!

mbfrahry · 2019-12-10T22:05:37Z

Thanks for your patience here @brennerm. This should go out the door in the next release

ghost · 2019-12-16T12:50:35Z

This has been released in version 1.39.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 = "~> 1.39.0"
}
# ... other configuration ...

devblackops · 2019-12-16T19:05:56Z

website/docs/r/app_configuration.html.markdown

+
+---
+
+# azurerm_container_registry


Copy/paste error here? This should be # azurerm_app_configuration

Correct, fixed with #5186.

ghost · 2020-01-10T14:50:25Z

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 added the size/XL label Nov 12, 2019

brennerm mentioned this pull request Nov 12, 2019

App Configuration resources #3514

Closed

ghost added size/XXL dependencies and removed size/XL labels Nov 12, 2019

brennerm marked this pull request as ready for review November 13, 2019 07:29

mbfrahry requested changes Nov 13, 2019

View reviewed changes

mbfrahry added the new-resource label Nov 13, 2019

brennerm force-pushed the add-app-configuration-resource branch from 702b264 to b76d319 Compare November 13, 2019 21:14

brennerm requested a review from mbfrahry November 15, 2019 20:27

mbfrahry requested changes Nov 18, 2019

View reviewed changes

azurerm/data_source_app_configuration_value.go Outdated Show resolved Hide resolved

ghost added the documentation label Nov 18, 2019

brennerm and others added 10 commits November 20, 2019 00:07

add resource azurerm_app_configuration

e1ada85

add appconfiguration dependency

d3fd5b9

fix error handling in deletion method

053b7e2

adjust fetching of access keys

7d1d6c4

- Terraform provider linters only allow string literals for ResourceData.Set() - switched to nested if-statements instead of string interpolation

remove unused variable

605e96e

fix log statement

02194f6

use create instead of create/update timeout

2a9a681

Co-Authored-By: Matthew Frahry <[email protected]>

align future handling in deletion method

5c7d877

add missing nil check

5807463

make access key of schema type list

eea91fb

brennerm force-pushed the add-app-configuration-resource branch from 9b7db45 to 015480a Compare November 19, 2019 23:13

ghost removed the waiting-response label Nov 28, 2019

katbyte added this to the v1.38.0 milestone Nov 28, 2019

extract sku name in read method

60eb5c4

brennerm force-pushed the add-app-configuration-resource branch from be712d5 to 60eb5c4 Compare November 28, 2019 20:52

Merge branch 'add-app-configuration-resource' of https://github.com/b…

3393355

…rennerm/terraform-provider-azurerm into brennerm-add-app-configuration-resource

mbfrahry requested changes Dec 2, 2019

View reviewed changes

tombuildsstuff modified the milestones: v1.38.0, v1.39.0 Dec 3, 2019

brennerm added 3 commits December 5, 2019 19:27

remove MaxItems from access key fields

ea7d0fb

fix APIKey nil check

5a12c39

remove SchemaVersion

41fe134

brennerm requested a review from mbfrahry December 5, 2019 19:17

mbfrahry added 3 commits December 9, 2019 12:38

Merge branch 'add-app-configuration-resource' of https://github.com/b…

222a017

…rennerm/terraform-provider-azurerm into brennerm-add-app-configuration-resource

Merge with master

bf7fb13

Updating with master and fixing nil check

f26fcb7

Merge pull request #1 from terraform-providers/brennerm-add-app-confi…

2fecf7a

…guration-resource2

mbfrahry approved these changes Dec 10, 2019

View reviewed changes

mbfrahry changed the title ~~add resource azurerm_app_configuration~~ New Resource: azurerm_app_configuration Dec 10, 2019

mbfrahry merged commit 4887819 into hashicorp:master Dec 10, 2019

mbfrahry added a commit that referenced this pull request Dec 10, 2019

Update changelog for #4859

cbb2b05

devblackops reviewed Dec 16, 2019

View reviewed changes

ghost locked and limited conversation to collaborators Jan 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Resource: `azurerm_app_configuration` #4859

New Resource: `azurerm_app_configuration` #4859

brennerm commented Nov 12, 2019

brennerm commented Nov 12, 2019

brennerm commented Nov 12, 2019

mbfrahry left a comment

brennerm commented Nov 16, 2019

mbfrahry commented Nov 18, 2019

brennerm commented Nov 18, 2019

mbfrahry left a comment

brennerm commented Nov 20, 2019

mbfrahry commented Nov 20, 2019

katbyte commented Nov 28, 2019

brennerm commented Nov 28, 2019

brennerm commented Nov 28, 2019

mbfrahry left a comment

mbfrahry Dec 2, 2019

brennerm Dec 5, 2019

mbfrahry Dec 10, 2019

mbfrahry Dec 2, 2019

mbfrahry commented Dec 9, 2019

brennerm commented Dec 9, 2019

mbfrahry left a comment

mbfrahry commented Dec 10, 2019

ghost commented Dec 16, 2019

devblackops Dec 16, 2019 •

edited

Loading

brennerm Dec 16, 2019

ghost commented Jan 10, 2020

New Resource: azurerm_app_configuration #4859

New Resource: azurerm_app_configuration #4859

Conversation

brennerm commented Nov 12, 2019

brennerm commented Nov 12, 2019

brennerm commented Nov 12, 2019

mbfrahry left a comment

Choose a reason for hiding this comment

brennerm commented Nov 16, 2019

mbfrahry commented Nov 18, 2019

brennerm commented Nov 18, 2019

mbfrahry left a comment

Choose a reason for hiding this comment

brennerm commented Nov 20, 2019

mbfrahry commented Nov 20, 2019

katbyte commented Nov 28, 2019

brennerm commented Nov 28, 2019

brennerm commented Nov 28, 2019

mbfrahry left a comment

Choose a reason for hiding this comment

mbfrahry Dec 2, 2019

Choose a reason for hiding this comment

brennerm Dec 5, 2019

Choose a reason for hiding this comment

mbfrahry Dec 10, 2019

Choose a reason for hiding this comment

mbfrahry Dec 2, 2019

Choose a reason for hiding this comment

mbfrahry commented Dec 9, 2019

brennerm commented Dec 9, 2019

mbfrahry left a comment

Choose a reason for hiding this comment

mbfrahry commented Dec 10, 2019

ghost commented Dec 16, 2019

devblackops Dec 16, 2019 • edited Loading

Choose a reason for hiding this comment

brennerm Dec 16, 2019

Choose a reason for hiding this comment

ghost commented Jan 10, 2020

New Resource: `azurerm_app_configuration` #4859

New Resource: `azurerm_app_configuration` #4859

devblackops Dec 16, 2019 •

edited

Loading