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

Hi

On Thu, May 3, 2018 at 2:20 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:

>
>
> On Thu, May 3, 2018 at 6:58 AM, Akshay Joshi <
> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>
>> Hi
>>
>> On Wed, May 2, 2018 at 9:05 PM, Victoria Henry <vhenry(at)pivotal(dot)io> wrote:
>>
>>> Hi Akshay,
>>>
>>> Thanks for sending this updated patch. The linter and tests are all
>>> passing.
>>>
>>>> - utils/driver/psycopg2/server_manager.py
>>>>> - Do we have Unit Tests around this?
>>>>
>>>> No.
>>>
>>>
>>> In our opinion, server_manager.py and connection.py should have tests.
>>> Are you finding it difficult to add tests to these files?
>>>
>>
>> We will have to write test cases from scratch for both the files and
>> it will take time, there is no point keeping these important feature(SSH
>> Tunnel) on hold. We can create a separate task for this as we have for
>> utility(Backup, Maintenance, Restore) modules.
>>
>> @Dave your thoughts on this?
>>
>
> I agree. Please add a ticket to add these tests in the future. General
> tests for those files should not hold up this feature.
>

Done. Can you please review and commit this feature.

>
>
>>
>>> Sincerely,
>>>
>>> Victoria & Joao
>>>
>>>
>>> On Wed, May 2, 2018 at 5:58 AM, Akshay Joshi <
>>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>>
>>>> Hi Joao
>>>>
>>>>
>>>> On Thu, Apr 26, 2018 at 10:26 PM, Joao De Almeida Pereira <
>>>> jdealmeidapereira(at)pivotal(dot)io> wrote:
>>>>
>>>>> 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.
>>>>>
>>>>
>>>> If I understood you correctly, you want separate connect and create
>>>> function for SSH Tunnel. If yes I don't think so we should move the SSH
>>>> code and make the rest of the code duplicated in two different functions.
>>>> For "create" function I have just added logic to pass SSH tunnel parameter
>>>> while creating server object, encrypt the tunnel password and send it to
>>>> connect method. For "connect" function encrypt the password and send it
>>>> connect method as parameter. There is no point for such small changes we
>>>> should create two separate functions.
>>>>
>>>>>
>>>>> - utils/driver/psycopg2.py same comments has above
>>>>>
>>>>
>>>> I think you are talking about "utils/driver/psycopg2/connection.py".
>>>> Same comments as 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?
>>>>>
>>>>
>>>> Fixed.
>>>>
>>>>> - 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
>>>>>
>>>> Model creation is the main functionality of the "server.js" (or
>>>> any other module file), code readability wise it should be in the same
>>>> file. If we will do it for rest of the modules then there are so many java
>>>> script files where model creation is in separate file.
>>>>
>>>> - We could also convert this file to ES6
>>>>>
>>>>
>>>> I am new to this, so will need to learn first. We can create a
>>>> separate task to do this.
>>>>
>>>>>
>>>>> - utils/driver/psycopg2/server_manager.py
>>>>> - Do we have Unit Tests around this?
>>>>>
>>>>
>>>> No.
>>>>
>>>>> - 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
>>>>>
>>>>
>>>> According to me SSH Tunnel parameters is the part of server
>>>> manager as we do have other parameters of Server dialog. We can
>>>> isolate the SSH part in other class, but most of the modules (including
>>>> Server module) have access to the ServerManager. If we will isolate
>>>> that part then anyways we will have to write wrapper functions in
>>>> ServerManager which will eventually call functions of new SSH class.
>>>>
>>>> As one ServerManager object belongs to one server, similarly one
>>>> SSH Tunnel belongs to one server. When SSH tunnel gets created it will
>>>> return local bind port, where rest of the communication should be done on
>>>> local host and the local bind port return by the "SSHTunnelForwarder"
>>>> class, so that need to be in the ServerManager.
>>>>
>>>> Considering above I have kept that logic in ServerManager.
>>>>
>>>>>
>>>>>
>>>>> - 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
>>>>>
>>>>
>>>> Added one test case for SSHTunnelConnectionLost.
>>>>
>>>>>
>>>>>
>>>>> 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?
>>>>>
>>>>
>>>> Fixed. Replace "mgr" with "manager" almost at 69 places in the file.
>>>>
>>>> Attached is the modified patch. Please review it.
>>>>
>>>>>
>>>>>
>>>>> 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>*
>>>>>>
>>>>>
>>>>
>>>>
>>>> --
>>>> *Akshay Joshi*
>>>>
>>>> *Sr. Software Architect *
>>>>
>>>>
>>>>
>>>> *Phone: +91 20-3058-9517Mobile: +91 976-788-8246*
>>>>
>>>
>>>
>>
>>
>> --
>> *Akshay Joshi*
>>
>> *Sr. Software Architect *
>>
>>
>>
>> *Phone: +91 20-3058-9517Mobile: +91 976-788-8246*
>>
>
>
>
> --
> 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-9517Mobile: +91 976-788-8246*