Merge lp:~therp-nl/openupgrade-server/7.0_lp1265463_use_password into lp:openupgrade-server

Proposed by Ronald Portier (Therp)
Status: Merged
Merged at revision: 4644
Proposed branch: lp:~therp-nl/openupgrade-server/7.0_lp1265463_use_password
Merge into: lp:openupgrade-server
Diff against target: 74 lines (+38/-8)
1 file modified
scripts/migrate.py (+38/-8)
To merge this branch: bzr merge lp:~therp-nl/openupgrade-server/7.0_lp1265463_use_password
Reviewer Review Type Date Requested Status
Holger Brunn (Therp) code review Approve
Stefan Rijnhart (Opener) Needs Information
Ronald Portier (Therp) Needs Resubmitting
Review via email: mp+200301@code.launchpad.net

Description of the change

Migrate script will use all database connection info found in the config file that is already a required parameter.

To post a comment you must log in.
Revision history for this message
Holger Brunn (Therp) (hbrunn) wrote :

10ff: You need to check if this option exists (has_option) before accessing it, as also SafeConfigParser raises ConfigParser.NoOptionError on nonexisting options. Then you can remove 12f

Could you explain why you do the extra checks and unset PGPASSWORD? The environment you change will die with the script process, and the only other processes who see it are the openupgrade servers it starts, or am I mistaken?

review: Needs Fixing (code review)
4642. By Ronald Portier (Therp)

[FIX] Properly check for existance of option before use.

Revision history for this message
Ronald Portier (Therp) (rportier1962) wrote :

@Holger

I changed the code to properly test for the existance of a configuration key before use.

I took over the unsetting of PGPASSWORD from the database back-up code of standard OpenERP. I think it is not wrong to clear a plaintext password from memory as soon as possible. Although ofcourse there already is a certain security risk in having the password in a plain text config file in the first place.

review: Needs Resubmitting
Revision history for this message
Stefan Rijnhart (Opener) (stefan-opener) wrote :

By the looks of it, the whole environment manipulation is only done to copy a database. Would it be an idea to simply copy the database using the cursor, using

    CREATE DATABASE newdb WITH TEMPLATE originaldb OWNER dbuser;

?

review: Needs Information
Revision history for this message
Holger Brunn (Therp) (hbrunn) wrote :

That would be a splendid idea, if not this choked if originaldb is in use, whereas pg_dump can work on locked databases. Some people might be brave or careless enough to run the script on their production database...

Thanks for the other changes!

review: Approve (code review)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'scripts/migrate.py'
2--- scripts/migrate.py 2013-05-09 13:49:52 +0000
3+++ scripts/migrate.py 2014-01-20 21:57:21 +0000
4@@ -94,13 +94,25 @@
5
6 config.read(options.config)
7
8-db_user=config.get('options', 'db_user')
9+conn_parms = {}
10+for parm in ('host', 'port', 'user', 'password'):
11+ db_parm = 'db_' + parm
12+ if config.has_option('options', db_parm):
13+ conn_parms[parm] = config.get('options', db_parm)
14+
15+if not 'user' in conn_parms:
16+ print 'No user found in configuration'
17+ sys.exit()
18+db_user = conn_parms['user']
19+
20 db_name=options.database or config.get('options', 'db_name')
21
22 if not db_name or db_name=='' or db_name.isspace() or db_name.lower()=='false':
23 parser.print_help()
24 sys.exit()
25
26+conn_parms['database'] = db_name
27+
28 if options.inplace:
29 db=db_name
30 else:
31@@ -185,18 +197,36 @@
32 if not options.inplace:
33 print('copying database %(db_name)s to %(db)s...' % {'db_name': db_name,
34 'db': db})
35- conn=psycopg2.connect(database=db_name, user=db_user)
36+ conn = psycopg2.connect(**conn_parms)
37 conn.set_isolation_level(psycopg2.extensions.ISOLATION_LEVEL_AUTOCOMMIT)
38 cur=conn.cursor()
39 cur.execute('drop database if exists "%(db)s"' % {'db': db})
40 cur.execute('create database "%(db)s"' % {'db': db})
41 cur.close()
42- os.system(('pg_dump --format=custom --user=%(user)s %(db_name)s | pg_restore '+
43- '--dbname=%(db)s') % {
44- 'user': db_user,
45- 'db_name': db_name,
46- 'db': db,
47- })
48+
49+ os.environ['PGUSER'] = db_user
50+ if ('host' in conn_parms and conn_parms['host']
51+ and not os.environ.get('PGHOST')):
52+ os.environ['PGHOST'] = conn_parms['host']
53+
54+ if ('port' in conn_parms and conn_parms['port']
55+ and not os.environ.get('PGPORT')):
56+ os.environ['PGPORT'] = conn_parms['port']
57+
58+ password_set = False
59+ if ('password' in conn_parms and conn_parms['password']
60+ and not os.environ.get('PGPASSWORD')):
61+ os.environ['PGPASSWORD'] = conn_parms['password']
62+ password_set = True
63+
64+ os.system(
65+ ('pg_dump --format=custom --no-password %(db_name)s ' +
66+ '| pg_restore --no-password --dbname=%(db)s') %
67+ {'db_name': db_name, 'db': db}
68+ )
69+
70+ if password_set:
71+ del os.environ['PGPASSWORD']
72
73 for version in options.migrations.split(','):
74 print 'running migration for '+version

Subscribers

People subscribed via source and target branches