Merge lp://staging/~amanica/loggerhead/loggerheadd into lp://staging/loggerhead

Proposed by Marius Kruger
Status: Merged
Merged at revision: not available
Proposed branch: lp://staging/~amanica/loggerhead/loggerheadd
Merge into: lp://staging/loggerhead
To merge this branch: bzr merge lp://staging/~amanica/loggerhead/loggerheadd
Reviewer Review Type Date Requested Status
Tim Penhey Approve
Martin Albisetti Approve
Michael Hudson-Doyle Pending
Review via email: mp+1205@code.staging.launchpad.net
To post a comment you must log in.
231. By Marius Kruger

add restart

232. By Marius Kruger

fix whitespace

233. By Marius Kruger

extract stop and start methods

Revision history for this message
Martin Albisetti (beuno) wrote :

So, this looks very interesting.
I wondering how this would affect the installation.
Would we copy it by default, but not enable it?

review: Approve
Revision history for this message
Martin Albisetti (beuno) wrote :

I actually meant to comment :)
So, this is a question I'd like to figure out, as well as Michael's opinion.

Revision history for this message
Marius Kruger (amanica) wrote :

2008/10/21 Martin Albisetti <email address hidden>

> So, this looks very interesting.
> I wondering how this would affect the installation.

I'm not sure what your are asking, have you seen the changes to the install
instructions:

+To run loggerhead as a linux daemon:
+1) Copy loggerheadd to /etc/init.d
+2) Edit the file to configure where your loggerhead is installed, and which
+ serve-branches options you would like.
+3) Register the service
+ cd /etc/init.d
+ a) on upstart based systems like Ubuntu run:
+ update-rc.d loggerheadd defaults
+ b) on Sysvinit based systems like Centos or SuSE run:
+ chkconfig --add loggerheadd

>
> Would we copy it by default, but not enable it?
>

Are you talking about apt-get install? In that case I think it should enable
it by default (will need to fix config then),
which is how other services work as far as I know.

--
<| regards
U| Marius
H| <><
Z| http://amanica.blogspot.com/
<|
E| http://bazaar-vcs.org/
<| because I don't trust version control systems with less than 12994 unit
tests

Revision history for this message
Martin Albisetti (beuno) wrote :

I meant on setup.py, actually.
Or do you feel this should be something people do entirely manually.

Revision history for this message
Tim Penhey (thumper) wrote :

I have a couple of questions.

Is /opt/loggerhead where the packaged loggerhead gets installed by default?

Does the directory /var/log/loggerhead get created anywhere? Through very rudimentary testing it seems that it should be made in the start call.

Also, the default directory that is served seems weird. I'm sure it makes sense for you but we should have a more generic location specified in trunk.

I can't verify that the netstat query is valid, but I'm happy if others know how it all works.

Revision history for this message
Martin Albisetti (beuno) wrote :

Hi Marius!
I'm keen to include this in the upcoming release.
Would you have time to address the concerns in the review?

Revision history for this message
Marius Kruger (amanica) wrote :

Hi Martin

I'll see what I can do over the weekend.

* I think most of Tim's comments should be easy to resolve.
* WRT what to do in setup.py,
  - I dont think its feasible to install automatically,
    unless we ask a bunch of questions about what is the bzrroot, the port
etc.
    OR if we choose good defaults.
  - the script won't work on anything but linux, so setup should not install
it for non-supported platforms.
  - ATM the config is hardcoded in the script, but I think we can read it
from /etc/loggerhead .

To make it really nice might take some time, so we might settle on just
improving the installation instructions for people who can do it manually.
For this release.

2008/11/7 Martin Albisetti <email address hidden>

> Hi Marius!
> I'm keen to include this in the upcoming release.
> Would you have time to address the concerns in the review?
> --
> https://code.edge.launchpad.net/~amanica/loggerhead/loggerheadd/+merge/1205<https://code.edge.launchpad.net/%7Eamanica/loggerhead/loggerheadd/+merge/1205>
> You are subscribed to branch lp:~amanica/loggerhead/loggerheadd.
>

--
<| regards
U| Marius
H| <><
Z| http://amanica.blogspot.com/
<|
E| http://bazaar-vcs.org/
<| because I don't trust version control systems with less than 12994 unit
tests

Revision history for this message
Martin Albisetti (beuno) wrote :

On Fri, Nov 7, 2008 at 3:24 PM, Marius Kruger <email address hidden> wrote:
> I'll see what I can do over the weekend.

That would be fantastic, thank you!

> To make it really nice might take some time, so we might settle on just
> improving the installation instructions for people who can do it manually.
> For this release.

Sounds good to me.

thanks,

--
Martin

234. By Marius Kruger

* Change init info tabs->spaces
* some refactoring
* make paths more suitable to installed version
* starting/stopping will give better info

Revision history for this message
Marius Kruger (amanica) wrote :

2008/10/24 Tim Penhey <email address hidden>

> Is /opt/loggerhead where the packaged loggerhead gets installed by default?

no.
I saw that setup.py puts serve-branches in the path, so by default I now
assume
it is in the path. If the user has it somewhere else he should configure
accordingly.

> Does the directory /var/log/loggerhead get created anywhere? Through very
> rudimentary testing it seems that it should be made in the start call.

oops.
it will now be created when starting loggerhead (if it doesn't exist).

> Also, the default directory that is served seems weird. I'm sure it makes
> sense for you but we should have a more generic location specified in trunk.

:) oops again
I could not think up a safe default so I made it /bzrroot for now, which
should force the user to configure this properly

> I can't verify that the netstat query is valid, but I'm happy if others
> know how it all works.

I improved it a little.

I uploaded my latest changes.

WRT auto installing it using setup.py as I said earlied, I dont think its
feasible to install automatically. Maybe somebody else knows how to do this,
or whether its a good idea or not.

regards
marius

Revision history for this message
Tim Penhey (thumper) wrote :

Looks good.

review: Approve

Subscribers

People subscribed via source and target branches