AWS: Switch to aws-sdk-go #8297
Conversation
| } | ||
|
|
||
| type goamzMetadata struct { | ||
| var metadataClient = http.Client{ | ||
| Timeout: time.Second * 1, |
There was a problem hiding this comment.
Probably safer to increase this a little bit.... 10 seconds?
|
Looking through the code, this looks awesome. I had one nit that a 1 second timeout on the metadata service feels very short, but it is local (I think) so it'll probably be fine. I'd like to run the e2e tests, but otherwise LGTM. |
|
@justinsb let me know how the e2e run went and I'll merge :) |
| res, err := self.ec2.DescribeInstances(&ec2.DescribeInstancesInput{ | ||
| InstanceIDs: stringPointerArray(instanceIds), | ||
| Filters: filters, | ||
| NextToken: &nextToken, |
There was a problem hiding this comment.
Turns out AWS is sort of fussy about nil vs empty string. You have to pass a nil pointer when you have no "next".
I suggest:
var nextToken *string
...
NextToken: nextToken
|
A few problems that need to be corrected, but once those are done I'd like to see this go in. I was able to build on this & the nodeport work to get ELB working and passing e2e tests (except for one unrelated failure) @iterion here's the branch where I did this work if my comments don't make sense, sadly there's quite a few branches coming together in there so it's not just these changes: https://github.com/justinsb/kubernetes/tree/aws_integration |
|
@justinsb Thanks for the detailed review! I think I've fixed all the issues you mentioned. Let me know if there is anything else. The pointer issue inside of the loop probably would had baffled me had you not caught it, good to know now. New language, new quirks I guess. This is my first real attempt at any Go, so I really appreciate the review. |
|
This LGTM. I may have missed some edge cases, but e2e and the work that I'm going to be doing to add ELB should pick them up (and I am suffering from branch overload). Let's merge and then I will fix any e2e issues on AWS. Apparently it is e2e day! Nice work @iterion |
vmarmol
added
the
lgtm
|
Awesome! We're currently on a merge freeze, but will merge once that is done. |
|
Looks like AWS e2e is broken, I'm digging in to it. Can we get this merged as well, while I'm fixing things anyway? :-) |
|
@dchen1107 we can merge this |
|
I am merging this one. Thanks for the patience. |
Previously we were using the goamz package. This PR switches us to the official AWS sdk.
This is taking a stab at #6162 - still a little bit WIP.
@justinsb PTAL