-
Notifications
You must be signed in to change notification settings - Fork 220
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 repository setup instructions #45
Conversation
repository-setup.md
Outdated
|
||
For triggering and running tests from github, the Kubeflow org uses Prow, K8s' continuous integration tool. | ||
|
||
Follow this [pull request](https://github.com/kubernetes/test-infra/pull/7313/files) as a guide for setting up Prow. Your pull request should: |
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.
Can you link to these instructions
https://github.com/kubeflow/testing#adding-an-e2e-test-for-a-new-repository
instead of the pull request
repository-setup.md
Outdated
|
||
Additionally make sure that the `k8s-ci-robot` is given write access to the repo and that an `OWNERS` file is added to the repository. The `OWNERS` file, like [this one](https://github.com/kubeflow/kubeflow/blob/master/OWNERS), will specify who can review and approve on this repo. | ||
|
||
## Setting up basic tests |
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.
I think this information should go in our testing repo
https://github.com/kubeflow/testing#adding-an-e2e-test-for-a-new-repository
repository-setup.md
Outdated
|
||
Finally to set up your repository, there are some configurations that should be made in GitHub. | ||
|
||
First, it's probably a good idea to protect your `master` branch to avoid accidently deleting or overwriting the code. Instructions on protection a branch can be found [here](https://help.github.com/articles/configuring-protected-branches/). Be sure not to enable `Require pull request reviews before merging`. This setting conflicts with Tide as seen in this [issue](https://github.com/kubeflow/tf-operator/issues/433). |
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.
Can you list which protections to add and not add?
I think we should note that we don't want to "require branch to be up to date" as this status check will become outdated as soon as another commit happens and force unnecessary resyncs.
Can you also list the options we want to check. I think these are currently
- Protect this branch
- Require status checks to pass
repository-setup.md
Outdated
|
||
This container will run workflows based on a `prow_config.yaml`. An example can be seen [here](https://github.com/kubeflow/kubeflow/blob/master/prow_config.yaml). Each workflow submits a ksonnet definition to the cluster, often in the form of an [Argo](https://github.com/argoproj/argo/blob/master/examples/README.md) workflow. | ||
|
||
An example of a basic argo workflow, created using ksonnet, can be found in this [pull request](https://github.com/kubeflow/pytorch-operator/pull/13/files). This workflow simply checks out your repository and provides prow with the artifacts needed to provide logs and determine the result of the test. Adding additional steps to the argo workflow will allow you to run tests using the checked out code. |
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.
Can you add a section about third party apps that we typically enable for repos; these should be
- Reviewable
- We should note that this should not be configured as a status check because this causes problems for prow
tide plugin should ignore Reviewable and possibly other status checks kubernetes/test-infra#7140
- We should note that this should not be configured as a status check because this causes problems for prow
- Travis
- Coveralls
- This should be configured with a threshold large enough that a small decrease in test coverage won't be reported as a status check failure and block automerge.
repository-setup.md
Outdated
- Add your respository as a presubmit and postsubmit | ||
- Add your pre and post submits to the test-grid | ||
|
||
Additionally make sure that the `k8s-ci-robot` is given write access to the repo and that an `OWNERS` file is added to the repository. The `OWNERS` file, like [this one](https://github.com/kubeflow/kubeflow/blob/master/OWNERS), will specify who can review and approve on this repo. |
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.
Can we have a section about repo permissions and note the following.
-
Teams not individual users should be used
-
Add the teams
ci-bots with write access (gives access to k8s-ci-robot)
core approvers write access -
additional teams as necessary
/uncc @ewilderj /cc @jlewi |
@jlewi Let me know if this looks better. I linked some updates the testing documentation as well |
This is great thanks /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jlewi 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 |
Adding some instructions on setting up a repository. Related to #31
This change is