Skip to content

Conversation

kalexand-rh
Copy link
Contributor

No description provided.

@kalexand-rh kalexand-rh added this to the Next Release milestone Aug 7, 2020
@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 7, 2020
@openshift-docs-preview-bot

The preview will be available shortly at:

Copy link
Contributor

@chrisnegus chrisnegus 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. Just a few comments.

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 this one doesn't start with "export"? I think it should be:

$ export REMOVABLE_MEDIA_PATH=<path>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wking, is there a reason that we're not exporting the flash drive path when you mirror install/update content?

Copy link
Member

Choose a reason for hiding this comment

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

I still think we should only export variables that need to be consumed by sub-processes, and not the rest of these local placeholders (#22008).

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. That makes sense. I just wasn't sure why the one variable was different than the others.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is correct to break this into two steps. However, you should cut "Generate the thumbprint." and start the step with: "Install the clevis package, if it is not already installed:" Notice that I added the "if" which was missing.

Copy link
Contributor

Choose a reason for hiding this comment

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

The "Generate a thumbprint..." step should be step "5." and not "a."

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand why input and output are being separated in our new style. However, the challenge here is that the "Example output" is a mix of input and output. So the "y" after the words "Do you wish to trust..." is actually something the user types.

Copy link
Contributor

Choose a reason for hiding this comment

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

You did a good job separating the input from the output. However, I have a question about how the highlighting is supposed to work. Everything from (cat <<EOM to ) | base64 -w0 is actually typed. But the color highlighting shows the text color as being the same as the output. That applies to other cat << examples in the text below as well.

Copy link
Member

Choose a reason for hiding this comment

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

Harder to copy/past, but you could add leading > as a common continuation prompt ($PS2). Or have the input highlighted as a shell script instead of a terminal session and remove all the prompts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jboxman, will you PTAL at this styling?

Copy link
Contributor

Choose a reason for hiding this comment

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

Same issue I noted before about cat <<EOF examples are input all the way down to the last EOF. But the text after the first line all appears grey, so it looks like output.

@kalexand-rh kalexand-rh force-pushed the other_install_codeblocks branch from a7e1f18 to c6e2900 Compare August 10, 2020 15:41
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wking, will you PTAL?

@kalexand-rh kalexand-rh added the peer-review-needed Signifies that the peer review team needs to review this PR label Aug 11, 2020
Copy link
Contributor

@jeana-redhat jeana-redhat left a comment

Choose a reason for hiding this comment

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

One note where the output looked odd, otherwise lgtm

@jeana-redhat jeana-redhat added 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 Aug 11, 2020
@kalexand-rh kalexand-rh force-pushed the other_install_codeblocks branch from c6e2900 to 75260a4 Compare August 11, 2020 21:20
@kalexand-rh kalexand-rh force-pushed the other_install_codeblocks branch from 75260a4 to 82d8532 Compare August 13, 2020 15:21
@kalexand-rh kalexand-rh merged commit 0b90ad5 into openshift:master Aug 13, 2020
@kalexand-rh
Copy link
Contributor Author

/cherrypick enterprise-4.5

@kalexand-rh
Copy link
Contributor Author

/cherrypick enterprise-4.6

@openshift-cherrypick-robot

@kalexand-rh: new pull request created: #24794

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

@kalexand-rh: new pull request created: #24795

In response to this:

/cherrypick enterprise-4.6

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.5 branch/enterprise-4.6 peer-review-done Signifies that the peer review team has reviewed this PR 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.

8 participants