Conversation
@@ -65,6 +82,9 @@ private SenderClient(Builder builder) { | |||
this.proxyUser = builder.proxyUser; | |||
this.proxyPassword = builder.proxyPassword; | |||
this.proxyType = builder.proxyType; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Tolis, would you mind to move the code below to another class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Bruno. This code is part of a previous already accepted PR, not the one related with the custom truststore support. I will move it to another class and create a new refactoring PR. Do you agree?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for moving it, not sure about a new PR. I think we can do it here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd think that a new PR for those refactorings might be a bit cleaner; but
is just my personal preference
On Wed, Mar 19, 2014 at 12:54 PM, Bruno Oliveira
notifications@github.comwrote:
In src/main/java/org/jboss/aerogear/unifiedpush/SenderClient.java:
@@ -65,6 +82,9 @@ private SenderClient(Builder builder) {
this.proxyUser = builder.proxyUser;
this.proxyPassword = builder.proxyPassword;
this.proxyType = builder.proxyType;+1 for moving it, not sure about a new PR. I think we can do it here
Reply to this email directly or view it on GitHubhttps://github.com//pull/36/files#r10745617
.
Matthias Wessendorf
blog: http://matthiaswessendorf.wordpress.com/
sessions: http://www.slideshare.net/mwessendorf
twitter: http://twitter.com/mwessendorf
I have added the refactoring commit and updated-improved the unit tests to comply with the new changes. End2End tests passed for both insecure and secure endpoints. I have not tested the proxy. Maybe some integration tests for testing the proxy would be useful. If you're ok with these changes, I'll rebase and create a single commit. |
@tolis-e totally fine with the rebase, I will try this week. @sebastienblanc any thoughts on it? @tolis-e thank you so much |
I will test it also this week. |
@sebastienblanc @abstractj sure, I will add some instructions in the README file. thanks |
Added custom truststore support and tested it by using the integration tests.