Skip to content
This repository has been archived by the owner on Sep 24, 2021. It is now read-only.

Several updates #2

Merged
merged 5 commits into from
Jun 21, 2019
Merged

Several updates #2

merged 5 commits into from
Jun 21, 2019

Conversation

chuckha
Copy link
Contributor

@chuckha chuckha commented Jun 21, 2019

  • Use machine name instead of generated name tied to kind
  • Improve delete functionality to remove node from child cluster
  • Set nodeRefs

Signed-off-by: Chuck Ha [email protected]

@chuckha
Several updates
d180db9 
* Use machine name instead of generated name tied to kind
* Improve delete functionality to remove node from child cluster
* Set nodeRefs

Signed-off-by: Chuck Ha <[email protected]>
@k8s-ci-robot
Copy link
Contributor

Welcome @chuckha!

It looks like this is your first PR to kubernetes-sigs/cluster-api-provider-docker 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api-provider-docker has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chuckha

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 21, 2019
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 21, 2019
pablochacin
pablochacin suggested changes Jun 21, 2019
View reviewed changes
Copy link

@pablochacin pablochacin left a comment

Choose a reason for hiding this comment

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

First quick round or review. Will continue later. Need to get more familiar with code.

actuators/machine.go Show resolved Hide resolved
actuators/machine.go Outdated Show resolved Hide resolved
kind/actions/cluster_actions.go Outdated Show resolved Hide resolved
@sethp-nr
Copy link
Contributor

sethp-nr commented Jun 21, 2019 •
edited
Loading

The changes look reasonable enough to me so far, but I'm running into some trouble trying this out:

$ capdctl setup
Error: error loading config: decoding failure: no kind "Cluster" is registered for version "kind.sigs.k8s.io/v1alpha3" in scheme "sigs.k8s.io/kind/pkg/cluster/config/encoding/scheme.go:34"
panic: exit status 1

goroutine 1 [running]:
main.makeManagementCluster(0x1713884, 0x4)
	/Users/spellegrino/Code/Go/src/github.com/kubernetes-sigs/cluster-api-provider-docker/cmd/capdctl/main.go:186 +0x188
main.main()
	/Users/spellegrino/Code/Go/src/github.com/kubernetes-sigs/cluster-api-provider-docker/cmd/capdctl/main.go:82 +0x6cb

I'll see if I can figure that out and get back to you. Oh, my kind version was outdated. Things are underway now.

Also, I found a place in the README the rename missed: https://github.com/kubernetes-sigs/cluster-api-provider-docker/blame/master/README.md#L30-L32

@chuckha
proper delete
7c10ce7 
Signed-off-by: Chuck Ha <[email protected]>
@chuckha
Copy link
Contributor Author

chuckha commented Jun 21, 2019

@sethp-nr ah yeah, I'm also actively developing on the :latest tag, so I need to move over to dev and re-push that back. I'd recommend you build your own image at the moment

@chuckha chuckha mentioned this pull request Jun 21, 2019
Closed
@sethp-nr
Copy link
Contributor

Yeah, that's what I ended up doing – it looks like the README references a script that I didn't find. I've got another PR I'm about to send you for a couple quick things.

sethp-nr
sethp-nr reviewed Jun 21, 2019
View reviewed changes
Copy link
Contributor

@sethp-nr sethp-nr left a comment

Choose a reason for hiding this comment

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

I haven't tried multiple control planes / deleting one yet, but otherwise everything looks great and is working like I expect.

kind/actions/cluster_actions.go Outdated
@@ -306,15 +306,19 @@ func GetNodeRefUID(clusterName, nodeName string) (string, error) {
return strings.TrimSpace(lines[0]), nil
}

// DeleteClusterNode will remove
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is missing a few more words

@chuckha
Copy link
Contributor Author

chuckha commented Jun 21, 2019

I haven't tried multiple control planes / deleting one yet, but otherwise everything looks great and is working like I expect.

sweet, I'm going to fix up the delete comment and go ahead and merge it, thanks for the feedback @pablochacin and @sethp-nr !

The multi-control plane one needs a timeout of 15 minutes on an XL instance on AWS

@vincepri
Copy link

/lgtm
/hold

Feel free to unhold when ready

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Jun 21, 2019
@chuckha
Fixup comment
bfe1e2a 
Signed-off-by: Chuck Ha <[email protected]>
@k8s-ci-robot k8s-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 21, 2019
@chuckha
Copy link
Contributor Author

chuckha commented Jun 21, 2019

/hold cancel
/lgtm

image

@k8s-ci-robot
Copy link
Contributor

@chuckha: you cannot LGTM your own PR.

In response to this:

/hold cancel
/lgtm

image

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 21, 2019
@vincepri
Copy link

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 21, 2019
@sethp-nr
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot merged commit 6372508 into kubernetes-retired:master Jun 21, 2019
@chuckha chuckha deleted the updates branch August 16, 2019 02:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sethp-nr sethp-nr sethp-nr left review comments

@pablochacin pablochacin pablochacin requested changes

@justinsb justinsb Awaiting requested review from justinsb

@vincepri vincepri Awaiting requested review from vincepri

Assignees

@vincepri vincepri

@sethp-nr sethp-nr

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. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@chuckha @k8s-ci-robot @sethp-nr @vincepri @pablochacin