Skip to content
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

Adding three new funcs for cloud providers, for refactoring delete and get #3442

Conversation

chrislovecnm
Copy link
Contributor

@chrislovecnm chrislovecnm commented Sep 24, 2017

Adding three new methods for cloud providers

  1. DeleteInstance deletes a cloud instance
  2. DeleteGroup delete a group of cloud instances
  3. GetCloudGroups returns a map of cloud groups that back a kops cluster

These three methods will allow the refactoring of funcs in instancegroups.go https://github.com/kubernetes/kops/blob/master/pkg/instancegroups/instancegroups.go#L437

This refactoring will allow for rolling-updates across multiple cloud providers.

This code is not wired in.

The actual code that is included in this PR is copied from resources and will be refactored out later.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 24, 2017
@chrislovecnm
Copy link
Contributor Author

Marking this a WIP, I want to get in a PR first with the aws code first, and get that tested.

@chrislovecnm
Copy link
Contributor Author

/approve

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrislovecnm

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 /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 24, 2017
@@ -76,6 +79,18 @@ func NewCloud(region string) (*Cloud, error) {
}, nil
}

func (c *Cloud) GetCloudGroups(cluster *kops.Cluster, instancegroups []*kops.InstanceGroup, warnUnmatched bool, nodeMap map[string]*v1.Node) (map[string]*fi.CloudGroup, error) {
return nil, fmt.Errorf("not implemented yet")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From someone that has been bitten by this many times: it is nice to print fmt.Errorf("Cloud::GetCloudGroups not implemented yet") because otherwise you just see "not implemented yet" and you don't know which method it is!

Or you can panic() or glog.Fatalf, both of which give a stack trace

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will update

@chrislovecnm
Copy link
Contributor Author

Moving out of WIP, will update the err messages. Testing is looking great.

@chrislovecnm
Copy link
Contributor Author

@justinsb PTLA - added debug logging and better error messages

@justinsb justinsb added this to the backlog milestone Sep 25, 2017
@chrislovecnm
Copy link
Contributor Author

We have #3446 ... need to close

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 30, 2017
@k8s-github-robot
Copy link

@chrislovecnm PR needs rebase

@chrislovecnm
Copy link
Contributor Author

Closing

@chrislovecnm chrislovecnm deleted the new-cloud-interface-methods branch December 30, 2017 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. scheduled-to-close size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants