-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
azurerm_network_interface
- upgrade API from 2022-07-01
to 2023-02-01
#22479
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🔬
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @ms-zhenhua, this is mostly good but tags
isn't quite right and I've detailed how to fix it going forward for future sdk swaps
config.ApplicationGatewayBackendAddressPools = &applicationGatewayBackendAddressPools | ||
config.LoadBalancerBackendAddressPools = &loadBalancerBackendAddressPools | ||
config.LoadBalancerInboundNatRules = &loadBalancerInboundNatRules | ||
if *config.Properties.PrivateIPAddressVersion != networkinterfaces.IPVersionIPvFour { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line can be cosolidated with this line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code updated.
"github.com/hashicorp/terraform-provider-azurerm/helpers/azure" | ||
"github.com/hashicorp/terraform-provider-azurerm/internal/clients" | ||
"github.com/hashicorp/terraform-provider-azurerm/internal/services/network/parse" | ||
"github.com/hashicorp/terraform-provider-azurerm/internal/tags" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This import wants to be swapped to
"github.com/hashicorp/terraform-provider-azurerm/internal/tags" | |
"github.com/hashicorp/go-azure-helpers/resourcemanager/tags" |
And the schema will be
"tags": commonschema.TagsDataSource(),
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code updated.
@@ -264,5 +272,9 @@ func dataSourceNetworkInterfaceRead(d *pluginsdk.ResourceData, meta interface{}) | |||
d.Set("enable_accelerated_networking", props.EnableAcceleratedNetworking) | |||
} | |||
|
|||
return tags.FlattenAndSet(d, resp.Tags) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing that will let us keep this line the same without doing what you've done below
} | ||
|
||
if d.HasChange("tags") { | ||
tagsRaw := d.Get("tags").(map[string]interface{}) | ||
update.Tags = tags.Expand(tagsRaw) | ||
update.Tags = utils.ExpandPtrMapStringString(tagsRaw) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apply that same tag
logic from the datasource here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code updated.
Hi @mbfrahry, thank you for your review. I have updated the code. Kindly take another review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @ms-zhenhua
Thanks for pushing those changes - I've taken a look and left one comment inline, since this needs a rebase and we're going to want to move the Network
package to using a Meta Client sooner rather than later, I hope you don't mind but I'm going to push a commit to fix this comment/rebase this so that we can get this merged.
Thanks!
…, rather than via a pointer
489c27a
to
7138dd6
Compare
dismissing since changes have been pushed
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. |
Description
Upgrade API version to support newly added properties: (new properties will be in another PR)
auxiliaryMode
auxiliarySku
Test Result
--- PASS: TestAccNetworkInterface_disappears (309.71s)
--- PASS: TestAccNetworkInterface_ipv6 (388.27s)
--- PASS: TestAccNetworkInterface_multipleIPConfigurations (440.64s)
--- PASS: TestAccNetworkInterface_internalDomainNameLabel (480.79s)
--- PASS: TestAccNetworkInterface_pointToGatewayLB (534.90s)
--- PASS: TestAccNetworkInterface_multipleIPConfigurationsSecondaryAsPrimary (542.93s)
--- PASS: TestAccNetworkInterface_enableAcceleratedNetworking (546.67s)
--- PASS: TestAccNetworkInterface_enableIPForwarding (551.44s)
--- PASS: TestAccNetworkInterface_basic (570.84s)
--- PASS: TestAccNetworkInterface_dnsServers (582.01s)
--- PASS: TestAccNetworkInterface_tags (521.22s)
--- PASS: TestAccNetworkInterface_updateMultipleParameters (456.69s)
--- PASS: TestAccNetworkInterface_requiresImport (355.55s)
--- PASS: TestAccNetworkInterface_static (343.84s)
--- PASS: TestAccNetworkInterface_update (463.97s)
--- PASS: TestAccNetworkInterface_publicIP (391.04s)
--- PASS: TestAccDataSourceArmNetworkInterface_basic (227.24s)
--- PASS: TestAccNetworkInterfaceApplicationSecurityGroupAssociation_deleted (264.22s)
--- PASS: TestAccNetworkInterfaceApplicationSecurityGroupAssociation_multipleIPConfigurations (311.62s)
--- PASS: TestAccNetworkInterfaceApplicationSecurityGroupAssociation_requiresImport (338.72s)
--- PASS: TestAccNetworkInterfaceApplicationSecurityGroupAssociation_basic (369.77s)
--- PASS: TestAccNetworkInterfaceApplicationSecurityGroupAssociation_updateNIC (382.26s)