(#99) Implemented basic authentication and HttpClient decorator to inject authentication header #110

llorllale · 2018-05-15T20:56:43Z

This PR:

solves RtImages.java:96-100: Several API calls required an... #99
uses the Authentication Docs as reference
Defines new Auth interface. The Credentials implementation is for basic user credentials
Defines new AuthHttpClient to decorate other http clients by automatically injecting the required auth header in requests
Leaves puzzle to continue work

…ator to inject authentication header

0crat · 2018-05-15T20:58:05Z

Job #110 is now in scope, role is REV

coveralls · 2018-05-15T20:58:46Z

Pull Request Test Coverage Report for Build 193

19 of 28 (67.86%) changed or added relevant lines in 2 files are covered.
7 unchanged lines in 3 files lost coverage.
Overall coverage decreased (-1.09%) to 83.791%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/main/java/com/amihaiemil/docker/AuthHttpClient.java	8	17	47.06%

Files with Coverage Reduction	New Missed Lines	%
src/main/java/com/amihaiemil/docker/RtImages.java	1	97.67%
src/main/java/com/amihaiemil/docker/RtDocker.java	2	82.35%
src/main/java/com/amihaiemil/docker/RemoteDocker.java	4	50.0%

Totals
Change from base Build 173:	-1.09%
Covered Lines:	305
Relevant Lines:	364

💛 - Coveralls

llorllale · 2018-05-15T21:04:00Z

@amihaiemil FYI, coverage drops due to all those unused HttpClient operations in AuthHttpClient

0crat · 2018-05-15T21:37:27Z

@amihaiemil /z everybody who has role REV are banned at #110; I won't be able to assign anyone automatically; consider assigning someone manually (as in §19), or invite more people (as in §51), or remove the job from the scope (as in §14)

0crat · 2018-05-16T06:09:50Z

This pull request #110 is assigned to @amihaiemil /z, here is why; the budget is 15 minutes, see §4; please, read §27 and when you decide to accept the changes, inform @amihaiemil /z (the architect) right in this ticket; if you decide that this PR should not be accepted ever, also inform the architect; this blog post will help you understand what is expected from a code reviewer

0crat · 2018-05-16T06:13:01Z

Manual assignment of issues is discouraged, see §19: -5 point(s) just awarded to @amihaiemil /z

amihaiemil

@llorllale Really nice, I like it. Just a few questions. And let me know how much you worked (was it really 30min?) so I can boost the task :)

amihaiemil · 2018-05-16T06:13:13Z

src/main/java/com/amihaiemil/docker/Credentials.java

+    /**
+     * The base64-encoded JSON structure holding the credentials.
+     */
+    private final Supplier<String> encoded;


@llorllale Why did you make this a Supplier? As it is now, it doesn't make much sense. Maybe you wanted to add another ctor and let the clients specify a Supplier?

@amihaiemil just wanted to coalesce all those parameters into one single instance attribute

@llorllale I see, ok

amihaiemil · 2018-05-16T06:14:00Z

src/test/java/com/amihaiemil/docker/AuthHttpClientTestCase.java

+    /**
+     * An http client that does nothing.
+     */
+    private static class NoOpHttpClient implements HttpClient {


@llorllale Do we need this class? Can't we just use Mockito.mock(HttpClient.class)?

@amihaiemil we can use Mockito, yes, although I don't like it because the mocking introduces too much noise in tests. Do you want mockito?

@llorllale Well, yes, if the mock is not supposed to do anything, I would rather use Mockito. I understand that mocks can get really complex, but it's not the case here.

amihaiemil · 2018-05-16T06:16:55Z

src/main/java/com/amihaiemil/docker/Auth.java

+ * @version $Id$
+ * @see <a href="https://docs.docker.com/engine/api/v1.35/#section/Authentication">Authentication</a>
+ * @since 0.0.1
+ * @todo #99:30min Implement a new auth named 'Token' that will hold the user's


@llorllale Why do we need the Token implementation and how would it be used?

@amihaiemil the token would be for the identity token alternative described here

llorllale · 2018-05-16T15:57:20Z

@amihaiemil unfortunately I lost track of time because I kept getting interrupted

amihaiemil · 2018-05-16T19:33:43Z

@llorllale I see. Just use mockito for that NoOp client and we're good here. Do you think 90min are ok? You don't have even a rough idea how much did this take?

llorllale · 2018-05-16T19:45:28Z

@amihaiemil definitely not 90min. Let's agree on 45min.

0crat · 2018-05-21T06:36:16Z

@amihaiemil /z this job was assigned to you 5days ago. It will be taken away from you soon, unless you close it, see §8. Read this and this, please.

amihaiemil · 2018-05-23T08:33:51Z

@llorllale Can you use mockito instead of that NoOp client? That's my only request here, it looks ok otherwise :)

…ator to inject authentication header As per PR review: * AuthHttpClientTestCase: now mocking HttpClient by using Mockito

llorllale · 2018-05-23T14:07:27Z

@amihaiemil please check now

amihaiemil · 2018-05-23T14:54:48Z

@llorllale looks ok, thanks :)

amihaiemil · 2018-05-23T14:54:56Z

@rultor merge it

rultor · 2018-05-23T14:57:45Z

@rultor merge it

@amihaiemil OK, I'll try to merge now. You can check the progress of the merge here

amihaiemil · 2018-05-23T15:00:25Z

@0crat extra 90min @llorllale

rultor · 2018-05-23T15:00:49Z

@rultor merge it

@amihaiemil Done! FYI, the full log is here (took me 3min)

0crat · 2018-05-23T15:00:54Z

@0crat extra 90min @llorllale (here)

@amihaiemil The user @llorllale /z works for free in this project, see §49

amihaiemil · 2018-05-23T15:10:46Z

@llorllale Can you open an Issue in /farm for this? I think 0crat should pay the reputation even if the rate is 0.

0crat · 2018-05-23T15:15:47Z

Order was finished: +15 point(s) just awarded to @amihaiemil /z

0crat · 2018-05-23T15:15:50Z

The job #110 is now out of scope

llorllale · 2018-05-23T19:39:30Z

@amihaiemil according to this, the command is pay.

Or maybe you should try boost?

amihaiemil · 2018-05-23T19:41:02Z

@llorllale I think boost is used for tasks which are assigned and in scope. This one is out of scope already.

amihaiemil · 2018-05-23T19:41:16Z

@0crat pay 90min @llorllale

0crat · 2018-05-23T19:41:26Z

@0crat pay 90min @llorllale (here)

@amihaiemil The user @llorllale /z works for free in this project, see §49

(amihaiemil#99) Implemented basic authentication and HttpClient decor…

25329a8

…ator to inject authentication header

llorllale mentioned this pull request May 15, 2018

RtImages.java:96-100: Several API calls required an... #99

Closed

amihaiemil self-assigned this May 16, 2018

amihaiemil reviewed May 16, 2018

View reviewed changes

(amihaiemil#99) Implemented basic authentication and HttpClient decor…

9f7d884

…ator to inject authentication header As per PR review: * AuthHttpClientTestCase: now mocking HttpClient by using Mockito

rultor merged commit 9f7d884 into amihaiemil:master May 23, 2018

llorllale deleted the 99 branch May 23, 2018 19:33

Uh oh!

(#99) Implemented basic authentication and HttpClient decorator to inject authentication header #110

(#99) Implemented basic authentication and HttpClient decorator to inject authentication header #110

Uh oh!

Conversation

llorllale commented May 15, 2018

Uh oh!

0crat commented May 15, 2018

Uh oh!

coveralls commented May 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 193

💛 - Coveralls

Uh oh!

llorllale commented May 15, 2018

Uh oh!

0crat commented May 15, 2018

Uh oh!

0crat commented May 16, 2018

Uh oh!

0crat commented May 16, 2018

Uh oh!

amihaiemil left a comment

Choose a reason for hiding this comment

Uh oh!

amihaiemil May 16, 2018

Choose a reason for hiding this comment

Uh oh!

llorllale May 16, 2018

Choose a reason for hiding this comment

Uh oh!

amihaiemil May 16, 2018

Choose a reason for hiding this comment

Uh oh!

amihaiemil May 16, 2018

Choose a reason for hiding this comment

Uh oh!

llorllale May 16, 2018

Choose a reason for hiding this comment

Uh oh!

amihaiemil May 16, 2018

Choose a reason for hiding this comment

Uh oh!

amihaiemil May 16, 2018

Choose a reason for hiding this comment

Uh oh!

llorllale May 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

llorllale commented May 16, 2018

Uh oh!

amihaiemil commented May 16, 2018

Uh oh!

llorllale commented May 16, 2018

Uh oh!

0crat commented May 21, 2018

Uh oh!

amihaiemil commented May 23, 2018

Uh oh!

llorllale commented May 23, 2018

Uh oh!

amihaiemil commented May 23, 2018

Uh oh!

amihaiemil commented May 23, 2018

Uh oh!

rultor commented May 23, 2018

Uh oh!

amihaiemil commented May 23, 2018

Uh oh!

rultor commented May 23, 2018

Uh oh!

0crat commented May 23, 2018

Uh oh!

amihaiemil commented May 23, 2018

Uh oh!

0crat commented May 23, 2018

Uh oh!

0crat commented May 23, 2018

Uh oh!

coveralls commented May 15, 2018 •

edited

Loading

llorllale May 16, 2018 •

edited

Loading