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

Update docs for contributing #697

Merged
merged 1 commit into from
Mar 21, 2019
Merged

Conversation

rramkumar1
Copy link
Contributor

@rramkumar1 rramkumar1 commented Mar 19, 2019

This PR organizes our contributor docs a bit better and removes lots of stale info. Specifically, there is now more information on:

  1. how to get your dev environment setup
  2. how to run tests and build
  3. how to run the controller locally to test changes.

There are also other misc. updates.

Ref: #557

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 19, 2019
@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 19, 2019
@rramkumar1 rramkumar1 changed the title Update docs for contributing [WIP] Update docs for contributing Mar 20, 2019
@rramkumar1
Copy link
Contributor Author

@spencerhance Please take a look and leave comments. Would be great if you could walk through some of the steps to make sure they make sense + work.

Copy link
Contributor

@spencerhance spencerhance left a comment

Choose a reason for hiding this comment

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

🙌 Only a few comments

docs/contrib/cluster-setup.md Outdated Show resolved Hide resolved
docs/deploy/gce/README.md Outdated Show resolved Hide resolved
docs/contrib/dev-setup.md Show resolved Hide resolved
Copy link
Member

@MrHohn MrHohn left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -29,14 +23,14 @@ under [core]/project is the one that contains your cluster. Second, ensure that
the current logged-in account listed under [core]/account is the owner of the project.
In other words, this account should have full project ownership permissions and be listed as
having the "Owner" role on the IAM page of your GCP project. You might ask why this
is needed? Well, the reason is that the script invokes a kubectl command which
is needed? Well, the reason is that our [script](../resources/gke-self-managed.sh) invokes a kubectl command which
Copy link
Member

Choose a reason for hiding this comment

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

Need to fix this link ../resources/gke-self-managed.sh

that comes by default:

```console
kubectl delete pod l7-lb-controller
Copy link
Member

Choose a reason for hiding this comment

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

Humm.. IIUC kubelet on the master will bring this pod back as long as the manifest still exists. We probably need something like

$ gcloud compute ssh e2e-test-zihongz-master
$ sudo rm /etc/kubernetes/manifests/glbc.manifest

```

Note that the above command automatically destroys any existing cluster that
was previously created with --up.
Copy link
Member

Choose a reason for hiding this comment

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

kubectl delete pod l7-lb-controller
```

This ensures we can run our own copy of the controller locally.
Copy link
Member

Choose a reason for hiding this comment

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

Would be great to link to docs/deploy/local/README.md?

project-id = foo-project
network-name = foo-network
subnetwork-name = foo-subnetwork
node-instance-prefix = gke-foo-cluster
Copy link
Member

Choose a reason for hiding this comment

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

Humm, what does it looks like in the e2e cluster case? Something like node-instance-prefix = e2e-test-zihongz?

Copy link
Member

@bowei bowei left a comment

Choose a reason for hiding this comment

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

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bowei, rramkumar1

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 merged commit 21fcc14 into kubernetes:master Mar 21, 2019
@rramkumar1
Copy link
Contributor Author

@MrHohn This got merged before I resolved your comments. Will open another PR with those fixes plus others. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants