Print out warning when interrupt in prepare-dev.py after syncing database has started

Review Request #6845 — Created Jan. 31, 2015 and submitted

viettran1989
Review Board
master
3714
255221a...
reviewboard, students

Problem:
- If the user runs prepare-dev.py and interrupts after the syncing database has started, they will end up with no super user when re-run the commmand. This is because the reviewboard.db is already existed.
Fix:
- The database reviewboard.db only created when site.sync_database is called. I put a try catch KeyBoardInterrupt around the whole if options.sync_db. After calling prepare-dev.py, if we interrupt before syncing database happens, the reviewboard.db hasn't created yet. If the user calls it again, the super user will be promted again. Therefore, we only need to put the try catch when the site.sync_database is called to prompt the user about rm reviewboard.db before re-run the command.

Manual test: Interrupt after Synchronizing database is called and checked that the warning is printed to commandline. If the interrupt happens before the database is created, no warning is printed which is expected.

Description From Last Updated

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

reviewbotreviewbot

Col: 1 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 74 W291 trailing whitespace

reviewbotreviewbot

Col: 27 W604 backticks are deprecated, use 'repr()'

reviewbotreviewbot

Col: 51 E225 missing whitespace around operator

reviewbotreviewbot

Col: 54 E226 missing whitespace around arithmetic operator

reviewbotreviewbot

We are inconsistent all over the code base, but for all new strings we want to be using single quotes. ...

brenniebrennie

We probably don't want to print this out if there's an interrupt. Maybe move it inside the try after sync_database?

daviddavid

Can you remove the extra blank line you added here? We generally keep 2 blank lines between top-level functions.

anselinaanselina

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

"preparing process" sounds a little awkward. How about, "development preparation process"

mike_conleymike_conley

"create super user" -> "create a super user"

mike_conleymike_conley

/src/reviewboard isn't necessarily correct - that's probably only true if the developer is using vagrant. We can probably dynamically figure ...

mike_conleymike_conley

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

reviewbotreviewbot

Col: 68 E231 missing whitespace after ','

reviewbotreviewbot

Col: 13 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Undo this change.

brenniebrennie

Instead of doing this, you can import reviewboard and use reviewboard.__file__

brenniebrennie

'reviewboard' imported but unused

reviewbotreviewbot

'realpath' imported but unused

reviewbotreviewbot

'reviewboard' imported but unused

reviewbotreviewbot

This should go in another import group because it is from the reviewboard package.

brenniebrennie

You should use os.path.dirname(reviewboard.__file__) instead of joining '..' twice. Your code makes it seem as if you are moving up ...

brenniebrennie

Add a blank line between these two.

daviddavid

'reviewboard' imported but unused

reviewbotreviewbot

It might be nice to align everything to 4 space boundaries: warnings.warn( 'The development preparation process exited before completing. In ...

daviddavid

'reviewboard' imported but unused

reviewbotreviewbot

Something to note here.. Exiting at any point during the syncdb may do more than fail to create a superuser. ...

chipx86chipx86

'reviewboard' imported but unused

reviewbotreviewbot

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

reviewbotreviewbot

'reviewboard' imported but unused

reviewbotreviewbot
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        contrib/internal/prepare-dev.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        contrib/internal/prepare-dev.py
    
    
  2. contrib/internal/prepare-dev.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (94 > 79 characters)
    
  3. 
      
VI
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        contrib/internal/prepare-dev.py
    
    
  2. contrib/internal/prepare-dev.py (Diff revision 2)
     
     
    Col: 1
     E128 continuation line under-indented for visual indent
    
  3. contrib/internal/prepare-dev.py (Diff revision 2)
     
     
    Col: 74
     W291 trailing whitespace
    
  4. contrib/internal/prepare-dev.py (Diff revision 2)
     
     
    Col: 27
     W604 backticks are deprecated, use 'repr()'
    
  5. contrib/internal/prepare-dev.py (Diff revision 2)
     
     
    Col: 51
     E225 missing whitespace around operator
    
  6. contrib/internal/prepare-dev.py (Diff revision 2)
     
     
    Col: 54
     E226 missing whitespace around arithmetic operator
    
  7. 
      
VI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        contrib/internal/prepare-dev.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        contrib/internal/prepare-dev.py
    
    
  2. 
      
VI
brennie
  1. You don't need to mention the issue number in the title and the summary. Since its in the bugs field, it will automatically be added to the commit message. when it gets landed.

  2. contrib/internal/prepare-dev.py (Diff revision 3)
     
     
     
     
     

    We are inconsistent all over the code base, but for all new strings we want to be using single quotes.

    Also, for user-visible strings we want to use "" instead of Markdown back-ticks.

  3. 
      
VI
david
  1. In your summary and description, can you clarify that it's when you interrupt prepare-dev.py? There are a lot of things in Review Board that could be "interrupted"

    1. Your summary/description are improved but this request still applies.

  2. contrib/internal/prepare-dev.py (Diff revision 3)
     
     
     
     

    We probably don't want to print this out if there's an interrupt. Maybe move it inside the try after sync_database?

    1. If we move it after the sync_database, do we assume the options.sync_db is always on? Also, if we move that inside the if options.sync_db, if someone add a new options after this block and that new option fails to execute, this print statement will still be printed out. Is this a desire effect?

    2. Ah yes, good point. How about we make the try/catch wrap the whole if options.sync_db: block?

    3. That might work. Fixed that.

  3. 
      
VI
VI
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        contrib/internal/prepare-dev.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        contrib/internal/prepare-dev.py
    
    
  2. 
      
VI
VI
anselina
  1. 
      
  2. contrib/internal/prepare-dev.py (Diff revision 4)
     
     

    Can you remove the extra blank line you added here? We generally keep 2 blank lines between top-level functions.

  3. 
      
VI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        contrib/internal/prepare-dev.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        contrib/internal/prepare-dev.py
    
    
  2. contrib/internal/prepare-dev.py (Diff revision 5)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  3. 
      
VI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        contrib/internal/prepare-dev.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        contrib/internal/prepare-dev.py
    
    
  2. 
      
mike_conley
  1. 
      
  2. contrib/internal/prepare-dev.py (Diff revision 6)
     
     

    "preparing process" sounds a little awkward. How about, "development preparation process"

  3. contrib/internal/prepare-dev.py (Diff revision 6)
     
     

    "create super user" -> "create a super user"

  4. contrib/internal/prepare-dev.py (Diff revision 6)
     
     

    /src/reviewboard isn't necessarily correct - that's probably only true if the developer is using vagrant. We can probably dynamically figure out the appropriate path here, can we not?

  5. 
      
VI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        contrib/internal/prepare-dev.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        contrib/internal/prepare-dev.py
    
    
  2. contrib/internal/prepare-dev.py (Diff revision 7)
     
     
    Col: 80
     E501 line too long (86 > 79 characters)
    
  3. contrib/internal/prepare-dev.py (Diff revision 7)
     
     
    Col: 68
     E231 missing whitespace after ','
    
  4. 
      
VI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        contrib/internal/prepare-dev.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        contrib/internal/prepare-dev.py
    
    
  2. contrib/internal/prepare-dev.py (Diff revision 8)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  3. 
      
VI
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        contrib/internal/prepare-dev.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        contrib/internal/prepare-dev.py
    
    
  2. 
      
brennie
  1. 
      
  2. contrib/internal/prepare-dev.py (Diff revision 9)
     
     

    Undo this change.

  3. contrib/internal/prepare-dev.py (Diff revision 9)
     
     

    Instead of doing this, you can import reviewboard and use reviewboard.__file__

    1. the reviewboard.file yields /src/reviewboard/reviewboard/file path, so I still have to use '..' twice to traverse up the directory. But it doesn't look clearer.

  4. 
      
VI
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        contrib/internal/prepare-dev.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        contrib/internal/prepare-dev.py
    
    
  2. contrib/internal/prepare-dev.py (Diff revision 10)
     
     
     'reviewboard' imported but unused
    
    1. Seem like a bug since I actually use reviewboard.file

  3. contrib/internal/prepare-dev.py (Diff revision 10)
     
     
     'realpath' imported but unused
    
  4. 
      
VI
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        contrib/internal/prepare-dev.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        contrib/internal/prepare-dev.py
    
    
  2. contrib/internal/prepare-dev.py (Diff revision 11)
     
     
     'reviewboard' imported but unused
    
    1. I did use reviewboard.file.

  3. 
      
brennie
  1. 
      
  2. contrib/internal/prepare-dev.py (Diff revision 11)
     
     

    This should go in another import group because it is from the reviewboard package.

    1. I'm not sure I understand this comment. Can you clarify?

    2. We split up imports into three groups in Review Board.

      The first group is for Python standard library stuff (e.g. os).

      The second group is for third-party imports (e.g. Django imports).

      The third group is for project imports (e.g. everything beginning with reviewboard)

  3. contrib/internal/prepare-dev.py (Diff revision 11)
     
     

    You should use os.path.dirname(reviewboard.__file__) instead of joining '..' twice.

    Your code makes it seem as if you are moving up two directories from the reviewboard file instead of one (because of '..', '..'). Using dirname makes this more clear.

  4. 
      
VI
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        contrib/internal/prepare-dev.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        contrib/internal/prepare-dev.py
    
    
  2. contrib/internal/prepare-dev.py (Diff revision 12)
     
     
     'reviewboard' imported but unused
    
  3. 
      
david
  1. 
      
  2. contrib/internal/prepare-dev.py (Diff revision 12)
     
     
     

    Add a blank line between these two.

  3. contrib/internal/prepare-dev.py (Diff revision 12)
     
     
     
     
     
     
     
     

    It might be nice to align everything to 4 space boundaries:

    warnings.warn(
        'The development preparation process exited before completing. In order '
        'to create a  super user, you need to run "rm reviewboard.db" in %s and '
        'run prepare-dev.py again.'
        % os.path.abspath(
            os.path.join(os.path.dirname(reviewboard.__file__), '..')))
    
  4. 
      
VI
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        contrib/internal/prepare-dev.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        contrib/internal/prepare-dev.py
    
    
  2. contrib/internal/prepare-dev.py (Diff revision 13)
     
     
     'reviewboard' imported but unused
    
  3. 
      
mike_conley
  1. Code looks good to me.

  2. 
      
chipx86
  1. 
      
  2. contrib/internal/prepare-dev.py (Diff revision 13)
     
     
     
     
     
     
     

    Something to note here.. Exiting at any point during the syncdb may do more than fail to create a superuser. It can actually leave the database in an inconsistent state.

    I would change this error to instead say that the database may be in an inconsistent state and to remove the file and start over.

    Also, warnings.warn is the wrong thing here. That's more for programattic warning involving something like a deprecated module or function being used. I would use sys.stderr.write(..) instead.

  3. 
      
VI
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        contrib/internal/prepare-dev.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        contrib/internal/prepare-dev.py
    
    
  2. contrib/internal/prepare-dev.py (Diff revision 14)
     
     
     'reviewboard' imported but unused
    
  3. contrib/internal/prepare-dev.py (Diff revision 14)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  4. 
      
VI
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        contrib/internal/prepare-dev.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        contrib/internal/prepare-dev.py
    
    
  2. contrib/internal/prepare-dev.py (Diff revision 15)
     
     
     'reviewboard' imported but unused
    
  3. 
      
david
  1. I'm going to make some small changes to the wording and push this. Thanks!

  2. 
      
VI
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-2.0.x (449c614)
Loading...