Allow upgrading all configured sites at once
Review Request #4204 — Created June 4, 2013 and submitted
Allow upgrading all configured sites at once Store installation paths to sitelist file during 'rb-site install'
Manually installed two sites, verified that their paths were properly added to the sitelist file (specified at the command-line), then ran 'rb-site upgrade' and verified that it looped through both.
From c1c05ebe4313ac29d006be9064c0e323597409ab Mon Sep 17 00:00:00 2001
From: Stephen Gallagher <sgallagh@redhat.com>
Date: Tue, 4 Jun 2013 15:19:01 -0400
Subject: [PATCH 2/2] Allow upgrading all configured sites at once
---
reviewboard/cmdline/rbsite.py | 38 ++++++++++++++++++++++++++++----------
1 file changed, 28 insertions(+), 10 deletions(-)
diff --git a/reviewboard/cmdline/rbsite.py b/reviewboard/cmdline/rbsite.py
index ed7f1f575490c17219b69ad44af137c3dface38b..5e2c8c8e3479a05400e6e3f2f016c6b5f0bfa843 100755
--- a/reviewboard/cmdline/rbsite.py
+++ b/reviewboard/cmdline/rbsite.py
@@ -1843,6 +1843,9 @@ class UpgradeCommand(Command):
group.add_option("--no-db-upgrade", action="store_false",dest="upgrade_db", default=True,
help="don't upgrade the database")
+ group.add_option("--all-sites", action="store_true",+ dest="all_sites", default=False,
+ help="Upgrade all
From 86298ee27bac6458abdd4a9c2fd5027136bdb2ee Mon Sep 17 00:00:00 2001
From: Stephen Gallagher <sgallagh@redhat.com>
Date: Tue, 4 Jun 2013 14:47:38 -0400
Subject: [PATCH 1/2] Store installation paths to sitelist file during 'rb-site
install'
---
reviewboard/cmdline/rbsite.py | 76 +++++++++++++++++++++++++++++++++++++++++--
1 file changed, 74 insertions(+), 2 deletions(-)
diff --git a/reviewboard/cmdline/rbsite.py b/reviewboard/cmdline/rbsite.py
index 6b72c83bf7a4aec16d9bada270571ab4af822467..1b0b67febd4b7ba94c179bb3b2fee84560956bf7 100755
--- a/reviewboard/cmdline/rbsite.py
+++ b/reviewboard/cmdline/rbsite.py
@@ -18,6 +18,8 @@ from reviewboard import get_version_string
DOCS_BASE = "http://www.reviewboard.org/docs/manual/dev/"
+SITELIST_FILE_UNIX = "/etc/reviewboard/sites"
+
# See if GTK is a possibility.
try:
@@ -639,6 +641,58 @@ class Site(object):
fp.close()
+class SiteList(object):
+ """Maintains the list of sites installed on the system."""
+ def __
| Description | From | Last Updated |
|---|---|---|
|
Col: 1 E302 expected 2 blank lines, found 1 |
 reviewbot
|
|
|
Col: 80 E501 line too long (82 > 79 characters) |
reviewbot
|
|
|
I'd prefer "reviewboard" instead of "ReviewBoard." Also, there should be two blank lines here. |
 chipx86
|
|
|
Should be one line. You can get rid of "A class that" |
chipx86
|
|
|
Sentences in comments should end with a period. Same goes with other comments in this change. |
chipx86
|
|
|
Instead of a dict, this should probably just be a set(). |
chipx86
|
|
|
For consistency, can you explicitly add the ", 'r'" ? |
chipx86
|
|
|
Blank line before this. |
chipx86
|
|
|
Blank line before/after loops and other blocks. |
chipx86
|
|
|
This won't work on Python 2.5, which we still support. |
chipx86
|
|
|
Where is this set? I only see it below, but it doesn't look like it'd be accessible here. Also, should … |
chipx86
|
|
|
Should be "is_win", or better, just collapse this with the conditional and don't use a variable. Blank line after this. … |
chipx86
|
|
|
Blank line above. |
chipx86
|
|
|
This can just be print. Like above, we can't use format(). Should use the old-style formatting arguments. |
chipx86
|
|
|
Indentation is funky. The "or" should be on the previous line, and the "(len(args) ..." should be aligned with the … |
chipx86
|
|
|
Should be one line, like: """Maintains blah blah.""" |
chipx86
|
|
|
We generally prefer more explicit variables, like ordered_sites. |
chipx86
|
|
|
This can just be "w", since we don't need to read from it. Blank line after. |
chipx86
|
|
|
sites is already public, and is fine to use. We don't need this method. |
chipx86
|
|
|
It seems we still benefit from having this, since we're doing the Windows comparison twice. (Although, while you're here, is_windows … |
chipx86
|
|
|
This would be 'if not isWin' |
chipx86
|
|
|
Should use != instead of not. |
chipx86
|
|
|
I'm not sure users need to see this every time. It's really an internal detail. (If we were using logging … |
chipx86
|
|
|
Just a thought.. Any harm in letting this accept variable arguments, now that upgrade supports upgrading multiple sites? Probably only … |
chipx86
|
|
|
The one thing I'm trying to figure out is whether we want this to happen automatically, or whether we want … |
chipx86
|
|
|
This will fail if /etc/reviewboard doesn't already exist. We should attempt to make the path, and log an error if … |
chipx86
|
|
|
Blank line before this. |
chipx86
|
|
|
undefined name 'error' |
reviewbot
|
- Change Summary:
-
Updated patch with syntax corrections from ReviewBot. Also added simple pydoc describing SiteList class.
- Diff:
-
Revision 2 (+80 -12)
-
-
-
-
-
-
-
-
-
-
Where is this set? I only see it below, but it doesn't look like it'd be accessible here. Also, should be is_win.
-
Should be "is_win", or better, just collapse this with the conditional and don't use a variable. Blank line after this. Also, when possible, use single quotes instead of double. Looks cleaner.
-
-
This can just be print. Like above, we can't use format(). Should use the old-style formatting arguments.
-
Indentation is funky. The "or" should be on the previous line, and the "(len(args) ..." should be aligned with the "args[0] not in ..." Can this be combined with the check below? Something like: if len(args) == 2 and command in COMMANDS: install_dirs = [args[1]] elif len(args) < 2 and command == 'upgrade': sitelist = ... install_dirs = ... else: print_help
- Change Summary:
-
Updated the patch with the requested changes from Christian's review. I've also attached the git-format-patch version of the patches for easier application to the tree.
- Diff:
-
Revision 3 (+85 -18)
- Added Files:
-
-
-
-
-
-
It seems we still benefit from having this, since we're doing the Windows comparison twice. (Although, while you're here, is_windows would be more correct.)
-
-
-
I'm not sure users need to see this every time. It's really an internal detail. (If we were using logging in here, I'd suggest logging.debug).
-
Just a thought.. Any harm in letting this accept variable arguments, now that upgrade supports upgrading multiple sites? Probably only makes sense for the upgrade command.
-
The one thing I'm trying to figure out is whether we want this to happen automatically, or whether we want to require an option (say, --all-sites). Right now, doing 'rb-site install' or 'rb-site upgrade' will give you usage information on them. This code changes behavior to now make 'rb-site upgrade' *maybe* give you usage information, but *probably* upgrade all your sites. I'm leaning toward wanting an --all-sites option for this, so that the behavior is explicit. That also gives us the ability to give a meaningful error if that was requested but no sites were found (such as for existing installs that don't have an entry, or for Windows).
- Change Summary:
-
Updated patches should address all of the provided concerns. I also changed the variable "install_dirs" to "site_paths" to be more accurate and moved the initialization routines for the user interface to be run before looping through the sites.
- Diff:
-
Revision 4 (+84 -14)
- Removed Files:
-
This is a review from Review Bot. Tool: PEP8 Style Checker Processed Files: reviewboard/cmdline/rbsite.py Ignored Files:
-
This is a review from Review Bot. Tool: Pyflakes Processed Files: reviewboard/cmdline/rbsite.py Ignored Files:
-
-
This will fail if /etc/reviewboard doesn't already exist. We should attempt to make the path, and log an error if we can't.
-
-
Is this related to the change? If not, it should probably be left, or documented with a comment. My only concern is that, while it's probably a correct change, it does have the potential to break things if anything relies upon the admin's home directory at all.
- Change Summary:
-
New patches should address the concerns with the last review.
- Diff:
-
Revision 5 (+102 -12)
- Removed Files:
- Added Files:
-
This is a review from Review Bot. Tool: PEP8 Style Checker Processed Files: reviewboard/cmdline/rbsite.py Ignored Files:
- Change Summary:
-
Forgot to squish a typo fix before submitting.
- Diff:
-
Revision 6 (+102 -12)
- Removed Files:
- Added Files:
-
This is a review from Review Bot. Tool: PEP8 Style Checker Processed Files: reviewboard/cmdline/rbsite.py Ignored Files:
This is a review from Review Bot. Tool: PEP8 Style Checker Processed Files: reviewboard/cmdline/rbsite.py Ignored Files: