CODENVY-739: pass proxy credentials to jgit #1707

dmytro-ndp · 2016-07-11T13:47:55Z

@vinokurig, @evoevodin: please, review this request.

Signed-off-by: Dmytro Nochevnov dnochevnov@codenvy.com

voievodin · 2016-07-11T14:10:09Z

wsagent/che-core-git-impl-jgit/src/main/java/org/eclipse/che/git/impl/jgit/JGitConnection.java

+    private static Authenticator httpsProxyAuth;
+    private static Authenticator httpProxyAuth;
+    static {
+        if (nonNull(System.getProperty("http.proxyUser")) && nonNull(System.getProperty("http.proxyPassword"))) {


I don't see any reason to use nonNull(...) against != null.

I don't think that camel cased properties are good choice, i would preferable change them to http.proxy_user and http.proxy_password

Authenticator looks not good as anonymous class to me, i think you should extract it to the private static NAMED inner class, and it seems that findbugs will suggest you to do the same.

(1). I don't see any reason to use nonNull(...) against != null.

Agree.

(2). I don't think that camel cased properties are good choice, i would preferable change them to http.proxy_user and http.proxy_password

I don't think so. Camel case is good enough here, moreover - there is a convention of Java proxy settings naming which we can't change: http://rolandtapken.de/blog/2012-04/java-process-httpproxyuser-and-httpproxypassword

(3). Authenticator looks not good as anonymous class to me, i think you should extract it to the private static NAMED inner class, and it seems that findbugs will suggest you to do the same.

Agree.

Why did you use java.util.Objects#nonNull instead of com.google.common.base.Strings#isNullOrEmpty?
Empty property value may means that this property is not configured, isn't it?

Empty property shouldn't affect the code. But usage of Strings#isNullOrEmpty is definitely great suggestion. Thank you!

voievodin · 2016-07-11T14:10:27Z

reviewed, check the comment

dmytro-ndp · 2016-07-11T14:39:34Z

Thank you! Please, see my answers.

voievodin · 2016-07-11T15:09:50Z

Well, then there is nothing to add. LGTM Good Job 👍

codenvy-ci · 2016-07-11T16:11:41Z

Build success. http://ci.codenvy-dev.com/jenkins/job/che-pullrequests-build/1178/

sleshchenko · 2016-07-12T07:19:56Z

wsagent/che-core-git-impl-jgit/src/main/java/org/eclipse/che/git/impl/jgit/JGitConnection.java

@@ -1590,13 +1616,22 @@ public void configure(Transport transport) {
                        sshTransport.setSshSessionFactory(sshSessionFactory);
                    }
                });
+
+                if (nonNull(httpProxyAuth)) {
+                    Authenticator.setDefault(httpProxyAuth);


Please, can you explain why we should use httpProxyAuth for ssh connections and httpsProxyAuth for http/https ?

I guess there can be some problem with concurrency. I mean you can send two requests with ssh and http links. Then there can be situation when before invoking of command.call() will be invoked Authenticator.setDefault(httpsProxyAuth); for ssh link. Am I wrong?

(1) Please, can you explain why we should use httpProxyAuth for ssh connections and httpsProxyAuth for http/https ?

You found an error, and we even don't need setup proxy for ssh.

(2) I guess there can be some problem with concurrency. I mean you can send two requests with ssh and http links. Then there can be situation when before invoking of command.call() will be invoked Authenticator.setDefault(httpsProxyAuth); for ssh link. Am I wrong?

You are right, but I would prefer don't work around synchronization of invoking authenticator so as it will lead to redundant delays.

codenvy-ci · 2016-07-13T10:59:34Z

Build success. http://ci.codenvy-dev.com/jenkins/job/che-pullrequests-build/1199/

dmytro-ndp · 2016-07-13T11:00:45Z

@evoevodin, @sleshchenko: please, review my fixup on your notices.

sleshchenko · 2016-07-13T11:04:06Z

wsagent/che-core-git-impl-jgit/src/main/java/org/eclipse/che/git/impl/jgit/JGitConnection.java

    @Inject
    JGitConnection(Repository repository, CredentialsLoader credentialsLoader, SshKeyProvider sshKeyProvider,
                   GitUserResolver userResolver) {
        this.repository = repository;
        this.credentialsLoader = credentialsLoader;
        this.sshKeyProvider = sshKeyProvider;
        this.userResolver = userResolver;
+        this.proxyAuthenticator = new ProxyAuthenticator();


Seems that this instance is unused

You are right.

codenvy-ci · 2016-07-13T13:13:49Z

Build success. http://ci.codenvy-dev.com/jenkins/job/che-pullrequests-build/1202/

Signed-off-by: Dmytro Nochevnov <dnochevnov@codenvy.com>

sleshchenko · 2016-07-13T14:29:42Z

wsagent/che-core-api-git/src/test/java/org/eclipse/che/git/impl/PullTest.java

@@ -137,4 +137,29 @@ public void testWhenThereAreNoAnyRemotes(GitConnectionFactory connectionFactory)
        PullRequest request = newDto(PullRequest.class);
        connection.pull(request);
    }
+
+    @Test(dataProvider = "GitConnectionFactory", dataProviderClass = GitConnectionFactoryProvider.class,
+        expectedExceptions = GitException.class, expectedExceptionsMessageRegExp = "No remote repository specified.  " +


Are you sure that this test check your use case?

sleshchenko · 2016-07-13T14:32:55Z

OK for me

sleshchenko · 2016-07-13T14:37:29Z

wsagent/che-core-git-impl-jgit/src/main/java/org/eclipse/che/git/impl/jgit/JGitConnection.java

-    @Inject
-    JGitConnection(Repository repository, CredentialsLoader credentialsLoader, SshKeyProvider sshKeyProvider,
-                   GitUserResolver userResolver) {
+    @Inject JGitConnection(Repository repository, CredentialsLoader credentialsLoader, SshKeyProvider sshKeyProvider,


revert formatting

vinokurig · 2016-07-13T14:50:59Z

core/che-core-api-core/src/main/java/org/eclipse/che/commons/proxy/ProxyAuthenticator.java

+        private PasswordAuthentication passwordAuthentication = createPasswordAuthentication();
+
+        private PasswordAuthentication createPasswordAuthentication() {
+            if (! (Strings.isNullOrEmpty(getProxyUserSystemProperty())


Please use static import of isNullOrEmpty

Thank you for advice.

Signed-off-by: Dmytro Nochevnov <dnochevnov@codenvy.com>

dmytro-ndp · 2016-07-13T15:32:55Z

@evoevodin: please, review new fixup.

voievodin · 2016-07-13T16:00:03Z

LGTM

dmytro-ndp · 2016-07-13T16:04:23Z

Thank @evoevodin, @sleshchenko, @vinokurig for your helpful notices!

codenvy-ci · 2016-07-13T17:17:19Z

Build # 1207 - FAILED

Please check console output at http://ci.codenvy-dev.com/jenkins/job/che-pullrequests-build/1207/ to view the results.

dmytro-ndp force-pushed the CODENVY-739 branch from 7713e95 to 695e795 Compare July 11, 2016 13:58

voievodin reviewed Jul 11, 2016
View reviewed changes

sleshchenko reviewed Jul 12, 2016
View reviewed changes

dmytro-ndp force-pushed the CODENVY-739 branch 2 times, most recently from 29d9086 to f19dc32 Compare July 13, 2016 10:58

sleshchenko reviewed Jul 13, 2016
View reviewed changes

dmytro-ndp force-pushed the CODENVY-739 branch from 3f773d9 to c4a3600 Compare July 13, 2016 14:15

Dmytro Nochevnov added 2 commits July 13, 2016 17:16

CODENVY-739: pass proxy credentials to jgit

44ad9e2

Signed-off-by: Dmytro Nochevnov <dnochevnov@codenvy.com>

fixup! CODENVY-739: pass proxy credentials to jgit

5eedc0a

Signed-off-by: Dmytro Nochevnov <dnochevnov@codenvy.com>

sleshchenko reviewed Jul 13, 2016
View reviewed changes

dmytro-ndp force-pushed the CODENVY-739 branch from c4a3600 to 4ce625f Compare July 13, 2016 14:42

vinokurig reviewed Jul 13, 2016
View reviewed changes

fixup! fixup! CODENVY-739: pass proxy credentials to jgit

d11c12f

Signed-off-by: Dmytro Nochevnov <dnochevnov@codenvy.com>

dmytro-ndp force-pushed the CODENVY-739 branch from 4ce625f to d11c12f Compare July 13, 2016 15:31

dmytro-ndp merged commit 8b2cd2e into master Jul 13, 2016

dmytro-ndp deleted the CODENVY-739 branch July 13, 2016 16:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CODENVY-739: pass proxy credentials to jgit #1707

CODENVY-739: pass proxy credentials to jgit #1707

dmytro-ndp commented Jul 11, 2016

voievodin Jul 11, 2016 •

edited

Loading

dmytro-ndp Jul 11, 2016 •

edited

Loading

vzhukovs Jul 11, 2016

dmytro-ndp Jul 11, 2016 •

edited

Loading

voievodin commented Jul 11, 2016

dmytro-ndp commented Jul 11, 2016 •

edited

Loading

voievodin commented Jul 11, 2016

codenvy-ci commented Jul 11, 2016

sleshchenko Jul 12, 2016 •

edited

Loading

dmytro-ndp Jul 12, 2016

codenvy-ci commented Jul 13, 2016

dmytro-ndp commented Jul 13, 2016

sleshchenko Jul 13, 2016

dmytro-ndp Jul 13, 2016

codenvy-ci commented Jul 13, 2016

sleshchenko Jul 13, 2016

dmytro-ndp Jul 13, 2016

sleshchenko commented Jul 13, 2016

sleshchenko Jul 13, 2016

dmytro-ndp Jul 13, 2016

vinokurig Jul 13, 2016

dmytro-ndp Jul 13, 2016

dmytro-ndp commented Jul 13, 2016

voievodin commented Jul 13, 2016

dmytro-ndp commented Jul 13, 2016

codenvy-ci commented Jul 13, 2016

CODENVY-739: pass proxy credentials to jgit #1707

CODENVY-739: pass proxy credentials to jgit #1707

Conversation

dmytro-ndp commented Jul 11, 2016

voievodin Jul 11, 2016 • edited Loading

Choose a reason for hiding this comment

dmytro-ndp Jul 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmytro-ndp Jul 11, 2016 • edited Loading

Choose a reason for hiding this comment

voievodin commented Jul 11, 2016

dmytro-ndp commented Jul 11, 2016 • edited Loading

voievodin commented Jul 11, 2016

codenvy-ci commented Jul 11, 2016

sleshchenko Jul 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codenvy-ci commented Jul 13, 2016

dmytro-ndp commented Jul 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codenvy-ci commented Jul 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sleshchenko commented Jul 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmytro-ndp commented Jul 13, 2016

voievodin commented Jul 13, 2016

dmytro-ndp commented Jul 13, 2016

codenvy-ci commented Jul 13, 2016

voievodin Jul 11, 2016 •

edited

Loading

dmytro-ndp Jul 11, 2016 •

edited

Loading

dmytro-ndp Jul 11, 2016 •

edited

Loading

dmytro-ndp commented Jul 11, 2016 •

edited

Loading

sleshchenko Jul 12, 2016 •

edited

Loading