web-platform-tests / pulls.web-platform-tests.org

[Deprecated] Some functionalities are now provided by wpt-pr-bot https://github.com/web-platform-tests/wpt-pr-bot
7 stars 23 forks source link

Add db migration #19

Closed bobholt closed 7 years ago

bobholt commented 7 years ago

This is a pre-requisite for adding a column to the pull_request table to fix the multi-comment situation (and for future database changes we want to perform on a live server).

Note this includes the standalone certbot versioning fix (https://github.com/w3c/wptdash/pull/18) as a different commit SHA. If that lands, this should be rebased.


This change is Reviewable

jgraham commented 7 years ago

Reviewed 8 of 8 files at r1. Review status: all files reviewed at latest revision, 3 unresolved discussions.


migrations/env.py, line 1 at r1 (raw file):

from __future__ import with_statement

Surely this isn't needed here?


migrations/env.py, line 17 at r1 (raw file):


# add your model's MetaData object here
# for 'autogenerate' support

Let's remove some of the commented out copy-paste (and maybe make the comments more relevant).


migrations/README, line 1 at r1 (raw file):

Generic single-database configuration.

This doesn't seem too helpful. Can we link to wherever this configuration came from?


Comments from Reviewable

bobholt commented 7 years ago

Review status: all files reviewed at latest revision, 3 unresolved discussions.


migrations/env.py, line 1 at r1 (raw file):

Previously, jgraham wrote…
Surely this isn't needed here?

This file was auto-generated by flask-migrate, so I didn't even look at it. I don't believe it will by any other automated scripts, so I'll clean it up.


migrations/README, line 1 at r1 (raw file):

Previously, jgraham wrote…
This doesn't seem too helpful. Can we link to wherever this configuration came from?

Another auto-generated file. I can remove it or add a note that all files in this directory are auto-generated by the migration script.


Comments from Reviewable

jgraham commented 7 years ago

Review status: all files reviewed at latest revision, 3 unresolved discussions.


migrations/README, line 1 at r1 (raw file):

Previously, bobholt (Bob Holt) wrote…
Another auto-generated file. I can remove it or add a note that all files in this directory are auto-generated by the migration script.

I assume the idea is that one cleans this up. But either way is fine.


Comments from Reviewable

bobholt commented 7 years ago

Removed the README and cleaned up some comments. Remaining comments seem useful to me.


Review status: all files reviewed at latest revision, 3 unresolved discussions.


migrations/env.py, line 17 at r1 (raw file):

Previously, jgraham wrote…
Let's remove some of the commented out copy-paste (and maybe make the comments more relevant).

Done.


migrations/README, line 1 at r1 (raw file):

Previously, jgraham wrote…
I assume the idea is that one cleans this up. But either way is fine.

Done.


Comments from Reviewable

jgraham commented 7 years ago

Reviewed 3 of 3 files at r2. Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable