-
Notifications
You must be signed in to change notification settings - Fork 1.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
Introduce the --use flag when creating a cluster #517
Conversation
Welcome @frederiko! |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: frederiko If they are not already assigned, you can assign the PR to them by writing 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 |
Hi @frederiko. Thanks for your PR. I'm waiting for a kubernetes-sigs or kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
/assign @munnerz |
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.
@frederiko
thanks for the PR.
added minor comments.
also please squash your commits to 1.
thanks.
/ok-to-test |
looks like there's a dependency issue, go.mod also seems to not be updated despite the new import 😬 |
Jobs are failing with the following error
|
a1f1711
to
571d698
Compare
/test pull-kind-conformance-parallel |
[this feature is really neat, there's just been a lot of other things to tackle a little more urgently.] looking back over this, I suspect for it to work properly when multiple clusters are involved we need something like #512 I'm also slightly hesisitant to expand dependencies on upstream kubernetes since we test kubernetes with kind 🤔 ... will check what kops does since they're in a similar boat |
Fair enough. I just thought it would be useful and felt like helping, but
understand if you don’t feel like merging it. No big deal, although i do
believe it has its value.
On this particular PR, IMHO a proper cleaning is missing when the cluster
is deleted, although if a new cluster with the same name is created, it
simply overwrites its entry. As far as working with multiple cluster, I did
not find a problem, but willing to look into it again.
On Mon, Jun 24, 2019 at 6:15 PM Benjamin Elder ***@***.***> wrote:
[this feature is really neat, there's just been a lot of other things to
tackle a little more urgently.]
looking back over this, I suspect for it to work properly when multiple
clusters are involved we need something like #512
<#512>
I'm also slightly hesisitant to expand dependencies on upstream kubernetes
since we test kubernetes with kind 🤔 ... will check what kops does since
they're in a similar boat
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#517?email_source=notifications&email_token=AADNIYEU4PMBIMIVRNSSCKDP4FWTDA5CNFSM4HL77RY2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYOVGSI#issuecomment-505238345>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADNIYELPAAWFO6PNJ3PW7LP4FWTDANCNFSM4HL77RYQ>
.
--
Sent from Gmail Mobile
|
Er it certainly has value, I'm just wondering if it's worth it to implement it without client-go or not and if we've reviewed this for all of the potential problem cases... 🤔 Cleaning is definitely another issue to think about, I was thinking about previous problems merging multiple kind configs because of the identical users (hence #512). Again, worth wise it's not that it isn't worth it at all! thank you for the PR! :-) We've just been fixing various bugs and kubernetes testing needs, lots going on 😅v0.4.0 was supposed to come out today and didn't due to some of those... I want to look at this more closely for the next release. |
I think a lot of people would really appreciate this feature 🙃 |
@frederiko: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
Hi @BenTheElder, is there any intention on bringing this PR back to life again? I could try resolving the conflicts if you'd like. |
#850 is largely implemented to closely march other tools, including setting kubeconfig with a more complete behavior match to client-go including importantly file "locks" for modifying kubeconfigs. Thanks for the PR 🙃 |
This PR introduces the
--use
that sets up the newly created cluster as the current context. It reads the required configuration fromkind-config-<cluster-name>
and adds the context to$HOME/.kube/config
.