Data Source: azurerm_route_table: Added support for: disable_bgp_route_propagation #21940
Conversation
| @@ -25,6 +25,11 @@ func dataSourceRouteTable() *pluginsdk.Resource { | |||
| }, | |||
|
|
|||
| Schema: map[string]*pluginsdk.Schema{ | |||
| "disable_bgp_route_propagation": { | |||
There was a problem hiding this comment.
We have a convention for the names of boolean properties so can we rename this to
| "disable_bgp_route_propagation": { | |
| "bgp_route_propagation_enabled": { |
and also move this property down in the schema, since it's computed it should be defined after the required arguments.
There was a problem hiding this comment.
I wanted to reflect this property to be the same as it is in resource: https://github.com/hashicorp/terraform-provider-azurerm/blob/main/internal/services/network/route_table_resource.go
Here we have "disable_bgp_route_propagation". I don't think we should change it to "bgp_route_propagation_enabled" - let's keep things consistent. Please let me know is that ok with you, thanks.
There was a problem hiding this comment.
Apologies for taking a while to circle back to this.
It looks like the property in the resource was added quite a while ago, before the naming standard for booleans across the provider was introduced and began being enforced. These should be phased out and renamed - could you please add a comment above the property in the resource to the effect of
//TODO rename to bgp_route_propagation_enabled in 4.0
And rename this property to bgp_route_propagation_enabled.
There was a problem hiding this comment.
I've applied fixes based on your feedback, thanks.
| @@ -106,6 +111,10 @@ func dataSourceRouteTableRead(d *pluginsdk.ResourceData, meta interface{}) error | |||
| if err := d.Set("subnets", flattenRouteTableDataSourceSubnets(props.Subnets)); err != nil { | |||
| return err | |||
| } | |||
|
|
|||
| if err := d.Set("disable_bgp_route_propagation", props.DisableBgpRoutePropagation); err != nil { | |||
There was a problem hiding this comment.
| if err := d.Set("disable_bgp_route_propagation", props.DisableBgpRoutePropagation); err != nil { | |
| if err := d.Set("bgp_route_propagation_enabled", !props.DisableBgpRoutePropagation); err != nil { |
There was a problem hiding this comment.
Thanks, let me check if I can apply this.
stephybun
force-pushed
the
dis_bgp_route_prop_in_data_rt
branch
from
5a2707f to
e115d68
Compare
stephybun
force-pushed
the
dis_bgp_route_prop_in_data_rt
branch
from
9d50541 to
ebd6436
Compare
stephybun
force-pushed
the
dis_bgp_route_prop_in_data_rt
branch
from
ebd6436 to
04fb809
Compare
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
The purpose of this PR is to add support for missing 'disable_bgp_route_propagation' field in the data source 'azurerm_route_table'
Details:
I'm parsing TF statefile using my code to extract resources that are already deployed to Azure in my environment.
I've noticed a situation where 'disable_bgp_route_propagation' is missing in terraform statefile when particular RouteTable is defined as data source even though this field is supported in regular tf resource.
I investigated this and it turned out that this field is not supported for data source route_table so I've added this into provider code.
I also noticed that we're missing Unittests for Data Source RouteTable (TestAccDataSourceRouteTable_basic) where other fields are not tested at all but documentation claims that when extending DataSource - unittest should be developed, so I've decided to add missing unittest to have whole Resource fields tested.
After adding support for a missing field - under "attributes" in statefile we can see 'disable_bgp_route_propagation' with boolean value.
That's my first time contributing to this project so please feel free to leave any comments on this if needed.
Tests: