Skip to content

Conversation

yhontyk
Copy link
Contributor

@yhontyk yhontyk commented Apr 27, 2020

Branch 4.4

@openshift-docs-preview-bot

The preview will be available shortly at:

Copy link
Contributor

Choose a reason for hiding this comment

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

s/always/also

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest same change in the Helm module too.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the placement is good. The other option is to add it to the end of the section. But I think the current option tells users that this section has CLI instructions, but if he wants to use the console he can access it before reading all of the CLI instructions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest: The following...different platforms using the CLI.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest adding an intro line here similar to that in odo: The following section describes how to install Helm on different platforms using the CLI.

Suggest adding the note before the prerequisite section, immediately after the above intro line.

@Preeticp
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 27, 2020
@Preeticp Preeticp removed their assignment Apr 27, 2020
@bergerhoffer bergerhoffer added branch/enterprise-4.4 peer-review-needed Signifies that the peer review team needs to review this PR labels Apr 27, 2020
Copy link
Contributor

@bergerhoffer bergerhoffer left a comment

Choose a reason for hiding this comment

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

Just two comments

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 up to you, but to me, it feels like this warning about not being able to use odo in a restricted env belongs in the NOTE. And then the additional info about finding the binaries in the web console belongs in normal text below the NOTE.

Copy link
Contributor

Choose a reason for hiding this comment

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

  • Suggest simplifying to something like "You can also download the latest binaries from the {product-title} web console." Or even combining into "You can also download the latest binaries from the {product-title} web console by clicking the ? icon in the upper-right corner and select Command Line Tools."
  • Use a comma instead of a semicolon in the second sentence.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can't really download them this way. Clicking the ? icon in the upper-right corner and selecting Command Line Tools will lead user to the page with titles and URLs of different CLI tools' folders on the file server (odo, helm etc). And there, on the file server, they can download the actual binary file.

Otherwise, good call, thank you! I've shortened the sentence and included it as a part of introduction in both modules.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough!

@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 27, 2020
@yhontyk
Copy link
Contributor Author

yhontyk commented May 6, 2020

@bergerhoffer I completely forgot about this PR! Could you merge please if everything's okay?

@bergerhoffer bergerhoffer added branch/enterprise-4.5 peer-review-done Signifies that the peer review team has reviewed this PR and removed peer-review-needed Signifies that the peer review team needs to review this PR labels May 6, 2020
@bergerhoffer bergerhoffer added this to the Next Release milestone May 6, 2020
@bergerhoffer
Copy link
Contributor

Sure thing @boczkowska, merging! Just FYI this will go into both 4.5 and 4.4 now.

@bergerhoffer bergerhoffer merged commit 457ca87 into openshift:master May 6, 2020
@bergerhoffer
Copy link
Contributor

/cherrypick enterprise-4.5

@bergerhoffer
Copy link
Contributor

/cherrypick enterprise-4.4

@openshift-cherrypick-robot

@bergerhoffer: new pull request created: #21858

In response to this:

/cherrypick enterprise-4.5

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.

@openshift-cherrypick-robot

@bergerhoffer: new pull request created: #21859

In response to this:

/cherrypick enterprise-4.4

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

branch/enterprise-4.4 branch/enterprise-4.5 peer-review-done Signifies that the peer review team has reviewed this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants