-
Notifications
You must be signed in to change notification settings - Fork 268
same-named fields in Input and Output shapes with different Go types #178
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
Labels
kind/bug
Categorizes issue or PR as related to a bug.
Milestone
Comments
Another data point is CreateDhcpOptions.
The difference is DhcpConfiguration.Value is of type |
ack-bot
pushed a commit
to aws-controllers-k8s/code-generator
that referenced
this issue
Nov 11, 2021
Adds a new SetFieldConfig struct to the code generator config that allows teams to instruct the code generator to handle the SetResource generator specially for a particular `pkg/model.OpType`. This is useful for at least a couple use cases: 1. Different Go types in input and output shapes When the Go type of a field in the Create Input shape differs from the Go type of the same-named field in another operation's Output shape, the code generator doesn't know how to handle this situation and ends up outputting invalid Go code. An example of this behaviour is in the RDS controller's DBInstance resource. The DBSecurityGroups field in the Create Input shape is of type `[]*string`, but the same field in the Create, ReadMany and Update output shapes is of type `[]*DBSecurityGroupMembership`. The `DBSecurityGroupMembership` struct has two fields, `DBSecurityGroupName` and `Status`. The only part of that struct that is relevant to the `Spec.DBSecurityGroups` field is the `DBSecurityGroupName` field, which corresponds to the string identifier of the DB security group provided by the user in the `CreateDBInstanceRequest.DBSecurityGroups` field. The code generator ends up calling `pkg/generate/code.SetResource` for the DBInstance resource, gets to the `Spec.DBSecurityGroups` field and just doesn't know how to handle the different Go types and so ends up outputting nothing, which breaks compilation and has caused us to manually comment out or fix the generated code. operation. See: https://github.com/aws-controllers-k8s/rds-controller/blob/dc3271fa7b455fc99c3531ce372ea221c9f5b8e7/pkg/resource/db_instance/sdk.go#L853-L864 In the RDS DBInstance DBSecurityGroups case, we'd add the following to the RDS generator.yaml file to instruct the code generator how to transform the response from the DescribeDBInstances Output shape for setting the value of `Spec.DBSecurityGroups` from the set of `DBSecurityGroupMembership.DBSecurityGroupName` values: ```yaml resources: DBInstance: fields: DBSecurityGroups: set: - on: Create from: DBSecurityGroupName - on: ReadOne from: DBSecurityGroupName ``` 2. Ignoring fields in output shapes containing stale data For some service APIs, the returned value for a field in the Output shape of an Update operation contains the *originally-set* value instead of the value of the field that was set in the Update operation itself. An example of this situation is the ElastiCache ModifyReplicationGroup API call. If you pass some value for the LogDeliveryConfiguration field in the Input shape (which comes from the Spec.LogDeliveryConfiguration field of the ReplicationGroup resource), the value that is returned in the Output shape's LogDeliveryConfiguration is actually the *old* (stale) value for the field. The reason for this return of stale data is because the log delivery configuration is asynchronously mutated. For these cases, we want to be able to tell the code generator to ignore these fields when outputting code for the `pkg/generate/code.SetResource` function, but only for the Update operation. In this case, we'd add the following to the ElastiCache generator.yaml file: ```yaml resources: ReplicationGroup: fields: LogDeliveryConfiguration: set: - on: Update ignore: true ``` Signed-off-by: Jay Pipes <[email protected]> Issue: aws-controllers-k8s/community#178 By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Closing since aws-controllers-k8s/code-generator#232 implements basis of issue. Follow-ups to come |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
When building the RDS service controller, there are still two failures when running
make test
after generating the controller withscripts/build.controllers.sh rds
.The first failure is in
services/rds/pkg/resources/db_proxy/sdk.go
and the other inservices/rds/pkg/resources/db_instance/sdk.go
Both of those failures are due to the following: when the name of a field in a Create Input shape is the same as the name of a field in the corresponding ReadOne or ReadMany output shape, but the data types of those fields are different, we run into problems. The RDS APIs have two places where this happens. The first case is CreateDBInstance's Input shape has a field "DBSecurityGroups". It is of type
[]*string
:https://github.com/aws/aws-sdk-go/blob/master/models/apis/rds/2014-10-31/api-2.json#L2838.
However, the DescribeDBInstanceGroups API's Output shape has a DBSecurityGroups field that is of type
[]*DBSecurityGroupMembership
:https://github.com/aws/aws-sdk-go/blob/master/models/apis/rds/2014-10-31/api-2.json#L3629.
The code that is generated to handle the setting of Spec and Status fields does so by looking for Spec and Status fields that match the field names (member names) of an Output shape:
https://github.com/aws/aws-controllers-k8s/blob/main/pkg/model/crd.go#L1222-L1278
Unfortunately, that code assumes that the data types of the Spec and Status fields with the same member name are the same as the data types of the member name in the Output shape. And, as noted for RDS, there are two exceptions to this behaviour.
The fastest way to solve the above problem is to add something to the GeneratorConfig that will instruct the code generator about these types of exceptional behaviours. Long term, we might be able to do something more magical and introspective...
The text was updated successfully, but these errors were encountered: