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

Improve "Installing the Knative CLI" #2230

Merged
merged 1 commit into from
Jul 7, 2020
Merged

Conversation

MIBc
Copy link
Contributor

@MIBc MIBc commented Feb 21, 2020

Proposed Changes

  • Make install kn with Go more detailed.
  • Give example on how to run kn using container.

Fixes #knative/client#648

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Feb 21, 2020
@knative-prow-robot
Copy link
Contributor

Hi @MIBc. 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.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@knative-prow-robot knative-prow-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 21, 2020
docs/install/install-kn.md Outdated Show resolved Hide resolved
@MIBc
Copy link
Contributor Author

MIBc commented Feb 21, 2020

/assign @rhuss

Copy link
Contributor

@rhuss rhuss left a comment

Choose a reason for hiding this comment

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

@MIBc thanks a lot for your PR ! Really appreciated for taking care.

There are some issues though as pointed out below. We need to fix that, but I'm on PTO next week.
Please check with @navidshaikh in the meantime.

You can run `kn` from container. For example:
```bash
docker pull https://gcr.io/knative-releases/knative.dev/client/cmd/kn
docker run --rm --name <your-container-name> -v "$HOME/.kube/config:<your-kubeconfig-path>" gcr.io/knative-releases/knative.dev/client/cmd/kn:latest service list
Copy link
Contributor

Choose a reason for hiding this comment

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

  • No need to select a name (just leave that out)
  • Instead of <your-kubeconfig-path> we need to provide the concrete directory.

Still have to figure that out, but @navidshaikh how is this image called in the tekton integration ?

In general it doesnt make sense to add placeholder in an installation intstructions if it is not explained what they should be (and of course shouldn't be added if not needed)

Copy link
Contributor

@navidshaikh navidshaikh Feb 24, 2020

Choose a reason for hiding this comment

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

We don't need to touch anything for kubeconfig while working with tekton.

IMO, we should remove this command example and add a sub-section here for example: 'Using kn with Tekton' which links to the kn catalog task at https://github.com/tektoncd/catalog/tree/master/kn

Copy link
Contributor Author

@MIBc MIBc Feb 25, 2020

Choose a reason for hiding this comment

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

May be we can both introduce Running kn in docker and Running kn with tekton task. For some users, they are not familiar with tekton.

docs/install/install-kn.md Show resolved Hide resolved
docs/install/install-kn.md Outdated Show resolved Hide resolved
docs/install/install-kn.md Show resolved Hide resolved
docs/install/install-kn.md Outdated Show resolved Hide resolved
docs/install/install-kn.md Outdated Show resolved Hide resolved
docs/install/install-kn.md Show resolved Hide resolved
@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 2, 2020
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 12, 2020
@MIBc MIBc force-pushed the client branch 2 times, most recently from 2c9688b to 7827f2a Compare March 12, 2020 07:38
@MIBc
Copy link
Contributor Author

MIBc commented Mar 12, 2020

Do you have more suggestion? @rhuss

Copy link
Contributor

@rhuss rhuss left a comment

Choose a reason for hiding this comment

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

Please see some suggestions inline. If you like I can help in the wording.

docs/install/install-kn.md Outdated Show resolved Hide resolved
docs/install/install-kn.md Outdated Show resolved Hide resolved
docs/install/install-kn.md Outdated Show resolved Hide resolved
@rhuss
Copy link
Contributor

rhuss commented Mar 23, 2020

/ok-to-test

@knative-prow-robot knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 23, 2020
@evankanderson
Copy link
Member

@rhuss any more comments? Is this good to go?

Copy link
Contributor

@rhuss rhuss left a comment

Choose a reason for hiding this comment

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

Looks good, thanks ! (and sorry for the delay).

In a followup PR we should also describe how to install with brew on macOS.

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 16, 2020
@rhuss
Copy link
Contributor

rhuss commented Apr 20, 2020

/assign @averikitsch

@averikitsch averikitsch removed their assignment Apr 20, 2020
Copy link
Member

@matzew matzew left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@lionelvillard
Copy link
Member

/approve

@MIBc
Copy link
Contributor Author

MIBc commented May 18, 2020

/assign @averikitsch

Copy link
Contributor

@abrennan89 abrennan89 left a comment

Choose a reason for hiding this comment

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

@MIBc added some comments. Thanks for adding this PR 🙂

@@ -26,6 +26,22 @@ To install the `kn` Client, you must download the executable binary for your sys

You must place the executable binary in your system path, and make sure that it is executable.

## Install `kn` using Go
1. Building `kn` requires Go v1.13 or newer. You will first need a working Go environment.
Copy link
Contributor

Choose a reason for hiding this comment

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

Make this a prerequisite instead of step 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -26,6 +26,22 @@ To install the `kn` Client, you must download the executable binary for your sys

You must place the executable binary in your system path, and make sure that it is executable.

## Install `kn` using Go
1. Building `kn` requires Go v1.13 or newer. You will first need a working Go environment.
1. Check out the [Client repository](https://github.com/knative/client).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
1. Check out the [Client repository](https://github.com/knative/client).
1. Check out the [Client repository](https://github.com/knative/client):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

```bash
hack/build.sh -f
```
1. Move `kn` into your system path, and verify whether kn is working properly. For example:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
1. Move `kn` into your system path, and verify whether kn is working properly. For example:
1. Move `kn` into your system path, and verify that `kn` commands are working properly. For example:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -34,3 +50,16 @@ Links to either the nightly container image or the latest stable container image

- [Nightly container image](https://gcr.io/knative-nightly/knative.dev/client/cmd/kn)
- [Latest release](https://gcr.io/knative-releases/knative.dev/client/cmd/kn)

You can run `kn` from container. For example:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
You can run `kn` from container. For example:
You can run `kn` from a container image. For example:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

You can also [run kn using Tekton](https://github.com/tektoncd/catalog/tree/master/kn).

## What's next
Now that you have installed `kn` in your system. To learn more about `kn`, you can read our [document](https://github.com/knative/client/blob/master/docs/cmd/kn.md).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Now that you have installed `kn` in your system. To learn more about `kn`, you can read our [document](https://github.com/knative/client/blob/master/docs/cmd/kn.md).
To learn more about using `kn`, see the [documentation](https://github.com/knative/client/blob/master/docs/cmd/kn.md).

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd also add a comment here to make sure this gets updated to a link within the docs at some point, rather than the client repo. @rhuss cc'ing you since you might want to update this once there's a kn reference section in docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your comments.

@abrennan89 abrennan89 self-assigned this Jun 15, 2020
@abrennan89
Copy link
Contributor

@MIBc just checking in on the status of this one - any plans to update based on the comments?
Thanks.

@MIBc
Copy link
Contributor Author

MIBc commented Jul 1, 2020

I will update it asap.

@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 1, 2020
@abrennan89
Copy link
Contributor

@MIBc thanks for the changes, is this ready to merge now?

Copy link
Contributor

@rhuss rhuss left a comment

Choose a reason for hiding this comment

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

one minor update, otherwise looks good for me now (sorry for the delay)

I a followup PR we should also document how to install via brew on macOs. (See https://github.com/knative/homebrew-client)

@@ -26,6 +26,22 @@ To install the `kn` Client, you must download the executable binary for your sys

You must place the executable binary in your system path, and make sure that it is executable.

## Install `kn` using Go
**Prerequisite:** Building `kn` requires Go v1.13 or newer. You will first need a working Go environment.
Copy link
Contributor

Choose a reason for hiding this comment

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

It's 1.14 now

@@ -26,6 +26,22 @@ To install the `kn` Client, you must download the executable binary for your sys

You must place the executable binary in your system path, and make sure that it is executable.

## Install `kn` using Go
**Prerequisite:** Building `kn` requires Go v1.13 or newer. You will first need a working Go environment.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
**Prerequisite:** Building `kn` requires Go v1.13 or newer. You will first need a working Go environment.
**Prerequisite:** Building `kn` requires Go v1.14 or newer. You will first need a working Go environment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

* Make `install kn with Go` more detailed.
* Give example on how to run kn using container.
@knative-prow-robot knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 7, 2020
@abrennan89
Copy link
Contributor

/lgtm
/approve

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 7, 2020
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abrennan89, lionelvillard, matzew, MIBc, rhuss

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.