Print out warning when interrupt in prepare-dev.py after syncing database has started
Review Request #6845 — Created Jan. 31, 2015 and submitted
| Information | |
|---|---|
| viettran1989 | |
| Review Board | |
| master | |
| 3714 | |
| 255221a... | |
| Reviewers | |
| 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) |
 reviewbot
|
|
|
Col: 1 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
Col: 74 W291 trailing whitespace |
reviewbot
|
|
|
Col: 27 W604 backticks are deprecated, use 'repr()' |
reviewbot
|
|
|
Col: 51 E225 missing whitespace around operator |
reviewbot
|
|
|
Col: 54 E226 missing whitespace around arithmetic operator |
reviewbot
|
|
|
We are inconsistent all over the code base, but for all new strings we want to be using single quotes. … |
 brennie
|
|
|
We probably don't want to print this out if there's an interrupt. Maybe move it inside the try after sync_database? |
 david
|
|
|
Can you remove the extra blank line you added here? We generally keep 2 blank lines between top-level functions. |
 anselina
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
"preparing process" sounds a little awkward. How about, "development preparation process" |
 mike_conley
|
|
|
"create super user" -> "create a super user" |
mike_conley
|
|
|
/src/reviewboard isn't necessarily correct - that's probably only true if the developer is using vagrant. We can probably dynamically figure … |
mike_conley
|
|
|
Col: 80 E501 line too long (86 > 79 characters) |
reviewbot
|
|
|
Col: 68 E231 missing whitespace after ',' |
reviewbot
|
|
|
Col: 13 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
Undo this change. |
brennie
|
|
|
Instead of doing this, you can import reviewboard and use reviewboard.__file__ |
brennie
|
|
|
'reviewboard' imported but unused |
reviewbot
|
|
|
'realpath' imported but unused |
reviewbot
|
|
|
'reviewboard' imported but unused |
reviewbot
|
|
|
This should go in another import group because it is from the reviewboard package. |
brennie
|
|
|
You should use os.path.dirname(reviewboard.__file__) instead of joining '..' twice. Your code makes it seem as if you are moving up … |
brennie
|
|
|
Add a blank line between these two. |
david
|
|
|
'reviewboard' imported but unused |
reviewbot
|
|
|
It might be nice to align everything to 4 space boundaries: warnings.warn( 'The development preparation process exited before completing. In … |
david
|
|
|
'reviewboard' imported but unused |
reviewbot
|
|
|
Something to note here.. Exiting at any point during the syncdb may do more than fail to create a superuser. … |
 chipx86
|
|
|
'reviewboard' imported but unused |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
'reviewboard' imported but unused |
reviewbot
|
VI
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 2 (+8 -1) |
-
Tool: PEP8 Style Checker Processed Files: contrib/internal/prepare-dev.py -
contrib/internal/prepare-dev.py (Diff revision 2) Col: 1 E128 continuation line under-indented for visual indent
-
-
contrib/internal/prepare-dev.py (Diff revision 2) Col: 27 W604 backticks are deprecated, use 'repr()'
-
-
contrib/internal/prepare-dev.py (Diff revision 2) Col: 54 E226 missing whitespace around arithmetic operator
VI
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 3 (+8 -1) |
-
Tool: Pyflakes Processed Files: contrib/internal/prepare-dev.py Tool: PEP8 Style Checker Processed Files: contrib/internal/prepare-dev.py
VI
Description: |
|
|||||||||
|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
-
You don't need to mention the issue number in the title and the summary. Since its in the
bugsfield, it will automatically be added to the commit message. when it gets landed. -
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.
VI
Summary: |
|
||||||
|---|---|---|---|---|---|---|---|
Description: |
|
-
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"
-
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
tryaftersync_database?
VI
Description: |
|
|---|
-
Tool: PEP8 Style Checker Processed Files: contrib/internal/prepare-dev.py Tool: Pyflakes Processed Files: contrib/internal/prepare-dev.py
-
-
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.
VI
Summary: |
|
||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||||||||
Commit: |
|
||||||||||||||||||
Diff: |
Revision 5 (+16 -9) |
-
Tool: Pyflakes Processed Files: contrib/internal/prepare-dev.py Tool: PEP8 Style Checker Processed Files: contrib/internal/prepare-dev.py -
VI
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 6 (+15 -8) |
-
Tool: Pyflakes Processed Files: contrib/internal/prepare-dev.py Tool: PEP8 Style Checker Processed Files: contrib/internal/prepare-dev.py
-
-
contrib/internal/prepare-dev.py (Diff revision 6) "preparing process" sounds a little awkward. How about, "development preparation process"
-
-
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?
VI
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 7 (+17 -9) |
-
Tool: Pyflakes Processed Files: contrib/internal/prepare-dev.py Tool: PEP8 Style Checker Processed Files: contrib/internal/prepare-dev.py -
-
VI
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 8 (+18 -9) |
-
Tool: Pyflakes Processed Files: contrib/internal/prepare-dev.py Tool: PEP8 Style Checker Processed Files: contrib/internal/prepare-dev.py -
contrib/internal/prepare-dev.py (Diff revision 8) Col: 13 E128 continuation line under-indented for visual indent
VI
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 9 (+19 -9) |
-
Tool: PEP8 Style Checker Processed Files: contrib/internal/prepare-dev.py Tool: Pyflakes Processed Files: contrib/internal/prepare-dev.py
-
-
-
contrib/internal/prepare-dev.py (Diff revision 9) Instead of doing this, you can
import reviewboardand usereviewboard.__file__
VI
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 10 (+20 -8) |
-
Tool: PEP8 Style Checker Processed Files: contrib/internal/prepare-dev.py Tool: Pyflakes Processed Files: contrib/internal/prepare-dev.py -
-
VI
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 11 (+20 -8) |
-
Tool: PEP8 Style Checker Processed Files: contrib/internal/prepare-dev.py Tool: Pyflakes Processed Files: contrib/internal/prepare-dev.py -
-
-
contrib/internal/prepare-dev.py (Diff revision 11) This should go in another import group because it is from the
reviewboardpackage. -
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
'..', '..'). Usingdirnamemakes this more clear.
VI
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 12 (+19 -8) |
-
Tool: PEP8 Style Checker Processed Files: contrib/internal/prepare-dev.py Tool: Pyflakes Processed Files: contrib/internal/prepare-dev.py -
-
-
-
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__), '..')))
VI
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 13 (+19 -8) |
-
Tool: PEP8 Style Checker Processed Files: contrib/internal/prepare-dev.py Tool: Pyflakes Processed Files: contrib/internal/prepare-dev.py -
-
-
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.warnis the wrong thing here. That's more for programattic warning involving something like a deprecated module or function being used. I would usesys.stderr.write(..)instead.
VI
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 14 (+19 -8) |
-
Tool: PEP8 Style Checker Processed Files: contrib/internal/prepare-dev.py Tool: Pyflakes Processed Files: contrib/internal/prepare-dev.py -
-
VI
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 15 (+19 -8) |
-
Tool: PEP8 Style Checker Processed Files: contrib/internal/prepare-dev.py Tool: Pyflakes Processed Files: contrib/internal/prepare-dev.py -
Tool: PEP8 Style Checker Processed Files: contrib/internal/prepare-dev.py Tool: Pyflakes Processed Files: contrib/internal/prepare-dev.py