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.

Sign up for GitHub

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

Jump to bottom

docs: Add quick start for the Docker provider #1484

Merged
merged 1 commit into from
Oct 16, 2019
Merged

docs: Add quick start for the Docker provider #1484

merged 1 commit into from
Oct 16, 2019

Conversation

dlipovetsky
Copy link
Contributor

What this PR does / why we need it:
Add Docker provider to the book quick start.

/cc @chuckha

@k8s-ci-robot k8s-ci-robot requested a review from chuckha October 3, 2019 00:50
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 3, 2019
chuckha
chuckha approved these changes Oct 3, 2019
View reviewed changes
Copy link
Contributor

@chuckha chuckha left a comment

Choose a reason for hiding this comment

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

/approve

this looks great to me, just a couple of questions for my own edification

/assign @detiber
for lgtm

docs/book/src/user/quick-start.md Show resolved Hide resolved
docs/book/src/user/quick-start.md Show resolved Hide resolved
docs/book/src/user/quick-start.md Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 3, 2019
detiber
detiber reviewed Oct 3, 2019
View reviewed changes
docs/book/src/user/quick-start.md Outdated Show resolved Hide resolved
docs/book/src/user/quick-start.md Outdated
```bash
kind create cluster --name=clusterapi
export KUBECONFIG="$(kind get kubeconfig-path --name="clusterapi")"
```
Copy link
Member

Choose a reason for hiding this comment

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

Not a blocker, but it would be nice if we could reference this block form other tabs rather than replicating in multiple places.

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer this as well, maybe we can have a special KIND configuration section when you have to pick your infrastructure provider?

docs/book/src/user/quick-start.md Outdated Show resolved Hide resolved
docs/book/src/user/quick-start.md Show resolved Hide resolved
docs/book/src/user/quick-start.md
name: capi-quickstart-worker
spec:
template:
spec: {}
Copy link
Member

Choose a reason for hiding this comment

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

Yuck, I'm wondering if we should make some of these fields optional rather than required.

vincepri
vincepri reviewed Oct 3, 2019
View reviewed changes
docs/book/src/user/quick-start.md Outdated
```bash
kind create cluster --name=clusterapi
export KUBECONFIG="$(kind get kubeconfig-path --name="clusterapi")"
```
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer this as well, maybe we can have a special KIND configuration section when you have to pick your infrastructure provider?

docs/book/src/user/quick-start.md Outdated Show resolved Hide resolved
@randomvariable
Copy link
Member

How many of the outstanding comments are blockers? Can we create issues and fast follow on improvements (e.g. swapping Calico for Weave)?

@detiber
Copy link
Member

detiber commented Oct 8, 2019

@randomvariable I'm good with issues and followups for the comments I raised.

@vincepri
Copy link
Member

We should target the release-0.2 branch for this PR and forward port the content once we have CAPD automation in place now that it has been merged on master

@dlipovetsky
Copy link
Contributor Author

Thanks everyone for your feedback! I got busy and wasn't able to look at the PR. Will look over comments now and will address minor things, and happy to file issues for major things, or where we don't have consensus right now.

@dlipovetsky dlipovetsky mentioned this pull request Oct 14, 2019
Closed
dlipovetsky
dlipovetsky commented Oct 14, 2019
View reviewed changes
Copy link
Contributor Author

@dlipovetsky dlipovetsky left a comment

Choose a reason for hiding this comment

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

no-op comment because GH thinks I'm leaving a review (on my own PR?) instead of leaving a comment...

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 14, 2019
This was referenced Oct 14, 2019
Closed
Closed
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 14, 2019
@dlipovetsky
Copy link
Contributor Author

Squashed and rebased on changes from #1318.

I removed the redundant "Prerequisites" section the top of the quick start, because the very same section appears at the top of the Installation doc, which is included in the quick start doc.

@dlipovetsky
Copy link
Contributor Author

Added the 📖 unicode symbol to the commit subject :-)

@dlipovetsky
Copy link
Contributor Author

Removed a line that got left during the rebase.

detiber
detiber reviewed Oct 15, 2019
View reviewed changes
Copy link
Member

@detiber detiber left a comment

Choose a reason for hiding this comment

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

This PR should likely be retargeted for release-0.2 since CAPD has been moved in-tree now.

@dlipovetsky
Copy link
Contributor Author

@detiber Won't a CAPD quick start also make sense in v1a3 (on master)?

@detiber
Copy link
Member

detiber commented Oct 15, 2019

@detiber Won't a CAPD quick start also make sense in v1a3 (on master)?

Yes, but without a release yet, I'm not sure what changes would be needed to make it work for master/v1alpha3.

@dlipovetsky dlipovetsky mentioned this pull request Oct 15, 2019
Closed
@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 15, 2019
@dlipovetsky dlipovetsky changed the base branch from master to release-0.2 October 15, 2019 21:40
@dlipovetsky
Copy link
Contributor Author

Rebased on release-0.2

@dlipovetsky
Copy link
Contributor Author

Argh, I pushed prior to switching the base... Now I need to clean up these extra commits, I guess

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 15, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chuckha, dlipovetsky

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

The pull request process is described here

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

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 15, 2019
@dlipovetsky
Copy link
Contributor Author

Nevermind, I had commits from master in my branch. Removed now.

@wfernandes
Copy link
Contributor

@dlipovetsky I was wondering what had happened. Thank you for this PR. I'm using it right now to try and quick start CAPD on my local mac machine.

@dlipovetsky
Copy link
Contributor Author

/hold Don't merge, again the "install infra provider" section for Docker is missing :(

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 15, 2019
@dlipovetsky
Copy link
Contributor Author

/hold cancel Fixed.

@randomvariable
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 15, 2019
@dlipovetsky
Copy link
Contributor Author

/hold cancel

(Didn't I cancel it already?)

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 16, 2019
@k8s-ci-robot k8s-ci-robot merged commit b518e2a into kubernetes-sigs:release-0.2 Oct 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ncdc ncdc ncdc left review comments

@chuckha chuckha chuckha approved these changes

@detiber detiber detiber left review comments

@randomvariable randomvariable randomvariable left review comments

@erwinvaneyk erwinvaneyk erwinvaneyk left review comments

@vincepri vincepri vincepri left review comments

Assignees

@detiber detiber

@randomvariable randomvariable

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@dlipovetsky @randomvariable @detiber @vincepri @k8s-ci-robot @wfernandes @ncdc @chuckha @erwinvaneyk