-
Notifications
You must be signed in to change notification settings - Fork 190
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
always ensure operation overrides work #375
Conversation
pkg/model/op.go
Outdated
return OpTypeAddChild | ||
case "AddChildren": | ||
case "cddchildren": |
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.
typo
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.
fixed.
} | ||
op, found := a.API.Operations[opID] | ||
if !found { | ||
panic("operation " + opID + " in generator.yaml 'operations:' object does not exist.") |
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.
Nit: have this error read ... does not exist in the upstream API.
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.
Just a few minor nits (a comment, an error message, and a typo)
Overall looks great, this is a great example of a well-documented bug and accompanying fix. Thank you for including a clear and thorough PR description!
There were two bugs in the code generator's construction of the OperationMap during API inference. The first bug was a non-deterministic behaviour that showed itself when an API had multiple Operations of a single operation type (say, READ_ONE). The code that was responsible for determining which Operation mapped to which operation type for a particular resource was looping over the `aws-sdk-go` `private/model/api:API`'s `Operations' field, which is a map. Depending on the parsing of the underlying api-2.json files, same-typed Operations with different IDs -- e.g. `DescribeStreamSummary` and `DescribeStream` -- were being mapped to a single operation type -- OpTypeGet. Depending on which Operation ID came last, that is the operation that was being mapped, regardless of which one appeared in the OperationConfig configuration option that handles overrides. The second bug was that the `OpTypeFromString` method was too strict in its comparison logic. The strings "get", "READ_ONE", "read_one" and "Get" should all lead to OpTypeGet, but only "Get" was being matched. Fixes Issue aws-controllers-k8s/community#1555 Signed-off-by: Jay Pipes <jaypipes@gmail.com>
/retest |
Github is broken again.. |
/lgtm |
@jljaco: changing LGTM is restricted to collaborators In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@jljaco added you as maintainer in code-generator-maintainer GH team. you should be able to /lgtm properly in here now :) |
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.
Nice catch 👍
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: A-Hilaly, jaypipes, jljaco The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There were two bugs in the code generator's construction of the OperationMap during API inference.
The first bug was a non-deterministic behaviour that showed itself when an API had multiple Operations of a single operation type (say, READ_ONE). The code that was responsible for determining which Operation mapped to which operation type for a particular resource was looping over the
aws-sdk-go
private/model/api:API
'sOperations
field, which is a map. Depending on the parsing of the underlying api-2.json files, same-typed Operations with different IDs -- e.g.DescribeStreamSummary
andDescribeStream
-- were being mapped to a single operation type -- OpTypeGet. Depending on which Operation ID came last, that is the operation that was being mapped, regardless of which one appeared in the OperationConfig configuration option that handles overrides.The second bug was that the
OpTypeFromString
method was too strict in its comparison logic. The strings "get", "READ_ONE", "read_one" and "Get" should all lead to OpTypeGet, but only "Get" was being matched.Fixes Issue aws-controllers-k8s/community#1555
Signed-off-by: Jay Pipes jaypipes@gmail.com
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.