Requests are one of the staples for collaboration in the OBS. You can review via the webui or with OSC.

WebUI

Lets take the request listing for openSUSE:Factory. Your normal workflow will probably end up as

1. middle mouse click on the little magnifying glass icon on the right.
2. review the request in a new tab
3. close the new tab
4. go back step 1

My issues with the WebUI

1. no advancing to the next request in my current list.
2. I have to manually unfold/fold many diff chunks for a proper review.
3. In the latest version of the WebUI: “We truncated the diff of some files because they were too big. If you want to see the full diff for every file, click here.” But even then I have to unfold every change myself again.

OSC - The normal way

1. one terminal: osc rq list -t submit -s new openSUSE:Factory
2. 2nd terminal: osc rq show -d ID - the ID is taken from the first listing.
3. 2nd or in worst case 3rd terminal: osc rq youraction ID
4. go back to step 2

My issues

1. all manually copy pasting of IDs

OSC interactive mode

My config:

# ~/.oscrc or ~/.config/osc/oscrc
# in [general section]
# go interactive by default
request_show_interactive = 1
# relevant for "osc review list -G groupname projectname"
# then we wont get asked for which review we want to do on every request
review_inherit_group = 1
# Only care about "incoming" requests
include_request_from_project = 0
# always show the build status
request_show_source_buildstatus=1

If we run osc now:

$ osc rq list -t submit -s review openSUSE:Factory
Request: #580596

  submit:       shells/zsh@184 -> openSUSE:Factory


Message:
<no message>

State:   review     2018-02-27T13:09:16 factory-auto
Comment: Please review build success

Review:  new        User: repo-checker                                 2018-02-27T13:09:14 factory-auto
  Please review build success
         new        Group: opensuse-review-team                        2018-02-27T13:09:12 factory-auto
  Please review sources
         new        Group: factory-staging                             2018-02-27T13:07:19
         accepted   Group: factory-auto                                2018-02-27T13:07:19 factory-auto
  Check script succeeded
         new        Group: legal-auto                                  2018-02-27T13:07:19

History: 2018-02-27T13:09:16 factory-auto Review got accepted
         2018-02-27T13:09:14 factory-auto Request got a new review request
         2018-02-27T13:09:12 factory-auto Request got a new review request
         2018-02-27T13:07:18 namtrac      Request created
d(i)ff/(a)ccept/(d)ecline/(r)evoke/(b)uildstatus/rpm(li)ntlog/c(l)one/(e)dit/co(m)ment/(s)kip/(c)ancel >

At the bottom you see the action bar. My first step would be “b” and then “i” to review the build status and diff. The nice things is … if you requested the diff and then pick e.g. decline, the message template contains the diff output and you can refer to chunks of it in your reject message. This helps me to give much more helpful reject messages. If I only want a short message e.g. for an accept, I can do a -m ok for me and the whole string after the “-m” will be used as the message. Once I am done with current request, I automatically get the next request presented. If I don’t want to handle the current request, I pick skip and cancel if i am done for this review round.

If your project uses review system, all this works the same if you do:

osc review list -G opensuse-review-team -t submit openSUSE:Factory

My Issues with interactive mode

1. Sometimes the OBS/OSC do not understand that 2 tarballs are related and I don’t get the tarball diff shown. It just tells me “tarball removed, tarball added”.
2. Skipping the same change in multiple files. e.g. the kernel-source package has N identical changes files, so I get N identical changes file diffs. This could be skipped and replaced with a message that the 2nd changes file had the same diff.

Why?

Maybe I should have led with this question. So why do I want to see all those things that are usually hidden by default in the WebUI?

For me package review is our first line of QA, everything that gets past the review can cause us a lot of work down the road, be it in QA, be it in debugging and bugfixing before the release, be it in maintenance later. So my premise for reviewing is not “Lets get the queue down as quickly as possible” but “Is this change really what we want and does it align with the current direction/goals of the project/product”.

Just the other day I glanced over a tarball diff and some code looked weird. In pseudocode it looked like this.

if ! directory_exists(somevar) then
   call "mkdir -p --mode 750 " + somevar
   call "chgrp somegroup " + somevar
call "dd ... somevar/filename"

The package had that code in almost the same form twice. You might say the code looks reasonable. Here is why it is weird for me:

1. Nowhere we properly set umask
2. We fix permissions and ownership for the directory, but we don’t do it for the file.
3. The file might end up, world readable or in worst case world writable. While the file is currently protected by the directory permissions … maybe at some later point the directory permissions change to 751, and suddenly the file is world readable. Did i mention that the file content is later used as a authentication key? So we might have caught a security issue early.
4. Why is the directory not owned by the package and has to be created on the fly?

After a quick discussion with the maintainer, we decided to reject the package and work on a better solution for this.

So if you review a package even just for your devel project, be thorough with your review. Try to understand what the change means and if it really is good and what we as a project want.

And if someone rejects your package, don’t take it personal. In many cases they just follow the packaging policy, which your submission might have violated. Fixing the issues found and resubmit will probably take less time than discussing it first. If you disagree with the policy, bring it up on the opensuse-packaging mailinglist.