-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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 Contribution 101 #1031
Add Contribution 101 #1031
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty good. I left some comments.
@@ -13,14 +13,77 @@ Please fill out either the individual or corporate Contributor License Agreement | |||
|
|||
Follow either of the two links above to access the appropriate CLA and instructions for how to sign and return it. Once we receive it, we'll be able to accept your pull requests. | |||
|
|||
## Composition of This Repository and Where/How to Contribute | |||
|
|||
The repository of the Kubernetes Python client consists of this main repository and a submodule, the [python-base](https://github.com/kubernetes-client/python-base) repository. The main repository contains mostly files that are generated by the OpenAPI generator from [this OpenAPI spec](scripts/swagger.json). The base repo is the utility part of the python client and allows developers to create their own kubernetes clients. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"The repository of the Kubernetes Python client consists of this main repository" might be better as "The Kubernetes Python client consists of this main repository, the python"
Also in the last sentence, by "base repo" I think you mean python-base which you refer to above as "submodule". It might be clearer to someone new if you keep the terms consistent.
|
||
### Where to Submit Your Patch | ||
|
||
These folders are automatically generated. You will need to submit a patch to the upstream kubernetes repo [kubernetes](https://github.com/kubernetes/kubernetes) or the OpenAPI generator repo [openapi-generator](https://github.com/OpenAPITools/openapi-generator). This contains: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you clarify "These folders" -- I think "The following folders are automatically generated" would work, and then you can remove the "This contains:" part.
Minor nitpick, but "kubernetes repo" should be uppercased.
|
||
### Windows Developers | ||
|
||
The symbolic links contained in this repo does not work for Windows operating systems. If you are a Windows developer, please run the [fix](scripts/windows-setup-fix.bat) inside the scripts folder or manually copy the content of the [kubernetes/base](https://github.com/kubernetes-client/python-base) folder into the [kubernetes](kubernetes) folder. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor nitpick: "does not work" should be "do not work".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! All other comments look good to me, but the kubernetes/base folder is what the Windows development sees after they clone the repo. This part is unchanged.
|
||
### Writing Tests | ||
|
||
In addition to running the your fix yourself and tell us that your fix works, you can demonstrate that your fix really works by using unit tests and end to end tests. These unit tests are mainly located in three places. You should put your tests into the places that they fit in. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: "In addition to running the your fix yourself and tell us..."
Suggestion: "In addition to running the fix yourself, you can demonstrate that your fix really works by using unit tests and end to end tests. Tests are mainly located in three places: "
|
||
If you write a new end to end (e2e) test, or change behaviors that affect e2e tests, you should set up a local cluster and test them on your machine. The following steps will help you run the unit tests. | ||
|
||
1. Acquire a local cluster. [Minikube](https://github.com/kubernetes/minikube) is a good choice for Windows and Linux developers. Alternatively if you are on Linux, you can clone the [kubernetes](https://github.com/kubernetes/kubernetes) and run [install-etcd.sh](https://github.com/kubernetes/kubernetes/blob/master/hack/install-etcd.sh) and then [local-up-cluster.sh](https://github.com/kubernetes/kubernetes/blob/master/hack/local-up-cluster.sh) to get a local cluster up and running. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where it says, "you can clone the kubernetes and..." I think you forgot to add "repo" at the end.
|
||
## Update the Base Submodule in the Main Repo After Your python-base PR Is Merged | ||
|
||
Your contribution of the base repo will not be automatically reflected in the main repo after your PR is merged. Instead, please update the ```base``` submodule in your fork of the main repo as follows: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Your contribution of the base repo" -> "Your contribution to the base repo"
This is awesome, it adds a lot of context and info! Added some comments. |
/lgtm Feel free to address comments from @scottilee and add more context in a followup PR. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: micw523, roycaihw 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 |
Fixes #266
Fixes #1026
Fixes #910
Adds contribution 101.
Progress: