Launchpad itself

Code review comment for lp:~stub/launchpad/trivial

trivial
Merge into devel

Revision history for this message

Christian Reis (kiko) wrote on 2009-07-24:

On Fri, Jul 24, 2009 at 02:18:08AM -0000, Stuart Bishop wrote:
> >> bin/run isn't suitable for use on production as it launches an appserver.
> >
> > Could it not be made to run the librarian as well? I guess at this point
> > whatever's easiest to do and causes less problems is sanest.
>
> It was easier to add the new script. The existing code runs daemons in
> foreground mode but puts them in the background using
> subprocess.Popen - it is designed for devs and around having the
> services shutdown when the Launchpad instance does. Its also

And for production deployments, what should be used to launch the
launchpad instances, and stuff like scripts and daemons? Do/should they
all have their own scripts in the same line as start_librarian?

> undocumented with no command line help, and that doesn't look fixable
> without maybe breaking other call sites that depend on it such as
> ec2test.

Can you file a bug on it being undocumented, maybe assign to Gary?

> >> === modified file 'lib/canonical/launchpad/scripts/runlaunchpad.py'
> >> - # We want to make sure that the Launchpad process will have the
> >> - # benefit # of all of the dependency paths inserted by the buildout
> >> - # bin/run script. We pass them via PYTHONPATH.
> >> - env = dict(os.environ)
> >> - env['PYTHONPATH'] = os.path.pathsep.join(sys.path)
> >
> > Will this not cause problems anywhere else?
>
> No - buildout handles this for us now by building _pythonpath.py will
> all the libraries we are supposed to need. If you don't like it, we
> can land this bit another time - it is harmless and just a drive by
> cleanup to keep our variables down when trying to debug things.

Well, we're likely to test this works on a production-like box before
actually updating the librarian, so I think this is fine. rc=kiko or can
I do something like

approve
merge-approved

to do this via email? :-)
--
Christian Robottom Reis | [+55 16] 3376 0125 | http://launchpad.net/~kiko
| [+55 16] 9112 6430 | http://async.com.br/~kiko

On Fri, Jul 24, 2009 at 02:18:08AM -0000, Stuart Bishop wrote:
> >> bin/run isn't suitable for use on production as it launches an appserver.
> >
> > Could it not be made to run the librarian as well? I guess at this point
> > whatever's easiest to do and causes less problems is sanest.
> 
> It was easier to add the new script. The existing code runs daemons in
> foreground mode but puts them in the background  using
> subprocess.Popen - it is designed for devs and around having the
> services shutdown when the Launchpad instance does. Its also

> undocumented with no command line help, and that doesn't look fixable
> without maybe breaking other call sites that depend on it such as
> ec2test.

Can you file a bug on it being undocumented, maybe assign to Gary?

> >> === modified file 'lib/canonical/launchpad/scripts/runlaunchpad.py'
> >> -        # We want to make sure that the Launchpad process will have the
> >> -        # benefit # of all of the dependency paths inserted by the buildout
> >> -        # bin/run script. We pass them via PYTHONPATH.
> >> -        env = dict(os.environ)
> >> -        env['PYTHONPATH'] = os.path.pathsep.join(sys.path)
> >
> > Will this not cause problems anywhere else?
> 
> No - buildout handles this for us now by building _pythonpath.py will
> all the libraries we are supposed to need. If you don't like it, we
> can land this bit another time - it is harmless and just a drive by
> cleanup to keep our variables down when trying to debug things.

Well, we're likely to test this works on a production-like box before
actually updating the librarian, so I think this is fine. rc=kiko or can
I do something like

approve
    merge-approved

to do this via email? :-)
-- 
Christian Robottom Reis | [+55 16] 3376 0125 | http://launchpad.net/~kiko
                        | [+55 16] 9112 6430 | http://async.com.br/~kiko

« Back to merge proposal