Add Authentication

[valkum]

Open up for discussion, a PR for adding Authentication.

Currently the URL generation has a lot of repeated code. My solution to add the auth parameters only when needed feels a little bit hacky currently. Setting db and auth parameters can be done before the if-statement.
Are there arguments against this auth method (Params in URL)? Should we support all of the three possibilities (Basic auth, Query parameters in the URL and in the body )

Testing needs to be added as well.

Closes #9

[Empty2k12]

Thank you very much for this PR!! 💚

I really like how you implemented the feature. I do have some comments which I would love to discuss:

• We can remove the url dependency since you switched reqwest::Url 🎉
• possibly creating a second new_with_auth method, that takes an auth parameter, in addition to new which is unauthed
• Doctests should be fixed
• It would be nice to test authed and unauthed connections in CI

What do you think about these suggestions? I can also help with these suggestions if you do not want to implement them 💪

[valkum]

I thought about getting rid of the url dep but had not checked the rest of the code.

I am not sure about having a new and something else. Looking at other crates we could rename new to connect(url, database) and then add with_auth(url, database, auth) which takes the InfludxDbAuthentication stuct.

For CI we would need to spin up a authed influxdb instance. Should we provide a config in the repo or copy and edit the config in place for the authed instance?

[Empty2k12]

I am not sure about having a new and something else. Looking at other crates we could rename new to connect(url, database) and then add with_auth(url, database, auth) which takes the InfludxDbAuthentication stuct.

I thought about making it a builder pattern, this would be the best solution!

For CI we would need to spin up a authed influxdb instance. Should we provide a config in the repo or copy and edit the config in place for the authed instance?

Not sure about the best solution, would be fine with either.

If you want, we could also switch to Github Actions as the CI, but that might warrant a different PR.

[valkum]

I am not sure about having a new and something else. Looking at other crates we could rename new to connect(url, database) and then add with_auth(url, database, auth) which takes the InfludxDbAuthentication stuct.

I thought about making it a builder pattern, this would be the best solution!

By adding a Builder for the client or by making with_auth return a &mut InfluxDbClient?

[Empty2k12]

I thought about the client so you can do both InfluxDbClient::new("http://localhost:8086", "test") for unauthenticated connections and InfluxDbClient::new("http://localhost:8086", "test").with_auth(InfluxDbAuthentication::new("admin", "password")). with_auth could even accept strings and use InfluxDbAuthentication internally. What do you think?

[valkum]

That is the same thing I came up with currently.

[valkum]

After fighting with futures here is an improved version including tests and changes to CI.
I added parsing of reqwest::StatsuCode, so proper errors for auth failures can be returned.
As it was a real pain to get it right, it would be nice if you can have a second look over the future chain for query.

[valkum]

It seems my sed command does not change the bind-address or something else is preventing the second instance from starting. Do we need to set different data dirs?

[valkum]

I found out you can easily use docker for running your influxdb instances. This seems clean to me for now. No need to change to actions.

[Empty2k12]

That's a great solution with the Docker in Travis 👌! Should I do a last round of code review to get this merged?

[Empty2k12]

Great work, I really like where this is going! Thank you very much for the effort!

Some comments - also we should try to bump that coverage up again, I left some comments on where I think that is possible.

Generally, I like to keep coverage equal or bump it in a PR

[Empty2k12]

Is there a better option than cloning?

[valkum]

We could change the into impl to

impl<'a> Into<Vec<(String, String)>> for &'a InfluxDbClient { fn into(self) -> Vec<(String, String)> { let mut vec: Vec<(String, String)> = Vec::new(); vec.push(("db".to_string(), self.database.to_owned())); if let Some(auth) = &self.auth { vec.push(("u".to_string(), auth.username.to_owned())); vec.push(("p".to_string(), auth.password.to_owned())); } vec } }

Then we would not need the cloning

[valkum]

I added this one as a second impl so that &InfluxDbClient and InfluxDbClient can be used.

Feedback was incorporated

[valkum]

Not sure how to increase the coverage more. Tests for ProtocolError, DeserializationError as well as InvalidQueryError seem hard.

[Empty2k12]

Let's see what it comes out to. I think it's fine 👍

[Empty2k12]

Just some warning and lints: https://travis-ci.org/Empty2k12/influxdb-rust/jobs/572918173

Other than that it looks ready to merge!