Closed Bug 1262548 Opened 8 years ago Closed 8 years ago

Move all rbbz code into mozreview

Categories

(MozReview Graveyard :: Review Board: Extension, defect)

Production
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mcote, Assigned: davidwalsh)

Details

Attachments

(10 files, 16 obsolete files)

58 bytes, text/x-review-board-request
smacleod
: review+
Details
58 bytes, text/x-review-board-request
smacleod
: review+
Details
58 bytes, text/x-review-board-request
smacleod
: review+
Details
58 bytes, text/x-review-board-request
smacleod
: review+
Details
58 bytes, text/x-review-board-request
smacleod
: review+
Details
58 bytes, text/x-review-board-request
smacleod
: review+
Details
58 bytes, text/x-review-board-request
smacleod
: review+
Details
58 bytes, text/x-review-board-request
smacleod
: review+
Details
58 bytes, text/x-review-board-request
smacleod
: review+
Details
58 bytes, text/x-review-board-request
smacleod
: review+
Details
We originally tried to separate code along certain lines into multiple extensions.  Currently they are in version-control-tools as pylib/rbbz and pylib/mozreview.  This didn't really turn out to be useful, and now it's more-or-less random what is found in rbbz vs mozreview.  We've since restricted new development to mozreview only, which occasionally necessitates moving code over from rbbz.  We should just move everything over to mozreview now to make future development easier.
:davidwalsh, can you please move things over in logical units, using multiple commits, rather than using a single large commit that moves all the code. It will make it much easier to review when the move is finished :D
https://reviewboard.mozilla.org/r/46507/#review43119

I need to break from this for the week but review can start; with this commit, however, I'm receiving the following error when I try to refresh:

```
$ ./mozreview refresh
rbweb>
PLAY [localhost] ************************************************************** rbweb>
rbweb>
GATHERING FACTS ***************************************************************
hgrb>
PLAY [localhost] ************************************************************** hgrb>

GATHERING FACTS ***************************************************************
bmoweb> httpd: stopped
httpd: started
rbweb> ok: [localhost]rbweb>

TASK: [docker-rbweb | Build mozreview extension] ******************************
hgrb> ok: [localhost]hgrb>

TASK: [hg-reviewboard | determine if running in Docker] ***********************
hgrb> ok: [localhost]

TASK: [hg-reviewboard | synchronize version-control-tools (Docker only)] ******
hgrb> changed: [localhost -> 127.0.0.1]

PLAY RECAP ********************************************************************
localhost                  : ok=3    changed=1    unreachable=0    failed=0

hgrb> refreshed hgrb container successfully
rbweb> failed: [localhost] => {"changed": true, "cmd": ["/venv/bin/python", "setup.py", "bdist_egg"], "delta": "0:00:02.320680", "end": "2016-04-14 19:32:15.957329", "rc": 1, "start": "2016-04-14 19:32:13.636649", "warnings": []}
stderr: /venv/lib/python2.6/site-packages/setuptools/dist.py:285: UserWarning: Normalizing '0.1.2alpha0' to '0.1.2a0'
  normalized_version,
/venv/lib/python2.6/site-packages/djblets/extensions/packaging.py:147: DeprecationWarning: Parameters to load are deprecated.  Call .resolve and .require separately.
  extension = entrypoint.load(require=False)
Traceback (most recent call last):
  File "setup.py", line 31, in <module>
    'static/**/*.js'
  File "/venv/lib/python2.6/site-packages/reviewboard/extensions/packaging.py", line 48, in setup
    setuptools_setup(**setup_kwargs)
  File "/usr/lib64/python2.6/distutils/core.py", line 152, in setup
    dist.run_commands()
  File "/usr/lib64/python2.6/distutils/dist.py", line 975, in run_commands
    self.run_command(cmd)
  File "/usr/lib64/python2.6/distutils/dist.py", line 995, in run_command
    cmd_obj.run()
  File "/venv/lib/python2.6/site-packages/setuptools/command/bdist_egg.py", line 161, in run
    cmd = self.call_command('install_lib', warn_dir=0)
  File "/venv/lib/python2.6/site-packages/setuptools/command/bdist_egg.py", line 147, in call_command
    self.run_command(cmdname)
  File "/usr/lib64/python2.6/distutils/cmd.py", line 333, in run_command
    self.distribution.run_command(command)
  File "/usr/lib64/python2.6/distutils/dist.py", line 995, in run_command
    cmd_obj.run()
  File "/venv/lib/python2.6/site-packages/setuptools/command/install_lib.py", line 10, in run
    self.build()
  File "/usr/lib64/python2.6/distutils/command/install_lib.py", line 117, in build
    self.run_command('build_py')
  File "/usr/lib64/python2.6/distutils/cmd.py", line 333, in run_command
    self.distribution.run_command(command)
  File "/usr/lib64/python2.6/distutils/dist.py", line 995, in run_command
    cmd_obj.run()
  File "/venv/lib/python2.6/site-packages/djblets/extensions/packaging.py", line 283, in run
    self.run_command('build_static_files')
  File "/usr/lib64/python2.6/distutils/cmd.py", line 333, in run_command
    self.distribution.run_command(command)
  File "/usr/lib64/python2.6/distutils/dist.py", line 995, in run_command
    cmd_obj.run()
  File "/venv/lib/python2.6/site-packages/djblets/extensions/packaging.py", line 147, in run
    extension = entrypoint.load(require=False)
  File "/venv/lib/python2.6/site-packages/pkg_resources/__init__.py", line 2229, in load
    return self.resolve()
  File "/venv/lib/python2.6/site-packages/pkg_resources/__init__.py", line 2235, in resolve
    module = __import__(self.module_name, fromlist=['__name__'], level=0)
  File "/version-control-tools/pylib/mozreview/mozreview/extension.py", line 47, in <module>
    from mozreview.bugzilla.auth import BugzillaBackend
  File "/version-control-tools/pylib/mozreview/mozreview/bugzilla/auth.py", line 27, in <module>
    from mozreview.forms import BugzillaAuthSettingsForm
  File "/version-control-tools/pylib/mozreview/mozreview/forms.py", line 9, in <module>
    class MozReviewSettingsForm(SettingsForm):
  File "/version-control-tools/pylib/mozreview/mozreview/forms.py", line 13, in MozReviewSettingsForm
    mozreview.extension.SETTINGS_PATH)
AttributeError: 'module' object has no attribute 'extension'
stdout: running bdist_egg
running egg_info
writing requirements to mozreview.egg-info/requires.txt
writing mozreview.egg-info/PKG-INFO
writing top-level names to mozreview.egg-info/top_level.txt
writing dependency_links to mozreview.egg-info/dependency_links.txt
writing entry points to mozreview.egg-info/entry_points.txt
reading manifest file 'mozreview.egg-info/SOURCES.txt'
writing manifest file 'mozreview.egg-info/SOURCES.txt'
installing library code to build/bdist.linux-x86_64/egg
running install_lib
running build_py
running build_static_files

FATAL: all hosts have already failed -- aborting

PLAY RECAP ********************************************************************
           to retry, use: --limit @/root/docker-rbweb.retry

localhost                  : ok=1    changed=0    unreachable=0    failed=1
```

The attribute is definitely there so the issue is probably something else, I just haven't been able to identify it yet.
Comment on attachment 8740992 [details]
MozReview Request: MozReview: Migrate rbbz/forms.py to mozreview/forms.py (Bug 1262548). r?smacleod

https://reviewboard.mozilla.org/r/46109/#review44193
Attachment #8740992 - Flags: review?(smacleod) → review+
Comment on attachment 8741009 [details]
MozReview Request: MozReview: Migrate rbbz/errors.py to mozreview/errors.py (Bug 1262548). r?smacleod

https://reviewboard.mozilla.org/r/46115/#review44199

::: pylib/mozreview/mozreview/errors.py:33
(Diff revision 1)
> +class InvalidBugsError(PublishError):
> +    def __init__(self):
> +        PublishError.__init__(self, 'Exactly one bug ID must be provided.')
> +

Rather than migrate this we should just nuke it if it's unused.
Attachment #8741009 - Flags: review?(smacleod)
Comment on attachment 8741066 [details]
MozReview Request: MozReview: Migrate rbbz/middleware.py to mozreview/middleware.py (Bug 1262548). r?smacleod

https://reviewboard.mozilla.org/r/46167/#review44201

::: pylib/mozreview/mozreview/middleware.py:3
(Diff revision 1)
>  from mozreview.models import get_profile, MozReviewUserProfile
>  
> +from django.conf import settings

django imports should go in a block *before* any mozreview imports.

Generally imports are grouped by:
1. Standard library
2. Third party imports
3. Local imports

Since We're so heavily tied to reviewboard/django though you'll see we sometimes do:
1. Std lib
2. Third party excluding rb/django
3. reviewboard and django
4. local imports

::: pylib/rbbz/rbbz/extension.py:45
(Diff revision 1)
>  
>  logger = logging.getLogger(__name__)
>  
>  
>  class BugzillaExtension(Extension):
>      middleware = [CorsHeaderMiddleware]

You'll want to migrate this over to mozreview, rather than just the middleware itself.
Attachment #8741066 - Flags: review?(smacleod)
Comment on attachment 8741081 [details]
MozReview Request: MozReview: Migrate rbbz/diffs.py to mozreview/diffs.py (Bug 1262548). r?smacleod

https://reviewboard.mozilla.org/r/46181/#review44203
Attachment #8741081 - Flags: review?(smacleod) → review+
Attachment #8741106 - Flags: review?(smacleod)
Comment on attachment 8741106 [details]
MozReview Request: MozReview: Migrate rbbz/auth.py to mozreview/bugzilla/auth.py (Bug 1262548). r?smacleod

https://reviewboard.mozilla.org/r/46203/#review44205

::: pylib/rbbz/rbbz/extension.py:52
(Diff revision 1)
>      resources = [
>          bugzilla_cookie_login_resource,
>      ]
>  
>      def initialize(self):
>          AuthBackendHook(self, BugzillaBackend)

This also needs to be migrated - which will require changes to the test environment and deployment scripts for setting up the auth backend. Without looking forward, you may or may not do this in a later commit, I'm unsure.

