Adds Resource azurerm_app_service_slot_virtual_network_swift_connection #5916

cimnine · 2020-02-26T18:05:06Z

Adds a new resource azurerm_app_service_virtual_network_swift_connection_slot which uses the corresponding Azure API and is named after it.

Fixes #5458

Sponsored by nxt Engineering.

katbyte

Hey @cimnine,

I've given this PR a quick review as its a draft (is it ready?) and left some comments inline that should be addressed. Additionally while running the tests i got:

"azurerm/internal/services/web/tests/resource_arm_app_service_slot_virtual_network_association_test.go:178:9: Sprintf format %d reads arg #6, but call has 5 args"

CHANGELOG.md

website/docs/r/app_service_virtual_network_swift_connection_slot.html.markdown

cimnine · 2020-02-27T07:58:20Z

Thank's for the early feedback.

I've given this PR a quick review as its a draft (is it ready?) ...

Indeed, I'm still validating the proposed change, hence the draft status of this PR.

... and left some comments inline that should be addressed. Additionally while running the tests i got: [error]

I have incorporated the feedback and fixed the test so far.

Except for the failing test – which was obviously a shortcoming on my side – would there have been any other way than the feedback to learn about these code conventions? I'm only asking because I did look out for a CONTRIBUTION.md or similar, but I have not succeeded in finding any relevant documentation.

cimnine · 2020-02-27T12:37:14Z

The acceptance tests for the modified resources ran successfully on my machine, also the regular test suite. The new resource azurerm_app_service_virtual_network_swift_connection_slot is modelled according to the existing resource azurerm_app_service_virtual_network_swift_connection.

An early idea was making the azurerm_app_service_virtual_network_swift_connection resource capable of doing both, slot and regular swift connections. But I think that the chosen solution – copying the existing resource code and adjusting the relevant lines – yields code that is easier to comprehend and therefore more maintainable in the long run.

cimnine · 2020-03-04T07:21:18Z

Just because I was not sure that this is clear: From my point of view this PR is ready to be reviewed and eventually be merged.

cimnine · 2020-03-12T13:15:22Z

Rebased onto latest master

katbyte

Hi @cimnine,

Thanks for the new resource, overall this looks good. I've left some comments inline that once addressed this will be good to merge!

azurerm/internal/services/web/registration.go

azurerm/internal/services/web/resource_arm_app_service_virtual_network_swift_connection_slot.go

...nternal/services/web/tests/resource_arm_app_service_slot_virtual_network_association_test.go

cimnine · 2020-04-07T19:51:48Z

I have implemented all but one requested change, for which I've made my case. The branch is also rebased onto the latest master.

cimnine · 2020-05-05T07:55:53Z

Rebased onto master.

cimnine · 2020-05-14T11:26:37Z

Rebased onto master and renames resource to azurerm_app_service_slot_virtual_network_swift_connection as suggested by @katbyte.

JSkimming · 2020-06-16T16:29:59Z

Just wondering where we are with this. Judging by the replies it's awaiting your review @katbyte

schlbra · 2020-06-16T21:29:47Z

Any updates on this @katbyte ?

This is done by the maintainers after the PR is merged.

The examples for the `virtual_network_swift_connect` and `virtual_network_swift_connect_slot` have been updated to be nicer to read. I.e. everything _test_ has been replaced by _example_.

Fixes the following error in the tests: "azurerm/internal/services/web/tests/resource_arm_app_service_slot_virtual_network_association_test.go:178:9: Sprintf format %d reads arg #6, but call has 5 args"

…nSlot ... to TestAccAzureRMAppServiceSlotVirtualNetworkSwiftConnection. The reason is that otherwise it's not possible to just run the test for `TestAccAzureRMAppServiceVirtualNetworkSwiftConnection`, as the `TestAccAzureRMAppServiceVirtualNetworkSwiftConnectionSlot` also matches that filter.

The value `slot.Name` contains "app_service_name/slot_name". If this were retained as `slot_name` in the Terraform state then terraform would try to re-create the resource in every run. Thankfully, this was caught during the integration tests.

cimnine · 2020-06-17T13:39:28Z

IMO it's ready to merge. I've rebased just now once more onto master in order to resolve the merge conflict.

azurerm/internal/services/web/parse/slot_virtual_network_swift_connection.go

jackofallops

Thanks @cimnine!

I've pushed some updates while I had opportunity to resolve some todo's on the resource you based your change on and reflected those in the new resource too. I'm on vacation today, so I'll run the tests on CI and merge when I get back Monday if one of the team doesn't get to it before then.

Thanks again

schlbra

Lgtm

jackofallops · 2020-07-06T07:42:05Z

Tests passing

changes superseded

cimnine · 2020-07-06T07:55:58Z

Thanks for merging 🥳

ghost · 2020-07-10T01:24:38Z

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

ghost · 2020-08-05T14:50:07Z

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/XL documentation labels Feb 26, 2020

katbyte requested changes Feb 26, 2020

View reviewed changes

katbyte added service/app-service waiting-response labels Feb 26, 2020

