Explicit Initialization

[vv9k]

What did you implement:

A safe way to initialize Docker. Current initialization methods can panic when uri is malformed. This PR adds a new safe way to initialize both TCP and UNIX clients. There are also multiple unwraps that I got rid of .

How did you verify your change:

Running tests locally.

What (if anything) would need to be called out in the CHANGELOG for the next release:

This is a big breaking change. It completely overhauls initialization of Docker. There are now 3 ways to initialize Docker:

1. Docker::new(uri) which takes a Into<String> and chooses appropriate transport based on the scheme. This method errors out if uri is malformed.
2. Docker::from_env() works as old Docker::new and reads DOCKER_HOST environment variable for the uri. What changed is that it now returns a Result.
3. Docker::default() this is a way to initialize Docker without a Result. It defaults to unix:///var/run/docker.sock.

[elihunter173]

Big fan of these changes overall. Confused why tcp and unix were made private tho.

[thomaseizinger]

Liking this as well!

[thomaseizinger]

Two more drive-by comments :)

[matthiasbeyer]

I did not do an in-depth review here, just a very basic one.

Either way, I'm all in for a big rebase before this is merged, because the commit history looks not so clean 😄

[vv9k]

I rebased to current master, cleaned up the history and fixed the links as you suggested :)

[matthiasbeyer]

Codewise this looks good, but to be honest, I'm not a fan at all of accessing environment variables in library crates. I would remove Docker::new and impl Default for Docker completely, because Default::default() (IMO) should never fail, not even panic... and new doing something else depending on how the crate is compiled and depending on ENV also seems unintuitive.

But I'm open for discussion here.

@elihunter173 what do you think?

[vv9k]

I completely agree, libraries shouldn't access environment variables and the get_docker_for_tcp also shouldn't use DOCKER_CERT_PATH as well as DOCKER_TLS_VERIFY but perhaps take this as arguments to the Docker::tcp constructor.

I would perhaps get rid of the new, try_from_env and Default and rename try_new to new. I think it's worth keeping a new method that actually handles features and parsing uri properly, but if we also add new parameters to Docker::tcp then perhaps the new method might be pointless.

[vv9k]

I think Rust is a language where explicitness is a key value thus I went ahead and rebased this PR to new master and applied initial changes mentioned in my previous comment.

What do you think of this approach?

Perhaps we might want to verify the validity of the passed uri and return Results?

[thomaseizinger]

Like I mentioned here, I am in favor of a "smart" initialization function that makes it easier for people to use shiplift in a cross-platform context.

However, I also think that the changes in this branch provide a much better foundation for achieving this. I think it would be good to strive for backwards-compatible changes so perhaps there is a middle ground here where we can retain the public API as much as possible but still change things under the hood to already be organized into the different ways of initializing a docker client.

[vv9k]

I think there should be no need to stay backwards compatible while the public API is still pre-1.0 but that's just my 2 cents.

[matthiasbeyer]

Perhaps we might want to verify the validity of the passed uri and return Results?

IMO yes. If the user has a Docker object, they should be able to use it because initialization worked. If they can create a Docker that does not work, that's unintuitive, IMHO.

[vv9k]

I pushed some changes, now tcp, tls and Unix constructors take only the host path that comes after the Scheme as an argument and the new method does the magic matching based on features and the scheme.

I've decided to keep tls as a separate constructor as it complicates some things.

I'm not entirely sure how to validate the host path without connecting to it to verify if the connector works.

[vv9k]

So I cleaned up the commit history and code a bit, any suggestions? Is this the right direction? Should the constructors ping the server to check if it's working and return a Result (I don't really like this)?

[matthiasbeyer]

I like the direction this is going, I'm just not yet 100% happy with the Docker::new() interface 😆

I hope you don't mind...

[matthiasbeyer]

I am not sure whether I like the automatic finding of the scheme here, because a user must rely on the correct implementation of the parsing.
Maybe we can have something like this:

Docker::new(DockerConnectionScheme::from_str("unix://...")?)

So that we keep this general idea, but also give the user the option to be explicit, be having him construct a DockerConnectionScheme themselves:

pub enum DockerConnectionScheme {
    UnixSocket(PathBuf),
    Tcp(url::Url), // or similar - strongly typed
    Http(url::Url), //...
}

And with an appropriate std::str::FromStr implementation that does the parsing. This way a user can chose what they want!

What do you think?

---

(Above code is just a quick mockup of my idea, details may be different)

[vv9k]

Sorry for the hustle, I'm actually working on my own library now so I'm not sure I'll be able to contribute here too. I'll close this issue for now.

[matthiasbeyer]

Would you mind if I take your commits and work with them? From this PR as well as from the other PRs you closed?

[vv9k]

Sure, not a problem. Feel free to rebase the PRs and update them accordingly :)