Update Development Automation and Pull Request Process #1016

xiangpengzhao · 2017-09-04T04:11:04Z

fixes: #834

cblecker · 2017-09-04T18:48:06Z

contributors/devel/automation.md

-     * Jenkins unit/integration
+  * The PR must have the label `cncf-cla: yes` or `cla: human-approved`
+  * The PR must be mergeable, aka cannot need a rebase
+  * All of the [Required Github CI Tests](https://github.com/kubernetes/test-infra/blob/master/mungegithub/submit-queue/deployment/kubernetes/configmap.yaml#L15) must be green


This list is available in a more user friendly version at https://submit-queue.k8s.io/#/info on the Merge Requirements tab. That tab is also dynamically updated, so perhaps it's a better source for this list?

cblecker · 2017-09-04T18:50:20Z

contributors/devel/automation.md


 ### Merge process

 Merges _only_ occur when the [critical builds](http://submit-queue.k8s.io/#/e2e)


There is no e2e tab anymore and no such thing as critical builds. All the ci builds are non-blocking.

cblecker · 2017-09-04T18:50:49Z

contributors/devel/automation.md

 If the PR has the `retest-not-required` label, it is simply merged. If the PR does
-not have this label the e2e, unit/integration, and node  tests are re-run. If these
-tests pass a second time, the PR will be merged as long as the `critical builds` are
+not have this label, the [Required Github CI Tests](https://github.com/kubernetes/test-infra/blob/master/mungegithub/submit-queue/deployment/kubernetes/configmap.yaml#L15) 


Same as above, the list of retests needed is on the merge requirements page

cblecker · 2017-09-04T18:51:54Z

contributors/devel/automation.md


-This runs repeatedly over github pulls and issues and runs modular "mungers"
-similar to "mungedocs." The mungers include the 'submit-queue' referenced above along
+This runs repeatedly over github pulls and issues and runs modular `mungers`


single backticks are usually for commands/labels.. "mungers" should probably stay double quoted

cblecker · 2017-09-04T18:52:30Z

contributors/devel/automation.md


 ## Github Munger

 We run [github "mungers"](https://github.com/kubernetes/contrib/tree/master/mungegithub).


This doesn't talk about prow and the difference between mungegithub and prow.

Also the link is wrong.

cblecker · 2017-09-04T18:54:04Z

contributors/devel/pull-requests.md

    1. No changes made since last `lgtm` label applied
 1. If tests fail, resolve issues by pushing edits to your PR branch
-1. If the failure is a flake, a member can comment `@k8s-bot [e2e|unit] test this issue: #<flake issue>`
+1. If the failure is a flake, anyone can comment `/retest`


This isn't true if the PR hasn't been marked okay (e.g. a single test run was started with /test all or by /lgtm, without the PR itself being marked trusted).

bgrant0607 · 2017-09-06T07:12:27Z

cc @kubernetes/sig-testing-pr-reviews @kubernetes/sig-contributor-experience-pr-reviews

xiangpengzhao · 2017-09-07T07:09:56Z

@cblecker Most of the review comments addressed. PTAL. Thanks!

Please note that I didn't address below comment since I think this is a somehow big topic and I'm not sure how to talk about it now :) Hope other folks can do it in the near future.

This doesn't talk about prow and the difference between mungegithub and prow.

cblecker · 2017-09-07T17:19:43Z

contributors/devel/automation.md

-with that message.  ("please" is optional, but remember to treat your robots with
-kindness...)
+Before a PR from an unknown user is run, the PR builder bot (`k8s-ci-robot`) asks to
+a message from a kubernetes member that a PR is `ok-to-test`, the member replies


"kubernetes member that a PR is safe to test. If it is, the member can reply with the /ok-to-test command on a single line to begin CI testing."

cblecker · 2017-09-07T17:20:39Z

contributors/devel/automation.md

+  * The PR must have the `lgtm` label. The `lgtm` label is automatically applied
+    following a review comment consisting of `/lgtm` (case-insensitive) in a single line
+  * The PR must not have been updated since the `lgtm` label was applied
+  * The PR must have the `approved` label. The `approved` label is automatically applied after all approvers have approved the PR 


nit: line length should wrap at 80 chars

cblecker · 2017-09-07T17:20:43Z

contributors/devel/automation.md

+  * The PR must not have been updated since the `lgtm` label was applied
+  * The PR must have the `approved` label. The `approved` label is automatically applied after all approvers have approved the PR 
+    by a comment consisting of `/approve` (case-insensitive) in a single line
+  * The PR must not have the `do-not-merge` label or any of other `do-not-merge/*` labels, e.g., `do-not-merge/release-note-label-needed`


nit: line length should wrap at 80 chars

cblecker · 2017-09-07T17:21:37Z

contributors/devel/automation.md

-the issue number you found or filed.
+during the original test. It would be good to file flakes as an
+[issue](https://github.com/kubernetes/kubernetes/issues?q=is%3Aopen+is%3Aissue+label%3Akind%2Fflake). 
+k8s-ci-robot will comment to tell you which test(s) failed and how to re-test. 


nit: @k8s-ci-robot

cblecker · 2017-09-07T17:23:45Z

contributors/devel/pull-requests.md

 ## The Testing and Merge Workflow

-The Kubernetes merge workflow uses comments to run tests and labels for merging PRs. NOTE: For pull requests that are in progress but not ready for review, prefix the PR title with "WIP" and track any remaining TODOs in a checklist in the pull request description.
+The Kubernetes merge workflow uses comments to run tests and labels for merging PRs. NOTE: For pull requests that are in progress but not ready for review, prefix the PR title with `WIP` and track any remaining TODOs in a checklist in the pull request description.


nit: 80 characters. also I think it's [WIP] isn't it?

xiangpengzhao · 2017-09-08T06:25:49Z

contributors/devel/pull-requests.md

-The Kubernetes merge workflow uses comments to run tests and labels for merging PRs. NOTE: For pull requests that are in progress but not ready for review, prefix the PR title with `WIP` and track any remaining TODOs in a checklist in the pull request description.
+The Kubernetes merge workflow uses comments to run tests and labels for merging PRs.
+NOTE: For pull requests that are in progress but not ready for review,
+prefix the PR title with `WIP` or `[WIP]` and track any remaining TODOs


also I think it's [WIP] isn't it?

@cblecker we allow prefix with both WIP and [WIP].

https://github.com/kubernetes/test-infra/blob/master/prow/plugins/wip/wip-label.go#L40

xiangpengzhao · 2017-09-08T06:26:20Z

@cblecker PTAL. Thanks!

cblecker

I'm good with this (you can squash from addressing my comments). I'd like one more person to give it a look over before merge.

xiangpengzhao · 2017-09-09T02:12:49Z

@cblecker squashed :)

spiffxp

/lgtm

The "PR is considered ready for merging" description is likely to fall out of date. It also makes some assumptions about prow and mungegithub's submit-queue being active for a given repo and configured a certain way. So most of this only applies to e.g.: kubernetes/kubernetes. But this is much better than what was there before.

k8s-github-robot · 2017-09-20T16:26:38Z

Automatic merge from submit-queue. .

xiangpengzhao · 2017-09-20T16:38:19Z

Thanks @spiffxp ! Yes the document can be better and should be kept fresh each time when we update the process and/or the bots' behaviors :)

spiffxp · 2017-09-20T21:57:17Z

Sorry... didn't really complete my thought above. Linking directly to the submit queue's merge requirements would be more ideal. The docs are generated based on which flags are enabled/disabled, and they're close to the code that implements the logic they describe, so they're more likely to be up to date.

That said, only a few repos have a submit queue on them, and in ways I can't articulate right now, they submit queue's docs appear less clear than the docs here. So there's room for improvement over there before we just link directly to eg: https://community.submit-queue.k8s.io/#/info

xiangpengzhao · 2017-09-21T06:50:16Z

@spiffxp #1110 sent. PTAL. Thanks!

@spiffxp

Automatic merge from submit-queue. . Update merge requirements in doc fixes: #1016 (comment) /cc @spiffxp

k8s-ci-robot requested review from bgrant0607 and spiffxp September 4, 2017 04:11

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 4, 2017

k8s-github-robot assigned cblecker and calebamiles Sep 4, 2017

cblecker requested changes Sep 4, 2017

View reviewed changes

cblecker requested a review from spxtr September 4, 2017 18:54

k8s-ci-robot added sig/testing Categorizes an issue or PR as relevant to SIG Testing. sig/contributor-experience Categorizes an issue or PR as relevant to SIG Contributor Experience. labels Sep 6, 2017

cblecker reviewed Sep 7, 2017

View reviewed changes

xiangpengzhao force-pushed the fix-doc branch from a2d04e5 to ab71413 Compare September 8, 2017 06:20

xiangpengzhao commented Sep 8, 2017

View reviewed changes

cblecker approved these changes Sep 8, 2017

View reviewed changes

xiangpengzhao force-pushed the fix-doc branch from ab71413 to 629d174 Compare September 9, 2017 02:04

Update Development Automation and Pull Request Process

629d174

k8s-ci-robot assigned spiffxp Sep 20, 2017

spiffxp approved these changes Sep 20, 2017

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 20, 2017

k8s-github-robot merged commit 608f341 into kubernetes:master Sep 20, 2017

xiangpengzhao deleted the fix-doc branch September 20, 2017 16:34

xiangpengzhao mentioned this pull request Sep 21, 2017

Update merge requirements in doc #1110

Merged

k8s-github-robot pushed a commit that referenced this pull request Sep 22, 2017

Merge pull request #1110 from xiangpengzhao/update-doc

d196a4f

Automatic merge from submit-queue. . Update merge requirements in doc fixes: #1016 (comment) /cc @spiffxp

xiangpengzhao mentioned this pull request Nov 7, 2017

Documentation outdated and confusing #1334

Closed

danehans pushed a commit to danehans/community that referenced this pull request Jul 18, 2023

Add gssjl2008 to members (kubernetes#1016)

271ab97


		### Merge process

		Merges _only_ occur when the [critical builds](http://submit-queue.k8s.io/#/e2e)


		## Github Munger

		We run [github "mungers"](https://github.com/kubernetes/contrib/tree/master/mungegithub).

Uh oh!

Update Development Automation and Pull Request Process #1016

Update Development Automation and Pull Request Process #1016

Uh oh!

Conversation

xiangpengzhao commented Sep 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bgrant0607 commented Sep 6, 2017

Uh oh!

xiangpengzhao commented Sep 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xiangpengzhao commented Sep 8, 2017

Uh oh!

cblecker left a comment

Choose a reason for hiding this comment

Uh oh!

xiangpengzhao commented Sep 9, 2017

Uh oh!

spiffxp left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-github-robot commented Sep 20, 2017

Uh oh!

xiangpengzhao commented Sep 20, 2017

Uh oh!

spiffxp commented Sep 20, 2017

Uh oh!

xiangpengzhao commented Sep 21, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants