Arvados

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.

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).

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.

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!