Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIX] runbot_travis2docker: Fix server options #107

Merged
merged 1 commit into from Nov 15, 2016

Conversation

lasley
Copy link
Contributor

@lasley lasley commented Nov 15, 2016

  • Fix interpolation on server options due to multi line

This should fix log issue reported in #105. Multiline string & .format weren't playing nicely, resulting in the following in SERVER_OPTIONS:

"--log-db=postgres://{cfg[db_user]}:{cfg[db_password]}@localhost/runbot"

@moylop260
Copy link
Contributor

Sorry, I don't understand what is wrong with the original runbot-formating

Could you help to understand?

@lasley
Copy link
Contributor Author

lasley commented Nov 15, 2016

@moylop260 We added #94 to allow for log-db to work using Travis2Docker. In #94, I used basically the same code you linked from Odoo.

The difference between our's and their's is PEP8. The string is multiline, but the format is only on the last. This PR makes the string itself single line in order for the interpolation to affect all of it instead of just the second part.

@lasley
Copy link
Contributor Author

lasley commented Nov 15, 2016

Contrived example:

>>> '{a[test]}'+ \
... '{a[test]}'.format(a={'test':'test val'})
'{a[test]}test val'

@moylop260
Copy link
Contributor

I got it
Thanks for clarify

* Fix interpolation on server options due to multi line
@lasley lasley force-pushed the bugfix/9.0/docker-server-options branch from 93564cc to e77e144 Compare November 15, 2016 21:18
@@ -121,8 +121,10 @@ def job_20_test_all(self, cr, uid, build, lock_path, log_path):
] + pr_cmd_env
logdb = cr.dbname
if config['db_host'] and not travis_branch.startswith('7.0'):
logdb = 'postgres://{cfg[db_user]}:{cfg[db_password]}@' +\
'{cfg[db_host]}/{db}'.format(cfg=config, db=cr.dbname)
logdb = 'postgres://%s:%s@%s/%s' % (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about use:

('postgres://{cfg[db_user]}:{cfg[db_password]}@'
 '{cfg[db_host]}/{db}').format(cfg=config, db=cr.dbname)

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO the string not spanning multiple lines looks better, but I have no explicit preference. I was also adhering to the OCA convention to use % over .format

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right!

@moylop260 moylop260 merged commit 8044166 into OCA:9.0 Nov 15, 2016
@moylop260
Copy link
Contributor

Thanks!

@lasley lasley deleted the bugfix/9.0/docker-server-options branch November 15, 2016 21:34
hbrunn pushed a commit that referenced this pull request Apr 10, 2018
* Fix interpolation on server options due to multi line
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants