post-review: Args required in reviewboard should be enforced in the command line

Review Request #1410 — Created Feb. 11, 2010 and discarded

Information

psa
RBTools

Reviewers

Require summary, description and a set of reviewers

 
PS
Review request changed
Summary:
Args required in reviewboard should be enforced in the command line
post-review: Args required in reviewboard should be enforced in the command line
chipx86
  1. There shouldn't be any requirement for specifying/guessing a summary, description, testing done or groups on the command line. These are optional and in a lot of cases it's not even what you'd want.
    1. Part of the reason for this patch was to bring post-review in line with the web interface.
      
      The web interface (at least a default install of 1.1alpha2) forbids publishing reviews if you haven't met these requirements but post-review does not and the inconsistency grated on me (and I can't think of a good reason you should be posting a review without at least a summary and someone to do the review).
    2. You can post one on the web UI without that information. You just can't publish it. That's the same with post-review.
      
      The publishing validation must be server-side regardless. It's perfectly okay (and common) to post a review request without this information and then to edit it on the server. I do this with nearly every change, for instance. And with some repository types (such as Perforce) the server actually supplies this information, meaning you wouldn't want to either specify this or guess in post-review.
    3. It is possible to run "post-review -p" and publish without meeting any of the requirements. It was this realization that lead to the patch.
      
      I do agree though that it should also be server side. I'm still inclined towards doing checks at the client level too.
      
      For things like Perforce, maybe a check that says not to enforce this for servers where reviewboard will collect that information from the repository (or, I've not played with it, does reviewboard collect it or does it need to be forced to do so?)?
    4. How about we just require them with -p?
    5. Even then, the server should enforce them. Just because you passed -p and didn't pass --summary or something doesn't mean that the server doesn't have a summary and other necessary information.
      
      I think if we pass --publish, and we get an error from the server (which we must make sure we enforce, and I think we do in 1.5?), then we should yell. Otherwise, I don't think there's any value of checking these options prior to calling the server.
    6. I just checked against 1.5B1 and it doesn't enforce it yet.
      
      post-review --server=http://reviews2. -p
      Review request #3 posted.
      
      http://reviews2/r/3
      
    7. Yeah.. We definitely need to fix that server-side then.
  2.