Summary

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

Review Request #6845 — Created Jan. 31, 2015 and submitted Feb. 17, 2015, 12:21 p.m.

Information

Owner

viettran1989

Repository

Review Board

Branch

master

Bugs

3714

Depends On

Commit

255221a...

Reviewers

Groups

reviewboard, students

People

Description

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.

Testing Done

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.

Issues

Description	From	Last Updated
Col: 80 E501 line too long (94 > 79 characters)	reviewbot	Jan. 31, 2015, 10:48 a.m.
Col: 1 E128 continuation line under-indented for visual indent	reviewbot	Jan. 31, 2015, 10:48 a.m.
Col: 74 W291 trailing whitespace	reviewbot	Jan. 31, 2015, 10:48 a.m.
Col: 27 W604 backticks are deprecated, use 'repr()'	reviewbot	Jan. 31, 2015, 10:48 a.m.
Col: 51 E225 missing whitespace around operator	reviewbot	Jan. 31, 2015, 10:48 a.m.
Col: 54 E226 missing whitespace around arithmetic operator	reviewbot	Jan. 31, 2015, 10:48 a.m.
We are inconsistent all over the code base, but for all new strings we want to be using single quotes. …	brennie	Jan. 31, 2015, 3:12 p.m.
We probably don't want to print this out if there's an interrupt. Maybe move it inside the try after sync_database?	david	Jan. 31, 2015, 3:53 p.m.
Can you remove the extra blank line you added here? We generally keep 2 blank lines between top-level functions.	anselina	Jan. 31, 2015, 5:08 p.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Jan. 31, 2015, 5:08 p.m.
"preparing process" sounds a little awkward. How about, "development preparation process"	mike_conley	Feb. 1, 2015, 6:47 p.m.
"create super user" -> "create a super user"	mike_conley	Feb. 1, 2015, 6:47 p.m.
/src/reviewboard isn't necessarily correct - that's probably only true if the developer is using vagrant. We can probably dynamically figure …	mike_conley	Feb. 1, 2015, 6:48 p.m.
Col: 80 E501 line too long (86 > 79 characters)	reviewbot	Feb. 1, 2015, 7:17 p.m.
Col: 68 E231 missing whitespace after ','	reviewbot	Feb. 1, 2015, 7:17 p.m.
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	Feb. 1, 2015, 7:27 p.m.
Undo this change.	brennie	Feb. 3, 2015, 11:53 a.m.
Instead of doing this, you can import reviewboard and use reviewboard.__file__	brennie	Feb. 3, 2015, 11:54 a.m.
'reviewboard' imported but unused	reviewbot	Feb. 3, 2015, 11:57 a.m.
'realpath' imported but unused	reviewbot	Feb. 3, 2015, 11:57 a.m.
'reviewboard' imported but unused	reviewbot	Feb. 3, 2015, 11:58 a.m.
This should go in another import group because it is from the reviewboard package.	brennie	Feb. 6, 2015, 10:01 a.m.
You should use os.path.dirname(reviewboard.__file__) instead of joining '..' twice. Your code makes it seem as if you are moving up …	brennie	Feb. 6, 2015, 10:01 a.m.
Add a blank line between these two.	david	Feb. 7, 2015, 6:44 p.m.
'reviewboard' imported but unused	reviewbot	Feb. 6, 2015, 10:02 a.m.
It might be nice to align everything to 4 space boundaries: warnings.warn( 'The development preparation process exited before completing. In …	david	Feb. 7, 2015, 6:44 p.m.
'reviewboard' imported but unused	reviewbot	Feb. 7, 2015, 8:40 p.m.
Something to note here.. Exiting at any point during the syncdb may do more than fail to create a superuser. …	chipx86	Feb. 8, 2015, 1 p.m.
'reviewboard' imported but unused	reviewbot	Feb. 8, 2015, 1:01 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Feb. 8, 2015, 1:03 p.m.
'reviewboard' imported but unused	reviewbot	Feb. 8, 2015, 4:55 p.m.

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 1)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (94 > 79 characters)
```

Commit:

-	ab76a8672d1dcb0bdb3ee773836dae1452c1319a
+	b0290fbf447714e8f37ce2ed23b29f0040d0c69a

Diff:

Revision 2 (+8 -1)

Show changes

contrib/internal/prepare-dev.py

Tool: PEP8 Style Checker
Processed Files:
    contrib/internal/prepare-dev.py

contrib/internal/prepare-dev.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 1
 E128 continuation line under-indented for visual indent
```
contrib/internal/prepare-dev.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 74
 W291 trailing whitespace
```
contrib/internal/prepare-dev.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 27
 W604 backticks are deprecated, use 'repr()'
```
contrib/internal/prepare-dev.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 51
 E225 missing whitespace around operator
```
contrib/internal/prepare-dev.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 54
 E226 missing whitespace around arithmetic operator
```

Commit:

-	b0290fbf447714e8f37ce2ed23b29f0040d0c69a
+	6629785c2da3d27aefe00fe5bfc10f44f9dcb259

Diff:

Revision 3 (+8 -1)

Show changes

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

Description:

~		Issue-3714: print out warning when interrupt
	~	Issue-3714: Print out warning when interrupt. The database reviewboard.db only created when site.sync_database is called. I put a try catch KeyBoardInterrupt around this to print out the warning.

Testing Done:

-		The database reviewboard.db only created when site.sync_database is called. I put a try catch KeyBoardInterrupt around this to print out the warning.
-
		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.

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.

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

The issue has been resolved. Show all issues

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.

Summary:

-	Issue-3714: print out warning when interrupt
+	Print out warning when interrupt

Description:

~		Issue-3714: Print out warning when interrupt. The database reviewboard.db only created when site.sync_database is called. I put a try catch KeyBoardInterrupt around this to print out the warning.
	~	Print out warning when interrupt. The database reviewboard.db only created when site.sync_database is called. I put a try catch KeyBoardInterrupt around this to print out the warning.

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"

david Jan. 31, 2015, 4:08 p.m.

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

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

The issue has been resolved. Show all issues

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

viettran1989 Jan. 31, 2015, 3:25 p.m.

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?

david Jan. 31, 2015, 3:27 p.m.

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

VI

viettran1989 Jan. 31, 2015, 3:53 p.m.
```
That might work. Fixed that.
```

Description:

~		Print out warning when interrupt. The database reviewboard.db only created when site.sync_database is called. I put a try catch KeyBoardInterrupt around this to print out the warning.
	~	Print out warning when interrupt. The database reviewboard.db only created when site.sync_database is called. I put a try catch KeyBoardInterrupt around this to print out the warning.
	+	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.

Diff:

Revision 4 (+15 -7)

Show changes

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

Status: Discarded

Status: Re-opened

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

The issue has been resolved. Show all issues

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

Summary:

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

Description:

~		Print out warning when interrupt. The database reviewboard.db only created when site.sync_database is called. I put a try catch KeyBoardInterrupt around this to print out the warning.
~		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.
	~	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.

Commit:

+	fd04c35d78223f4e2d4b9d85b4108eaaf633ab8c

Diff:

Revision 5 (+16 -9)

Show changes

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

contrib/internal/prepare-dev.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Col: 1
 W293 blank line contains whitespace
```

Commit:

-	fd04c35d78223f4e2d4b9d85b4108eaaf633ab8c
+	026f57da79b3f731612f90f1152b0d620be2efff

Diff:

Revision 6 (+15 -8)

Show changes

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

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

The issue has been resolved. Show all issues

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

contrib/internal/prepare-dev.py (Diff revision 6)
The issue has been resolved. Show all issues
```
"create super user" -> "create a super user"
```

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

The issue has been resolved. Show all issues

/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?

Commit:

-	026f57da79b3f731612f90f1152b0d620be2efff
+	98e94b45e5d784266fe1123e15124036ff5dac98

Diff:

Revision 7 (+17 -9)

Show changes

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

contrib/internal/prepare-dev.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (86 > 79 characters)
```
contrib/internal/prepare-dev.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Col: 68
 E231 missing whitespace after ','
```

Commit:

-	98e94b45e5d784266fe1123e15124036ff5dac98
+	106e06bad1cecf22a50658f410dec0b10bbf67f2

Diff:

Revision 8 (+18 -9)

Show changes

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

contrib/internal/prepare-dev.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Col: 13
 E128 continuation line under-indented for visual indent
```

Commit:

-	106e06bad1cecf22a50658f410dec0b10bbf67f2
+	211ce179634424706fd6f163fe6b43e2b8ec3ac1

Diff:

Revision 9 (+19 -9)

Show changes

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

contrib/internal/prepare-dev.py (Diff revision 9)
The issue has been resolved. Show all issues
```
Undo this change.
```

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

The issue has been resolved. Show all issues

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

viettran1989 Feb. 8, 2015, 9:30 a.m.

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.

Commit:

-	211ce179634424706fd6f163fe6b43e2b8ec3ac1
+	005e8040d391cda64d80474c776d626e1690f25b

Diff:

Revision 10 (+20 -8)

Show changes

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

contrib/internal/prepare-dev.py (Diff revision 10)
The issue has been dropped. Show all issues
```
 'reviewboard' imported but unused
```
1. VI
  
  viettran1989 Feb. 4, 2015, 4:57 p.m.
  Seem like a bug since I actually use reviewboard.file
contrib/internal/prepare-dev.py (Diff revision 10)
The issue has been resolved. Show all issues
```
 'realpath' imported but unused
```

Commit:

-	005e8040d391cda64d80474c776d626e1690f25b
+	9a1a97e3a8ab98435e98d2f7a9bbdf25f1cabd8a

Diff:

Revision 11 (+20 -8)

Show changes

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

contrib/internal/prepare-dev.py (Diff revision 11)
The issue has been dropped. Show all issues
```
 'reviewboard' imported but unused
```
1. VI
  
  viettran1989 Feb. 4, 2015, 4:57 p.m.
  I did use reviewboard.file.

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

The issue has been resolved. Show all issues

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

viettran1989 Feb. 4, 2015, 8:39 a.m.

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

brennie Feb. 5, 2015, 11:06 a.m.

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)

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

The issue has been resolved. Show all issues

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.

Commit:

-	9a1a97e3a8ab98435e98d2f7a9bbdf25f1cabd8a
+	53f251fb7d9645b0a2782259a6bd37dfad7da928

Diff:

Revision 12 (+19 -8)

Show changes

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

contrib/internal/prepare-dev.py (Diff revision 12)
The issue has been dropped. Show all issues
```
 'reviewboard' imported but unused
```

contrib/internal/prepare-dev.py (Diff revision 12)
The issue has been resolved. Show all issues
```
Add a blank line between these two.
```

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

The issue has been resolved. Show all issues

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__), '..')))

Commit:

-	53f251fb7d9645b0a2782259a6bd37dfad7da928
+	f9b7a449daa8b7e92f2f9f1210fd8b117b13745c

Diff:

Revision 13 (+19 -8)

Show changes

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

contrib/internal/prepare-dev.py (Diff revision 13)
The issue has been dropped. Show all issues
```
 'reviewboard' imported but unused
```

Ship it!

```
Code looks good to me.
```

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

The issue has been resolved. Show all issues

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.

Commit:

-	f9b7a449daa8b7e92f2f9f1210fd8b117b13745c
+	574147d9fac16c2790b38db0bd6f2376b81b5371

Diff:

Revision 14 (+19 -8)

Show changes

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

contrib/internal/prepare-dev.py (Diff revision 14)
The issue has been dropped. Show all issues
```
 'reviewboard' imported but unused
```
contrib/internal/prepare-dev.py (Diff revision 14)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```

Commit:

-	574147d9fac16c2790b38db0bd6f2376b81b5371
+	255221a4d64da657d51c0f90fe2287d6ecfb3776

Diff:

Revision 15 (+19 -8)

Show changes

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

contrib/internal/prepare-dev.py (Diff revision 15)
The issue has been dropped. Show all issues
```
 'reviewboard' imported but unused
```

Ship it!

I'm going to make some small changes to the wording and push this. Thanks!

Status: Closed (submitted)

Change Summary:

Pushed to release-2.0.x (449c614)