Skip to content

Updating install flow with minor changes to reflect desired prototype in UX/11#3411

Merged
knative-prow-robot merged 22 commits into
mainfrom
omerbensaadon-patch-7
Apr 8, 2021
Merged

Updating install flow with minor changes to reflect desired prototype in UX/11#3411
knative-prow-robot merged 22 commits into
mainfrom
omerbensaadon-patch-7

Conversation

@omerbensaadon
Copy link
Copy Markdown

@omerbensaadon omerbensaadon commented Apr 2, 2021

Changes:

  1. Moved Kn CLI to pre-requisite
  2. Added homebrew brew install kn command to pre-reqs
  3. Changed flow of install from install -> Serving or Eventing to Install -> Pre-Reqs -> Serving or Eventing
  4. Added reference to the Install files in each of the Serving and Eventing Installs
  5. Fix broken link
  6. Aligned any reference to "Serving" or "Eventing" to Knative Word and Phrase List

Omer B added 11 commits April 2, 2021 10:02
Changing link on the install page to link to pre-requisites
Added links to install either Serving or Eventing in Pre-Req
- Edited to include `kn` CLI as a pre-req
- pulled out Homebrew command 
- bolded important portions of the page
- conformed "Serving" and "Eventing" to the  [Knative Word and Phrase List](https://docs.google.com/spreadsheets/d/1p1_kBUd6ZvonxHkMcEJPayf6QIpExuFf5cFq0ptar7I/edit#gid=0) 
- Added link to Private Registry documentation
- removed reference to KN CLI (now in Pre-Reqs) 
- trimmed the fat around Installing Kantive Eventing
- Added relevant context to optional extensions
Added same context from Eventing Page
Added same context from Eventing Page, added (like TLS) to second bullet.
Removed reference to KN CLI (now in Pre-Reqs)
added reference to install files
@knative-prow-robot knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 2, 2021
@google-cla google-cla Bot added the cla: yes Indicates the PR's author has signed the CLA. label Apr 2, 2021
Omer B and others added 4 commits April 2, 2021 11:00
made wording less bad
… install out of Github and into knative.dev site (#3410)

* Update install-kn.md

Adding brew install kn command directly to page and making it the first option

* Update docs/client/install-kn.md

Co-authored-by: Mike Petersen <mpetason@gmail.com>

Co-authored-by: Mike Petersen <mpetason@gmail.com>
Comment thread docs/client/install-kn.md
Comment thread docs/install/_index.md
Comment thread docs/install/install-serving-with-yaml.md Outdated
Comment thread docs/install/prerequisites.md Outdated
Copy link
Copy Markdown
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.

It doesn't make sense to me to add kn to the prereqs, this is an optional CLI not a prereq and it can't be used to install anything. Left some other comments too.

@omerbensaadon
Copy link
Copy Markdown
Author

@abrennan89 left responses!

I think kn CLI should be a pre-req, in the sense that anyone doing anything with Knative could more easily do so through the tools we've built in our CLI.

In the future when we follow up the Install with a Tutorial, we would ideally have folks be interacting with the kn CLI and seeing where it fits into the picture.

It's a very handy piece of tech that we have an entire WG dedicated to, I think we should bubble it up more often instead of defaulting to YAML!

@abrennan89
Copy link
Copy Markdown
Contributor

abrennan89 commented Apr 5, 2021

I think kn CLI should be a pre-req, in the sense that anyone doing anything with Knative could more easily do so through the tools we've built in our CLI.

This isn't strictly true, they can't do anything with it, only certain things.

It's a very handy piece of tech that we have an entire WG dedicated to, I think we should bubble it up more often instead of defaulting to YAML!

My point is that it's not "defaulting" to YAML when we're talking about installing serving or eventing, it has to be done either via YAML or the Operator - there's no option to install components like serving or eventing using the kn CLI, so I feel like it's confusing to say people must install it when they really don't have to.

FWIW I love kn and use it for interacting with Knative in terms of creating other things, but I think putting it in install prereqs is just confusing because it's not actually required. The same way that you don't have to install both serving and eventing, we don't tell people they have to install both just because we'd like them to. The same thing IMHO should apply for kn IMHO; it's completely optional.

@omerbensaadon
Copy link
Copy Markdown
Author

OK let's kick this can down the road, going to move the KN CLI out of pre-requisites

@omerbensaadon
Copy link
Copy Markdown
Author

@abrennan89 all requested changes made!

Comment thread docs/install/install-eventing-with-yaml.md Outdated
Comment thread docs/install/install-eventing-with-yaml.md Outdated

- If you want to add optional enhancements to your installation, see [Installing optional extensions](./install-extensions.md)

- [Installing Knative Serving using YAML files](./install-serving-with-yaml.md)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not sure it's clear from this link that this is optional.

- Install the [Knative CLI](./install-kn) to use `kn` commands.
- [Installing Knative Eventing using YAML files](./install-eventing-with-yaml.md)

- If you want to add optional enhancements to your installation, see [Installing optional extensions](./install-extensions.md).
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
- If you want to add optional enhancements to your installation, see [Installing optional extensions](./install-extensions.md).
- To add optional enhancements to your installation, see [Installing optional extensions](./install-extensions.md).

- If you want to add extra features to your installation, see [Installing optional extensions](./install-extensions.md).
- If you want to install the Knative Eventing component, see [Installing Eventing using YAML files](./install-eventing-with-yaml.md)
- Install the [Knative CLI](./install-kn) to use `kn` commands.
- [Installing Knative Eventing using YAML files](./install-eventing-with-yaml.md)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not sure it's clear from this link that this is optional.


- If you want to add optional enhancements to your installation, see [Installing optional extensions](./install-extensions.md).

- To easily interact with Knative Services, [download the `kn` CLI](/docs/client/install-kn.md)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

See my comment above on the eventing topic. I imagine the same text can be used here.

Comment thread docs/install/prerequisites.md Outdated
- Your Kubernetes cluster must have access to the internet, since Kubernetes needs to be able to fetch images.
- Your Kubernetes cluster must have access to the internet, since Kubernetes needs to be able to fetch images. (To pull from a private registry, see [Deploying images from a private container registry](https://knative.dev/docs/serving/deploying/private-registry/))

## Install Knative Serving and Eventing
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Having this on the prerequisites topic seems confusing. It implies that installing Knative Serving and Eventing is a prerequisite.

Instead, maybe we could move the System requirements to the index page for installing Knative and remove the prerequisites topic? This new section at the bottom seems to repeat the info on the index page anyway.

@omerbensaadon
Copy link
Copy Markdown
Author

@snneji how about now? :D

Copy link
Copy Markdown
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.

LGTM. I'll let @snneji add the lgtm label when she's done reviewing.
Thanks Omer! 🎉

@abrennan89
Copy link
Copy Markdown
Contributor

/approve

@knative-prow-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: abrennan89

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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

@omerbensaadon
Copy link
Copy Markdown
Author

omerbensaadon commented Apr 7, 2021

@snneji added the "component" vs "Component" bit to Knative Word and Phrase List

accidentally left this out
@abrennan89
Copy link
Copy Markdown
Contributor

/retest

Copy link
Copy Markdown
Contributor

@snneji snneji left a comment

Choose a reason for hiding this comment

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

Just a couple more tiny tweaks. I requested changes to the other cross refs for the Knative CLI so that they match the one you applied feedback to. I'll add the LGTM label after those changes are applied. The other changes you made look good.

Comment thread docs/install/install-extensions.md Outdated

## Next steps

- To easily interact with Knative Services and Eventing Components, [download the `kn` CLI](/docs/client/install-kn.md)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
- To easily interact with Knative Services and Eventing Components, [download the `kn` CLI](/docs/client/install-kn.md)
- To easily interact with Knative Services and Eventing components, [install the `kn` CLI](/docs/client/install-kn.md)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is Knative Services another way to describe Knative Serving?


- To add optional enhancements to your installation, see [Installing optional extensions](./install-extensions.md).

- To easily interact with Knative Services, [download the `kn` CLI](/docs/client/install-kn.md)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
- To easily interact with Knative Services, [download the `kn` CLI](/docs/client/install-kn.md)
- To easily interact with Knative Services, [install the `kn` CLI](/docs/client/install-kn.md)

* Adding @snneji changes

* Adding @snneji changes round 2
@omerbensaadon
Copy link
Copy Markdown
Author

That should do it? @snneji

CC: @abrennan89 I would really like to cherrypick this in 0.22 so I can test this flow 👍🏼

@snneji
Copy link
Copy Markdown
Contributor

snneji commented Apr 8, 2021

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 8, 2021
@omerbensaadon
Copy link
Copy Markdown
Author

/cherrypick release-0.22

@knative-prow-robot
Copy link
Copy Markdown
Contributor

@omerbensaadon: once the present PR merges, I will cherry-pick it on top of release-0.22 in a new PR and assign it to you.

Details

In response to this:

/cherrypick release-0.22

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
Copy link
Copy Markdown
Contributor

@omerbensaadon: new pull request created: #3440

Details

In response to this:

/cherrypick release-0.22

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.

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. 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.

4 participants