Add delegation support for Subnet #2042

metacpp · 2018-10-09T06:22:43Z

In order to support vNet for ACI, subnet needs to support delegations.

This PR also involves upgrade of Network package to 2018-08-01 version.

Add delegation for subnet to support ACI VNet.

Fix the type issue for `service_delegation`.

Add delegation for subnet to integrate with ACI.

tombuildsstuff

Thanks for this PR - I've taken a look through and left some comments inline but this is off to a good start. Generally we take a two-step approach to upgrading an existing SDK where a dependency needs it - first upgrading to the new version in one PR and then adding the new functionality in another (since this makes it simpler to review) - but I don't think we need to split that out in this case.

Thanks!

azurerm/resource_arm_subnet.go

azurerm/resource_arm_subnet_test.go

website/docs/r/subnet.html.markdown

azurerm/resource_arm_subnet.go

vendor/vendor.json

tombuildsstuff · 2018-10-25T19:17:46Z

hi @metacpp

After chatting with @VaijanathB yesterday it sounds like there's an issue in the underlying API here during deletion; whilst he's tracking that down I'm going to mark this as blocked and we'll re-evaluate once the API's been fixed.

Thanks!

metacpp · 2018-10-29T03:44:37Z

hi @metacpp

After chatting with @VaijanathB yesterday it sounds like there's an issue in the underlying API here during deletion; whilst he's tracking that down I'm going to mark this as blocked and we'll re-evaluate once the API's been fixed.

Thanks!

Do you mean the upgrade network API to 2018-08-01? I manually tested through terraform destroy, did not see any error.

Tests pass ``` $ acctests azurerm TestAccAzureRMSubnet_removeNetworkSecurityGroup === RUN TestAccAzureRMSubnet_removeNetworkSecurityGroup --- PASS: TestAccAzureRMSubnet_removeNetworkSecurityGroup (140.01s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 141.927s ```

tombuildsstuff · 2018-11-07T12:36:45Z

@metacpp I've pushed a commit to fix the broken TestAccAzureRMSubnet_removeNetworkSecurityGroup test - and the tests now LGTM:

Co-Authored-By: metacpp <1684739+metacpp@users.noreply.github.com>

metacpp · 2019-01-04T01:43:02Z

@tombuildsstuff the PR is updated and all tests passed.

tombuildsstuff

hey @metacpp

Thanks for pushing those changes - if we can fix up the crash points this otherwise LGTM 👍

Thanks!

azurerm/resource_arm_subnet.go

Co-Authored-By: metacpp <1684739+metacpp@users.noreply.github.com>

metacpp · 2019-01-05T03:09:31Z

@tombuildsstuff the PR is updated with nil check and I reran the test successfully:

katbyte

@metacpp,

Thanks for the updates, i've left a couple comment inline that should be addressed before merge

azurerm/resource_arm_subnet.go

website/docs/r/subnet.html.markdown

azurerm/resource_arm_subnet_test.go

metacpp · 2019-01-07T22:33:42Z

@katbyte the PR is updated with your suggestions.

katbyte · 2019-01-08T04:48:05Z

Thanks for the changes @metacpp, LGTM now 🙂

ghost · 2019-03-05T14:12:03Z

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!

metacpp added 3 commits October 7, 2018 21:53

✨ (Subnet.) Add delegations for subnet.

913460a

Add delegation for subnet to support ACI VNet.

🐛 (Delegation of Subnet.) Resolve flatten method issue.

8f9b6ef

Fix the type issue for `service_delegation`.

♻️ (Delegation of Subnet.) Add delegation for subnet.

2150745

Add delegation for subnet to integrate with ACI.

ghost added the size/XXL label Oct 9, 2018

metacpp requested review from tombuildsstuff, WodansSon and JunyiYi October 9, 2018 06:22

metacpp added enhancement service/subnets labels Oct 9, 2018

metacpp added this to the Soon milestone Oct 9, 2018

tombuildsstuff suggested changes Oct 11, 2018

View reviewed changes

tombuildsstuff added the waiting-response label Oct 11, 2018

This comment has been minimized.

Sign in to view

tombuildsstuff modified the milestones: Soon, Being Sorted Oct 25, 2018

tombuildsstuff added the upstream/microsoft Indicates that there's an upstream issue blocking this issue/PR label Oct 25, 2018

tombuildsstuff modified the milestones: Being Sorted, Blocked Oct 25, 2018

Merge changes from master branch.

0f2d267

metacpp added 3 commits October 28, 2018 20:46

Resolve issues in PR and network package upgrade.

cf7d75b

Fix: linting errors in test files.

1ed0eff

Undo unnecessary changes in network related tests.

09178ac

tombuildsstuff removed upstream/microsoft Indicates that there's an upstream issue blocking this issue/PR waiting-response labels Nov 7, 2018

