Use sybpydb for Sybase access on Py3 #7
Conversation
| @@ -170,7 +174,7 @@ def fetch(self, expr, vals=None,): | |||
|
|
|||
| :rtype: Generator that will get one row of database as dict() via next() | |||
| """ | |||
| self.execute(expr, vals) | |||
| self.execute(expr, vals, commit=False) | |||
There was a problem hiding this comment.
This change will apply to sqlite as well. I can code around it (make conditional for sybase), but I think it is ok? For sybase, if the "select" statement is "committed", the cursor has nothing left to do and apparently cannot return the results.
There was a problem hiding this comment.
I have no idea if it's OK in sqlite, but I guess if tests and ska_testr pass then we're good.
There was a problem hiding this comment.
Thanks. The Python sqlite docs suggest to me that this is a no-op difference on that side https://docs.python.org/3/library/sqlite3.html#controlling-transactions , and that if we really care we probably need to add isolation_level control to the DBI init.
|
With the updated flt_envs, all module tests pass on CentOS5, 6, and 7 HEAD machines. |
|
I note the test are also correctly skipped on chimchim and the sqlite tests still pass. |
|
I'm a little confused about testing because of: This would lead me to think that on Ska3 all sybase tests are being skipped. I was originally wondering if sybase insertion is working since the code logic would now use the sqlite syntax for a sybase insert with |
|
Ah. Good point. I forgot I had a testing commit that was not pushed. Though I'm not sure I follow your second sentence. |
|
Regarding |
Ska/DBI/tests/test_dbi.py
Outdated
| import os | ||
| import pytest | ||
| import numpy as np | ||
| from Ska.DBI import DBI | ||
|
|
||
| HAS_SYBASE = six.PY2 | ||
|
|
||
| module = os.path.join(os.environ['SYBASE'], os.environ['SYBASE_OCS'], |
There was a problem hiding this comment.
This is going to raise an exception if those env vars are not defined.
There was a problem hiding this comment.
Good point. I just tested in "production-like" environments so far so hadn't considered/noticed. Which brings up the separate point that I could use some automated testing because our requirements are becoming all combinations of CentOS 5,6,7 production, 6,7 root not production, and 6,7 non-root + OSX root and non-root, + CentOS5 32 production, + Windows whatever.
| elif self.dbi == 'sybase': | ||
| if replace: | ||
| raise ValueError('Using replace=True not allowed for Sybase DBI') | ||
| colrepls = tuple('@'+x for x in cols) |
There was a problem hiding this comment.
The point of my other comment is that previously, calling with dbi='sybase' and replace=False would do the insert but using a different definition for colrepls and vals than used for dbi='sqlite3'. With this current code the sybase insert is done using the same syntax as for sqlite3. I was just wondering if that actually works now (API change in the DB interface?).
There was a problem hiding this comment.
Well, it isn't really an API change in the interface because this is a different interface, but my assumption was yes. And apologies it took me a bit to catch up as I thought you'd already seen the code simplification.
I largely just fiddled until manual unit tests and the existing tests passed, so didn't much reference the docs. So if our insert tests are not sufficient across data types or the like we may end up with errors later.
When you say "actually works now" I didn't think that we'd get test passing without it working, though when I was fiddling with inserts I did use sqsh to eyeball the test db and confirmed stuff was getting into it.
|
Thanks again for the catch on the env vars issue. I just set it to check for the SYBASE_OCS variable first before looking for the module path (built from env vars) in the latest commit. I did decide that checking for SYBASE_OCS was sufficient (and am not checking for SYBASE, SYBASE_OCS, and correct LD_LIBRARY_PATH in the test Ska.DBI.DBI). If the user has SYBASE_OCS set but not SYBASE, that's on them. |
|
Sounds fine. |
Use sybpydb for Sybase access on Py3