Use psql to run migrations #73
Conversation
k4nar
requested review from
ewjoachim,
mgu,
nadege and
samuelhamard
as code owners
There was a problem hiding this comment.
I'm wondering if it would be worth it to keep a Psycopg2Executor alongside a PsqlExecutor. We still entertain the idea that septentrion could evolve to a tool that is not solely centered around PeopleDoc migrations. Doesn't have to be, though. What's your take ?
|
@ewjoachim : I think keeping two different runners would make things more complicated to explain & maintain. The code that is being replaced was written specifically for PeopleDoc migrations, to handle the The new code is still handling the I do understand that adding a direct dependency to the psql binary is a big downside though. But it's the only solution I see for our internal issue :( . |
septentrion/runner.py
Outdated
| for name in ['HOST', 'PORT', 'DBNAME', 'USERNAME', 'PASSWORD']: | ||
| value = getattr(self.settings, name) | ||
| if value: | ||
| creds += ['--' + name.lower(), value] |
There was a problem hiding this comment.
I think it would be much clearer to pass all those via environment. What do you think ?
There was a problem hiding this comment.
If I define env vars, I must also pass mandatory env vars like PATH, otherwise psql won't be found. I guess I could do something like env={'PGHOST': self.settings.HOST [...], **os.environ}, but I don't know if it's a good practice.
There was a problem hiding this comment.
Yes, I was thinking of doing that. Is there any reason this wouldn't be a good practice ?
(I really wish subprocess had an "additional_env" or "env_override", but eh).
There was a problem hiding this comment.
psql will prioritize the cmd arguments over the env vars. I'm not sure of the priority in Septentrion, but passing explicit values to psql will ensure that we don't end up in a situation were Septentrion connects to a DB, but psql launches the migrations in another one.
There was a problem hiding this comment.
not convinced but this can be dealt with later.
|
Ok, let's go with psql and no psycopg2. Does this means we can get rid of the dependency ? I think not from the top of my head, but I haven't checked. |
|
This is now ready for review. |
| @@ -22,7 +22,7 @@ | |||
| folder.expanduser().resolve() / DEDICATED_CONFIGURATION_FILENAME | |||
| for folder in CONFIGURATION_PATHS | |||
| ] | |||
| print(DEDICATED_CONFIGURATION_FILES) | |||
|
|
|||
septentrion/runner.py
Outdated
| for name in ['HOST', 'PORT', 'DBNAME', 'USERNAME', 'PASSWORD']: | ||
| value = getattr(self.settings, name) | ||
| if value: | ||
| creds += ['--' + name.lower(), value] |
There was a problem hiding this comment.
not convinced but this can be dealt with later.
Codecov Report
@@ Coverage Diff @@
## master #73 +/- ##
==========================================
+ Coverage 76.41% 77.51% +1.09%
==========================================
Files 15 15
Lines 687 627 -60
Branches 92 79 -13
==========================================
- Hits 525 486 -39
+ Misses 147 130 -17
+ Partials 15 11 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
Thanks @ewjoachim ! |
Septentrion is not compatible with migrations containing
psqlinstructions. Those can be quite useful for data migrations sometimes (eg: to use\gsetor\gexec).This PR tries to replace the migration runner by a wrapper around
psql. This is still a WIP.