Please review PR #214

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

Please review PR #214

brunobat
Hi,

I think the code for this example: TOMEE-2283 New Example: Websocket
with TLS and Basic Auth <https://issues.apache.org/jira/browse/TOMEE-2283>

Is ready for review here: https://github.com/apache/tomee/pull/214

Can one of you please take a look?

Cheers

--
Bruno Baptista
https://twitter.com/brunobat_


Reply | Threaded
Open this post in threaded view
|

Re: Please review PR #214

Romain Manni-Bucau
Hello Bruno,

I put some suggestions on the PR, hope it helps.

Romain Manni-Bucau
@rmannibucau <https://twitter.com/rmannibucau> |  Blog
<https://rmannibucau.metawerx.net/> | Old Blog
<http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> |
LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
<https://www.packtpub.com/application-development/java-ee-8-high-performance>


Le lun. 26 nov. 2018 à 18:02, Bruno Baptista <[hidden email]> a écrit :

> Hi,
>
> I think the code for this example: TOMEE-2283 New Example: Websocket
> with TLS and Basic Auth <https://issues.apache.org/jira/browse/TOMEE-2283>
>
> Is ready for review here: https://github.com/apache/tomee/pull/214
>
> Can one of you please take a look?
>
> Cheers
>
> --
> Bruno Baptista
> https://twitter.com/brunobat_
>
>
>
Reply | Threaded
Open this post in threaded view
|

Re: Please review PR #214

brunobat
Hi Romain,

Thanks for you feedback.

I've pushed changes and added a comment to the PR.

Cheers.

Bruno Baptista
https://twitter.com/brunobat_


On 26/11/18 17:11, Romain Manni-Bucau wrote:

> Hello Bruno,
>
> I put some suggestions on the PR, hope it helps.
>
> Romain Manni-Bucau
> @rmannibucau <https://twitter.com/rmannibucau> |  Blog
> <https://rmannibucau.metawerx.net/> | Old Blog
> <http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> |
> LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
> <https://www.packtpub.com/application-development/java-ee-8-high-performance>
>
>
> Le lun. 26 nov. 2018 à 18:02, Bruno Baptista <[hidden email]> a écrit :
>
>> Hi,
>>
>> I think the code for this example: TOMEE-2283 New Example: Websocket
>> with TLS and Basic Auth <https://issues.apache.org/jira/browse/TOMEE-2283>
>>
>> Is ready for review here: https://github.com/apache/tomee/pull/214
>>
>> Can one of you please take a look?
>>
>> Cheers
>>
>> --
>> Bruno Baptista
>> https://twitter.com/brunobat_
>>
>>
>>
Reply | Threaded
Open this post in threaded view
|

TOMEE-2283 New Example: Websocket,with TLS and Basic Auth - PR #214

brunobat
Hi,

Can some committer please decide if this is good to merge?

https://github.com/apache/tomee/pull/214

Thanks

Bruno Baptista
https://twitter.com/brunobat_


On 26/11/18 17:58, Bruno Baptista wrote:

> Hi Romain,
>
> Thanks for you feedback.
>
> I've pushed changes and added a comment to the PR.
>
> Cheers.
>
> Bruno Baptista
> https://twitter.com/brunobat_
>
>
> On 26/11/18 17:11, Romain Manni-Bucau wrote:
>> Hello Bruno,
>>
>> I put some suggestions on the PR, hope it helps.
>>
>> Romain Manni-Bucau
>> @rmannibucau <https://twitter.com/rmannibucau> |  Blog
>> <https://rmannibucau.metawerx.net/> | Old Blog
>> <http://rmannibucau.wordpress.com> | Github
>> <https://github.com/rmannibucau> |
>> LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
>> <https://www.packtpub.com/application-development/java-ee-8-high-performance>
>>
>>
>>
>> Le lun. 26 nov. 2018 à 18:02, Bruno Baptista <[hidden email]> a
>> écrit :
>>
>>> Hi,
>>>
>>> I think the code for this example: TOMEE-2283 New Example: Websocket
>>> with TLS and Basic Auth
>>> <https://issues.apache.org/jira/browse/TOMEE-2283>
>>>
>>> Is ready for review here: https://github.com/apache/tomee/pull/214
>>>
>>> Can one of you please take a look?
>>>
>>> Cheers
>>>
>>> --
>>> Bruno Baptista
>>> https://twitter.com/brunobat_
>>>
>>>
>>>
Reply | Threaded
Open this post in threaded view
|

Re: TOMEE-2283 New Example: Websocket,with TLS and Basic Auth - PR #214

ivanjunckes
Bruno, there is a conflict in the PR.

On Thu, Nov 29, 2018 at 9:49 AM Bruno Baptista <[hidden email]> wrote:

> Hi,
>
> Can some committer please decide if this is good to merge?
>
> https://github.com/apache/tomee/pull/214
>
> Thanks
>
> Bruno Baptista
> https://twitter.com/brunobat_
>
>
> On 26/11/18 17:58, Bruno Baptista wrote:
> > Hi Romain,
> >
> > Thanks for you feedback.
> >
> > I've pushed changes and added a comment to the PR.
> >
> > Cheers.
> >
> > Bruno Baptista
> > https://twitter.com/brunobat_
> >
> >
> > On 26/11/18 17:11, Romain Manni-Bucau wrote:
> >> Hello Bruno,
> >>
> >> I put some suggestions on the PR, hope it helps.
> >>
> >> Romain Manni-Bucau
> >> @rmannibucau <https://twitter.com/rmannibucau> |  Blog
> >> <https://rmannibucau.metawerx.net/> | Old Blog
> >> <http://rmannibucau.wordpress.com> | Github
> >> <https://github.com/rmannibucau> |
> >> LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
> >> <
> https://www.packtpub.com/application-development/java-ee-8-high-performance>
>
> >>
> >>
> >>
> >> Le lun. 26 nov. 2018 à 18:02, Bruno Baptista <[hidden email]> a
> >> écrit :
> >>
> >>> Hi,
> >>>
> >>> I think the code for this example: TOMEE-2283 New Example: Websocket
> >>> with TLS and Basic Auth
> >>> <https://issues.apache.org/jira/browse/TOMEE-2283>
> >>>
> >>> Is ready for review here: https://github.com/apache/tomee/pull/214
> >>>
> >>> Can one of you please take a look?
> >>>
> >>> Cheers
> >>>
> >>> --
> >>> Bruno Baptista
> >>> https://twitter.com/brunobat_
> >>>
> >>>
> >>>
>
Reply | Threaded
Open this post in threaded view
|

Re: TOMEE-2283 New Example: Websocket,with TLS and Basic Auth - PR #214

brunobat
Yeah... fixing it. It's sitting for too long.

Bruno Baptista
https://twitter.com/brunobat_


On 29/11/18 11:51, Ivan Junckes Filho wrote:

> Bruno, there is a conflict in the PR.
>
> On Thu, Nov 29, 2018 at 9:49 AM Bruno Baptista <[hidden email]> wrote:
>
>> Hi,
>>
>> Can some committer please decide if this is good to merge?
>>
>> https://github.com/apache/tomee/pull/214
>>
>> Thanks
>>
>> Bruno Baptista
>> https://twitter.com/brunobat_
>>
>>
>> On 26/11/18 17:58, Bruno Baptista wrote:
>>> Hi Romain,
>>>
>>> Thanks for you feedback.
>>>
>>> I've pushed changes and added a comment to the PR.
>>>
>>> Cheers.
>>>
>>> Bruno Baptista
>>> https://twitter.com/brunobat_
>>>
>>>
>>> On 26/11/18 17:11, Romain Manni-Bucau wrote:
>>>> Hello Bruno,
>>>>
>>>> I put some suggestions on the PR, hope it helps.
>>>>
>>>> Romain Manni-Bucau
>>>> @rmannibucau <https://twitter.com/rmannibucau> |  Blog
>>>> <https://rmannibucau.metawerx.net/> | Old Blog
>>>> <http://rmannibucau.wordpress.com> | Github
>>>> <https://github.com/rmannibucau> |
>>>> LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
>>>> <
>> https://www.packtpub.com/application-development/java-ee-8-high-performance>
>>
>>>>
>>>>
>>>> Le lun. 26 nov. 2018 à 18:02, Bruno Baptista <[hidden email]> a
>>>> écrit :
>>>>
>>>>> Hi,
>>>>>
>>>>> I think the code for this example: TOMEE-2283 New Example: Websocket
>>>>> with TLS and Basic Auth
>>>>> <https://issues.apache.org/jira/browse/TOMEE-2283>
>>>>>
>>>>> Is ready for review here: https://github.com/apache/tomee/pull/214
>>>>>
>>>>> Can one of you please take a look?
>>>>>
>>>>> Cheers
>>>>>
>>>>> --
>>>>> Bruno Baptista
>>>>> https://twitter.com/brunobat_
>>>>>
>>>>>
>>>>>
Reply | Threaded
Open this post in threaded view
|

Re: TOMEE-2283 New Example: Websocket,with TLS and Basic Auth - PR #214

brunobat
Fixed

Bruno Baptista
https://twitter.com/brunobat_


On 29/11/18 11:55, Bruno Baptista wrote:

> Yeah... fixing it. It's sitting for too long.
>
> Bruno Baptista
> https://twitter.com/brunobat_
>
>
> On 29/11/18 11:51, Ivan Junckes Filho wrote:
>> Bruno, there is a conflict in the PR.
>>
>> On Thu, Nov 29, 2018 at 9:49 AM Bruno Baptista <[hidden email]>
>> wrote:
>>
>>> Hi,
>>>
>>> Can some committer please decide if this is good to merge?
>>>
>>> https://github.com/apache/tomee/pull/214
>>>
>>> Thanks
>>>
>>> Bruno Baptista
>>> https://twitter.com/brunobat_
>>>
>>>
>>> On 26/11/18 17:58, Bruno Baptista wrote:
>>>> Hi Romain,
>>>>
>>>> Thanks for you feedback.
>>>>
>>>> I've pushed changes and added a comment to the PR.
>>>>
>>>> Cheers.
>>>>
>>>> Bruno Baptista
>>>> https://twitter.com/brunobat_
>>>>
>>>>
>>>> On 26/11/18 17:11, Romain Manni-Bucau wrote:
>>>>> Hello Bruno,
>>>>>
>>>>> I put some suggestions on the PR, hope it helps.
>>>>>
>>>>> Romain Manni-Bucau
>>>>> @rmannibucau <https://twitter.com/rmannibucau> | Blog
>>>>> <https://rmannibucau.metawerx.net/> | Old Blog
>>>>> <http://rmannibucau.wordpress.com> | Github
>>>>> <https://github.com/rmannibucau> |
>>>>> LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
>>>>> <
>>> https://www.packtpub.com/application-development/java-ee-8-high-performance>
>>>
>>>
>>>>>
>>>>>
>>>>> Le lun. 26 nov. 2018 à 18:02, Bruno Baptista <[hidden email]> a
>>>>> écrit :
>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> I think the code for this example: TOMEE-2283 New Example: Websocket
>>>>>> with TLS and Basic Auth
>>>>>> <https://issues.apache.org/jira/browse/TOMEE-2283>
>>>>>>
>>>>>> Is ready for review here: https://github.com/apache/tomee/pull/214
>>>>>>
>>>>>> Can one of you please take a look?
>>>>>>
>>>>>> Cheers
>>>>>>
>>>>>> --
>>>>>> Bruno Baptista
>>>>>> https://twitter.com/brunobat_
>>>>>>
>>>>>>
>>>>>>
Reply | Threaded
Open this post in threaded view
|

Re: TOMEE-2283 New Example: Websocket,with TLS and Basic Auth - PR #214

jgallimore
Merged. We'll see how the build does. It looks like this has fixed ports -
8081 and 8443, I think we might have to look to see if we can use a random
rather than fixed port. What do you think?

Jon

On Thu, Nov 29, 2018 at 12:29 PM Bruno Baptista <[hidden email]> wrote:

> Fixed
>
> Bruno Baptista
> https://twitter.com/brunobat_
>
>
> On 29/11/18 11:55, Bruno Baptista wrote:
> > Yeah... fixing it. It's sitting for too long.
> >
> > Bruno Baptista
> > https://twitter.com/brunobat_
> >
> >
> > On 29/11/18 11:51, Ivan Junckes Filho wrote:
> >> Bruno, there is a conflict in the PR.
> >>
> >> On Thu, Nov 29, 2018 at 9:49 AM Bruno Baptista <[hidden email]>
> >> wrote:
> >>
> >>> Hi,
> >>>
> >>> Can some committer please decide if this is good to merge?
> >>>
> >>> https://github.com/apache/tomee/pull/214
> >>>
> >>> Thanks
> >>>
> >>> Bruno Baptista
> >>> https://twitter.com/brunobat_
> >>>
> >>>
> >>> On 26/11/18 17:58, Bruno Baptista wrote:
> >>>> Hi Romain,
> >>>>
> >>>> Thanks for you feedback.
> >>>>
> >>>> I've pushed changes and added a comment to the PR.
> >>>>
> >>>> Cheers.
> >>>>
> >>>> Bruno Baptista
> >>>> https://twitter.com/brunobat_
> >>>>
> >>>>
> >>>> On 26/11/18 17:11, Romain Manni-Bucau wrote:
> >>>>> Hello Bruno,
> >>>>>
> >>>>> I put some suggestions on the PR, hope it helps.
> >>>>>
> >>>>> Romain Manni-Bucau
> >>>>> @rmannibucau <https://twitter.com/rmannibucau> | Blog
> >>>>> <https://rmannibucau.metawerx.net/> | Old Blog
> >>>>> <http://rmannibucau.wordpress.com> | Github
> >>>>> <https://github.com/rmannibucau> |
> >>>>> LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
> >>>>> <
> >>>
> https://www.packtpub.com/application-development/java-ee-8-high-performance>
>
> >>>
> >>>
> >>>>>
> >>>>>
> >>>>> Le lun. 26 nov. 2018 à 18:02, Bruno Baptista <[hidden email]> a
> >>>>> écrit :
> >>>>>
> >>>>>> Hi,
> >>>>>>
> >>>>>> I think the code for this example: TOMEE-2283 New Example: Websocket
> >>>>>> with TLS and Basic Auth
> >>>>>> <https://issues.apache.org/jira/browse/TOMEE-2283>
> >>>>>>
> >>>>>> Is ready for review here: https://github.com/apache/tomee/pull/214
> >>>>>>
> >>>>>> Can one of you please take a look?
> >>>>>>
> >>>>>> Cheers
> >>>>>>
> >>>>>> --
> >>>>>> Bruno Baptista
> >>>>>> https://twitter.com/brunobat_
> >>>>>>
> >>>>>>
> >>>>>>
>
Reply | Threaded
Open this post in threaded view
|

Re: TOMEE-2283 New Example: Websocket,with TLS and Basic Auth - PR #214

brunobat
Hi Jon,

Thanks for the merge.

About the ports... Good that you mentioned it. They are hardcoded
because I couldn't start the server with the arquillian.xml port
definitions set to:

  <property name="httpPort">-1</property>
  <property name="httpsPort">-1</property>

The HTTPS port was never set. I think it's not working. I wonder were to
file a bug so we can look further into it. Would this be an
arquillian-tomee-remote bug?

Because I want to use the bundled sever.xml, I wonder which env.
variable I would need to use to make sure the server is started with the
random port defined by Arquillian.

Bruno Baptista
https://twitter.com/brunobat_


On 29/11/18 21:50, Jonathan Gallimore wrote:

> Merged. We'll see how the build does. It looks like this has fixed ports -
> 8081 and 8443, I think we might have to look to see if we can use a random
> rather than fixed port. What do you think?
>
> Jon
>
> On Thu, Nov 29, 2018 at 12:29 PM Bruno Baptista <[hidden email]> wrote:
>
>> Fixed
>>
>> Bruno Baptista
>> https://twitter.com/brunobat_
>>
>>
>> On 29/11/18 11:55, Bruno Baptista wrote:
>>> Yeah... fixing it. It's sitting for too long.
>>>
>>> Bruno Baptista
>>> https://twitter.com/brunobat_
>>>
>>>
>>> On 29/11/18 11:51, Ivan Junckes Filho wrote:
>>>> Bruno, there is a conflict in the PR.
>>>>
>>>> On Thu, Nov 29, 2018 at 9:49 AM Bruno Baptista <[hidden email]>
>>>> wrote:
>>>>
>>>>> Hi,
>>>>>
>>>>> Can some committer please decide if this is good to merge?
>>>>>
>>>>> https://github.com/apache/tomee/pull/214
>>>>>
>>>>> Thanks
>>>>>
>>>>> Bruno Baptista
>>>>> https://twitter.com/brunobat_
>>>>>
>>>>>
>>>>> On 26/11/18 17:58, Bruno Baptista wrote:
>>>>>> Hi Romain,
>>>>>>
>>>>>> Thanks for you feedback.
>>>>>>
>>>>>> I've pushed changes and added a comment to the PR.
>>>>>>
>>>>>> Cheers.
>>>>>>
>>>>>> Bruno Baptista
>>>>>> https://twitter.com/brunobat_
>>>>>>
>>>>>>
>>>>>> On 26/11/18 17:11, Romain Manni-Bucau wrote:
>>>>>>> Hello Bruno,
>>>>>>>
>>>>>>> I put some suggestions on the PR, hope it helps.
>>>>>>>
>>>>>>> Romain Manni-Bucau
>>>>>>> @rmannibucau <https://twitter.com/rmannibucau> | Blog
>>>>>>> <https://rmannibucau.metawerx.net/> | Old Blog
>>>>>>> <http://rmannibucau.wordpress.com> | Github
>>>>>>> <https://github.com/rmannibucau> |
>>>>>>> LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
>>>>>>> <
>> https://www.packtpub.com/application-development/java-ee-8-high-performance>
>>
>>>>>
>>>>>>>
>>>>>>> Le lun. 26 nov. 2018 à 18:02, Bruno Baptista <[hidden email]> a
>>>>>>> écrit :
>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> I think the code for this example: TOMEE-2283 New Example: Websocket
>>>>>>>> with TLS and Basic Auth
>>>>>>>> <https://issues.apache.org/jira/browse/TOMEE-2283>
>>>>>>>>
>>>>>>>> Is ready for review here: https://github.com/apache/tomee/pull/214
>>>>>>>>
>>>>>>>> Can one of you please take a look?
>>>>>>>>
>>>>>>>> Cheers
>>>>>>>>
>>>>>>>> --
>>>>>>>> Bruno Baptista
>>>>>>>> https://twitter.com/brunobat_
>>>>>>>>
>>>>>>>>
>>>>>>>>
Reply | Threaded
Open this post in threaded view
|

Re: TOMEE-2283 New Example: Websocket,with TLS and Basic Auth - PR #214

jgallimore
Possibly a bug, but I think we do something to handle it... I'll take a
peek and give you some pointers.

Jon

On Fri, Nov 30, 2018 at 9:48 AM Bruno Baptista <[hidden email]> wrote:

