opened TODO issue: #1995

this fixes ctr, but seems like this leaves a hole in the overall client API that currently doesn't say anything about option "precedence." Are there other cases where behavior is different based on ordering?

@crosbymichael did you ever implement your idea to take care of this somewhere?

Thats the problem with ctr, the ordering is what we made it. As far as other clients go, the supported usecase of the API, the ordering/precedence is what they define.

I guess my point is slightly different; if I create a new client for containerd using the published Go API, there is nothing that tells me the way I order my 'opts' when using containerd's API for NewContainer could affect outcome (e.g. error versus working container). Do we need some note and/or, if we know the "valid" orderings, should we/can we fix it on the API implementation side.

I don't know how we can fix that. Since opts can be created by the user the best I know to do is give them the information they need to setup things correctly.

It shouldn't be too difficult. The problem is the dependency is not explicit from the function signature. So we can fix that by changing it to:

func WithUserID(uid uint32, snapshotter snapshots.Snapshotter, rootfs string)

Of course this is more work to use, but it does make the dependencies obvious.

A change like this would also potentially allow us to remove client Client from the args in SpecOpt, which I think is a good thing. It's rarely used (these 2 functions are the only place it's referenced) and it introduces problems like this (where dependencies aren't clear).

Any SpecOpt that actually needs client (or something from client), can accept that as an arg to the function which returns the SpecOpt (like the WithUserID example above).

@dnephin good idea.

+1, but it should be probably separate PR for 1.1

I'm not sure about this change, I think the old code may have been wrong as well.

If WithUsername is successful, shouldn't it continue to run the code after the switch (setting cwd) ?

Alternatively may this can be refactored so that the CWD code is above this switch, which would let us keep this early return, and also let us return WithUserID(...) below, instead of checking err there as well.

fixed, thanks

This should use SplitN with an argument of 2, right?

I'm not sure. I think the current code is ok.