Skip to content
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

network: fix acc tests failures of versino beta 4 #26678

Merged
merged 3 commits into from
Jul 23, 2024

Conversation

wuxu92
Copy link
Contributor

@wuxu92 wuxu92 commented Jul 18, 2024

Community Note

  • Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
  • Please do not leave comments along the lines of "+1", "me too" or "any updates", they generate extra noise for PR followers and do not help prioritize for review

Description

Testing

  • My submission includes Test coverage as described in the Contribution Guide and the tests pass. (if this is not possible for any reason, please include details of why you did or could not add test coverage)

Over 260 tests failed when the 4.0 beta flag was enabled in the main branch, compared to only 25-30 failures in 3.0 mode.

image

there are less than 40 failures in this branch:

66 failures the prev commit:
image

I fixed some issues and reran part of the 66 failed cases; 27 of them passed. Now, fewer than 40 cases remain unresolved need to be checked manually.

image

Change Log

Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.

This is a (please select all that apply):

  • Bug Fix
  • New Feature (ie adding a service, resource, or data source)
  • Enhancement
  • Breaking Change

Related Issue(s)

Note

If this PR changes meaningfully during the course of review please update the title and description as required.

@wuxu92 wuxu92 force-pushed the acc4beta/network branch from 083c187 to c74fea2 Compare July 22, 2024 07:48
@wuxu92 wuxu92 marked this pull request as ready for review July 23, 2024 01:55
Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks for fixing these up @wuxu92. I just have one question about why we're changing the setting of a property from true to false, otherwise this looks good.

@@ -254,7 +254,7 @@ resource "azurerm_subnet" "test" {
virtual_network_name = azurerm_virtual_network.test.name
address_prefixes = ["10.5.4.0/24"]

enforce_private_link_service_network_policies = true
private_link_service_network_policies_enabled = false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many of these are getting set to false when they were true before, could you explain why?

Copy link
Contributor Author

@wuxu92 wuxu92 Jul 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because the original property enforce_private_link_service_network_policies has unusual expansion logic, which is the opposite of the new property private_link_service_network_policies_enabled and in version 4.0. The same applies to enforce_private_link_endpoint_network_policies and private_endpoint_network_policies_enabled. Therefore, the tool includes special logic to change the value from true to false.

// TODO 4.0: Remove expandEnforceSubnetPrivateLinkNetworkPolicy function
func expandEnforceSubnetNetworkPolicy(enabled bool) string {
// This is strange logic, but to get the schema to make sense for the end user
// I exposed it with the same name that the Azure CLI does to be consistent
// between the tool sets, which means true == Disabled.
if enabled {
return string(subnets.VirtualNetworkPrivateEndpointNetworkPoliciesDisabled)
}
return string(subnets.VirtualNetworkPrivateEndpointNetworkPoliciesEnabled)
}

Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @wuxu92 LGTM 💯

@stephybun stephybun merged commit b85a31b into hashicorp:main Jul 23, 2024
33 checks passed
@github-actions github-actions bot added this to the v3.114.0 milestone Jul 23, 2024
Copy link

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants