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

Hi Akshay,

Some suggestions:
- browser/server_groups/servers/__init__py
This file could have been split into separate functionalities. There is a
chunk of changes for connect, why not move that out? Same thing for create.
Do we really need to have full integrationy tests that do a HTTP request
and connect to a real database to do make sure the functionalities are
working? If we isolate these into their own actions we can more easily get
more coverage on the code with tests that would be much faster and
directed. The big advantage of this is that by reading the tests we can
understand what the functions do. Self documenting code.

- utils/driver/psycopg2.py same comments has above

- browser/server_groups/servers/static/js/server.js:
- The patterm of using `m` for `model`? is a bad pattern, so why not
change it?
- We could extract the model creation from this file. This will allow us
to add some tests around disabled methods that are a bit everywhere
- We could also convert this file to ES6

- utils/driver/psycopg2/server_manager.py
- Do we have Unit Tests around this?
- Maybe this SSH part could be isolated into it's own class, as it is not
100% related to the class in question. We need to use it but is is not part
of the ServerManager domain

- JS template. Eventually I would like to see if completely removed, and
the information that we are generating using the template can be passed to
the frontend via a Ajax call as an example( Do not think this is the time
to do it.)

- start_running_query.py
- we could enrich the tests of this functionality

And example of naming is for example on psycopg2/connection.py

mgr = self.manager

How much to we win by having this variable name versus manager =
self.manager or even using the self.manager?

This is not for you in specific, but for @hackers in general:
The book https://www.amazon.com/dp/0132350882/
<https://www.amazon.com/dp/0132350882/?tag=stackoverflow17-20> is a pretty
nice book that gives you an introduction to clean code, that is self
documenting and that is much easily maintained.

Thanks
Joao

On Thu, Apr 26, 2018 at 3:44 AM Akshay Joshi <akshay(dot)joshi(at)enterprisedb(dot)com>
wrote:

> Hi Hackers,
>
> Attached is the updated patch which includes documentation of the feature
> and also updated screenshots of server dialog with new "SSH Tunnel" tab.
>
> On Wed, Apr 25, 2018 at 11:45 AM, Akshay Joshi <
> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>
>> Hi Joao
>>
>> On Tue, Apr 24, 2018 at 10:04 PM, Joao De Almeida Pereira <
>> jdealmeidapereira(at)pivotal(dot)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-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>*
>