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

Added more detailed instructions on how to setup the repo #1114

Merged
merged 3 commits into from Aug 14, 2017

Conversation

@crmejia
Copy link
Contributor

@crmejia crmejia commented Aug 9, 2017

This change adds instructions to setup the repo locally. On my first experience with the code base I lost a lot of time running a single test through docker. I was unable to run go test ./... because I had naively cloned my fork. So I did some research and found the proper way to set up the repo in the k8s dev guide So I decided to add those instructions to help people jumping in the code.

PS. I copied most of the words from those instructions and referenced the guide. Is this OK or do I need to paraphrase?

git clone https://github.com/$user/service-catalog.git
# or: git clone git@github.com:$user/service-catalog.git
cd $working_dir/service-catalog

This comment has been minimized.

@duglin

duglin Aug 9, 2017
Contributor

nit: technically it could just be cd service-catalog

The Service Catalog github repository can be found
[here](https://github.com/kubernetes-incubator/service-catalog.git).
### 1 Fork in the Cloud
1. Visit Visit https://github.com/kubernetes-incubator/service-catalog

This comment has been minimized.

@duglin

duglin Aug 9, 2017
Contributor

s/Visit Visit/Visit/

@@ -88,14 +88,50 @@ also need:
a Kubernetes cluster. As such, our build process only supports compilation of
linux/amd64 binaries suitable for execution within a Docker container.

## Cloning the Repo
## Workflow
We can set up the repo by following a process similar to the [dev guide for k8s]( https://github.com/kubernetes/community/blob/master/contributors/devel/development.md#1-fork-in-the-cloud)

This comment has been minimized.

@duglin

duglin Aug 9, 2017
Contributor

it'd be nice to wrap this at 80 cols.

This comment has been minimized.

@crmejia

crmejia Aug 10, 2017
Author Contributor

I think this one wraps at exactly 80 but the markup doesn't


$ git clone https://github.com/kubernetes-incubator/service-catalog.git
Per Go's workspace instructions, place Service Catalog's code on your GOPATH using the following cloning procedure.

This comment has been minimized.

@duglin

duglin Aug 9, 2017
Contributor

wrap at 80 please

@duglin
Copy link
Contributor

@duglin duglin commented Aug 9, 2017

CI failure looks real:
./docs/devguide.md: Can't find section '#cloning-the-repo' in ./docs/devguide.md

Overall, it SGTM, just a few nits.

crmejia added 2 commits Aug 10, 2017
@crmejia
Copy link
Contributor Author

@crmejia crmejia commented Aug 10, 2017

Fixed the issues.

@duglin Is it OK that I copied part of the k8s dev guide?

@duglin
Copy link
Contributor

@duglin duglin commented Aug 10, 2017

I don't see why not, but if it reads ok just having a pointer to the kube docs (so we don't duplicate or get our of sync) would be good too.

@crmejia
Copy link
Contributor Author

@crmejia crmejia commented Aug 13, 2017

I see your point and agree it works either way. Also, I think it won't be out of sync since these are standard git workflow instructions, so having them there saves a click.

Copy link
Contributor

@arschles arschles left a comment

LGTM. Thanks @crmejia for this contribution!

@arschles arschles added the LGTM1 label Aug 14, 2017
@vaikas vaikas added the LGTM2 label Aug 14, 2017
@vaikas vaikas merged commit 4e642d5 into kubernetes-sigs:master Aug 14, 2017
3 checks passed
3 checks passed
@thelinuxfoundation
cla/linuxfoundation crmejia authorized
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@service-catalog-jenkins
service-catalog-PR-testing2 Successful presubmits. Details at https://service-catalog-jenkins.appspot.com/job/service-catalog-PR-testing2/1760/.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants