-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Refactoring to use cloud method for getting cloud groups #3446
Refactoring to use cloud method for getting cloud groups #3446
Conversation
Still want to keep this in WIP, as I want to do a bit more testing. |
I'd rather we didn't move more into |
Aha - so an approach that might be easier:
I think then I can OK this, if you can beat our unit tests ;-) |
/retest |
47cc37d
to
548a829
Compare
8086922
to
f1d25d2
Compare
f1d25d2
to
d6aad99
Compare
d6aad99
to
a431eb3
Compare
/lgtm |
/lgtm cancel //PR changed after LGTM, removing LGTM. @chrislovecnm @justinsb |
pkg/resources/gce.go
Outdated
func (d *clusterDiscoveryGCE) listGCEInstanceTemplates() ([]*tracker.Resource, error) { | ||
var resourceTrackers []*tracker.Resource | ||
|
||
templates, err := d.findInstanceTemplates() | ||
templates, err := d.gceCloud.FindInstanceTemplates(d.clusterName) |
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.
We lost caching here
Can we get caching/memoization back please? Then LGTM |
f9275af
to
c9cc561
Compare
if err != nil { | ||
return nil, fmt.Errorf("error listing instance groups: %v", err) | ||
return nil, fmt.Errorf("unable to find intance templates: %v", err) |
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.
Probably just return nil, err
because one assumes that FindInstanceTemplates already wrapped the error, but NBD
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: justinsb The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
/test all [submit-queue is verifying that this PR is safe to merge] |
Automatic merge from submit-queue. |
Automatic merge from submit-queue. Minor cleanups to #3446 Felt easier than iterating in PR comments!
This builds on various other PRs. The only two SHAs to review are:
AWS is the only one that has GetCloudGroups implemented at this point. GCE is next.
TODO
Updates
I have moved more of the code into
/pkg/cloudinstances
per guidance from @justinsb! I am liking it more!