I’ve pushed up a Kubernetes change for review… now what?
Updated V2 – 6/8/2017
I’m assuming you’ve already gone to the Community page on contributing, for information on how to find issues to work on, how to build and test Kubernetes, signing the Contributors License Agreement (CLA), and followed the link on how to do a pull request.
You code is up there, ready for review. Now what?
How do you know who is reviewing the code, and what the exact steps are?
What if it is not getting reviewed?
Hopefully, the notes here will help…
By the way…
If it’s your first commit, I highly recommend doing something super simple, so that you can get the process down, and not have something technically challenging as well. Using labels, you can look for low-hanging-fruit issues or issues for new contributors.
Pull Request Posted…Now What?
Once you’ve forked the Kubernetes repo, pushed your changes up to your repo, clicked on the “Compare and Pull Request”, and filled out the pull request, you’ll see several things happen.
The k8s-robot will take some actions to make sure that you have a signed CLA…
And that the commit needs an OK to be tested…
The k8s-reviewer bot may leave a comment indicating that the PR is “reviewable” over at reviewable.kubernetes.io (at time of this posting, this seems to be enabled only occasionally):
Lastly, the bot will assign a reviewer and provide some useful info…
Note that it indicates the OWNERS file. You can go to that file and see the approvers, and depending on the file, reviewers that could review the commit.
What do you do, if the reviewer doesn’t respond to the review?
From the kubernetes-dev Google group, Erick Fejta gave three suggestions. Here’s some elaboration on them…
Un-assign the current reviewer
To do this, you can add a comment for the review that looks like this:
Now, I did this, but forgot the ‘@’ symbol. It didn’t do anything, and folks pointed out that going back and editing the comment to add the at-sign would not help, because the bot only looks at new comments. Whoops.
Use the slack channel
- Sign up for the Kubernetes “Team”, by going to slack.kubernetes.io. Once done, you can login at kubernetes.slack.com.
- Find the channel for the code you are changing. In my case, I had changed code in k8s.io/apimachinery/pkg/… so I used the sig-api-machinery channel.
- Ask on the channel for a recommendation of reviewers
- Use the /assign comment (again with an at-sign before the user name(s) to assign the review to people recommended).
You can also look for the reviewer in Slack, and when they are available, touch base with them to see if they have bandwidth to review. Thats what I did in one case, and the person indicated they were wicked busy.
Use the Owners file
Above I showed that the bot indicated the OWNERS file. For one of my commits, it had this OWNERS file for federation with a bunch of reviewers. Some, like the apimachinery one, may only have approvers shown (which I guess double as reviewers).
Now, you could randomly assign from that list, but a better method it to take into consideration the work load of the reviewers. To do that, go to the Pull Request Dashboard and specify the reviewer’s name. You’ll get a page that has this right below the page banner, along with a list of the reviews the person is working on:
As you can see, ‘lavalamp’ was pretty busy, at the time of this review. You can enter other people’s names from the OWNERS file, into the text field and see how busy they are to make a better decision. I suspect you could even try to ping them on Slack to see if they have time to review. Also, you can click on “Me” and see your outgoing commits/reviews.
How To House Train Your Bot
There is info on the various bot commands, what they do, and who can use them. For example, I had a case where I put NONE in the release note section of the PR form, instead of “`NONE“`, and the bot had labeled my PR as needing a release note. I did the following, and the bot then removed the incorrect label and added the correct one:
No CNCF-CLA label?
I had one commit, where the k8s-ci-robot didn’t add in the cncf-cla label, like it should have. Here’s what you’d normally see:
In this commit, I had forgotten to add the “fixed #12345” in the first commit comment to indicate the issue fixed, but from what I hear, this should not affect the labelling, which should use the author and email address. I checked against other commits, and the info was the same.
I’m not sure why the bot didn’t add the label (even with a “no” instead of “yes”). Erick Fejta tried closing and reopening the PR, but that didn’t seem to work.
I did a rebase with Kubernetes master and then pushed up the code again, and this time, the bot added the label for “cncf-cla: yes”. One thing I had done differently, was that I was using a new VM for local development and in that VM, I had the remote for my kubernetes repo set up using https: protocol. I had, however, set up github to use SSH keys (forgot about that, when setting up the VM).
I changed the remote to use git: protocol, like this:
git remote set-url origin git@github.com:pmichali/kubernetes.git
I then re-pushed the change to my github on the same branch (with the -f option to force). The PR got the new commit, and the bot added the label!
Label Me Purple
There is a way to add labels to an issue. I saw the following done by a contributor (can be done by anyone):
As you can see, it added a label to the issue. There is also a “/area” bot command, which also creates a label. You can see the available labels.
Can I Kick Off Testing?
Well, it depends… If you are a Kubernetes member, you can do a “@k8s-bot ok to test” comment. For newbies, you won’t be a member, and will need to wait for a member to do this command to enable testing of the pull request.
Granted, if one of your tests fail, you can use the directions in the bot comment to resubmit that specific test. For example, I had a test failure reported and I just cut and pasted in the mentioned “@k8s-bot pull-kubernetes-federation-e2e-gce test this” to re-check this, once the failure (a flake) was fixed.
There is also a “/retest” comment that can be entered to have all the tests re-run.
See my blob entry on test infra tools for info on looking at test results, checking on the health of test jobs, etc.
What If My Code Is Not Ready For Review?
If you want to create a PR of some work-in-progress code, so people can see your changes, but are not ready for review, you can add the prefix “WIP” to the subject.
Looks Good To Me
Once the reviewer is happy with your changes, they’ll mark it as lgtm:
If an approver is not already assigned you, or the reviewer will want to assign them (you can refer to the OWNERS list):
Once they approve, you’ll see a bunch of bot activity to invoke the tests and complete the process, merging in the PR(assuming all things go well):
It’ll also provide a button to allow you to remove your branch, as it is no longer needed:
What’s Reviewable?
I noticed that some people go to the”Files changed” tab, and then add comments to the code. Other’s click on the Reviewable button on the page:
This takes you to a page, where you can review the code, add comments, acknowledge changes, indicate done, see what files have been reviewed and by whom, and see all the revisions. I’m still trying to figure out all the ins and outs of this tool, and will try to add notes here. TODO
Note, however, not all PRs will have the “reviewable” button (I’m not sure it is enabled all the time – maybe its use is in beta?).
One thing I see at the bottom of the page at the reviewable.kubernetes.io page for my commit is:
The last link indicates to open an issue with the Kubernetes repo to connect it to Reviewable. Not sure if I should make that request. Maybe someone can comment.