Skip to content
This repository was archived by the owner on Mar 23, 2020. It is now read-only.

Readme updates #54

Merged
merged 9 commits into from
Sep 3, 2019
Merged

Readme updates #54

merged 9 commits into from
Sep 3, 2019

Conversation

sreichar
Copy link
Collaborator

@sreichar sreichar commented Aug 29, 2019

fix #26 Feedback on "What's going on here?"
and
fix #50 Outdated README.md instructions
and
fix #51 Allow custom log level

@sreichar sreichar requested review from rlopez133 and markmc August 29, 2019 16:36
Copy link
Collaborator

@rlopez133 rlopez133 left a comment

Choose a reason for hiding this comment

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

  • Not sure on the Make file and OpenShift/Make file changes as I haven't look at that code closely
  • line 32 (ease installation) - installation is spelled wrong
  • "Using the ISO will address " - change to: "Using the prepared ISO addresses the following:" , then list out everything. At the bottom right a note. "NOTE: Optional scripts may be executed if not using the prepared ISO that handle the above prerequisites."
  • "for the OpenShift installer - for example" - Change to "for the OpenShift installer. For example,
  • Typo spelled "aboave" fix to "above"
  • Typo spelled "preformthe" fix to "perform the"
  • line 68 can we say "configuring a storage VLAN on the internal interface labeled ???" (I assume it won't be labeled provisioning.
  • line 90 - "Most of these items... " change to: "The following items are discovered in a properly configured cluster:"
  • Typo line 112 "usinghte" change to : "using the"
  • line 113 "subdirectory, Make a copy of" change to "subdirectory, create a copy of config_example.sh using the existing user as part of the file name. For example, config_<username>.sh. Once the file has been created, set the required PULL_SECRET variable within the shell script."
  • line 116 add ticks to libvirt. Change to libvirt --- Libvirt will be installed by ISO so not sure how you want to rewrite all this portion in general.

@russellb
Copy link
Member

@rlopez133 you should be able to leave inline feedback in the source view. Try clicking to the left of lines you'd like to comment on when viewing the changes under "Files Changed".

```sh
make configure
make OpenShift
```

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add in the README file a note about using a LOGLEVEL environment variable to increase the loglevel output of the openshift-install command? Such as:

Note: In order to increase the log level ouput of openshift-install, a LOGLEVEL environment variable can be used as:

export LOGLEVEL="debug"
make OpenShift

This will fix Russel's comment in #51

I can do a follow up PR if you don't want to add it into this big PR tho :)

Thanks!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll attempt to get it in prior to @markmc review else may ask you to do the follow up

@@ -1,8 +1,13 @@
.PHONY: default all OpenShift OCS CNV
Copy link
Contributor

Choose a reason for hiding this comment

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

The script in the CNV dir has changed, so this file needs an update for that. Because of the script names changing, I was thinking a Makefile per directory could make sense, then the root level Makefile just pushd and calls make. So if you raise a PR to change a script name, you need only mess with the Makefile in your directory instead of worrying what other Makefiles call your scripts

johnbieren added a commit to johnbieren/install-scripts that referenced this pull request Aug 30, 2019
…ets in OCS/CNV Makefiles

Signed-off-by: Johnny Bieren <[email protected]>
@sreichar
Copy link
Collaborator Author

@johnbieren - would you mind given my Makefiel changes a once over

Copy link
Contributor

@johnbieren johnbieren left a comment

Choose a reason for hiding this comment

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

lgtm

@sreichar sreichar merged commit d997533 into openshift-kni:master Sep 3, 2019
@sreichar sreichar deleted the readme-updates branch September 3, 2019 20:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Outdated README.md instructions Feedback on "What's going on here?"
6 participants