Here's the latest...

  • No new tickets were filed this week. That's a relief.
  • No tickets were closed this week. Better get on that.

Some recent discussions you might care about...

david
#2 david

Having multiple remotes is definitely a complex case, and usually requires some additional configuration. Can you make it work if you use --tracking-branch to point to the remote which the Review Board server knows?

#2 beasleyr-vmw

We've had a long-standing goal to change up how default reviewers work. Right now they're regex-based, as you noted. The plan though is to let admins define rules using our Conditions support (which we use for service integrations). These would allow for anything from regex rules to "look up default reviewers from this file/URL" to "ask this extension/webhook for reviewers".

This is very encouraging news. I look forward to seeing it appear in the final product. :)

With that in mind, I believe we can close/resolve this public ticket. (I'm not sure if users can resolve tickets or if that's limited to Splat/project admins.)

chipx86
#7 chipx86

Fixed in 14b3b248cc9c079453fde5df43db755f2cd90b3a. This will go into 3.1.2 and 4.0.

chipx86
#1 chipx86

Hey Ryan.

Interesting idea. We'll have to mull this one over.

Review Board is probably still where we'd prefer to have default reviewer logic live, because:

  1. Not everyone uses RBTools (there are a lot of in-house clients and integrations out there using our API)
  2. Not everyone upgrades (as an example, your RBTools 2.0 is a major version behind, soon two major versions)
  3. Having default reviewer logic live both client-side and server-side is messy (and we don't want it to live fully client-side -- we have customer who require complete server-side control of reviewers)

We've had a long-standing goal to change up how default reviewers work. Right now they're regex-based, as you noted. The plan though is to let admins define rules using our Conditions support (which we use for service integrations). These would allow for anything from regex rules to "look up default reviewers from this file/URL" to "ask this extension/webhook for reviewers".

With that in place, an admin could define a rule that allows review requests posted against one set of repos to use CODEOWNERS while review requests against another set would have very strict default reviewers.

That'd have the additional benefits of:

  1. Keeping default reviewer management centralized, working whether RBTools is used (or whether people are even running latest versions -- yours is a major version behind, for example)
  2. Being able to always use the latest official source for a CODEOWNERS (people may be working locally in older branches that aren't up-to-date)
  3. Letting the CODEOWNERS inclusion be part of a ruleset
  4. Treating CODEOWNERS as just one possible source (we have customers today who use other sources and use hacks to tie into default reviewers)

We have some internal tracking around this work. I'm including this as part of that task.

chipx86
#6 chipx86

We'll get this into 3.1.2. Thanks!

rpowell
#5 rpowell

I can confirm that works.

$ rbt --version
RBTools 3.1.1 (Python 3.8.10)

before:

$ rbt patch 269521
Applying 1 patch from review request 269521 (diff revision 17)

CRITICAL: a bytes-like object is required, not 'str'

With change:

$ diff /home/bpowell/.local/lib/python3.8/site-packages/rbtools/clients/svn.py /home/bpowell/.local/lib/python3.8/site-packages/rbtools/clients/svn.py_orig
1117,1119c1117
<         rc, patch_output = self._run_svn(cmd,
<                                          return_error_code=True,
<                                          results_unicode=False)
---
>         rc, patch_output = self._run_svn(cmd, return_error_code=True)

patch applies without error

pprkut_2
#1 pprkut_2

The problem here is that for whatever reason the diff between master and test1 is never created. So only the diff between the parent and the current branch ends up on reviewboard, but the parent branch's remote is different from the main repo, so it's obviously not found.

#4 dushara

Trying again:

diff --git a/rbtools/clients/svn.py b/rbtools/clients/svn.py
index e6c6e02..5dae3ec 100644
--- a/rbtools/clients/svn.py
+++ b/rbtools/clients/svn.py
@@ -1114,7 +1114,9 @@ class SVNClient(SCMClient):

         cmd.append(six.text_type(patch_file))

-        rc, patch_output = self._run_svn(cmd, return_error_code=True)
+        rc, patch_output = self._run_svn(cmd,
+                                         return_error_code=True,
+                                         results_unicode=False)

         if self.supports_empty_files():
             try:
#3 dushara

I suggest the following patch as a fix. This mirrors the implementation of the git client:

diff --git a/rbtools/clients/svn.py b/rbtools/clients/svn.py
index e6c6e02..5dae3ec 100644
--- a/rbtools/clients/svn.py
+++ b/rbtools/clients/svn.py
@@ -1114,7 +1114,9 @@ class SVNClient(SCMClient):

     cmd.append(six.text_type(patch_file))
  • rc, patch_output = self._run_svn(cmd, return_error_code=True)
  • rc, patch_output = self._run_svn(cmd,
  • return_error_code=True,
  • results_unicode=False)
     if self.supports_empty_files():
         try: