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

On Fri, Jul 24, 2009 at 5:48 PM, Christian Reis<email address hidden> wrote:
> 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?

I don't know what things should look like ideally. Perhaps use
supervisord to fireup instances, automatically restart on failure
conditions, report back to a central monitoring server. For now, we
need something that starts the 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?

Bug 404008.

>> >> === 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

Yup. Now with the startup script in Launchpad, we should update
staging so it tests it to catch any future breakage.

> I do something like
>
>    approve
>    merge-approved
>
> to do this via email? :-)

Ta.

--
Stuart Bishop <email address hidden>
http://www.stuartbishop.net/