tickets/DM-2139 #2
Conversation
Code transferred from cat/bin/schema_to_metadata.py Converted the code to a class. Shortened function names. Added pretty print, and temporarily hardcoded input.
schema version was done through "CREATE TABLE AAA_Version_3_2_4 (version CHAR)" and description wasn't captured well. We also tried to capture file name and output from "git describe". This is now captured as data loaded into ZZZ_Schema_Description table
| ./tests.reinit.py | ||
| ./bin/metaBackend.py regDb DC_W13_Stripe82 L2 | ||
| ./bin/metaBackend.py regDb jacek_1mRows L3 | ||
| # load the metaserv schema and load some data |
There was a problem hiding this comment.
What are the prerequisites? Does it need mysql server running, accounts created?
| mysql --defaults-file=~/.lsst/dbAuth-dbServ.txt -e "create database XXX_metaServ_core" | ||
| mysql --defaults-file=~/.lsst/dbAuth-metaServ.txt < sql/global.sql | ||
| mysql --defaults-file=~/.lsst/dbAuth-metaServ.txt < sql/dbRepo.sql | ||
| mysql --defaults-file=~/.lsst/dbAuth-metaServ.txt < sql/fileRepo.sql |
There was a problem hiding this comment.
Why there are two defaults files here. If they are only different in database name then should we need both of them? Can you use dbAuth-metaServ.txt for all commands?
There was a problem hiding this comment.
No I can't. I can't do the second line to be precise (after I drop database, mysql refuses to connect when database=<just_deleted_db> is specified i the config file)
There was a problem hiding this comment.
Could you possibly provide database name on the command line to override the one in config file? Something like --database ="" might work.
|
git style: Shouldn't these commits be squashed pre-review? At least one comment looks like it is improperly formatted. Good habits are best maintained when used religiously. |
|
For fixed string comparisons, I think it's a lot more readable to use if ' DEFAULT ' not in str:than if re.search(' DEFAULT ', str) is None:which, as AndyS points out, should usually be more like: if not re.search(r' DEFAULT ', str):anyway. Of course, if you really want |
|
I did tons of squashing, what I ended up with, (and what you see) is what I considered a sweet spot. I do squash a lot. |
| (3050, "INST_EXISTS", "Institution already exists.."), | ||
| (3055, "INST_NOT_FOUND", "Institution not found."), | ||
| (9998, "NOT_IMPLEMENTED", "Feature not implemented yet."), | ||
| (9999, "INTERNAL", "Internal error.")]) |
There was a problem hiding this comment.
IF these numbers do not have any meaning they should not exist then. And we probably should revisit produceExceptionClass at some point, there are some things about it that bother me.
There was a problem hiding this comment.
Can I suggest that it's not a good idea to have lines that are aligned like this? While it does make it very readable right now, if you want to add another line with a name that's just a bit longer, you have two options, neither of which is particularly desirable:
- Re-do the alignment. But that means stealing the attribution on the lines so that
git blameisn't helpful. - Leave it as it is. But that defeats the purpose of aligning them to begin with.
There was a problem hiding this comment.
Hi Paul, point taken, but it is so convenient to read it that way! How about "3. Re-adjust alignment in one commit, and add the extra line in separate commit". Would that reduce your concern?
There was a problem hiding this comment.
That's an option, though I would point out it's more work, and git blame still shows the lines as belonging all together so it's harder to dig into the history (though the vc mode in emacs helps, as it allows you to dig with just a couple of key presses). I'm not saying don't merge because of this, it's just an observation that I hope may be helpful.
There was a problem hiding this comment.
I don't think the blame argument is very convincing given that you can ignore whitespace commits with git blame -w.
…Switched to python logging
Reviewing DM-2139