Also, I'm unsure if you're running the test suite between all these changes, but the tests must pass after each commit, rather than just at the end, and you should check this as you go.
Attachment #8741123 - Flags: review?(smacleod)
Comment on attachment 8741123 [details]
MozReview Request: MozReview: Migrate rbbz/resources.py to mozreview/resources/bugzilla_login.py (Bug 1262548). r?smacleod

https://reviewboard.mozilla.org/r/46209/#review44211

::: pylib/rbbz/rbbz/extension.py:47
(Diff revision 1)
>      resources = [
>          bugzilla_cookie_login_resource,
>      ]

This needs to be migrated, as well as anything that makes requests to this endpoint since the URL is going to change.

I *think* that we don't have any cookie login consumers left, but we need to make sure (they'd all be in the version-control-tools repository, so we don't have to worry about random third parties)
Comment on attachment 8741482 [details]
MozReview Request: MozReview: Migrate rbbz/extension.py to mozreview/extension.py, cleanup rbbz artifacts (Bug 1262548). r?smacleod

https://reviewboard.mozilla.org/r/46507/#review44213

A lot of the things migrated here should probably be migrated in the specific commit which moves the code for the feature, rather than treating extension.py as a unit itself.

Also, we've lost a bunch of the documentation from rbbz files that were deleted as the contents weren't migrated to be part of mozreview.

::: ansible/roles/docker-rbweb/files/install-reviewboard:66
(Diff revision 1)
> -site.run_manage_command('disable-extension',
>                          ['rbmotd.extension.MotdExtension'])
>  
>  print('enabling extensions')
>  site.run_manage_command('enable-extension', ['mozreview.extension.MozReviewExtension'])
> -site.run_manage_command('enable-extension', ['rbbz.extension.BugzillaExtension'])
> +site.run_manage_command('enable-extension', ['mozreview.extension.BugzillaExtension'])

This extension shouldn't (and doesn't that I can see) exist. All of its functionality should be rolled into the MozReviewExtension rather than having two Extensions in the extension.py - I think you've done this though, so nuke this stuff.

::: docs/hacking-mozreview.rst
(Diff revision 1)
>  Bugzilla.
>  
>  Code Locations
>  ==============
>  
> -``pylib/rbbz`` contains the modifications to Review Board to enable

The documentation for `docs/rbbz/rbbz.rst` was completely deleted - we need to migrate its contents somewhere since the code all still exists.

::: pylib/mozreview/mozreview/bugzilla/errors.py:20
(Diff revision 1)
> -        BugzillaError.__init__(self, 'No Bugzilla URL provided in rbbz '
> +        BugzillaError.__init__(self, 'No Bugzilla URL provided in mozreview '
>                                 'configuration.')

I feel like this would *have* to have test changes, which I don't see in this patch? We need to make sure all the tests are passing.

::: pylib/mozreview/mozreview/extension.py:10
(Diff revision 1)
> +from djblets.siteconfig.models import SiteConfiguration
>  
>  from reviewboard.diffviewer.opcode_generator import (

djblets imports dont need a space between them and reviewboard

::: pylib/mozreview/mozreview/extension.py:115
(Diff revision 1)
>  from mozreview.signal_handlers import (
>      initialize_signal_handlers,
>  )

All your SignalHooks should go in here.

::: pylib/mozreview/mozreview/extension.py:191
(Diff revision 1)
>          autoland_request_update_resource,
>          autoland_trigger_resource,
>          batch_review_request_resource,
>          batch_review_resource,
>          bugzilla_api_key_login_resource,
> +        bugzilla_cookie_login_resource,

We need to make sure any urls referencing this resource have been updated.

::: pylib/mozreview/mozreview/extension.py:327
(Diff revision 1)
>          # Instantiate the various signal handlers
>          initialize_signal_handlers(self)
>  
>          HostingServiceHook(self, HMORepository)
>  
> +        AuthBackendHook(self, BugzillaBackend)

The configuration code for production and the test environment need to be updated for this change. We might have to come up with a migration plan for the actual deployments.

::: pylib/mozreview/mozreview/extension.py:379
(Diff revision 1)
> +def get_reply_url(reply, site=None, siteconfig=None):
> +    """ Get the URL for a reply to a review.
> +
> +    Since replies can have multiple comments, we can't link to a specific
> +    comment, so we link to the parent review which the reply is targeted at.
> +    """
> +    return get_obj_url(reply.base_reply_to, site=site, siteconfig=siteconfig)

I *think* mozreview has its own version of this somewhere which should be used rather than having two copies.

::: pylib/mozreview/mozreview/extension.py:389
(Diff revision 1)
> +    """
> +    return get_obj_url(reply.base_reply_to, site=site, siteconfig=siteconfig)
> +
> +
> +@bugzilla_to_publish_errors
> +def on_review_publishing(user, review, **kwargs):

This signal belongs in the signal_handlers.py

::: pylib/mozreview/mozreview/extension.py:443
(Diff revision 1)
> +        if comment and not commented:
> +            b.post_comment(bug_id, comment)
> +
> +
> +@bugzilla_to_publish_errors
> +def on_reply_publishing(user, reply, **kwargs):

signal_handlers.py
Attachment #8741482 - Flags: review?(smacleod)
https://reviewboard.mozilla.org/r/46203/#review44205

> This also needs to be migrated - which will require changes to the test environment and deployment scripts for setting up the auth backend. Without looking forward, you may or may not do this in a later commit, I'm unsure.
> 
> Also, I'm unsure if you're running the test suite between all these changes, but the tests must pass after each commit, rather than just at the end, and you should check this as you go.

Tests haven't been working locally for myself or others, as I mentioned.  VM issues prevented me from running them.
Comment on attachment 8741009 [details]
MozReview Request: MozReview: Migrate rbbz/errors.py to mozreview/errors.py (Bug 1262548). r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46115/diff/1-2/
Attachment #8741009 - Flags: review?(smacleod)
Attachment #8741066 - Flags: review?(smacleod)
Attachment #8741106 - Flags: review?(smacleod)
Attachment #8741123 - Flags: review?(smacleod)
Attachment #8741482 - Flags: review?(smacleod)
Comment on attachment 8741066 [details]
MozReview Request: MozReview: Migrate rbbz/middleware.py to mozreview/middleware.py (Bug 1262548). r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46167/diff/1-2/
Comment on attachment 8741081 [details]
MozReview Request: MozReview: Migrate rbbz/diffs.py to mozreview/diffs.py (Bug 1262548). r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46181/diff/1-2/
Comment on attachment 8741106 [details]
MozReview Request: MozReview: Migrate rbbz/auth.py to mozreview/bugzilla/auth.py (Bug 1262548). r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46203/diff/1-2/
Comment on attachment 8741123 [details]
MozReview Request: MozReview: Migrate rbbz/resources.py to mozreview/resources/bugzilla_login.py (Bug 1262548). r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46209/diff/1-2/
Comment on attachment 8741482 [details]
MozReview Request: MozReview: Migrate rbbz/extension.py to mozreview/extension.py, cleanup rbbz artifacts (Bug 1262548). r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46507/diff/1-2/
https://reviewboard.mozilla.org/r/46167/#review44201

> You'll want to migrate this over to mozreview, rather than just the middleware itself.

By "this" do you mean you'd like to see the extension moved in this commit as well? (instead of here: https://reviewboard.mozilla.org/r/46507/  ?)
https://reviewboard.mozilla.org/r/46167/#review44201

> By "this" do you mean you'd like to see the extension moved in this commit as well? (instead of here: https://reviewboard.mozilla.org/r/46507/  ?)

Not the whole extension itself, just this piece of it. So you'd remove `CorsHeaderMiddleware` from `BugzillaExtension.middleware` and instead have it be part of `MozReviewExtension`.
Comment on attachment 8741066 [details]
MozReview Request: MozReview: Migrate rbbz/middleware.py to mozreview/middleware.py (Bug 1262548). r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46167/diff/2-3/
Comment on attachment 8741081 [details]
MozReview Request: MozReview: Migrate rbbz/diffs.py to mozreview/diffs.py (Bug 1262548). r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46181/diff/2-3/
Comment on attachment 8741106 [details]
MozReview Request: MozReview: Migrate rbbz/auth.py to mozreview/bugzilla/auth.py (Bug 1262548). r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46203/diff/2-3/
Comment on attachment 8741123 [details]
MozReview Request: MozReview: Migrate rbbz/resources.py to mozreview/resources/bugzilla_login.py (Bug 1262548). r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46209/diff/2-3/
Comment on attachment 8741482 [details]
MozReview Request: MozReview: Migrate rbbz/extension.py to mozreview/extension.py, cleanup rbbz artifacts (Bug 1262548). r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46507/diff/2-3/
Attachment #8740992 - Attachment is obsolete: true
Attachment #8741009 - Attachment is obsolete: true
Attachment #8741009 - Flags: review?(smacleod)
Attachment #8741066 - Attachment is obsolete: true
Attachment #8741066 - Flags: review?(smacleod)
Attachment #8741081 - Attachment is obsolete: true
Attachment #8741106 - Attachment is obsolete: true
Attachment #8741106 - Flags: review?(smacleod)
Attachment #8741123 - Attachment is obsolete: true
Attachment #8741123 - Flags: review?(smacleod)
Attachment #8741482 - Attachment is obsolete: true
Attachment #8741482 - Flags: review?(smacleod)
Attachment #8743912 - Attachment is obsolete: true
Attachment #8743912 - Flags: review?(smacleod)
Attachment #8743915 - Attachment is obsolete: true
Attachment #8743915 - Flags: review?(smacleod)
Attachment #8743919 - Attachment is obsolete: true
Attachment #8743919 - Flags: review?(smacleod)
Attachment #8743920 - Attachment is obsolete: true
Attachment #8743920 - Flags: review?(smacleod)
Attachment #8743921 - Attachment is obsolete: true
Attachment #8743921 - Flags: review?(smacleod)
Attachment #8743922 - Attachment is obsolete: true
Attachment #8743922 - Flags: review?(smacleod)
Review commit: https://reviewboard.mozilla.org/r/49759/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49759/
Attachment #8747235 - Flags: review?(smacleod)
Attachment #8747236 - Flags: review?(smacleod)
Attachment #8747237 - Flags: review?(smacleod)
Attachment #8747238 - Flags: review?(smacleod)
Attachment #8747239 - Flags: review?(smacleod)
Attachment #8747240 - Flags: review?(smacleod)
Attachment #8747241 - Flags: review?(smacleod)
https://reviewboard.mozilla.org/r/49771/#review46539

We'll need to decide how and where we want to integrate the rbbz documentation.  There's also a `docs/rbbz/rbbz.rst` file we'll need to decide what to do with.  This commit acts as a placeholder for all documentation updates.
https://reviewboard.mozilla.org/r/49773/#review46541

No `r?` request on this as it introduces 3 test failures (all commits prior to this last one pass with no test failures)

Any advice in finally removing rbbz would be appreciated.  Additionally the previous review mentioned possible need for deploy migrations -- we'll need to decide if that's necessary and in what capacity.
Comment on attachment 8747235 [details]
MozReview: Remove rbbz/errors.py (Bug 1262548).

https://reviewboard.mozilla.org/r/49759/#review51788

The commit message for this should really be changed to indicate we're just removing a file with unused code rather than migrating it.
Attachment #8747235 - Flags: review?(smacleod) → review+
Comment on attachment 8747236 [details]
MozReview: Migrate rbbz/diffs.py to mozreview/diffs.py (Bug 1262548).

https://reviewboard.mozilla.org/r/49761/#review51790
Attachment #8747236 - Flags: review?(smacleod) → review+
Comment on attachment 8747237 [details]
MozReview: Migrate rbbz/forms.py to mozreview/forms.py (Bug 1262548).

https://reviewboard.mozilla.org/r/49763/#review51792

::: pylib/mozreview/mozreview/forms.py:18
(Diff revision 1)
>                      mozreview.extension.SETTINGS_PATH)
>      )
> +
> +
> +class BugzillaAuthSettingsForm(SiteSettingsForm):
> +

Nuke this blank line as part of the move.
Attachment #8747237 - Flags: review?(smacleod) → review+
Attachment #8747238 - Flags: review?(smacleod) → review+
Comment on attachment 8747238 [details]
MozReview: Migrate rbbz/middleware.py to mozreview/middleware.py (Bug 1262548).

https://reviewboard.mozilla.org/r/49765/#review51802

::: pylib/rbbz/rbbz/extension.py:30
(Diff revision 1)
>  )
>  from mozreview.extra_data import (
>      is_parent,
>      is_pushed,
>  )
> +from mozreview.middleware import CorsHeaderMiddleware

unused import
Comment on attachment 8747239 [details]
MozReview: Remove rbbz/resources (BugzillaCookieLoginResource) as it doesn't appear to be used (Bug 1262548).

https://reviewboard.mozilla.org/r/49767/#review51804

::: pylib/rbbz/rbbz/extension.py
(Diff revision 1)
>  logger = logging.getLogger(__name__)
>  
>  
>  class BugzillaExtension(Extension):
> -    resources = [
> -        bugzilla_cookie_login_resource,

Can you mention that it's removing the BugzillaCookieLoginResource in the commit message so that it's greppable.
Attachment #8747239 - Flags: review?(smacleod) → review+
Comment on attachment 8747240 [details]
MozReview: Move rbbz/auth to mozreview extension (Bug 1262548).

https://reviewboard.mozilla.org/r/49769/#review51810
Attachment #8747240 - Flags: review?(smacleod) → review+
Comment on attachment 8747241 [details]
MozReview: Remove rbbz documentation (Bug 1262548).

https://reviewboard.mozilla.org/r/49771/#review51812

Ah, so I totally missed something. check out `docs/rbbz/*`, we should probably be modifying this in each commit removing the relvant parts. This folder would probably make a much better location for this documentation too, but we really should integrate it better.
Attachment #8747241 - Flags: review?(smacleod)
https://reviewboard.mozilla.org/r/49771/#review46539

heh, I really should have read this before making my review.

Most of `docs/rbbz/rbbz.rst` will be broken since we've moved the location of the python classes it references... I also don't think it's that important to maintain that type of documentation in there. As for where to stick the other stuff, I'd say either in the hacking mozreview guide, in docstrings on classes such as the AuthBackend, or in our deployment guide on mana.
https://reviewboard.mozilla.org/r/49773/#review51826

Okay, so for the broken tests I'm not 100% sure what's going on, so a few questions:
1. Are the tests broken with a fresh rebuild at this commit, an incremental build from the previous commit, or both?
2. What is the difference in the test output?
3. Are they still broken if you leave an rbbz extension class with has `pass` as the body?

::: pylib/mozreview/mozreview/signal_handlers.py:132
(Diff revision 1)
> +    # Any abortable signal hooks that talk to Bugzilla should have
> +    # sandbox_errors=False, since we don't want to complete the action if
> +    # updating Bugzilla failed for any reason.

This comment already exists in the docstring for this function.

::: pylib/mozreview/mozreview/signal_handlers.py:571
(Diff revision 1)
> +@bugzilla_to_publish_errors
> +def on_review_publishing(user, review, **kwargs):
> +    """Comment in the bug and potentially r+ or clear a review flag.
> +
> +    Note that a reviewer *must* have editbugs to set an attachment flag on
> +    someone else's attachment (i.e. the standard BMO review process).
> +
> +    TODO: Report lack-of-editbugs properly; see bug 1119065.
> +    """
> +    review_request = review.review_request
> +    logger.info('Publishing review for user: %s review id: %s '
> +                'review request id: %s' % (user, review.id,
> +                                            review_request.id))
> +

Can you please take care of these signals one by one each in their own commit, it makes it *much* easier for me to review.
Review commit: https://reviewboard.mozilla.org/r/55554/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55554/
Attachment #8757032 - Flags: review?(smacleod)
Attachment #8757033 - Flags: review?(smacleod)
Attachment #8757034 - Flags: review?(smacleod)
Attachment #8747241 - Flags: review?(smacleod)
Comment on attachment 8747235 [details]
MozReview: Remove rbbz/errors.py (Bug 1262548).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49759/diff/1-2/
Comment on attachment 8747236 [details]
MozReview: Migrate rbbz/diffs.py to mozreview/diffs.py (Bug 1262548).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49761/diff/1-2/
Comment on attachment 8747237 [details]
MozReview: Migrate rbbz/forms.py to mozreview/forms.py (Bug 1262548).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49763/diff/1-2/
Comment on attachment 8747238 [details]
MozReview: Migrate rbbz/middleware.py to mozreview/middleware.py (Bug 1262548).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49765/diff/1-2/
Comment on attachment 8747239 [details]
MozReview: Remove rbbz/resources (BugzillaCookieLoginResource) as it doesn't appear to be used (Bug 1262548).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49767/diff/1-2/
Comment on attachment 8747240 [details]
MozReview: Move rbbz/auth to mozreview extension (Bug 1262548).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49769/diff/1-2/
Comment on attachment 8747241 [details]
MozReview: Remove rbbz documentation (Bug 1262548).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49771/diff/1-2/
Attachment #8747242 - Attachment is obsolete: true
Comment on attachment 8747237 [details]
MozReview: Migrate rbbz/forms.py to mozreview/forms.py (Bug 1262548).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49763/diff/2-3/
Comment on attachment 8747238 [details]
MozReview: Migrate rbbz/middleware.py to mozreview/middleware.py (Bug 1262548).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49765/diff/2-3/
Comment on attachment 8747239 [details]
MozReview: Remove rbbz/resources (BugzillaCookieLoginResource) as it doesn't appear to be used (Bug 1262548).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49767/diff/2-3/
Comment on attachment 8747240 [details]
MozReview: Move rbbz/auth to mozreview extension (Bug 1262548).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49769/diff/2-3/
Comment on attachment 8747241 [details]
MozReview: Remove rbbz documentation (Bug 1262548).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49771/diff/2-3/
Comment on attachment 8757032 [details]
MozReview: Move 'on_review_publishing' signal to mozreview (Bug 1262548).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55554/diff/1-2/
Attachment #8757033 - Attachment is obsolete: true
Attachment #8757033 - Flags: review?(smacleod)
Attachment #8757034 - Attachment is obsolete: true
Attachment #8757034 - Flags: review?(smacleod)
Comment on attachment 8747239 [details]
MozReview: Remove rbbz/resources (BugzillaCookieLoginResource) as it doesn't appear to be used (Bug 1262548).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49767/diff/3-4/
Attachment #8747239 - Attachment description: MozReview Request: MozReview: Remove rbbz/resources as it doesn't appear to be used (Bug 1262548). r?smacleod → MozReview Request: MozReview: Remove rbbz/resources (BugzillaCookieLoginResource) as it doesn't appear to be used (Bug 1262548). r?smacleod
Comment on attachment 8747240 [details]
MozReview: Move rbbz/auth to mozreview extension (Bug 1262548).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49769/diff/3-4/
Comment on attachment 8747241 [details]
MozReview: Remove rbbz documentation (Bug 1262548).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49771/diff/3-4/
Comment on attachment 8757032 [details]
MozReview: Move 'on_review_publishing' signal to mozreview (Bug 1262548).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55554/diff/2-3/
Comment on attachment 8757344 [details]
MozReview: Move 'on_reply_publishing' signal to mozreview (Bug 1262548).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55820/diff/1-2/
Comment on attachment 8757345 [details]
MozReview:  Remove rbbz extension and artifacts (Bug 1262548).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55822/diff/1-2/
https://reviewboard.mozilla.org/r/49771/#review52496

What do we want to do with the rbbz-readme.md content?  I'm not seeing a logical place to shift this content into and I don't have much insight as to what of this documentation is important anymore.
Comment on attachment 8757345 [details]
MozReview:  Remove rbbz extension and artifacts (Bug 1262548).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55822/diff/2-3/
https://reviewboard.mozilla.org/r/49771/#review52496

I think we can ditch that.  Most of it is from way back when we thought this could be a generic solution for other Bugzilla instances to use.
Comment on attachment 8747241 [details]
MozReview: Remove rbbz documentation (Bug 1262548).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49771/diff/4-5/
Comment on attachment 8757032 [details]
MozReview: Move 'on_review_publishing' signal to mozreview (Bug 1262548).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55554/diff/3-4/
Comment on attachment 8757344 [details]
MozReview: Move 'on_reply_publishing' signal to mozreview (Bug 1262548).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55820/diff/2-3/
Comment on attachment 8757345 [details]
MozReview:  Remove rbbz extension and artifacts (Bug 1262548).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55822/diff/3-4/
https://reviewboard.mozilla.org/r/49757/#review52622

This should be good for another look!
https://reviewboard.mozilla.org/r/49759/#review55648

The commit message still needs to be changed, I won't `r-`, but please make sure to update it before landing.
Comment on attachment 8747241 [details]
MozReview: Remove rbbz documentation (Bug 1262548).

https://reviewboard.mozilla.org/r/49771/#review55650

Commit message no longer applicable.
Attachment #8747241 - Flags: review?(smacleod) → review-
Comment on attachment 8757032 [details]
MozReview: Move 'on_review_publishing' signal to mozreview (Bug 1262548).

https://reviewboard.mozilla.org/r/55554/#review55652

::: pylib/mozreview/mozreview/signal_handlers.py:631
(Diff revision 4)
> +    # TODO: Update all attachments in one call.  This is not possible right
> +    # now because we have to potentially mix changing and creating flags.

Hmm, this looks to be some left over cruft, we only update a single attachment when a review is publishing...

Might as well nuke this.
Attachment #8757032 - Flags: review?(smacleod) → review+
Attachment #8757344 - Flags: review?(smacleod) → review+
Comment on attachment 8757344 [details]
MozReview: Move 'on_reply_publishing' signal to mozreview (Bug 1262548).

https://reviewboard.mozilla.org/r/55820/#review55654
Comment on attachment 8757345 [details]
MozReview:  Remove rbbz extension and artifacts (Bug 1262548).

https://reviewboard.mozilla.org/r/55822/#review55682

::: ansible/roles/docker-rbweb/files/install-reviewboard
(Diff revision 4)
> -                        ['rbbz.extension.BugzillaExtension'])
> -site.run_manage_command('disable-extension',

(This isn't necessarily a code issue you need to fix, just something I must explicitly call out).

Deployment for this change could get a little interesting - what I believe might happen if we deploy as we normally do is the following:

- We'll end up with all of the `rbbz` functionality inside of `MozReview`.
- The old `rbbz` package will still be kicking around, with all of the duplicated functionality.
- The old rbbz extension information will be kicking around in the database.
- The old rbbz extension will be enabled.

I'm not exactly sure how all of this will interact, or if we can get away with just turning rbbz off / uninstalling it manually after the deploy. The best bet though would be to write a new playbook which will do this all automatically.
Attachment #8757345 - Flags: review?(smacleod) → review+
Comment on attachment 8747235 [details]
MozReview: Remove rbbz/errors.py (Bug 1262548).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49759/diff/2-3/
Attachment #8747235 - Attachment description: MozReview Request: MozReview: Migrate rbbz/errors.py to mozreview/errors.py (Bug 1262548). r?smacleod → MozReview: Remove rbbz/errors.py (Bug 1262548).
Attachment #8747236 - Attachment description: MozReview Request: MozReview: Migrate rbbz/diffs.py to mozreview/diffs.py (Bug 1262548). r?smacleod → MozReview: Migrate rbbz/diffs.py to mozreview/diffs.py (Bug 1262548).
Attachment #8747237 - Attachment description: MozReview Request: MozReview: Migrate rbbz/forms.py to mozreview/forms.py (Bug 1262548). r?smacleod → MozReview: Migrate rbbz/forms.py to mozreview/forms.py (Bug 1262548).
Attachment #8747238 - Attachment description: MozReview Request: MozReview: Migrate rbbz/middleware.py to mozreview/middleware.py (Bug 1262548). r?smacleod → MozReview: Migrate rbbz/middleware.py to mozreview/middleware.py (Bug 1262548).
Attachment #8747239 - Attachment description: MozReview Request: MozReview: Remove rbbz/resources (BugzillaCookieLoginResource) as it doesn't appear to be used (Bug 1262548). r?smacleod → MozReview: Remove rbbz/resources (BugzillaCookieLoginResource) as it doesn't appear to be used (Bug 1262548).
Attachment #8747240 - Attachment description: MozReview Request: MozReview: Move rbbz/auth to mozreview extension (Bug 1262548). r?smacleod → MozReview: Move rbbz/auth to mozreview extension (Bug 1262548).
Attachment #8747241 - Attachment description: MozReview Request: MozReview: Placeholder documentation update (Bug 1262548). r?smacleod → MozReview: Remove rbbz documentation (Bug 1262548).
Attachment #8757032 - Attachment description: MozReview Request: MozReview: Move 'on_review_publishing' signal to mozreview (Bug 1262548). r?smacleod → MozReview: Move 'on_review_publishing' signal to mozreview (Bug 1262548).
Attachment #8757344 - Attachment description: MozReview Request: MozReview: Move 'on_reply_publishing' signal to mozreview (Bug 1262548). r?smacleod → MozReview: Move 'on_reply_publishing' signal to mozreview (Bug 1262548).
Attachment #8757345 - Attachment description: MozReview Request: MozReview: Remove rbbz extension and artifacts (Bug 1262548). r?smacleod → MozReview: Remove rbbz extension and artifacts (Bug 1262548).
Attachment #8747241 - Flags: review- → review?(smacleod)
Comment on attachment 8747236 [details]
MozReview: Migrate rbbz/diffs.py to mozreview/diffs.py (Bug 1262548).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49761/diff/2-3/
Comment on attachment 8747237 [details]
MozReview: Migrate rbbz/forms.py to mozreview/forms.py (Bug 1262548).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49763/diff/3-4/
Comment on attachment 8747238 [details]
MozReview: Migrate rbbz/middleware.py to mozreview/middleware.py (Bug 1262548).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49765/diff/3-4/
Comment on attachment 8747239 [details]
MozReview: Remove rbbz/resources (BugzillaCookieLoginResource) as it doesn't appear to be used (Bug 1262548).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49767/diff/4-5/
Comment on attachment 8747240 [details]
MozReview: Move rbbz/auth to mozreview extension (Bug 1262548).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49769/diff/4-5/
Comment on attachment 8747241 [details]
MozReview: Remove rbbz documentation (Bug 1262548).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49771/diff/5-6/
Comment on attachment 8757032 [details]
MozReview: Move 'on_review_publishing' signal to mozreview (Bug 1262548).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55554/diff/4-5/
Comment on attachment 8757344 [details]
MozReview: Move 'on_reply_publishing' signal to mozreview (Bug 1262548).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55820/diff/3-4/
Comment on attachment 8757345 [details]
MozReview:  Remove rbbz extension and artifacts (Bug 1262548).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55822/diff/4-5/
Attachment #8747241 - Flags: review?(smacleod) → review+
https://reviewboard.mozilla.org/r/55822/#review55946

I re-ran tests and everything passed except for 2 timeouts, but I think it's my environment because another run did fine.  Really would like someone to run tests also so I'm not crazy.
(In reply to David Walsh :davidwalsh from comment #106)
> I re-ran tests and everything passed except for 2 timeouts, but I think it's
> my environment because another run did fine.  Really would like someone to
> run tests also so I'm not crazy.

after fixing merge conflicts the tests ran here without issue.
the docs still refer to the rbbz extension, and need to be updated.
Flags: documentation?
Pushed by bjones@mozilla.com:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/199c089da890
MozReview: Remove rbbz/errors.py . r=smacleod
https://hg.mozilla.org/hgcustom/version-control-tools/rev/71aa995b168a
MozReview: Migrate rbbz/diffs.py to mozreview/diffs.py . r=smacleod
https://hg.mozilla.org/hgcustom/version-control-tools/rev/57d3be17d927
MozReview: Migrate rbbz/forms.py to mozreview/forms.py . r=smacleod
https://hg.mozilla.org/hgcustom/version-control-tools/rev/99f2b326360a
MozReview: Migrate rbbz/middleware.py to mozreview/middleware.py . r=smacleod
https://hg.mozilla.org/hgcustom/version-control-tools/rev/bd8d1a59a508
MozReview: Remove rbbz/resources (BugzillaCookieLoginResource) as it doesn't appear to be used . r=smacleod
https://hg.mozilla.org/hgcustom/version-control-tools/rev/6a3a3391890f
MozReview: Move rbbz/auth to mozreview extension . r=smacleod
https://hg.mozilla.org/hgcustom/version-control-tools/rev/c1c14f237adb
MozReview: Remove rbbz documentation . r=smacleod
https://hg.mozilla.org/hgcustom/version-control-tools/rev/25f8ed25a92b
MozReview: Move 'on_review_publishing' signal to mozreview . r=smacleod
https://hg.mozilla.org/hgcustom/version-control-tools/rev/6cf0d893cebe
MozReview: Move 'on_reply_publishing' signal to mozreview . r=smacleod
https://hg.mozilla.org/hgcustom/version-control-tools/rev/661f9d930c98
MozReview:  Remove rbbz extension and artifacts . r=smacleod
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
https://reviewboard.mozilla.org/r/55822/#review55682

> (This isn't necessarily a code issue you need to fix, just something I must explicitly call out).
> 
> Deployment for this change could get a little interesting - what I believe might happen if we deploy as we normally do is the following:
> 
> - We'll end up with all of the `rbbz` functionality inside of `MozReview`.
> - The old `rbbz` package will still be kicking around, with all of the duplicated functionality.
> - The old rbbz extension information will be kicking around in the database.
> - The old rbbz extension will be enabled.
> 
> I'm not exactly sure how all of this will interact, or if we can get away with just turning rbbz off / uninstalling it manually after the deploy. The best bet though would be to write a new playbook which will do this all automatically.

Dropping because this has been pushed to production and completed by :glob.
Flags: documentation?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: