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

oc cluster up doc update #11624

Merged
merged 1 commit into from
Oct 29, 2016
Merged

Conversation

csrwng
Copy link
Contributor

@csrwng csrwng commented Oct 27, 2016

Adds explicit doc for Docker for Mac, Docker for Windows and adds iptables -F suggestion for Linux.

@csrwng
Copy link
Contributor Author

csrwng commented Oct 27, 2016

@coreydaley ptal ... this depends on #11585

@coreydaley
Copy link
Member

@csrwng I think that we should settle on a format for the version in this PR, either 1.3.x or 1.3+, and not keep going back and forth, as it could be confusing. It probably doesn't really matter which one is chosen, as long as we are consistent.

@stevekuznetsov
Copy link
Contributor

1.3.x is not equivalent to 1.3+ the latter is what we mean and what we should use

@coreydaley
Copy link
Member

@stevekuznetsov I mis-typed, it is actually choosing between "1.3.x or newer releases" and "1.3+", which would be similar, but using 1.3+ is much more succinct and less likely to be misinterpreted.

NOTE - this command was released with the 1.3+ version of oc client tools, so you must be using version 1.3+ or above for this command to work.
| NOTE |
| ---- |
| This command was released with the 1.3+ version of oc client tools, so you must be using version 1.3+ or above for this command to work. |
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess the "or above" here is redundant as well, will fix.

@csrwng csrwng force-pushed the clusterup_doc_update branch from ebe16dc to 6c79104 Compare October 27, 2016 19:39
@csrwng
Copy link
Contributor Author

csrwng commented Oct 27, 2016

updated, but wasn't sure about removing the "or newer" part because it could be interpreted as 1.3 but not 1.4 ?

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 28, 2016
@csrwng csrwng force-pushed the clusterup_doc_update branch from 6c79104 to 8ee60e2 Compare October 28, 2016 12:32
@csrwng csrwng changed the title WIP: oc cluster up doc update oc cluster up doc update Oct 28, 2016
@csrwng
Copy link
Contributor Author

csrwng commented Oct 28, 2016

updated and rebased, ptal again

@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 28, 2016
@coreydaley
Copy link
Member

@csrwng looks good to me

@csrwng
Copy link
Contributor Author

csrwng commented Oct 28, 2016

@bparees ptal


| WARNING |
| ------- |
| In some cases, networking for pods will not work for containers in your cluster, especially in Fedora 24. To fix this, run `$ sudo iptables -F` before running `oc cluster up` |
Copy link
Contributor

Choose a reason for hiding this comment

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

explain that this will flush the iptables. I don't like to run random sudo commands on my machine if i don't know what it's supposed to do :)


| NOTE |
| ---- |
| This feature requires an oc command v1.4 and newer |
Copy link
Contributor

Choose a reason for hiding this comment

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

s/and/or/

@bparees
Copy link
Contributor

bparees commented Oct 28, 2016

two nits and lgtm

@csrwng csrwng force-pushed the clusterup_doc_update branch 2 times, most recently from a882367 to 33a5dfb Compare October 28, 2016 15:17
@csrwng
Copy link
Contributor Author

csrwng commented Oct 28, 2016

comments addressed

- [Linux](#linux)
- [Mac OS with Docker for Mac](#mac-os-with-docker-for-mac)
Copy link
Member

Choose a reason for hiding this comment

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

Should be Max OS X same as the one below it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The latest version of Mac OS dropped the "X" ... and Docker for Mac only supports this latest version.
docker-machine can be run on OS X

Copy link
Member

Choose a reason for hiding this comment

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

I will defer to your mac-spertise

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually, it should be MacOS, will fix

@coreydaley
Copy link
Member

@csrwng added a couple small nits

@csrwng csrwng force-pushed the clusterup_doc_update branch from 33a5dfb to ee6ce02 Compare October 28, 2016 15:36
@csrwng csrwng force-pushed the clusterup_doc_update branch from ee6ce02 to 95e1853 Compare October 28, 2016 15:39
@csrwng
Copy link
Contributor Author

csrwng commented Oct 28, 2016

Updated

@bparees
Copy link
Contributor

bparees commented Oct 28, 2016

[merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 95e1853

@openshift-bot
Copy link
Contributor

[Test]ing while waiting on the merge queue

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 95e1853

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10802/) (Base Commit: 90c7b56)

@soltysh
Copy link
Contributor

soltysh commented Oct 28, 2016

Flake #11650

re-[merge]

@openshift-bot
Copy link
Contributor

openshift-bot commented Oct 29, 2016

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10823/) (Base Commit: ef95fd9) (Image: devenv-rhel7_5274)

@openshift-bot openshift-bot merged commit 53623c0 into openshift:master Oct 29, 2016
@csrwng csrwng deleted the clusterup_doc_update branch December 21, 2016 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants