search

kubernetes-retired / cluster-api-bootstrap-provider-kubeadm

LEGACY REPO. NEW CODE IS https://github.com/kubernetes-sigs/cluster-api/tree/master/bootstrap/kubeadm
Apache License 2.0
62 stars 67 forks source link

🐛 Refresh token for provisioning machines #250

Closed sethp-nr closed 5 years ago

sethp-nr commented 5 years ago

What this PR does / why we need it:

Refreshes the token for a machine that's not used it.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged): Fixes #248

k8s-ci-robot commented 5 years ago

Welcome @sethp-nr!

It looks like this is your first PR to kubernetes-sigs/cluster-api-bootstrap-provider-kubeadm 🎉. 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-bootstrap-provider-kubeadm 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. :smiley:

sethp-nr commented 5 years ago

I'm not attached to the new 9-minute sync interval, btw – I was considering trying to get clever with the requeueAfter duration for newly-minted tokens, but this was the simplest thing I could think of to do.

SataQiu commented 5 years ago

/retitle 🐛 Refresh token for provisioning machines

sethp-nr commented 5 years ago

@chuckha I added a test case for the new behavior. I've started trying to reconcile the current logic into discrete switch-shaped states, I'll let you know how it goes.

chuckha commented 5 years ago

sounds great, no worries if it doesn't work out

sethp-nr commented 5 years ago

I think it worked out: see 794be6ec35fa47. I left in the "early out" bits at the top to avoid patching / lookup on the cluster when we don't need to, but everything else was easy enough. I'm not sure if it's exactly what you were thinking though

sethp-nr commented 5 years ago

/test pull-cluster-api-bootstrap-provider-kubeadm-verify

chuckha commented 5 years ago

/approve

this looks great!

want to give this a look @detiber?

k8s-ci-robot commented 5 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chuckha, sethp-nr

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: - ~~[OWNERS](https://github.com/kubernetes-sigs/cluster-api-bootstrap-provider-kubeadm/blob/master/OWNERS)~~ [chuckha] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
chuckha commented 5 years ago

If you don't want to make var to const change that's ok, we can merge without it, but can you squash your commits down to 1?

sethp-nr commented 5 years ago

I squashed the commits – I didn't turn it back into a const because I made it configurable by CLI flag instead so I could set it to longer if we end up needing to push out the sync interval.

chuckha commented 5 years ago

/lgtm

Thank you!