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

Fix --admission-control link #165

Closed
wants to merge 1 commit into from
Closed

Conversation

bobcatfish
Copy link
Contributor

I think this was intended to be a link.

I think this was intended to be a link.
@bobcatfish bobcatfish requested a review from mattmoor February 13, 2018 22:34
@@ -31,8 +31,7 @@ You'll also need to setup:
kubectl create clusterrolebinding cluster-admin-binding \
--clusterrole=cluster-admin --user=${YOUR_KUBE_USER}
```
1. Kubernetes cluster must have MutatingAdmissionWebhook specified in the [--admission-control as per]:
(https://kubernetes.io/docs/admin/extensible-admission-controllers/#enable-external-admission-webhooks)
1. Kubernetes cluster must have MutatingAdmissionWebhook specified with [--admission-control](https://kubernetes.io/docs/admin/extensible-admission-controllers/#enable-external-admission-webhooks).
Copy link
Member

Choose a reason for hiding this comment

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

Can we move this into minikube docs? This isn't even settable on GKE (in this way), and I think that leads to confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we have any minikube docs yet. Does this mean we don't need this setting when we use GKE? I think we'd need it with other generic k8s clusters tho?

Maybe @vaikas-google can shed some light, I think he added this initially

Copy link
Contributor

Choose a reason for hiding this comment

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

I can confirm that you don't need this when using GKE.

Copy link
Member

Choose a reason for hiding this comment

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

I started a thread to try and just get minikube fixed :) cc'd @bobcatfish

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mattmoor how do you feel about merging this fix for the link in the meantime (it's broken right now) or would you rather:

  1. leave it for now
  2. delete this line entirely for now

?

@grantr
Copy link
Contributor

grantr commented Feb 14, 2018

I think this is superseded by #167 which explains how to accomplish this for minikube clusters (it's not needed for GKE)

@grantr
Copy link
Contributor

grantr commented Feb 14, 2018

Closing this but feel free to reopen if you think it's warranted @bobcatfish!

@grantr grantr closed this Feb 14, 2018
@mattmoor mattmoor deleted the bobcatfish-patch-1 branch February 15, 2018 15:00
@bobcatfish
Copy link
Contributor Author

Closing this but feel free to reopen if you think it's warranted @bobcatfish!

np, thanks @grantr !

mgencur pushed a commit to mgencur/serving-1 that referenced this pull request Jun 25, 2019
🤖 Triggering CI on branch 'release-next' after synching to upstream/master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants