Add aws support #640
Add aws support #640
Conversation
|
@manugarg can you take a look at this if you have time? that would help replace some of our custom code we have internally to supplement cloudprober. Thank you. |
Hello Rabun, Sorry for the long delay. I've started reviewing. I'll try to finish in a couple of days. Thanks for submitting this. |
I have addressed some of your feedback including the splits for AWS APIs. I'll work on splitting the PR into multiple pieces for easy reviews. Thanks for the reviews so far |
There was a problem hiding this comment.
I don't understand why this file is included in this change?
There was a problem hiding this comment.
it's imports tool, placing the internal package in the top section, seems like a good change but I can remove if you want
There was a problem hiding this comment.
I see, actually flag is in the middle for a reason. It's required for Google's importing tools to work properly. Please drop this file.
| // How often resources should be refreshed. | ||
| optional int32 re_eval_sec = 98 [default = 600]; // default 10 mins |
There was a problem hiding this comment.
I am assuming you'll eventually need names and filters for all these resources too?
internal/rds/aws/proto/config.proto
Outdated
| // Amazon Resource Name (ARN) of the load balancer | ||
| // if specified, only the corresponding load balancer information is returned. | ||
| repeated string name = 1; |
There was a problem hiding this comment.
As name can be slightly confusing, should we standardize on ARN (arn) for AWS resources?
There was a problem hiding this comment.
that was a wrong comment on my end, this was for the name of the LB not ARN, we can ARN later if needed. I plan to add support for LBs as a separate PR and then multiple options (ARN and name etc) can be added as filters if needed
|
Hi @rabunkosar-dd, do you want to continue working on it. Apologies for the initials delays in review, but I think you're pretty close. (P.S. Bigger PRs require blocking more time but if you break it up, we can get through them more quickly) |
Hey, I'm planning to resume working on it, got blocked on other projects. Will update the PR soon |
There was a problem hiding this comment.
I think it's getting close. Added some comments about dropping irrelevant file:
- cmd/cloudprober.go
- targets/endpoint/endpoint.go
- rds/client/client.go
Also, it will be great if you can trim down the proto file for now.
Please see my comments inline inline as well.
There was a problem hiding this comment.
I see, actually flag is in the middle for a reason. It's required for Google's importing tools to work properly. Please drop this file.
There was a problem hiding this comment.
To make this change more focussed, I'd leave this file too. Typo fix is not relevant to this PR.
There was a problem hiding this comment.
Can you please leave out this file too. Fixing of insecure package is an independent thing. Thanks!
| // Copyright 2019 The Cloudprober Authors. | ||
| // |
There was a problem hiding this comment.
I'd put the latest year in the copyright headers of new *.go files.
There was a problem hiding this comment.
If it's not too much trouble, can you please add resource types to protobuf as you actually add their logic. It will be easier to review them together like that. With that you'll have to change the Go code also.
So I'd prefer if this file contains only the following for now:
// AWS provider config.
message ProviderConfig {
// Profile for the session.
optional string profile_name = 1;
// AWS region
optional string region = 2;
// TODO: Add resource types in further PRs.
}
A proposed skeleton PR to support AWS resource expansion