Project

General

Profile

Actions

Task #2591

closed

Bug #2449: New Keep server can write

Review 2449-keep-flags

Added by Tim Pierce over 10 years ago. Updated over 10 years ago.

Status:
Closed
Priority:
Normal
Assigned To:
Misha Zatsman

Description

Review for issue #2463: add command line flags support to Keep

This branch adds -listen and -volume flags to Keep:

  • -listen specifies the interface on which to listen for requests. Examples:
    • -listen=127.0.0.1:4949
    • -listen=:25107 (listen on port 25107 on all interfaces)
  • -volumes specifies a list of directories to use as keep volumes. Example:
    • -volumes=/var/keep01,/var/keep02,/var/keep03/subdir
    • If -volumes is empty or is not present, Keep will select volumes by looking at currently mounted filesystems for /keep top-level directories.

git diff master...2449-keep-flags

Actions #1

Updated by Misha Zatsman over 10 years ago

  • Status changed from New to In Progress
  • Assigned To set to Misha Zatsman
Actions #2

Updated by Misha Zatsman over 10 years ago

Can you please add the examples to your flag descriptions to make this a little more user friendly? In particular, are they're examples that don't use the loopback address?

"If -volumes is empty or is not present, Keep will select volumes by looking at currently mounted filesystems for /keep top-level directories." This behavior description probably also belongs in the flag docstring.

But more importantly, do we really want that behavior in the production system, or is it just for ease of development? If it's just for ease of development we should probably log a warning that we're doing it.

Also, I know it's beyond the scope of this change, but it seems like we'd want to log the keep volumes we're using once we determine them.

Actions #3

Updated by Tim Pierce over 10 years ago

Misha Zatsman wrote:

Can you please add the examples to your flag descriptions to make this a little more user friendly? In particular, are they're examples that don't use the loopback address?

Added comments to describe flags and their arguments, including a non-loopback example. The general form is -listen=ipaddr:port. Is this still unclear? If so I'll add more.

"If -volumes is empty or is not present, Keep will select volumes by looking at currently mounted filesystems for /keep top-level directories." This behavior description probably also belongs in the flag docstring.

But more importantly, do we really want that behavior in the production system, or is it just for ease of development? If it's just for ease of development we should probably log a warning that we're doing it.

This is a good question. This is how the current keepd operates, so I'm preserving that behavior, but in production I think it might be better to require the admin to explicitly specify keep volumes (either on the command line or in a config file somewhere).

Also, I know it's beyond the scope of this change, but it seems like we'd want to log the keep volumes we're using once we determine them.

That's a good idea too. Technically it is logging them with log.Println, but all of the logging here really needs to be reviewed (I'll add a task to the backlog).

Actions #4

Updated by Misha Zatsman over 10 years ago

Tim Pierce wrote:

Misha Zatsman wrote:

Can you please add the examples to your flag descriptions to make this a little more user friendly? In particular, are they're examples that don't use the loopback address?

Added comments to describe flags and their arguments, including a non-loopback example. The general form is -listen=ipaddr:port. Is this still unclear? If so I'll add more.

Thanks, these are great, but what I actually meant was to add them into the description of the flags as well, so a user at the command line can learn their behavior easily. Can you modify those strings too

"If -volumes is empty or is not present, Keep will select volumes by looking at currently mounted filesystems for /keep top-level directories." This behavior description probably also belongs in the flag docstring.

But more importantly, do we really want that behavior in the production system, or is it just for ease of development? If it's just for ease of development we should probably log a warning that we're doing it.

This is a good question. This is how the current keepd operates, so I'm preserving that behavior, but in production I think it might be better to require the admin to explicitly specify keep volumes (either on the command line or in a config file somewhere).

Ok, great, let's record that as a TODO somewhere.

Also, I know it's beyond the scope of this change, but it seems like we'd want to log the keep volumes we're using once we determine them.

That's a good idea too. Technically it is logging them with log.Println, but all of the logging here really needs to be reviewed (I'll add a task to the backlog).

Great, thanks!

So the above just contains a couple easy things to do. If you agree with them, feel free to check in this change after doing them, I don't want you to have to wait for me to get back from lunch.

Actions #5

Updated by Tim Pierce over 10 years ago

Misha Zatsman wrote:

Tim Pierce wrote:

Misha Zatsman wrote:

Can you please add the examples to your flag descriptions to make this a little more user friendly? In particular, are they're examples that don't use the loopback address?

Added comments to describe flags and their arguments, including a non-loopback example. The general form is -listen=ipaddr:port. Is this still unclear? If so I'll add more.

Thanks, these are great, but what I actually meant was to add them into the description of the flags as well, so a user at the command line can learn their behavior easily. Can you modify those strings too

Done, sorry for misunderstanding. There seem to be better ways of documenting tools with godoc, but this isn't a good use of our time right now. I've added bug #2593 to the backlog to rewrite documentation and comments to take advantage of godoc.

This is a good question. This is how the current keepd operates, so I'm preserving that behavior, but in production I think it might be better to require the admin to explicitly specify keep volumes (either on the command line or in a config file somewhere).

Ok, great, let's record that as a TODO somewhere.

Yup, added.

Changes pushed, thanks for reviewing!

Actions #6

Updated by Tim Pierce over 10 years ago

  • Status changed from In Progress to Closed
  • Remaining (hours) changed from 1.0 to 0.0
Actions

Also available in: Atom PDF