Document Prow usage in KEP. #585
Conversation
jbarrick-mesosphere
requested review from
alenkacz,
gerred,
runyontr and
fabianbaier
| @@ -89,7 +89,7 @@ Go coverage reports will be used to ensure that changes do not reduce test cover | |||
|
|
|||
| #### Integration tests | |||
|
|
|||
| Integration tests test the KUDO controller in its entirety. These tests can be run against either a real Kubernetes cluster or a local control plane. Integration tests will be manually run after review, but prior to merging using the [Circle CI manual approval feature](https://circleci.com/docs/2.0/triggers/#manual-approval). Integration tests can be written using the test harness designed in KEP-0008. | |||
| Integration tests test the KUDO controller in its entirety. These tests can be run against either a real Kubernetes cluster or a local control plane. Integration tests will be manually run after review, but prior to merging using `/test` command supported by Prow. Integration tests can be written using the test harness designed in KEP-0008. | |||
There was a problem hiding this comment.
Isn't this automatic? The statement
Integration tests will be manually run after review, but prior to merging using
/testcommand supported by Prow.
Does not give this impression.
There was a problem hiding this comment.
For the most part yes, but the integration test jobs we want to manually trigger on the PR as needed (and run on master). We can configure Prow this which, which is what I'm talking about in this paragraph.
There was a problem hiding this comment.
Oops, actually, it's the E2E tests that should be explicitly run.. will update.
|
|
||
| #### Pull Requests | ||
|
|
||
| All Pull Requests into master need to have the following checks pass. These should be ordered in fastest to slowest to reduce the time spent when/if failures occur. | ||
|
|
||
| 0. Check author has signed CLA | ||
| 1. If the user is not a contributor, a contributor must write `/ok-to-test` on the pull request before it will be triggered. |
There was a problem hiding this comment.
^ - Kudo community / contributors might want to pitch in on this.
cc: @gerred
There was a problem hiding this comment.
I believe this is a security requirement, as otherwise anyone can run whatever code they want to in our Kubernetes cluster just by opening a pull request.
There was a problem hiding this comment.
This is usually the case for large projects and when tests are run against physical infrastructure, such as launching aws machines.
For most open source projects. I have seen the tests are the automatically triggered, and situation is optimized for immediate feedback without human intervention.
I just thought that kudo contributors might have an some views/thoughts on this behavior.
keps/0014-pull-request-process.md
Outdated
|
|
||
| ## Proposal | ||
|
|
||
| A Prow cluster will be deployed in Kubernetes for running CI and PR/issue automation. This cluster lives at https://prow.toolsinfra.mesosphe.re/ and will take action on Github as the `ci-mergebot` user. KEP-0004 outlines the infrastructure required. |
There was a problem hiding this comment.
Could this be an isolated KUDO Community resource of Prow and not attached to Mesosphe.re? As discussed here I think it would make more sense to make it isolated. https://www.youtube.com/watch?v=eW0qfhEVTTY&feature=youtu.be&t=1999
There was a problem hiding this comment.
So here's what I'm thinking:
- Let's run it at https://prow.kudo.dev/.
- It will be for the
kudobuilder/organization only. - Run it on GKE.
- The cluster configuration is open source and lives in
kudobuilder/test-infra.
|
|
||
| Users listed in `approvers` are allowed to approve a pull request. Users listed in `reviewers` are eligible for being automatically requested as reviewers by Prow on new pull requests (Prow will select two users from the list at random). | ||
|
|
||
| Prow will look for `OWNERS` files covering the directories being changed, e.g., the root `OWNERS` file applies to all changes, but `./tests/OWNERS` only applies to changes that in `./tests/`. |
There was a problem hiding this comment.
Whats the difference between OWNERS and/or the one we have already as CODEOWNERS ? https://github.com/kudobuilder/kudo/blob/master/.github/CODEOWNERS
There was a problem hiding this comment.
OWNERS is understood by Prow, whereas CODEOWNERS is not
|
|
||
| ### Test environment | ||
|
|
||
| Tests and builds run in Prow which runs jobs as pods in Kubernetes. The pods are created with privileged mode enabled, so are able to use docker-in-docker to build, push, and run Docker images, and will have necessary credentials mounted in to support creating Kubernetes clusters in GCP and AWS. |
There was a problem hiding this comment.
Will there be labels or options to determine on which platforms to run certain PRs or tests?
jbarrick-mesosphere
force-pushed
the
prow
branch
from
f0b1efd
to
ef73297
Compare
| * `/cc`: request review from a specific user. | ||
| * `/hold`: prevent a pull request from being merged. | ||
|
|
||
| Once two contributors have marked `/approve` (or `/lgtm`, which implies `/approve`) and one types `/lgtm` and all tests pass, a pull request will be automatically merged. |
There was a problem hiding this comment.
- I don't get the diff between approve and lgtm... they seem basically the same... why have 2? generally when using GH people will "approve" with "lgtm" comment. It isn't clear what we are trying to achieve.
There was a problem hiding this comment.
Regarding this sentence. It seems implied (which we should make explicit) that the PR does not have a merge conflict.
There was a problem hiding this comment.
Good question, I'll have to do some digging to see what the rationale is for it to work that way. My guess would be that allows someone to differentiate between "I think this is good code" and "I think this is ready to be merged as soon as the tests pass" - not all approvers may want to pull that trigger.
There was a problem hiding this comment.
I'm not finding their rationale, but this is configurable so if we'd rather just have to /approve we can do that instead.
|
Let's merge this and we can update it with the nuances of our process as we work out how we want it to work. |
What type of PR is this?
/kind kep
What this PR does / why we need it:
Document how we will use Prow to manage PRs and issues.
Does this PR introduce a user-facing change?: