HOWTO PR review

[chrislovecnm]

This is an issue where we can start outlining a document on howto PR review in kops. We have three different types of PR reviews

1. documentation
2. code
3. code with API changes

We should determine what documentation we already have on this from the community repo kubernetes/community.

Basic principles for the PR review

1. unit tests
2. integrations tests
3. go docs
4. go conventions (camelCase etc)
5. sprelling
6. What else do we need

@justinsb mentioned that APIs are SUPER important for review, as they are nasty to change later down the line. Once you have them you are stuck with them, until you do beta or ga APIs.

/cc @castrojo @parispittman @robertojrojas @rdrgmnzs

/assign @robinpercy @mikesplain

[chrislovecnm]

HOWTO handle a brand new contributor vs old salts contributor :)

Also, gratitude and empathy are key. Always! We are serving the community via PR reviews.

[mikesplain]

As Chris mentioned in chat, for non-members, wait/nudge on CLA before /ok-to-test.

[chrislovecnm]

1. proper labels
2. proper build queue labels

[parispittman]

What would be the difference in reviews for kops vs core/rest of projects? Is the methodology different? More/less checks?

[chrislovecnm]

@mikesplain mentioned the question how often do we follow up on PRs

[mikesplain]

As discussed, during dev mentoring meeting, how often to follow up on PRs.

We have fejta-bot which addresses PRs older than 90 days. The conclusion was we should probably follow up in a few weeks if there's been no response. There's obviously also a judgement if we see a stronger need for a given PR to follow up more aggressively (or less aggressively).

[chrislovecnm]

Also if they are a contributor or a review, follow-ups are useful as well. I have a bunch open, and sometimes forget to clean house, or finish the work :(

[mikesplain]

Ensure all new or modified files have proper licensing at the top.

[chrislovecnm]

@mikesplain that should be picked up by our linter which checks for the boilerplate.

I would modify the statement to all scripts must end with sh suffix so that our linger parses them.

[robinpercy]

Ensure all commits have been squashed to represent meaningful work, as per https://github.com/kubernetes/community/blob/e038c259b12e4cf6615ed57a2816d6da402a0d89/contributors/guide/github-workflow.md#7-create-a-pull-request

[chrislovecnm]

When a go dependency is updated or added it should be the only change in the PR. No other code changes should be included.

[chrislovecnm]

Common challenges that I see often with PR reviews.

1. edit markdown docs that are generated from cobra - we have tried to mitigate this with notes in the markdown, has not worked. CI catches this though. Maybe instructions with PR "hey here is how to run CI"?
2. update addons that are in https://github.com/kubernetes/kops/blob/master/upup/pkg/fi/cloudup/bootstrapchannelbuilder.go and do not update the version - @blakebarnett any idea on how to unit test this one?
3. make api changes and do not follow the process documented here: https://github.com/kubernetes/kops/blob/master/docs/development/api_updates.md - we need CI that validates that no api changes are needed.
4. CI fails and the user does not run CI locally, and or does not know how to diagnose the unit test failure. Searching for "FAIL:" in travis helps me. - notes in contribution guide? CI guide?
5. e2e fails and the user does not know where the log files for the master are located at, or how to comprehend the problems with e2e, i.e. reading the logs is a fun process to learn. @krzyzacy do we have documentation on how to debug e2e failures?
6. addons under addons folder are updated and the channels bootstrap file is not updated - we do not have any testing for these, and need a documented process how to manage our addons?

[chrislovecnm]

@parispittman Great question! Sorry I missed it.

We have much overlap with other PR reviews but here is a list of differences:

1. vendoring is different between subprojects. kubernetes/kubernetes uses godep, we use submodules, and maybe dep soon
2. docs are generated differently in kubernetes/kubernetes and this causes some confusion with new contributors with code generated docs.
3. CI is different with Travis runs, and e2e is sorta the same, but tests kops differently
4. our integration testing framework with terraform is unique, because how we are testing that install works, instead of testing that k8s works
5. API modifications and additions are far less formal in kops than other projects but are still reviewed with a lot of deliberate focus.

All of these difference add to how kops PR reviews are different, also common problems that kops contributors have are different as well. We run into the same problems where contributors do not run gofmt for instance, but yah there are differences.

[blakebarnett]

@chrislovecnm if I was going to work on it, I'd much rather move it out of the code, so that configuring versions is a simple config change that gets put in the state bucket.

[mikesplain]

I attempted to get a non-org member to /lgtm a pr since they had tested it.. and k8s-ci-robot had a conniption.. so we should document the right way for non org members to acknowledge they approve/reviewed. I think that would be the github pr review/approve but we should probably form consensus and document.

[chrislovecnm]

@mikesplain I think that is documented in regards to how the OWNERS files work ...

[robinpercy]

FYI I've created #4536 to address:

6. addons under addons folder are updated and the channels bootstrap file is not updated - we do not have any testing for these, and need a documented process how to manage our addons?

[chrislovecnm]

Here is what I just put on a PR when the person edited our generated docs:

---

You have ventured into our docs that are automatically generated from the code.

Here is what to do:

1. get the project building locally - see https://github.com/kubernetes/kops/blob/master/docs/development/building.md
2. edit the appropriate go file, in this case, https://github.com/kubernetes/kops/blob/master/cmd/kops/create_cluster.go#L164
3. run the command make gen-cli-docs
4. commit both the go file and the markdown file

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[mikesplain]

/remove-lifecycle stale

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[mikesplain]

/remove-lifecycle stale

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[fejta-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

[k8s-ci-robot]

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[mikesplain]

/reopen

[k8s-ci-robot]

@mikesplain: Reopened this issue.

In response to this:

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[fejta-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

[k8s-ci-robot]

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.