We just completed an upgrade to reviewboard 2.0.1 and rbtools 0.6.  We
had to patch rbtools a bit, though (we maintained similar patches on
post-review in the past).

Some of these are unquestionably *not* the best way to be working
around the issues we're seeing, and I thought I'd share and ask for
some input, then possibly try to clean up and submit a pull request,
or a few bugs, or whatever makes the most sense.

=====

As a bit of background: our primary usage is in an svn post-commit
hook, run by apache.  The command would look like this:

/usr/bin/rbt post \
  --debug \
  --publish \
  --submit-as=leander \
  --server='https://rb.example.com/' \
  --username=buildsystem \
  --password=ELIDED \
  --repository=examplerepo \
  --repository-url='file:///data/svn/examplerepo' \
  --repository-type=svn \
  --summary='examplerepo 2349 - Testing rbt post[...]' \
  --description='rev 2349 - Testing rbt post, ignore this commit.' \
  --target-groups=examplerepo \
  2349

=====

The first patch is to address the second request to rb API failing,
but the first succeeding:

>>> Making HTTP GET request to https://rb.example.com/api/
>>> [...]
>>> Making HTTP GET request to https://rb.example.com/api/review-requests/
>>> Got HTTP error: 401: <!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML 2.0//EN">
<html><head>
<title>401 Authorization Required</title>
[...]

In rbtools/api/request.py, in PresetHTTPAuthHandler's http_request
method, we comment out "self.used = True", which seems to prevent
subsequent requests from getting the basic auth header.  I'm not sure
if this naive removal is causing the basic auth header to be added
twice in some cases.

=====

The second patch is to address the server's understanding of our
repository's URL (https://svn.example.com/whatever) not matching our
local understanding (file:///data/svn/whatever) -- if I'm reading
things correctly.

This -- in rbtools/clients/svn.py's SVNRepositoryInfo class, in
find_server_repository_info -- is just a pessimistic fallback to "it's
not relative, assume it's at root", see the "# 1P ADDED" comments:

#####

# We didn't find our locally matched repository, so scan based on UUID.
for repository in repositories:
    info = self._get_repository_info(server, repository)

    if not info or self.uuid != info['uuid']:
        continue

    repos_base_path = info['url'][len(info['root_url']):]
    relpath = self._get_relative_path(self.base_path, repos_base_path)

    if relpath:
        return SVNRepositoryInfo(info['url'], relpath, self.uuid)
    else: # 1P ADDED 2014-06-08 #
        return SVNRepositoryInfo(info['url'], '/', self.uuid) # 1P
ADDED 2014-06-08 #

# We didn't find a matching repository on the server. We'll just return
# self and hope for the best. In reality, we'll likely fail, but we
# did all we could really do.
return self

#####

I'm not even sure this one is 100% necessary, I need to look a little
more.  But it seems assuming root is perhaps better than just
returning self in this case?  I need to step through this code a bit
more thoroughly to understand what is going on here.

We could use "https://svn.example.com/whatever"; as our
--repository-url arg, but this requires that the "svn info" commands
have authentication cached somewhere, which turns out to be a little
tricky in our usecase (running as apache user with invalid homedir).

=====

Finally, related to a "can't find repository":

>>> Making HTTP POST request to https://rb.example.com/api/review-requests/
>>> Got API Error 206 (HTTP code 400): The repository path specified is not in 
>>> the list of known repositories.

In rbtools/commands/post.py, in post_request, there's this:

            # No review_request_id, so we will create a new review request.
            try:
                repository = (
                    # 1P DISABLED 2014-06-08 # self.options.repository_url or
                    self.options.repository_name or
                    self.get_repository_path(repository_info, api_root))

Rather than commenting out the first line there per the DISABLED
comment, I think it would be sufficient to push the repository_name up
a line, so we'd trust "--repository" a bit more than
"--repository-url".

The latter (--repository-url) seems to have two meanings at least vis
a vis svn: one is "the thing we're going to be calling 'svn info'
and/or collecting info from" and the second is "a thing we can use to
identify the repo to RB".  The former (--repository) is presumably
only the name as RB knows it?  (Those are guaranteed to be unique,
yes?)

=====

One other tiny note -- we've been keeping a patch over
reviewboard/templates/notifications that removes all the explicit
"font-size" (e.g. 8px, 9px, 10px) in favor of the occasional
"font-size: smaller;" or "font-size: larger;".  The default font shows
up hideously tiny in many of the clients we use with these explicit
sizes, and some browsers behave a lot better re zoom when the size
isn't explicit in pixels.

-- 
Leander

-- 
Get the Review Board Power Pack at http://www.reviewboard.org/powerpack/
---
Sign up for Review Board hosting at RBCommons: https://rbcommons.com/
---
Happy user? Let us know at http://www.reviewboard.org/users/
--- 
You received this message because you are subscribed to the Google Groups 
"reviewboard" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to reviewboard+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Reply via email to