tombuildsstuff removed this from the Blocked milestone Nov 7, 2018

tombuildsstuff removed the request for review from JunyiYi November 7, 2018 12:26

tombuildsstuff and others added 9 commits January 3, 2019 11:51

Update website/docs/r/subnet.html.markdown

caefa61

Co-Authored-By: metacpp <1684739+metacpp@users.noreply.github.com>

Update website/docs/r/subnet.html.markdown

5c720cf

Co-Authored-By: metacpp <1684739+metacpp@users.noreply.github.com>

Update azurerm/resource_arm_subnet_test.go

95bb542

Co-Authored-By: metacpp <1684739+metacpp@users.noreply.github.com>

Update azurerm/resource_arm_subnet.go

4a72e06

Co-Authored-By: metacpp <1684739+metacpp@users.noreply.github.com>

Update azurerm/resource_arm_subnet.go

047a946

Co-Authored-By: metacpp <1684739+metacpp@users.noreply.github.com>

Update azurerm/resource_arm_subnet.go

d15de3e

Co-Authored-By: metacpp <1684739+metacpp@users.noreply.github.com>

update the sample code in documentation.

e299a69

Add flatten function for delegation and resolve issues in PR.

1e80242

Fix the goimport issue.

cd06da2

tombuildsstuff removed the waiting-response label Jan 4, 2019

tombuildsstuff unassigned metacpp Jan 4, 2019

tombuildsstuff suggested changes Jan 4, 2019

View reviewed changes

azurerm/resource_arm_subnet.go Outdated Show resolved Hide resolved

azurerm/resource_arm_subnet.go Outdated Show resolved Hide resolved

azurerm/resource_arm_subnet.go Outdated Show resolved Hide resolved

azurerm/resource_arm_subnet.go Outdated Show resolved Hide resolved

tombuildsstuff assigned metacpp Jan 4, 2019

tombuildsstuff added the waiting-response label Jan 4, 2019

tombuildsstuff and others added 2 commits January 4, 2019 18:19

Update azurerm/resource_arm_subnet.go

981a6b8

Co-Authored-By: metacpp <1684739+metacpp@users.noreply.github.com>

Add nil check for flatten function

3eccbeb

katbyte requested changes Jan 6, 2019

View reviewed changes

azurerm/resource_arm_subnet.go Outdated Show resolved Hide resolved

website/docs/r/subnet.html.markdown Outdated Show resolved Hide resolved

azurerm/resource_arm_subnet_test.go Outdated Show resolved Hide resolved

Fix: alignment issue and nil check consistency.

70c4108

katbyte approved these changes Jan 8, 2019

View reviewed changes

katbyte merged commit 357fe46 into master Jan 8, 2019

katbyte deleted the subnet_delegation branch January 8, 2019 04:48

katbyte added a commit that referenced this pull request Jan 8, 2019

Update CHANGELOG.md to include #2042

cf63c6e

metacpp mentioned this pull request Jan 25, 2019

support for azurerm_container_group with private ip #2314

Closed

ghost locked and limited conversation to collaborators Mar 5, 2019

ghost removed the waiting-response label Mar 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add delegation support for Subnet #2042

Add delegation support for Subnet #2042

metacpp commented Oct 9, 2018 •

edited

Loading

tombuildsstuff left a comment

This comment has been minimized.

tombuildsstuff commented Oct 25, 2018

metacpp commented Oct 29, 2018

tombuildsstuff commented Nov 7, 2018 •

edited

Loading

metacpp commented Jan 4, 2019 •

edited

Loading

tombuildsstuff left a comment

metacpp commented Jan 5, 2019 •

edited

Loading

katbyte left a comment •

edited

Loading

metacpp commented Jan 7, 2019

katbyte commented Jan 8, 2019

ghost commented Mar 5, 2019

Add delegation support for Subnet #2042

Add delegation support for Subnet #2042

Conversation

metacpp commented Oct 9, 2018 • edited Loading

tombuildsstuff left a comment

Choose a reason for hiding this comment

This comment has been minimized.

tombuildsstuff commented Oct 25, 2018

metacpp commented Oct 29, 2018

tombuildsstuff commented Nov 7, 2018 • edited Loading

metacpp commented Jan 4, 2019 • edited Loading

tombuildsstuff left a comment

Choose a reason for hiding this comment

metacpp commented Jan 5, 2019 • edited Loading

katbyte left a comment • edited Loading

Choose a reason for hiding this comment

metacpp commented Jan 7, 2019

katbyte commented Jan 8, 2019

ghost commented Mar 5, 2019

metacpp commented Oct 9, 2018 •

edited

Loading

tombuildsstuff commented Nov 7, 2018 •

edited

Loading

metacpp commented Jan 4, 2019 •

edited

Loading

metacpp commented Jan 5, 2019 •

edited

Loading

katbyte left a comment •

edited

Loading