Allow upgrading all configured sites at once

Review Request #4204 — Created June 4, 2013 and submitted

Information

Review Board
master

Reviewers

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.

Description From Last Updated

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 80 E501 line too long (82 > 79 characters)

reviewbotreviewbot

I'd prefer "reviewboard" instead of "ReviewBoard." Also, there should be two blank lines here.

chipx86chipx86

Should be one line. You can get rid of "A class that"

chipx86chipx86

Sentences in comments should end with a period. Same goes with other comments in this change.

chipx86chipx86

Instead of a dict, this should probably just be a set().

chipx86chipx86

For consistency, can you explicitly add the ", 'r'" ?

chipx86chipx86

Blank line before this.

chipx86chipx86

Blank line before/after loops and other blocks.

chipx86chipx86

This won't work on Python 2.5, which we still support.

chipx86chipx86

Where is this set? I only see it below, but it doesn't look like it'd be accessible here. Also, should …

chipx86chipx86

Should be "is_win", or better, just collapse this with the conditional and don't use a variable. Blank line after this. …

chipx86chipx86

Blank line above.

chipx86chipx86

This can just be print. Like above, we can't use format(). Should use the old-style formatting arguments.

chipx86chipx86

Indentation is funky. The "or" should be on the previous line, and the "(len(args) ..." should be aligned with the …

chipx86chipx86

Should be one line, like: """Maintains blah blah."""

chipx86chipx86

We generally prefer more explicit variables, like ordered_sites.

chipx86chipx86

This can just be "w", since we don't need to read from it. Blank line after.

chipx86chipx86

sites is already public, and is fine to use. We don't need this method.

chipx86chipx86

It seems we still benefit from having this, since we're doing the Windows comparison twice. (Although, while you're here, is_windows …

chipx86chipx86

This would be 'if not isWin'

chipx86chipx86

Should use != instead of not.

chipx86chipx86

I'm not sure users need to see this every time. It's really an internal detail. (If we were using logging …

chipx86chipx86

Just a thought.. Any harm in letting this accept variable arguments, now that upgrade supports upgrading multiple sites? Probably only …

chipx86chipx86

The one thing I'm trying to figure out is whether we want this to happen automatically, or whether we want …

chipx86chipx86

This will fail if /etc/reviewboard doesn't already exist. We should attempt to make the path, and log an error if …

chipx86chipx86

Blank line before this.

chipx86chipx86

undefined name 'error'

reviewbotreviewbot
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/cmdline/rbsite.py
      Ignored Files:
    
    
  2. reviewboard/cmdline/rbsite.py (Diff revision 1)
     
     
    Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  3. reviewboard/cmdline/rbsite.py (Diff revision 1)
     
     
    Show all issues
    Col: 80
     E501 line too long (82 > 79 characters)
    
  4. 
      
sgallagh
chipx86
  1. 
      
  2. reviewboard/cmdline/rbsite.py (Diff revision 2)
     
     
    Show all issues
    I'd prefer "reviewboard" instead of "ReviewBoard."
    
    Also, there should be two blank lines here.
  3. reviewboard/cmdline/rbsite.py (Diff revision 2)
     
     
     
     
    Show all issues
    Should be one line.
    
    You can get rid of "A class that"
  4. reviewboard/cmdline/rbsite.py (Diff revision 2)
     
     
     
    Show all issues
    Sentences in comments should end with a period. Same goes with other comments in this change.
  5. reviewboard/cmdline/rbsite.py (Diff revision 2)
     
     
    Show all issues
    Instead of a dict, this should probably just be a set().
    1. Thanks, I didn't know about set(), so I was using a construct from C and PERL that I was familiar with.
  6. reviewboard/cmdline/rbsite.py (Diff revision 2)
     
     
    Show all issues
    For consistency, can you explicitly add the ", 'r'" ?
  7. reviewboard/cmdline/rbsite.py (Diff revision 2)
     
     
    Show all issues
    Blank line before this.
  8. reviewboard/cmdline/rbsite.py (Diff revision 2)
     
     
     
    Show all issues
    Blank line before/after loops and other blocks.
  9. reviewboard/cmdline/rbsite.py (Diff revision 2)
     
     
    Show all issues
    This won't work on Python 2.5, which we still support.
    1. Ok, I've converted both uses of .format(). I am conditioned to use this because it makes localization easier (since sentence structure differences can change the position of the substituted values).
  10. reviewboard/cmdline/rbsite.py (Diff revision 2)
     
     
    Show all issues
    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.
  11. reviewboard/cmdline/rbsite.py (Diff revision 2)
     
     
    Show all issues
    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.
  12. reviewboard/cmdline/rbsite.py (Diff revision 2)
     
     
    Show all issues
    Blank line above.
  13. reviewboard/cmdline/rbsite.py (Diff revision 2)
     
     
    Show all issues
    This can just be print.
    
    Like above, we can't use format(). Should use the old-style formatting arguments.
  14. reviewboard/cmdline/rbsite.py (Diff revision 2)
     
     
     
    Show all issues
    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
  15. 
      
sgallagh
chipx86
  1. 
      
  2. reviewboard/cmdline/rbsite.py (Diff revision 3)
     
     
     
     
    Show all issues
    Should be one line, like:
    
    """Maintains blah blah."""
  3. reviewboard/cmdline/rbsite.py (Diff revision 3)
     
     
     
    Show all issues
    We generally prefer more explicit variables, like ordered_sites.
  4. reviewboard/cmdline/rbsite.py (Diff revision 3)
     
     
    Show all issues
    This can just be "w", since we don't need to read from it.
    
    Blank line after.
    1. I could have sworn when I originally wrote it that I needed the + to have it create the file if it didn't already exist, but testing it right now, it works without the +. Removed.
  5. reviewboard/cmdline/rbsite.py (Diff revision 3)
     
     
     
    Show all issues
    sites is already public, and is fine to use. We don't need this method.
    1. That was a combination of a C upbringing demanding encapsulation and an early draft where I did the re-ordering in the getter.
  6. reviewboard/cmdline/rbsite.py (Diff revision 3)
     
     
    Show all issues
    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.)
  7. reviewboard/cmdline/rbsite.py (Diff revision 3)
     
     
    Show all issues
    This would be 'if not isWin'
  8. reviewboard/cmdline/rbsite.py (Diff revision 3)
     
     
    Show all issues
    Should use != instead of not.
  9. reviewboard/cmdline/rbsite.py (Diff revision 3)
     
     
     
    Show all issues
    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).
    1. I know this was dropped but I still think we shouldn't have it. Can you tell me your thoughts?
    2. Well, it's only shown when you install a new site. I'd argue that it's a useful detail to present, since it can act as a guide for an admin who wants to manage their systems with Puppet to know which config files need updating. I'd prefer to leave it in, since it's not very noisy.
    3. Do you have any feedback for me about these changes? I'd like to know if these patches need further massaging.
  10. reviewboard/cmdline/rbsite.py (Diff revision 3)
     
     
     
    Show all issues
    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.
    1. Curious about this too.
    2. Let's open an enhancement request for that later. Right now, I suspect it's only interesting to support one or all at once.
      
      Please file a bug in the regular bug tracker and assign it to me. I'll get to it eventually.
  11. reviewboard/cmdline/rbsite.py (Diff revision 3)
     
     
     
     
    Show all issues
    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).
    1. Yeah, this probably makes more sense. I'll add that in.
  12. 
      
sgallagh
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/cmdline/rbsite.py
      Ignored Files:
    
    
  2. 
      
reviewbot
  1. This is a review from Review Bot.
      Tool: Pyflakes
      Processed Files:
        reviewboard/cmdline/rbsite.py
      Ignored Files:
    
    
  2. 
      
sgallagh
chipx86
  1. 
      
  2. reviewboard/cmdline/rbsite.py (Diff revision 4)
     
     
    Show all issues
    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.
    1. I've added this but made it a non-fatal error if it cannot be created. We'll just warn to stdout and continue, since it doesn't prevent the system from working.
  3. reviewboard/cmdline/rbsite.py (Diff revision 4)
     
     
    Show all issues
    Blank line before this.
  4. reviewboard/cmdline/rbsite.py (Diff revision 4)
     
     
    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.
    1. If you look carefully at the diff, this already existed previously. I just moved it inside the loop so that it was reset every time the install_dir changed (to address a different site).
    2. Ahh sorry, I missed it. Nevermind :)
  5. 
      
sgallagh
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/cmdline/rbsite.py
      Ignored Files:
    
    
  2. 
      
reviewbot
  1. This is a review from Review Bot.
      Tool: Pyflakes
      Processed Files:
        reviewboard/cmdline/rbsite.py
      Ignored Files:
    
    
  2. reviewboard/cmdline/rbsite.py (Diff revision 5)
     
     
    Show all issues
     undefined name 'error'
    
  3. 
      
sgallagh
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/cmdline/rbsite.py
      Ignored Files:
    
    
  2. 
      
reviewbot
  1. This is a review from Review Bot.
      Tool: Pyflakes
      Processed Files:
        reviewboard/cmdline/rbsite.py
      Ignored Files:
    
    
  2. 
      
chipx86
  1. Ship It!
  2. 
      
sgallagh
Review request changed
Status:
Completed
Change Summary:
Pushed to release-1.7.x (cbd5e59)