Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: irisdingbj 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 DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
The message "The Cluster has enabled admission webhook. Automatic sidecar injection is supported." does not explain enough for a new user. I propose these messages:
|
|
Some of the messages (for example "Can create Namespace") are capitalized. Others (example: "can initialize the k8s client") are not. Please use the same capitalization for each sub-message. |
|
Can you create a Go test that uses mock client for several of the correct and incorrect cases the new function detects? |
|
@esnible Testing added and messages has been adjusted according to the comments. ` Kubernetes-apiCan initialize the Kubernetes client. Kubernetes-versionIstio is compatible with Kubernetes: v1.13.3. Istio-existenceIstio will be installed in the istio-system namespace. Kubernetes-setupCan create necessary Kubernetes configurations: Namespace,ClusterRole,ClusterRoleBinding,CustomResourceDefinition,Role,ServiceAccount,Service,Deployments,ConfigMap. SideCar-InjectorIstio Configured to use automatic sidecar injection for 'default' namespace.To enable injection for other namespaces see https://istio.io/docs/setup/kubernetes/additional-setup/sidecar-injection/#deploying-an-app Install Pre-Check passed! The cluster is ready for Istio installation.` |
|
|
||
| } | ||
|
|
||
| func (m *mockClientExecPreCheckConfig) checkAuthorization(s *authorizationapi.SelfSubjectAccessReview) (result *authorizationapi.SelfSubjectAccessReview, err error) { |
There was a problem hiding this comment.
line is 166 characters (from lll)
| type testcase struct { | ||
| description string | ||
| config *mockClientExecPreCheckConfig | ||
| expectedResult string |
There was a problem hiding this comment.
expectedResult is unused (from structcheck)
istioctl/pkg/install/pre-check.go
Outdated
| fmt.Fprintf(writer, "-----------------------\n") | ||
| ns, _ := c.getNameSpace(*istioNamespaceFlag) | ||
| if ns != nil { | ||
| return fmt.Errorf("Istio cannot be installed because the Istio namespace '%v' is already in use", *istioNamespaceFlag) |
There was a problem hiding this comment.
error strings should not be capitalized or end with punctuation or a newline (from golint)
istioctl/pkg/install/pre-check.go
Outdated
|
|
||
| if !response.Status.Allowed { | ||
| if len(response.Status.Reason) > 0 { | ||
| return fmt.Errorf("Istio installation will not succeed.Create permission lacking for:%s: %v", name, response.Status.Reason) |
There was a problem hiding this comment.
error strings should not be capitalized or end with punctuation or a newline (from golint)
istioctl/pkg/install/pre-check.go
Outdated
| if len(response.Status.Reason) > 0 { | ||
| return fmt.Errorf("Istio installation will not succeed.Create permission lacking for:%s: %v", name, response.Status.Reason) | ||
| } | ||
| return fmt.Errorf("Istio installation will not succeed. Create permission lacking for:%s", name) |
There was a problem hiding this comment.
error strings should not be capitalized or end with punctuation or a newline (from golint)
|
@irisdingbj: The following tests failed, say
DetailsInstructions 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. |
|
@esnible ^^^ |
|
@irisdingbj could we do master instead? Trying to limit 1.1 for critical fixes. For master (1.2), I would like to graduate this verify-install cmd from an experimental cmd (this can be a separate PR). @esnible any objections? |
Perfom pre-check if no installation file is specified which includes below pre-check:
istio-systemnamespace does not exist.1.11.Namespace, ClusterRole, CustomResourceDefinition, ConfigMap, ServiceAccount , ClusterRoleBinding , Service , Deployment , RoleExample output: