-
Notifications
You must be signed in to change notification settings - Fork 74
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
Add aws support #640
base: master
Are you sure you want to change the base?
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. |
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.
Thanks once again for sending this change and your patience. I've not finished reviewing, but sending in the comments so far. Any chance you can split this change it into:
- 1 PR to add AWS targets support.
- 1 PR to add elasticache and RDS.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for delay again. Please see my review inline.
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.
I don't understand why this file is included in this change?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am assuming you'll eventually need names and filters for all these resources too?
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.
yes that's correct
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As name can be slightly confusing, should we standardize on ARN (arn) for AWS resources?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd put the latest year in the copyright headers of new *.go files.
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.
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