Re: [pgAdmin4][Patch] Feature #1447 SSH Tunnel

Hi Joao

On Tue, Apr 24, 2018 at 10:04 PM, Joao De Almeida Pereira <jdealmeidapereira
@pivotal.io> wrote:

> Hi Akshay,
>
> After looking through the patch we found some one letter variable names
> and this is a regression on what we have been trying to accomplish in the
> last year.
>
> An objective that we have for pgAdmin source code is to increase the
> testability of it and make it more readable. If we keep on adding one
> letter variables and if we continue adding code to already convoluted
> source files it is going to be very hard to achieve this objective.
>

At my level I have tried not to give one letter variable names. Are
you talking about the variable "m" in server.js file which represents the
Model? If yes then I have followed the code written for whole schema and I
thought we have to maintain the consistency, so use that as it is. Apart
from that I haven't seen any other one letter variable, please correct me
so that I'll rename it.

>
> Our recommendations for this change are:
> - Name the variables with comprehensive names
>
Can you please suggest from the patch.

> - Extract functions where we can and try to wrap some tests around them
> (ex: the javascript disabled functions)
>

I have tried to do that too, if you can see the "server/__init__.py"
file I have created "*get_response_for_password" function to remove
redundant code. Based on the condition it will return the json response.*

> - We really need to find a better pattern than templated Javascript to
> pass information from the backend to the frontend
>
- When changing a piece of code, if we see code that is confusing or that
> is hard to read, we should refactor instead of adding to the pattern.
>

Please elaborate more with respect to my patch, which part of code
should required modification?

>
> Thanks
> Victoria & Joao
>
>
> On Tue, Apr 24, 2018 at 10:13 AM Akshay Joshi <
> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>
>> Hi Hackers
>>
>> As per suggestion by Dave, I have moved "Advanced" tab at the last for
>> Server dialog. Attached is the modified patch.
>>
>> On Mon, Apr 23, 2018 at 7:32 PM, Anthony Emengo <aemengo(at)pivotal(dot)io>
>> wrote:
>>
>>> For what it is worth, I manually verified that the feature worked, as
>>> well as looked through the code.
>>>
>>> I'd like to see end-to-end testing for regression sake, but it's hard to
>>> so at this moment.
>>>
>>> - Anthony and Joao.
>>>
>>> On Mon, Apr 23, 2018 at 5:09 AM, Akshay Joshi <
>>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>>
>>>>
>>>>
>>>> On Mon, Apr 23, 2018 at 1:30 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>>
>>>>> Hi
>>>>>
>>>>> On Thu, Apr 19, 2018 at 6:56 PM, Anthony Emengo <aemengo(at)pivotal(dot)io>
>>>>> wrote:
>>>>>
>>>>>> Hey Akshay
>>>>>>
>>>>>> This patch passed our test pipelines.
>>>>>>
>>>>>
>>>>> Did you test the feature and//or review the code and tests? Passing
>>>>> the tests is great, *if* the whole feature is covered (and the nature of
>>>>> this patch will make that quite difficult, maybe impossible to do without
>>>>> external infrastructure and config).
>>>>>
>>>>
>>>> Agreed, it's been difficult to write test case to test the complete
>>>> feature.
>>>>
>>>>>
>>>>>
>>>>>>
>>>>>> Anthony and Victoria
>>>>>>
>>>>>> On Thu, Apr 19, 2018 at 1:48 AM, Akshay Joshi <
>>>>>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>>>>>
>>>>>>> Hi Hackers
>>>>>>>
>>>>>>> I have implemented the SSH Tunnel support using https://pypi.org/
>>>>>>> project/sshtunnel/ python package. Added "SSH Tunnel" Tab in server
>>>>>>> dialog. This implementation supports user name /password and private/public
>>>>>>> key combination with Passphrase to crate SSH Tunnel. I have added
>>>>>>> regression test case to add server using SSH Tunnel options.
>>>>>>>
>>>>>>> The given python package(https://pypi.org/project/sshtunnel/) support
>>>>>>> Python version *2.7, 3.4+*.
>>>>>>> It uses Paramiko (Python implementation of SSHv2 protocol) which
>>>>>>> actually drops support for Python 2.6. So I have added
>>>>>>> *SUPPORT_SSH_TUNNEL* parameter in config.py which checks the python
>>>>>>> version and set the flag accordingly. In case of Python 2.6, 3.0, 3.1, 3.2
>>>>>>> and 3.3 control on the "SSH Tunnel" tab of server dialog will be disabled.
>>>>>>>
>>>>>>> Please review it, and if looks good please commit the code.
>>>>>>>
>>>>>>> --
>>>>>>> *Akshay Joshi*
>>>>>>>
>>>>>>> *Sr. Software Architect *
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> *Phone: +91 20-3058-9517 <+91%2020%203058%209517>Mobile: +91
>>>>>>> 976-788-8246 <+91%2097678%2088246>*
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Dave Page
>>>>> Blog: http://pgsnake.blogspot.com
>>>>> Twitter: @pgsnake
>>>>>
>>>>> EnterpriseDB UK: http://www.enterprisedb.com
>>>>> The Enterprise PostgreSQL Company
>>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> *Akshay Joshi*
>>>>
>>>> *Sr. Software Architect *
>>>>
>>>>
>>>>
>>>> *Phone: +91 20-3058-9517 <+91%2020%203058%209517>Mobile: +91
>>>> 976-788-8246 <+91%2097678%2088246>*
>>>>
>>>
>>>
>>
>>
>> --
>> *Akshay Joshi*
>>
>> *Sr. Software Architect *
>>
>>
>>
>> *Phone: +91 20-3058-9517 <+91%2020%203058%209517>Mobile: +91 976-788-8246
>> <+91%2097678%2088246>*
>>
>

--
*Akshay Joshi*

*Sr. Software Architect *

*Phone: +91 20-3058-9517Mobile: +91 976-788-8246*