Virtual network gateway #31
Conversation
There was a problem hiding this comment.
Hi @thetonymaster,
Thank you for the PR, I have left some comments inline with my primary concern being evaluateSchemaValidateFunc. It seems to be the only function in that file required for the PR, as well as only used by this resource.
I think we should move it out into the resource file for now and in the future if something else uses it move it somewhere shared.
Otherwise my other comments are of the usual validation and cosmetic variety.
| "name": { | ||
| Type: schema.TypeString, | ||
| Required: true, | ||
| }, |
There was a problem hiding this comment.
Could we get a ValidateFunc: validation.NoZeroValues, here?
| Type: schema.TypeString, | ||
| Computed: true, | ||
| }, | ||
| "private_ip_address_allocation": { |
There was a problem hiding this comment.
Could we get spaces between the properties to better mach the other resources:
"name": {
Type: schema.TypeString,
Computed: true,
},
"private_ip_address_allocation": {
Type: schema.TypeString,
Computed: true,
},
``|
|
||
| d.Set("type", string(gw.GatewayType)) | ||
| d.Set("enable_bgp", gw.EnableBgp) | ||
| //d.Set("active_active", gw.ActiveActive) |
There was a problem hiding this comment.
If this is commented out because it is not supported in stack yet, a comment explaining would be great.
| } | ||
| flat["revoked_certificate"] = schema.NewSet(hashVirtualNetworkGatewayDataSourceRevokedCert, revokedCerts) | ||
|
|
||
| // vpnClientProtocols := &schema.Set{F: schema.HashString} |
There was a problem hiding this comment.
same as above, a comment explaining why this is commented out would be great.
|
|
||
| ```hcl | ||
| resource "azurestack_resource_group" "test" { | ||
| name = "test" |
There was a problem hiding this comment.
Could we align these assignments?
| }, | ||
| VpnClientRootCertificates: &rootCerts, | ||
| VpnClientRevokedCertificates: &revokedCerts, | ||
| // VpnClientProtocols: &vpnClientProtocols, |
| } | ||
| flat["revoked_certificate"] = schema.NewSet(hashVirtualNetworkGatewayRevokedCert, revokedCerts) | ||
|
|
||
| // vpnClientProtocols := &schema.Set{F: schema.HashString} |
azurestack/test_utils.go
Outdated
| armName := rs.Primary.Attributes["name"] | ||
| armResourceGroup, hasResourceGroup := rs.Primary.Attributes["resource_group_name"] | ||
| if !hasResourceGroup { | ||
| return "", "", fmt.Errorf("Bad: no resource group found in state for virtual network gateway: %s", name) |
There was a problem hiding this comment.
As this is a generic function, could we update the error message?
azurestack/test_utils.go
Outdated
| return "", "", fmt.Errorf("Not found: %s", name) | ||
| } | ||
|
|
||
| armName := rs.Primary.Attributes["name"] |
There was a problem hiding this comment.
It might be best to also check here for hasName? as well as that name is not empty
azurestack/validators.go
Outdated
| return | ||
| } | ||
|
|
||
| func evaluateSchemaValidateFunc(i interface{}, k string, validateFunc schema.SchemaValidateFunc) (bool, error) { |
There was a problem hiding this comment.
Checking AzureRm it also appears this is only used by the network gateway resource, so could we rename it and move it to that resource's file?
This appears to be the only function in this file actually required by the PR, so could we then remove it/the rest?
|
Backported comments to AzureRM here |
|
Also i recommend merging from master as validate & suppress are now vended in from AzureRm |
thetonymaster
force-pushed
the
virtual-network-gateway
branch
from
ae8438b to
ace1b69
Compare
|
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.