Route table #26
Route table #26
Conversation
If we have a route table associated with the subnet, the subnet can't be deleted
There was a problem hiding this comment.
Hi @thetonymaster,
Thank you for this PR. I've left some comments inline (mostly minor cosmetic/stylistic) with my main concern being the additional logic in resource_arm_subnet.go. Is this due to AzureStack behaving differently or a pre-existing bug in AzureRM and should be back ported? It would be great to get a acceptance test to check & test for this.
Also I see you've split the datasource out into a separate PR (#28). I would suggest including the datasource in this PR but its not a big deal. However I see that the datasource docs were included in this PR and they should be in whichever PR contains the datasource.
| return &schema.Resource{ | ||
| Create: resourceArmRouteTableCreate, | ||
| Read: resourceArmRouteTableRead, | ||
| Update: resourceArmRouteTableCreate, |
There was a problem hiding this comment.
Could we change this to resourceArmRouteTableCreateUpdate?
| } | ||
|
|
||
| err = future.WaitForCompletionRef(ctx, client.Client) | ||
| if err != nil { |
There was a problem hiding this comment.
very minor this could become:
if err = future.WaitForCompletionRef(ctx, client.Client); err != nil {
| } | ||
|
|
||
| err = future.WaitForCompletionRef(ctx, client.Client) | ||
| if err != nil { |
There was a problem hiding this comment.
Same as above: if err = future.WaitForCompletionRef(ctx, client.Client); err != nil {
| return nil | ||
| } | ||
|
|
||
| func expandRouteTableRoutes(d *schema.ResourceData) ([]network.Route, error) { |
There was a problem hiding this comment.
This function never returns an error, so error can be removed from the return values.
| addressPrefix := data["address_prefix"].(string) | ||
| nextHopType := data["next_hop_type"].(string) | ||
|
|
||
| properties := network.RoutePropertiesFormat{ |
There was a problem hiding this comment.
This can all be simplified to:
route := network.Route{
Name: utils.String(data["name"].(string)),
RoutePropertiesFormat: &network.RoutePropertiesFormat{
AddressPrefix: utils.String(data["address_prefix"].(string)),
NextHopType: network.RouteNextHopType(data["next_hop_type"].(string)),
},
}
if v := data["next_hop_in_ip_address"].(string); v != "" {
route.RoutePropertiesFormat.NextHopIPAddress = &v
}
| resource.TestCheckResourceAttr(resourceName, "route.1.next_hop_type", "VnetLocal"), | ||
| ), | ||
| }, | ||
| }, |
There was a problem hiding this comment.
There was an import test for this, so could we add a import test step here?
| func testAccAzureStackRouteTable_singleRoute(rInt int, location string) string { | ||
| return fmt.Sprintf(` | ||
| resource "azurestack_resource_group" "test" { | ||
| name = "acctestRG-%d" |
There was a problem hiding this comment.
Could we align the assignments here (and below) like the above tests?
azurestack/resource_arm_subnet.go
Outdated
| @@ -238,6 +237,35 @@ func resourceArmSubnetDelete(d *schema.ResourceData, meta interface{}) error { | |||
|
|
|||
| azureStackLockByName(routeTableName, routeTableResourceName) | |||
| defer azureStackUnlockByName(routeTableName, routeTableResourceName) | |||
|
|
|||
| // Get the subnet to dissasociate the route table, if we don't do this | |||
There was a problem hiding this comment.
This is different then the AzureRM code. Is this logic required for stack and not AzureRM? Could we get a test for this behaviour?
| page_title: "Azure Resource Manager: azurestack_route_table" | ||
| sidebar_current: "docs-azurestack-data-source-route-table" | ||
| description: |- | ||
| Manages information about a Route Table |
There was a problem hiding this comment.
As this is a data source this should be Gets
|
|
||
| # Data Source: azurestack_route_table | ||
|
|
||
| Manages information about a Route Table |
There was a problem hiding this comment.
As this is a data source this should be Gets
|
@katbyte I enabled the route table tests on the subnet test file, don't know if this enough since the problem comes up when the subnet has a route table. Anyway, please let me know. I also included the data source on this PR and closed the other one. |
There was a problem hiding this comment.
Hi @thetonymaster,
Thank you for the changes, it mostly LGTM aside from some minor style issues and the tabbing on the tests. Hope you don't mind but I have committed some fixes so we can get this merged today.
|
tests pass: |
|
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
locked and limited conversation to collaborators
No description provided.