ghost removed the waiting-response label Feb 27, 2020

cimnine marked this pull request as ready for review February 27, 2020 12:30

katbyte previously requested changes Mar 25, 2020

View reviewed changes

katbyte added the waiting-response label Mar 25, 2020

ghost removed the waiting-response label Apr 7, 2020

cimnine requested a review from katbyte May 5, 2020 08:24

cimnine changed the title ~~Adds Resource azurerm_app_service_virtual_network_swift_connection_slot~~ Adds Resource azurerm_app_service_slot_virtual_network_swift_connection May 14, 2020

cimnine added 9 commits June 17, 2020 15:36

Adds Resource azurerm_app_service_virtual_network_swift_connection_slot

d3b4b58

Remove CHANGELOG entry

8c6bf1b

This is done by the maintainers after the PR is merged.

Nicer examples

f2a8d06

The examples for the `virtual_network_swift_connect` and `virtual_network_swift_connect_slot` have been updated to be nicer to read. I.e. everything _test_ has been replaced by _example_.

Fix sprintf number of args

4a176e8

Fixes the following error in the tests: "azurerm/internal/services/web/tests/resource_arm_app_service_slot_virtual_network_association_test.go:178:9: Sprintf format %d reads arg #6, but call has 5 args"

Add missing side navigation link

bd4140e

Retain correct slot name

08fe31d

The value `slot.Name` contains "app_service_name/slot_name". If this were retained as `slot_name` in the Terraform state then terraform would try to re-create the resource in every run. Thankfully, this was caught during the integration tests.

Fix acceptance test

5b0ffb1

Extract ID parsing into a function

e7938d5

Rename to azurerm_app_service_slot_virtual_network_swift_connection

1f31b3d

jackofallops added 3 commits July 2, 2020 15:33

fixup resources and tests

e63a157

added validation to slot name

e0afabe

tweaked slot example, updated arg order to alphabetical

4e9bec2

ghost added size/XXL and removed size/XL labels Jul 3, 2020

cimnine commented Jul 3, 2020

View reviewed changes

azurerm/internal/services/web/parse/slot_virtual_network_swift_connection.go Outdated Show resolved Hide resolved

jackofallops approved these changes Jul 3, 2020

View reviewed changes

Fix typo in SlotVirtualNetworkSwiftConnectionId type's name

79ab0db

schlbra approved these changes Jul 3, 2020

View reviewed changes

jackofallops added 3 commits July 6, 2020 07:46

typo in test name prefix

96ade3c

fix slot import test config

9c7acbe

fix slot import test data

62812dd

jackofallops merged commit b65a290 into hashicorp:master Jul 6, 2020

jackofallops deleted the azurerm_app_service_virtual_network_swift_connection_slot branch July 6, 2020 07:43

jackofallops added a commit that referenced this pull request Jul 6, 2020

updated for #5916

f56808b

jackofallops added this to the v2.18.0 milestone Jul 6, 2020

hashicorp locked and limited conversation to collaborators Aug 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds Resource azurerm_app_service_slot_virtual_network_swift_connection #5916

Adds Resource azurerm_app_service_slot_virtual_network_swift_connection #5916

cimnine commented Feb 26, 2020 •

edited

katbyte left a comment

cimnine commented Feb 27, 2020

cimnine commented Feb 27, 2020 •

edited

cimnine commented Mar 4, 2020

cimnine commented Mar 12, 2020

katbyte left a comment

cimnine commented Apr 7, 2020

cimnine commented May 5, 2020

cimnine commented May 14, 2020

JSkimming commented Jun 16, 2020

schlbra commented Jun 16, 2020

cimnine commented Jun 17, 2020 •

edited

jackofallops left a comment

schlbra left a comment

jackofallops commented Jul 6, 2020

cimnine commented Jul 6, 2020

ghost commented Jul 10, 2020

ghost commented Aug 5, 2020

Adds Resource azurerm_app_service_slot_virtual_network_swift_connection #5916

Adds Resource azurerm_app_service_slot_virtual_network_swift_connection #5916

Conversation

cimnine commented Feb 26, 2020 • edited

katbyte left a comment

Choose a reason for hiding this comment

cimnine commented Feb 27, 2020

cimnine commented Feb 27, 2020 • edited

cimnine commented Mar 4, 2020

cimnine commented Mar 12, 2020

katbyte left a comment

Choose a reason for hiding this comment

cimnine commented Apr 7, 2020

cimnine commented May 5, 2020

cimnine commented May 14, 2020

JSkimming commented Jun 16, 2020

schlbra commented Jun 16, 2020

cimnine commented Jun 17, 2020 • edited

jackofallops left a comment

Choose a reason for hiding this comment

schlbra left a comment

Choose a reason for hiding this comment

jackofallops commented Jul 6, 2020

cimnine commented Jul 6, 2020

ghost commented Jul 10, 2020

ghost commented Aug 5, 2020

cimnine commented Feb 26, 2020 •

edited

cimnine commented Feb 27, 2020 •

edited

cimnine commented Jun 17, 2020 •

edited