> Hi Jon,
>
> Thanks for the merge.
>
> About the ports... Good that you mentioned it. They are hardcoded
> because I couldn't start the server with the arquillian.xml port
> definitions set to:
>
>   <property name="httpPort">-1</property>
>   <property name="httpsPort">-1</property>
>
> The HTTPS port was never set. I think it's not working. I wonder were to
> file a bug so we can look further into it. Would this be an
> arquillian-tomee-remote bug?
>
> Because I want to use the bundled sever.xml, I wonder which env.
> variable I would need to use to make sure the server is started with the
> random port defined by Arquillian.
>
> Bruno Baptista
> https://twitter.com/brunobat_
>
>
> On 29/11/18 21:50, Jonathan Gallimore wrote:
> > Merged. We'll see how the build does. It looks like this has fixed ports
> -
> > 8081 and 8443, I think we might have to look to see if we can use a
> random
> > rather than fixed port. What do you think?
> >
> > Jon
> >
> > On Thu, Nov 29, 2018 at 12:29 PM Bruno Baptista <[hidden email]>
> wrote:
> >
> >> Fixed
> >>
> >> Bruno Baptista
> >> https://twitter.com/brunobat_
> >>
> >>
> >> On 29/11/18 11:55, Bruno Baptista wrote:
> >>> Yeah... fixing it. It's sitting for too long.
> >>>
> >>> Bruno Baptista
> >>> https://twitter.com/brunobat_
> >>>
> >>>
> >>> On 29/11/18 11:51, Ivan Junckes Filho wrote:
> >>>> Bruno, there is a conflict in the PR.
> >>>>
> >>>> On Thu, Nov 29, 2018 at 9:49 AM Bruno Baptista <[hidden email]>
> >>>> wrote:
> >>>>
> >>>>> Hi,
> >>>>>
> >>>>> Can some committer please decide if this is good to merge?
> >>>>>
> >>>>> https://github.com/apache/tomee/pull/214
> >>>>>
> >>>>> Thanks
> >>>>>
> >>>>> Bruno Baptista
> >>>>> https://twitter.com/brunobat_
> >>>>>
> >>>>>
> >>>>> On 26/11/18 17:58, Bruno Baptista wrote:
> >>>>>> Hi Romain,
> >>>>>>
> >>>>>> Thanks for you feedback.
> >>>>>>
> >>>>>> I've pushed changes and added a comment to the PR.
> >>>>>>
> >>>>>> Cheers.
> >>>>>>
> >>>>>> Bruno Baptista
> >>>>>> https://twitter.com/brunobat_
> >>>>>>
> >>>>>>
> >>>>>> On 26/11/18 17:11, Romain Manni-Bucau wrote:
> >>>>>>> Hello Bruno,
> >>>>>>>
> >>>>>>> I put some suggestions on the PR, hope it helps.
> >>>>>>>
> >>>>>>> Romain Manni-Bucau
> >>>>>>> @rmannibucau <https://twitter.com/rmannibucau> | Blog
> >>>>>>> <https://rmannibucau.metawerx.net/> | Old Blog
> >>>>>>> <http://rmannibucau.wordpress.com> | Github
> >>>>>>> <https://github.com/rmannibucau> |
> >>>>>>> LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
> >>>>>>> <
> >>
> https://www.packtpub.com/application-development/java-ee-8-high-performance
> >
> >>
> >>>>>
> >>>>>>>
> >>>>>>> Le lun. 26 nov. 2018 à 18:02, Bruno Baptista <[hidden email]>
> a
> >>>>>>> écrit :
> >>>>>>>
> >>>>>>>> Hi,
> >>>>>>>>
> >>>>>>>> I think the code for this example: TOMEE-2283 New Example:
> Websocket
> >>>>>>>> with TLS and Basic Auth
> >>>>>>>> <https://issues.apache.org/jira/browse/TOMEE-2283>
> >>>>>>>>
> >>>>>>>> Is ready for review here:
> https://github.com/apache/tomee/pull/214
> >>>>>>>>
> >>>>>>>> Can one of you please take a look?
> >>>>>>>>
> >>>>>>>> Cheers
> >>>>>>>>
> >>>>>>>> --
> >>>>>>>> Bruno Baptista
> >>>>>>>> https://twitter.com/brunobat_
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
>