Skip to content

Conversation

@zxDiscovery
Copy link
Contributor

Fixes #557

Proposed Changes

  • Add the a new file install/Knative-with-ICP.md
  • Update the install/README.md

@knative-prow-robot knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 26, 2018
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: zxDiscovery
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: averikitsch

If they are not already assigned, you can assign the PR to them by writing /assign @averikitsch in a comment when ready.

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

@knative-prow-robot knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 26, 2018
@knative-prow-robot
Copy link
Contributor

Hi @zxDiscovery. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

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.

@@ -0,0 +1,171 @@
# Knative Install on ICP
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use full name instead of acronym.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1, we should never use short name for some public document, please use IBM Cloud Private instead.

Also please remove ICP from this document.

`LoadBalancer` to `NodePort` for the `istio-ingress` service).

```shell
curl -L https://raw.githubusercontent.com/knative/serving/v0.2.1/third_party/istio-1.0.2/istio.yaml \
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why you aren't installing Istio in the same way as the other install guides?


## Before you begin

Knative requires a ICP(IBM Cloud Private) cluster v2.1.0.3 or newer.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add instructions on provisioning a cluster on ICP.


Knative depends on Istio. Run the following to install Istio. (We are changing
`LoadBalancer` to `NodePort` for the `istio-ingress` service).

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add numbered steps to these instructions.


## Installing Knative Serving

Next, install [Knative Serving](https://github.com/knative/serving):
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add numbered steps to these instructions.


The ICP Pod
```shell
# kubectl get psp
Copy link
Contributor

Choose a reason for hiding this comment

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

uncomment

@@ -0,0 +1,171 @@
# Knative Install on ICP
Copy link
Contributor

Choose a reason for hiding this comment

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

+1, we should never use short name for some public document, please use IBM Cloud Private instead.

Also please remove ICP from this document.

`LoadBalancer` to `NodePort` for the `istio-ingress` service).

```shell
curl -L https://raw.githubusercontent.com/knative/serving/v0.2.1/third_party/istio-1.0.2/istio.yaml \
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use v0.2.2

Delete the Kubernetes cluster along with Knative, Istio, and any deployed apps:

```shell
kubectl delete
Copy link
Contributor

Choose a reason for hiding this comment

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

delete what?

```shell
vim knative-clusterrole.yaml
```
```shell
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not shell but yaml, ditto for other YAML files.

@samodell samodell added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 28, 2018
@samodell
Copy link
Contributor

@zxDiscovery Thanks for your PR! Please address Averi and Guang's comments to start. We want to keep these install guides as similar as possible, and there are some global changes pending to the install guides in #565, so you'll want to copy the changes there once they're merged in. Adding a hold for that reason.

@zxDiscovery
Copy link
Contributor Author

zxDiscovery commented Dec 3, 2018

There are some mistake on this pull request. So close this and create a new one #618

@zxDiscovery zxDiscovery closed this Dec 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants