Getting review request id by bug or using bug identifier instead rid in post-review

22 views
Skip to first unread message

Jan Koprowski

unread,
Apr 14, 2010, 10:20:33 AM4/14/10
to reviewboard-dev
Hi !

Is there simple way to send review post update by specified bug? In
my ReviewBoard usage I need send review update using post-review for
review request identified by bug number. I'am thinking about:

post-review --review-request-bug 1234

which means "update review request which closed_bugs contain 1234".
But there are many issues in here.
1) Is hard to find this kind of review requests. Is api/json/
reviewrequests/all/?closed_bugs=1234 is good? I think it is not.
Because it is hard to determine is some bug contain this bug. There
could be:
1234
9999, 1234
1234, 9999
8888, 1234, 9999

and we need check all of cases. Simple %LIKE% doesn't work because we
find also bugs 11234, 21234, 12345, 12341 etc... So this must check
number of conditions:
= 1234 or
LIKE 1234,% or
LIKE %, 1234 or
LIKE % 1234, %

this is a small horror and need additional method in ReviewBoard api
to check all conditions. But what more? There can be more then one
review request with this same number. What then? post-review should
handle some options like --choose-lower --choose-higher --ask-me-to-
choose.

Ok this is my point of view. So I think to get much more simple
solution. Create a little wrapper arround post-review which catch bug
id and check is their database is there one? If not rid wasn't be set.
If true they set rid. That is all :) What do You think about it? Meybe
there is simpler soultion?

Jan Koprowski

unread,
Apr 14, 2010, 12:18:26 PM4/14/10
to reviewboard-dev
I just red about redis and consider using it to this "database" :)

Christian Hammond

unread,
Apr 15, 2010, 9:48:09 PM4/15/10
to reviewb...@googlegroups.com
You're right in that it's a bit tricky. Since we store bug IDs as part of a string, they're difficult to accurately query quickly. We can't quite index by bug ID, so queries won't be as fast as we'd prefer. Also, as you said, there may be multiple review requests returned.

Personally, I think what you're doing is custom enough where I don't want the functionality to update by bug number in post-review. I don't mind adding APIs to query review requests based on bug ID, but that's only solving one part of this. Soonest we can add that is 1.5, but given the schedule, that may slip to 1.6. I'd want to wait until the new API work is done.

If you want to do the wrapper, that'd probably work best. Shouldn't end up being too complicated. That is, once we have querying available. Down the road, I'd like for us to have a tool in RBTools for querying Review Board for review requests and displaying information. At that point, your wrapper script would be easy. It's probably a little ways off, though.

I think it'd make the most sense to update all review requests with that bug number.

What are you looking to use this functionality for? I'm curious about the workflow.

Christian

--
Christian Hammond - chi...@chipx86.com
Review Board - http://www.reviewboard.org
VMware, Inc. - http://www.vmware.com




--
To unsubscribe, reply using "remove me" as the subject.

Jan Koprowski

unread,
Apr 16, 2010, 12:36:35 AM4/16/10
to reviewboard-dev
Thanks for advices :)

We trigger post-review from Issue Tracker. From how we now what was
changed? This is simple because issue ticket contain all changeset
with number of versions commited to repository. There is no way to
commit code to repo without notifying changed revisions in issue
tracket. We always commit "into a ticket". So When issue ticket
change state to the "review" the trigger hook which send changeset and
few fields to wrapper (online website, normal POST request with some
additional data). This data is formatted to the repository available
on machine where wrapper working, and passed as --revision-range to
post-review which post new review in ReviewBoard. But all what we
store is ticket id, nothing more. So when changes are made (ticket
just contain more revisions numbers) wrapper get only new revision-
range, ticket id and that is all. But they don't know anything about
review id. The only way to find out is query ReviewBoard about that.
In our company this is simple - one ticket id one review and some kind
of patches for rb-tools will work well, but this change is not work in
overall ReviewBoard. I don't want make changes which have not chance
to be include to release. So in cases like that I'am looking ways to
solve the problem outside ReviewBoard ecosystem. This allow me to fast
update ReviewBoard and RBTools to newest version without loosing my
changes specific for company workflow. I'am impatient when my changes
made until now go into the release :) This allow me use ReviewBoard
without modifications because all needed modifications will be
included :]

On Apr 16, 3:48 am, Christian Hammond <chip...@chipx86.com> wrote:
> You're right in that it's a bit tricky. Since we store bug IDs as part of a
> string, they're difficult to accurately query quickly. We can't quite index
> by bug ID, so queries won't be as fast as we'd prefer. Also, as you said,
> there may be multiple review requests returned.
>
> Personally, I think what you're doing is custom enough where I don't want
> the functionality to update by bug number in post-review. I don't mind
> adding APIs to query review requests based on bug ID, but that's only
> solving one part of this. Soonest we can add that is 1.5, but given the
> schedule, that may slip to 1.6. I'd want to wait until the new API work is
> done.
>
> If you want to do the wrapper, that'd probably work best. Shouldn't end up
> being too complicated. That is, once we have querying available. Down the
> road, I'd like for us to have a tool in RBTools for querying Review Board
> for review requests and displaying information. At that point, your wrapper
> script would be easy. It's probably a little ways off, though.
>
> I think it'd make the most sense to update all review requests with that bug
> number.
>
> What are you looking to use this functionality for? I'm curious about the
> workflow.
>
> Christian
>
> --
> Christian Hammond - chip...@chipx86.com
> Review Board -http://www.reviewboard.org
> VMware, Inc. -http://www.vmware.com

Christian Hammond

unread,
Apr 16, 2010, 3:40:06 AM4/16/10
to reviewb...@googlegroups.com
Hi,

First, let me apologize about the lack of movement on your current
contributions. I'm basically working all hours of the night to solve
some major compatibility bugs in Django Evolution before Django 1.2
comes out so that people don't end up with broken installs. I'm
putting everything else aside until that is done.

As far as the bug tracker integration goes, can you store the review
request ID in the bug tracker somehow? Either custom metadata or as an
automates post that can be parses later?

Christian
--

Jan Koprowski

unread,
Apr 16, 2010, 5:14:55 AM4/16/10
to reviewboard-dev
There is no reason to apologizes :) I'am just impatient - this is my
trait :)
Switching to Django 1.2 is very good move but I guess it also very
time consuming but IMHO worth it.
I'am thinking about storing review number or even URL in issue
tracker.
You ask can I parse post-review output on the issue tracker side? Yes
I can.
There is special field to "Review notes" or something like that. But I
don't trust this field until developer can diddle there. When I get
field which is readonly for the rest of the world I put there
informations. Until that I can do this but I need trusted database
"issue id -> review id". I'am thinkig about that from yesterday. I
will see.

Stephen Gallagher

unread,
Apr 16, 2010, 7:25:47 AM4/16/10
to reviewb...@googlegroups.com
On 04/16/2010 05:14 AM, Jan Koprowski wrote:
> There is no reason to apologizes :) I'am just impatient - this is my
> trait :)
> Switching to Django 1.2 is very good move but I guess it also very
> time consuming but IMHO worth it.
>

Slightly off-topic, but are you planning to switch to REQUIRING Django
1.2 in 1.0.7 and 1.5b2? Or will it be compatible with both?

Because this will have direct impact on my deployment plans :(


--
Subscription settings: http://groups.google.com/group/reviewboard-dev/subscribe?hl=en

Christian Hammond

unread,
Apr 16, 2010, 7:32:51 AM4/16/10
to reviewb...@googlegroups.com
We won't be requiring it in 1.0.7. There's a chance we will for 1.5.

Here's the situation, though, with 1.0.7 (and currently 1.5). The way the Python package dependencies work is that installing Review Board will fetch the latest stable version of Django. Soon, that will be 1.2, so by default you will get that instead. You *can* decide to install the 1.1 release, and installing Review Board will respect that (it shouldn't auto-upgrade).

The problem, and reason for my sudden rush to fix Django Evolution, is that if you did install Review Board on a fresh install without a previous version, and it fetched Django 1.2, then you'd have problems. Django Evolution wasn't compatible with Django 1.2, and would break when invoked to inspect your database (which happens on every rb-site upgrade). So, I've been working to fix this so that if you did have Django 1.2 installed, things would continue to work as normal.

Once these changes are in (I finished the last of the fixes mere minutes ago), then upgrading to Django 1.2 should be safe. I've been testing with it and haven't hit any issues lately. RB 1.0.6 is broken with it, but a 1.0.7 will be out probably this weekend to fix those last remaining issues. Again, this shouldn't impact you if you have Django 1.1.x installed anyway.

We may decide that there are things in Django 1.2 that would benefit Review Board 1.5. Currently, I don't think we need a hard requirement on it, and so far our code that uses Django 1.2 features does so conditionally (and are pretty much just admin UI polish).

Out of curiosity, what are the problems you'd hit with deploying Django 1.2? Is that more of a concern for 1.0.x, or 1.5?

Christian

--
Christian Hammond - chi...@chipx86.com
Review Board - http://www.reviewboard.org
VMware, Inc. - http://www.vmware.com


Stephen Gallagher

unread,
Apr 16, 2010, 7:48:13 AM4/16/10
to reviewb...@googlegroups.com
Actually, it's the other way around. I'm planning to launch (today
actually) a deployment of 1.5b1 for the Fedora Hosted environment. The
catch here is that I don't have any guarantees that I will be able to
upgrade to Django 1.2 when it is released, because all servers in the
Fedora Hosted environment run Red Hat Enterprise Linux 5, which has a
compatibility promise for its entire lifecycle (which means that
upgrading to a newer version of Django may not be approved, especially
if it deprecates any functionality).

So if you're telling me that ReviewBoard 1.5 final is going to require
Django 1.2, I may have to cancel my deployment and redo it for the 1.0
branch in order to retain functionality with Django 1.1.x. This would be
a disappointing state of affairs, as the reason we decided to go with
the 1.5 beta over 1.0 was because of the per-repo default reviewers
capabilities, which will come in very handy for the size of the
deployment we're doing.

Is it likely that 1.5 final is going to REQUIRE Django 1.2? Or more
likely that it will just be compatible?

Christian Hammond

unread,
Apr 16, 2010, 8:04:45 AM4/16/10
to reviewb...@googlegroups.com

Most likely, it will just be compatible, and not require it. I wouldn't worry much about it. The only things in there that I really care about for 1.5 are very optional. 1.6, however, most likely will require it.

I didn't think RHEL5 came with Django at all. Since RB basically auto-installs the version of Django needed, I'm not sure why it'd be any different with Django 1.2?

Django 1.2 is a feature enhancement release, but as far as I know it doesn't deprecate anything.

Christian

Stephen Gallagher

unread,
Apr 16, 2010, 8:09:14 AM4/16/10
to reviewb...@googlegroups.com
RHEL5 doesn't, but EPEL (Extra Packages for Enterprise Linux) which is a
Fedora subproject carries it. EPEL follows many of the same rules as
RHEL5 proper regarding API/ABI guarantees. So it Django 1.2 breaks ABI
or API, I wouldn't be able to upgrade to it.

Also, we aren't using any of the ez_install features in ReviewBoard.
Instead, several of my colleagues and I packaged the relevent
dependencies into the EPEL RPM repositories, so when we install the
ReviewBoard RPM, it acquires the necessary python packages. This
includes Django.

We find it safer to do things this way for exactly the reasons you were
just demonstrating: nothing can upgrade underneath us and break things.
We have to explicitly release a new package to the repositories.

Jan Koprowski

unread,
Apr 16, 2010, 8:19:06 AM4/16/10
to reviewboard-dev
I had similar problem in my company. I solve it doing something
"strange":
I compile my own python in my home directory and use virtualenv on it.
Then I install everything what I need independently from system. In
just run python setup.py develop. That is all. Without root
privileges. One thing I must do was hack apache start script to inject
PYTHONHOME variable showing which python should be use. Without that
mod_python just get nuts. Of course I must compile also mod_python but
I thinks this little eco environment was worth it because I'am
independent from that what companies administrators do with server.

On Apr 16, 2:09 pm, Stephen Gallagher <step...@gallagherhome.com>
wrote:
Reply all
Reply to author
Forward
0 new messages