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

Add more descriptive steps in Dev Documentation #1065

Merged
merged 3 commits into from
Aug 4, 2017

Conversation

diazjf
Copy link

@diazjf diazjf commented Aug 3, 2017

Adds more descriptive steps in the Development Documentation,
like more information on obtaining dependencies, building, and
deploying an image of the ingress controller. Also adds more
descriptive information on deploying as well as some fixes
on grammar and spelling, and some file renames.

Adds more descriptive steps in the Development Documentation,
like more information on obtaining dependencies, building, and
deploying an image of the ingress controller. Also adds more
descriptive information on deploying as well as some fixes
on grammar and spelling.
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 3, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 43.967% when pulling 86b52fa on diazjf:master into 1045e43 on kubernetes:master.

* [Build, test or release](releases.md) an existing controller
* [Setup a cluster](setup.md) to hack at an existing controller
* [Write your own](devel.md) controller
* [Build, test, release](getting_started.md) an existing controller
Copy link
Contributor

Choose a reason for hiding this comment

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

Would prefer hyphens than underscores - like the main docs (eg https://github.com/kubernetes/kubernetes.github.io/tree/master/docs).

Copy link
Author

Choose a reason for hiding this comment

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

Done

must be installed before building a binary/image. Occasionally, you
might need to update the dependencies.

This guide requires you to install the[godep](https://github.com/tools/godep)dependency
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing spaces.

Copy link
Author

Choose a reason for hiding this comment

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

Done

$ godep save ./...
```

In general, you can follow [this guide](https://github.com/kubernetes/kubernetes/blob/release-1.5/docs/devel/godep.md#using-godep-to-manage-dependencies)to update dependencies.
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing space.

Copy link
Author

Choose a reason for hiding this comment

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

Done

or push an image to a remote repository.

In order to use your local Docker, you may need to set the following environment variables:
* export TAG=0.0 # or whatever you want the version to be named
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer this TAG to be set in each command, vs exported once.

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense in case you want different images

or push an image to a remote repository.

In order to use your local Docker, you may need to set the following environment variables:
* export TAG=0.0 # or whatever you want the version to be named
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 wrap these exports in a code block please?

Copy link
Author

Choose a reason for hiding this comment

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

Done

In order to use your local Docker, you may need to set the following environment variables:
* export TAG=0.0 # or whatever you want the version to be named
* export DOCKER=docker
* export REGISTRY=index.docker.io
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a fan of opinionating on a registry here, could you just show all available options (or keep to the defaults)?

Copy link
Author

Choose a reason for hiding this comment

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

The default is set to gcr.io/google_containers which can cause a problem for first time developers or anyone not using gcloud. I can set it to something more like .

@@ -1,9 +1,10 @@
# Developer setup
# Cluster getting Started
Copy link
Contributor

Choose a reason for hiding this comment

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

Capitalize

Copy link
Author

Choose a reason for hiding this comment

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

Done

3. [Build the image](../../../docs/dev/getting_started.md)
4. Create the [default-backend](default-backend.yaml):
```console
$ kubectl apply -f default-backend.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

This is already done in the top of this readme.

Copy link
Author

Choose a reason for hiding this comment

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

Good catch

@@ -13,7 +13,7 @@ a 404 page.

## Controller

The Nginx Ingress Controller uses nginx (surprisingly!) to loadbalance requests that are coming to
The Nginx Ingress Controller uses nginx (surprisingly!) to loadbalancer requests that are coming to
Copy link
Contributor

Choose a reason for hiding this comment

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

"load balance" or "loadbalance" is the right term to use here.

Copy link
Author

Choose a reason for hiding this comment

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

My bad on this one

Further cleans up the development documentation with spacing and
more readable text.
@diazjf
Copy link
Author

diazjf commented Aug 3, 2017

@tonglil thanks for the review. I have added the changes you requested.
see ef499aa

@coveralls
Copy link

Coverage Status

Coverage remained the same at 43.994% when pulling ef499aa on diazjf:master into 1045e43 on kubernetes:master.

Copy link
Contributor

@tonglil tonglil left a comment

Choose a reason for hiding this comment

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

Thanks for the changes!


```console
$ export DOCKER=docker
$ export REGISTRY=<your-docker-registry>
Copy link
Contributor

Choose a reason for hiding this comment

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

How about this as a tradeoff, list common options like:

# "gcloud docker" (default) or "docker"
$ export DOCKER=<docker>

# "gcr.io/google_containers" (default), "index.docker.io", or your own registry
$ export REGISTRY=<registry>

Copy link
Author

Choose a reason for hiding this comment

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

Perfect, updated!

Adds common options to the environment variables for docker, and
registry.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 43.994% when pulling 827658a on diazjf:master into 1045e43 on kubernetes:master.

@aledbf aledbf self-assigned this Aug 4, 2017
@aledbf
Copy link
Member

aledbf commented Aug 4, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 4, 2017
@aledbf
Copy link
Member

aledbf commented Aug 4, 2017

@diazjf thanks!

@aledbf aledbf merged commit eccbe2e into kubernetes:master Aug 4, 2017
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants