Add Vpc resource #1
Conversation
There was a problem hiding this comment.
Super clean. Seems fairly straightforward and I hope will be a good basis for a lot of the other resources.
Biggest thing is just to remove the DryRun field from spec - add to list of ignored fields.
I've left some minor nits about code style, but nothing major from me otherwise.
|
Looks solid beside incorporating Nick's comments. |
|
Neat 👍 |
ack-bot
added
the
ok-to-test
|
holding until we add pre-submit jobs for ec2-controller (aws-controllers-k8s/test-infra#82) |
ack-bot
added
the
do-not-merge/hold
|
/unhold |
ack-bot
removed
the
do-not-merge/hold
There was a problem hiding this comment.
Nice work on this, @brycahta! Left some comments inline that are mostly to the ACK core team (/cc @vijtrip2 @RedbackThomson @a-hilaly) for things we need to do to the code-generator to avoid the need for some of the custom code you've had to place in here.
|
|
||
| // The VPN tunnel options. | ||
| type TunnelOption struct { | ||
| DpdTimeoutAction *string `json:"dpdTimeoutAction,omitempty"` |
There was a problem hiding this comment.
@brycahta "Dpd" here should be normalized to "DPD". If you'd like to submit a patch to the code-generator repo correcting this, @RedbackThomson or @a-hilaly can walk you through the process (it's a relatively simple change to https://github.com/aws-controllers-k8s/code-generator/blob/main/pkg/names/names.go)
There was a problem hiding this comment.
when you say normalized does that mean it should be aligned with go styling?
There was a problem hiding this comment.
Yeah, initialisms should be capitalized fully when exported and lowercases fully when not exported according to Go style guidelines: https://github.com/golang/go/wiki/CodeReviewComments#initialisms
| // Information about the instances to which the volume is attached. | ||
| type VolumeStatusAttachmentStatus struct { | ||
| InstanceID *string `json:"instanceID,omitempty"` | ||
| IoPerformance *string `json:"ioPerformance,omitempty"` |
There was a problem hiding this comment.
Likewise this should be normalized to "IOPerformance" (in a future PR to the code-generator, nothing to do for this PR)
| @@ -1,4 +1,6 @@ | |||
| ignore: | |||
| field_paths: | |||
| - CreateVpcInput.DryRun | |||
|
|
||
| // addIDToListRequest adds requested-resource VpcId to ListRequest. Return error to indicate to callers that the | ||
| // resource is not yet created. | ||
| func addIDToListRequest(r *resource, input *svcsdk.DescribeVpcsInput) error { |
There was a problem hiding this comment.
And this function should not be necessary if the code-generator's pkg/generate/code.SetSDK generator function understood that for ReadMany operations, if it finds a field on the input shape that is the plural of an identifier field (in this case, VPCID -> VpcIds) then it should populate the input shape for the ReadMany operation with a slice of one element containing the resource's identifier field.
We can work on adding this functionality (we need it for S3 as well)
|
/retest |
|
Added the EC2 test role |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brycahta, jaypipes, RedbackThomson 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 |
Issue: #489