Merge lp:~openstack-ubuntu-packagers/glance/fix-sql-connection into lp:~openstack-ubuntu-packagers/glance/ubuntu

Proposed by Jay Pipes on 2011-05-05
Status: Needs review
Proposed branch: lp:~openstack-ubuntu-packagers/glance/fix-sql-connection
Merge into: lp:~openstack-ubuntu-packagers/glance/ubuntu
Diff against target: 12 lines (+1/-1)
1 file modified
debian/glance.postinst (+1/-1)
To merge this branch: bzr merge lp:~openstack-ubuntu-packagers/glance/fix-sql-connection
Reviewer Review Type Date Requested Status
Dan Prince (community) Abstain on 2011-05-06
Soren Hansen (community) 2011-05-05 Disapprove on 2011-05-06
Devin Carlen (community) Approve on 2011-05-05
Review via email: mp+60070@code.launchpad.net

Description of the change

Just removes an extraneous slash in the SQL connection string...

To post a comment you must log in.
Devin Carlen (devcamcar) wrote :

lgtm

review: Approve
Dan Prince (dan-prince) wrote :

Looks good.

review: Approve
Soren Hansen (soren) wrote :

I don't think it's extraneous: http://www.sqlalchemy.org/docs/dialects/sqlite.html (search for "four slashes").

If it doesn't match the default config, then that's what needs fixing, probably.

review: Disapprove
Dan Prince (dan-prince) wrote :

Hey Soren,

You are correct. Good catch.

After a bit more investigation it looks like we have a couple things going that were causing my problems.

Yesterday the glance deb package source was changed so that it actually tries to run 'glance-manage db_sync' during an installation (bzr 29). On a clean machine this is actually causes an error.

See this ticket for details:

https://bugs.launchpad.net/glance/+bug/778463

In any case I no longer think we need this. At least not without changing the default string in Glance as well.

Dan

review: Abstain
Soren Hansen (soren) wrote :

2011/5/6 Dan Prince <email address hidden>:
> Yesterday the glance deb package source was changed so that it actually tries to run 'glance-manage db_sync' during an installation (bzr 29). On a clean machine this is actually causes an error.
>
> See this ticket for details:
>
> https://bugs.launchpad.net/glance/+bug/778463

That looks completely unrelated. I'll fix that up right away.

> In any case I no longer think we need this.

Oh? What changed?

--
Soren Hansen        | http://linux2go.dk/
Ubuntu Developer    | http://www.ubuntu.com/
OpenStack Developer | http://www.openstack.org/

Dan Prince (dan-prince) wrote :

One more thing to note. Maybe the point of confusion here is that patches/sql_conn.patch uses the extra slash where the default glance.conf.sample file in the actual glance source tree doesn't.

Soren Hansen (soren) wrote :

2011/5/6 Dan Prince <email address hidden>:
> In any case I no longer think we need this. At least not without changing the default string in Glance as well.

Having read this again, I'm not sure at all what "this" refers to here?

--
Soren Hansen        | http://linux2go.dk/
Ubuntu Developer    | http://www.ubuntu.com/
OpenStack Developer | http://www.openstack.org/

Dan Prince (dan-prince) wrote :

> 2011/5/6 Dan Prince <email address hidden>:
> > In any case I no longer think we need this. At least not without changing
> the default string in Glance as well.
>
> Having read this again, I'm not sure at all what "this" refers to here?

By 'this' I meant the change from 4 to 3 slashes. It looks like Jay was trying to make this match what is in the glance source tree which uses 3 slashes.

So I agree with you in that we should change the glance source to match what we do in the PPA packages using 4 slashes.

Jay Pipes (jaypipes) wrote :

OK, no problem. We can change the Glance etc/ configs. Sorry, I
thought 3 slashes was correct, not 4. My mistake.

On Fri, May 6, 2011 at 8:49 AM, Dan Prince <email address hidden> wrote:
>> 2011/5/6 Dan Prince <email address hidden>:
>> > In any case I no longer think we need this. At least not without changing
>> the default string in Glance as well.
>>
>> Having read this again, I'm not sure at all what "this" refers to here?
>
> By 'this' I meant the change from 4 to 3 slashes. It looks like Jay was trying to make this match what is in the glance source tree which uses 3 slashes.
>
> So I agree with you in that we should change the glance source to match what we do in the PPA packages using 4 slashes.
> --
> https://code.launchpad.net/~openstack-ubuntu-packagers/glance/fix-sql-connection/+merge/60070
> Your team OpenStack Ubuntu packagers is subscribed to branch lp:~openstack-ubuntu-packagers/glance/ubuntu.
>

Unmerged revisions

30. By Jay Pipes on 2011-05-05

Remove extraneous slash in default sql_connection

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'debian/glance.postinst'
2--- debian/glance.postinst 2011-05-05 13:17:26 +0000
3+++ debian/glance.postinst 2011-05-05 14:41:18 +0000
4@@ -9,7 +9,7 @@
5 adduser --system --home /var/lib/glance --no-create-home --shell /bin/bash glance
6 fi
7 chown glance -R /var/lib/glance /var/log/glance
8- if ! grep sql_connection /etc/glance/glance.conf | grep -qv "sql_connection = sqlite:////var/lib/glance/glance.sqlite"
9+ if ! grep sql_connection /etc/glance/glance.conf | grep -qv "sql_connection = sqlite:///var/lib/glance/glance.sqlite"
10 then
11 su -c 'glance-manage db_sync' glance
12 fi

Subscribers

People subscribed via source and target branches