Skip to content

Don't match Status field when nil in ReadMany operation #609

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Don't match Status field when nil in ReadMany operation #609

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 54 additions & 22 deletions pkg/generate/code/set_resource.go
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

out of scope for this PR, but something i wish set_resource did is evaluate "continuing/breaking" conditions the first. e.g

if name != x {
  continue
}
if status.something != y {
  continue
}
// we matched let's set the fields
...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah having the matching field checks act as a guard clause would be more readable.

Original file line number Diff line number Diff line change
Expand Up @@ -709,34 +709,66 @@ func setResourceReadMany(
)
}
default:
// When in Spec we can assume the matching field is initially populated
//
// if ko.Spec.CacheClusterID != nil {
// if *elem.CacheClusterId != *ko.Spec.CacheClusterID {
// continue
// }
// }
//
// When in the Status the matching field won't be set until after initial creation.
// If the Status field isn't set we need to assume that the field doesn't match anything non-nil value.
// Otherwise, prior to creation the Status field being nil will result in any pre-existing resource matching
// resulting in erroneous errors related to the new resource already existing when it does not.
//
// if ko.Status.ServiceID == nil. || *elem.ServiceId != *ko.Status.ServiceID {
// continue
// }
//
if util.InStrings(fieldName, matchFieldNames) {
out += fmt.Sprintf(
"%s\tif %s.%s != nil {\n",
innerForIndent,
targetAdaptedVarName,
f.Names.Camel,
)
out += fmt.Sprintf(
"%s\t\tif *%s != *%s.%s {\n",
innerForIndent,
sourceAdaptedVarName,
targetAdaptedVarName,
f.Names.Camel,
)
out += fmt.Sprintf(
"%s\t\t\tcontinue\n", innerForIndent,
)
out += fmt.Sprintf(
"%s\t\t}\n", innerForIndent,
)
out += fmt.Sprintf(
"%s\t}\n", innerForIndent,
)
inSpec, inStat := r.HasMember(fieldName, op.ExportedName)

if inSpec {
out += fmt.Sprintf(
"%s\tif %s.%s != nil {\n",
innerForIndent,
targetAdaptedVarName,
f.Names.Camel,
)
out += fmt.Sprintf(
"%s\t\tif *%s != *%s.%s {\n",
innerForIndent,
sourceAdaptedVarName,
targetAdaptedVarName,
f.Names.Camel,
)
out += fmt.Sprintf(
"%s\t\t\tcontinue\n", innerForIndent,
)
out += fmt.Sprintf(
"%s\t\t}\n", innerForIndent,
)
out += fmt.Sprintf(
"%s\t}\n", innerForIndent,
)
} else if inStat {
out += fmt.Sprintf(
"%s\tif %s.%s == nil || *%s != *%s.%s {\n",
innerForIndent,
targetAdaptedVarName,
f.Names.Camel,
sourceAdaptedVarName,
targetAdaptedVarName,
f.Names.Camel,
)
out += fmt.Sprintf(
"%s\t\tcontinue\n", innerForIndent,
)
out += fmt.Sprintf(
"%s\t}\n", innerForIndent,
)
}
}
// r.ko.Spec.CacheClusterID = elem.CacheClusterId
out += setResourceForScalar(
Expand Down
165 changes: 164 additions & 1 deletion pkg/generate/code/set_resource_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
package code_test

import (
"fmt"
"testing"

"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -204,7 +205,6 @@ func TestSetResource_APIGWv2_Route_ReadOne(t *testing.T) {
)
}


func TestSetResource_SageMaker_Domain_ReadOne(t *testing.T) {
assert := assert.New(t)
require := require.New(t)
Expand Down Expand Up @@ -5022,3 +5022,166 @@ func TestSetResource_WAFv2_RuleGroup_ReadOne(t *testing.T) {
code.SetResource(crd.Config(), crd, op, "resp", "ko", 1),
)
}

func TestSetResource_EC2_VpcEndpointServiceConfiguration_ReadMany(t *testing.T) {
// DescribeInstances returns a list of Reservations
// containing a list of Instances. The first Instance
// in the first Reservation list should be used to populate CR.
assert := assert.New(t)
require := require.New(t)

g := testutil.NewModelForServiceWithOptions(t, "ec2", &testutil.TestingModelOptions{
GeneratorConfigFile: "generator-with-vpc-endpoint-service-read-many.yaml",
})
op := model.OpTypeList

crd := testutil.GetCRDByName(t, g, "VpcEndpointServiceConfiguration")
require.NotNil(crd)

expected := `
found := false
for _, elem := range resp.ServiceConfigurations {
if elem.AcceptanceRequired != nil {
ko.Spec.AcceptanceRequired = elem.AcceptanceRequired
} else {
ko.Spec.AcceptanceRequired = nil
}
if elem.AvailabilityZones != nil {
ko.Status.AvailabilityZones = aws.StringSlice(elem.AvailabilityZones)
} else {
ko.Status.AvailabilityZones = nil
}
if elem.BaseEndpointDnsNames != nil {
ko.Status.BaseEndpointDNSNames = aws.StringSlice(elem.BaseEndpointDnsNames)
} else {
ko.Status.BaseEndpointDNSNames = nil
}
if elem.GatewayLoadBalancerArns != nil {
ko.Spec.GatewayLoadBalancerARNs = aws.StringSlice(elem.GatewayLoadBalancerArns)
} else {
ko.Spec.GatewayLoadBalancerARNs = nil
}
if elem.ManagesVpcEndpoints != nil {
ko.Status.ManagesVPCEndpoints = elem.ManagesVpcEndpoints
} else {
ko.Status.ManagesVPCEndpoints = nil
}
if elem.NetworkLoadBalancerArns != nil {
ko.Spec.NetworkLoadBalancerARNs = aws.StringSlice(elem.NetworkLoadBalancerArns)
} else {
ko.Spec.NetworkLoadBalancerARNs = nil
}
if elem.PayerResponsibility != "" {
ko.Status.PayerResponsibility = aws.String(string(elem.PayerResponsibility))
} else {
ko.Status.PayerResponsibility = nil
}
if elem.PrivateDnsName != nil {
ko.Spec.PrivateDNSName = elem.PrivateDnsName
} else {
ko.Spec.PrivateDNSName = nil
}
if elem.PrivateDnsNameConfiguration != nil {
f8 := &svcapitypes.PrivateDNSNameConfiguration{}
if elem.PrivateDnsNameConfiguration.Name != nil {
f8.Name = elem.PrivateDnsNameConfiguration.Name
}
if elem.PrivateDnsNameConfiguration.State != "" {
f8.State = aws.String(string(elem.PrivateDnsNameConfiguration.State))
}
if elem.PrivateDnsNameConfiguration.Type != nil {
f8.Type = elem.PrivateDnsNameConfiguration.Type
}
if elem.PrivateDnsNameConfiguration.Value != nil {
f8.Value = elem.PrivateDnsNameConfiguration.Value
}
ko.Status.PrivateDNSNameConfiguration = f8
} else {
ko.Status.PrivateDNSNameConfiguration = nil
}
if elem.RemoteAccessEnabled != nil {
ko.Status.RemoteAccessEnabled = elem.RemoteAccessEnabled
} else {
ko.Status.RemoteAccessEnabled = nil
}
if elem.ServiceId != nil {
if ko.Status.ServiceID == nil || *elem.ServiceId != *ko.Status.ServiceID {
continue
}
ko.Status.ServiceID = elem.ServiceId
} else {
ko.Status.ServiceID = nil
}
if elem.ServiceName != nil {
ko.Status.ServiceName = elem.ServiceName
} else {
ko.Status.ServiceName = nil
}
if elem.ServiceState != "" {
ko.Status.ServiceState = aws.String(string(elem.ServiceState))
} else {
ko.Status.ServiceState = nil
}
if elem.ServiceType != nil {
f13 := []*svcapitypes.ServiceTypeDetail{}
for _, f13iter := range elem.ServiceType {
f13elem := &svcapitypes.ServiceTypeDetail{}
if f13iter.ServiceType != "" {
f13elem.ServiceType = aws.String(string(f13iter.ServiceType))
}
f13 = append(f13, f13elem)
}
ko.Status.ServiceType = f13
} else {
ko.Status.ServiceType = nil
}
if elem.SupportedIpAddressTypes != nil {
f14 := []*string{}
for _, f14iter := range elem.SupportedIpAddressTypes {
var f14elem *string
f14elem = aws.String(string(f14iter))
f14 = append(f14, f14elem)
}
ko.Spec.SupportedIPAddressTypes = f14
} else {
ko.Spec.SupportedIPAddressTypes = nil
}
if elem.SupportedRegions != nil {
f15 := []*string{}
for _, f15iter := range elem.SupportedRegions {
var f15elem *string
f15 = append(f15, f15elem)
}
ko.Spec.SupportedRegions = f15
} else {
ko.Spec.SupportedRegions = nil
}
if elem.Tags != nil {
f16 := []*svcapitypes.Tag{}
for _, f16iter := range elem.Tags {
f16elem := &svcapitypes.Tag{}
if f16iter.Key != nil {
f16elem.Key = f16iter.Key
}
if f16iter.Value != nil {
f16elem.Value = f16iter.Value
}
f16 = append(f16, f16elem)
}
ko.Spec.Tags = f16
} else {
ko.Spec.Tags = nil
}
found = true
break
}
if !found {
return nil, ackerr.NotFound
}
`

actual := code.SetResource(crd.Config(), crd, op, "resp", "ko", 1)
fmt.Println(actual)

assert.Equal(expected, actual)
}
126 changes: 126 additions & 0 deletions pkg/testdata/models/apis/ec2/0000-00-00/generator-with-vpc-endpoint-service-read-many.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
ignore:
field_paths:
- CreateDhcpOptionsInput.DryRun
- CreateVpcInput.DryRun
- CreateVpcEndpointInput.DryRun
- Instance.ClientToken
- InstanceNetworkInterfaceSpecification.Groups
- RunInstancesInput.AdditionalInfo
- RunInstancesInput.ClientToken
- RunInstancesInput.DryRun
- LaunchTemplate.Operator
- Instance.CpuOptions.AmdSevSnp
- Instance.CurrentInstanceBootMode
- Instance.Ipv6Address
- Instance.MaintenanceOptions
- Instance.MetadataOptions.InstanceMetadataTags
- Instance.NetworkPerformanceOptions
- Instance.Operator
- Instance.PrivateDnsNameOptions
- Instance.Placement.GroupId
- Instance.TpmSupport
- InstanceIpv6Address.IsPrimaryIpv6
resource_names:
- AccountAttribute
- CapacityReservation
- CarrierGateway
- ClientVpnEndpoint
- ClientVpnRoute
- CustomerGateway
- DefaultSubnet
- DefaultVpc
- DhcpOptions
- EgressOnlyInternetGateway
- Fleet
- FpgaImage
- Image
- Instance
- InstanceExportTask
- InternetGateway
- KeyPair
- LaunchTemplateVersion
- LaunchTemplate
- LocalGatewayRouteTableVpcAssociation
- LocalGatewayRoute
- ManagedPrefixList
- NatGateway
- NetworkAclEntry
- NetworkAcl
- NetworkInsightsPath
- NetworkInterfacePermission
- NetworkInterface
- PlacementGroup
- ReservedInstancesListing
- RouteTable
- Route
- SecurityGroup
- Snapshot
- SpotDatafeedSubscription
- Subnet
- TrafficMirrorFilterRule
- TrafficMirrorFilter
- TrafficMirrorSession
- TrafficMirrorTarget
- TransitGatewayConnectPeer
- TransitGatewayConnect
- TransitGatewayMulticastDomain
- TransitGatewayPeeringAttachment
- TransitGatewayPrefixListReference
- TransitGatewayRouteTable
- TransitGatewayRoute
- TransitGatewayVpcAttachment
- TransitGateway
- Volume
- VpcEndpointConnectionNotification
# - VpcEndpointServiceConfiguration
- VpcEndpoint
- Vpc
- VpcCidrBlock
- VpcPeeringConnection
- VpnConnectionRoute
- VpnConnection
- VpnGateway

operations:
CreateVpcEndpointServiceConfiguration:
output_wrapper_field_path: ServiceConfiguration
DeleteVpcEndpointServiceConfigurations:
operation_type:
- Delete
resource_name: VpcEndpointServiceConfiguration
CreateVpcEndpointServiceConfiguration:
output_wrapper_field_path: ServiceConfiguration
DeleteVpcEndpointServiceConfigurations:
operation_type:
- Delete
resource_name: VpcEndpointServiceConfiguration
resources:
VpcEndpointServiceConfiguration:
fields:
AllowedPrincipals:
from:
operation: ModifyVpcEndpointServicePermissions
path: AddAllowedPrincipals
ServiceID:
is_primary_key: true
is_read_only: true
print:
path: Status.serviceID
name: ServiceID
ServiceState:
is_read_only: true
print:
path: Status.serviceState
name: ServiceState
Tags:
from:
operation: CreateTags
path: Tags
synced:
when:
- path: Status.ServiceState
in:
- available
list_operation:
match_fields:
- ServiceId