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

[Dave Page]

On Thu, May 3, 2018 at 10:06 AM, Akshay Joshi  wrote:

> Hi
>
> On Thu, May 3, 2018 at 2:20 PM, Dave Page  wrote:
>
>>
>>
>> On Thu, May 3, 2018 at 6:58 AM, Akshay Joshi <
>> akshay.jo...@enterprisedb.com> wrote:
>>
>>> Hi
>>>
>>> On Wed, May 2, 2018 at 9:05 PM, Victoria Henry 
>>> 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.
>

Thanks - patch applied with minor changes to the help text, and to change
the Password/Passphrase label to Password.



>
>>
>>>
 Sincerely,

 Victoria & Joao


 On Wed, May 2, 2018 at 5:58 AM, Akshay Joshi <
 akshay.jo...@enterprisedb.com> wrote:

> Hi Joao
>
>
> On Thu, Apr 26, 2018 at 10:26 PM, Joao De Almeida Pereira <
> jdealmeidapere...@pivotal.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 

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

[Akshay Joshi]

Hi

On Thu, May 3, 2018 at 2:20 PM, Dave Page  wrote:

>
>
> On Thu, May 3, 2018 at 6:58 AM, Akshay Joshi <
> akshay.jo...@enterprisedb.com> wrote:
>
>> Hi
>>
>> On Wed, May 2, 2018 at 9:05 PM, Victoria Henry  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.jo...@enterprisedb.com> wrote:
>>>
 Hi Joao


 On Thu, Apr 26, 2018 at 10:26 PM, Joao De Almeida Pereira <
 jdealmeidapere...@pivotal.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. 

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

[Dave Page]

On Thu, May 3, 2018 at 6:58 AM, Akshay Joshi 
wrote:

> Hi
>
> On Wed, May 2, 2018 at 9:05 PM, Victoria Henry  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.


>
>> Sincerely,
>>
>> Victoria & Joao
>>
>>
>> On Wed, May 2, 2018 at 5:58 AM, Akshay Joshi <
>> akshay.jo...@enterprisedb.com> wrote:
>>
>>> Hi Joao
>>>
>>>
>>> On Thu, Apr 26, 2018 at 10:26 PM, Joao De Almeida Pereira <
>>> jdealmeidapere...@pivotal.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 

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

[Akshay Joshi]

Hi

On Wed, May 2, 2018 at 9:05 PM, Victoria Henry  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?

>
> Sincerely,
>
> Victoria & Joao
>
>
> On Wed, May 2, 2018 at 5:58 AM, Akshay Joshi <
> akshay.jo...@enterprisedb.com> wrote:
>
>> Hi Joao
>>
>>
>> On Thu, Apr 26, 2018 at 10:26 PM, Joao De Almeida Pereira <
>> jdealmeidapere...@pivotal.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?
>>>
>>
>>

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

[Victoria Henry]

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?

Sincerely,

Victoria & Joao


On Wed, May 2, 2018 at 5:58 AM, Akshay Joshi 
wrote:

> Hi Joao
>
>
> On Thu, Apr 26, 2018 at 10:26 PM, Joao De Almeida Pereira <
> jdealmeidapere...@pivotal.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/
>>  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 

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

[Joao De Almeida Pereira]

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/
 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 
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.jo...@enterprisedb.com> wrote:
>
>> Hi Joao
>>
>> On Tue, Apr 24, 2018 at 10:04 PM, Joao De Almeida Pereira <
>> jdealmeidapere...@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.jo...@enterprisedb.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 
 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 

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

[Akshay Joshi]

Hi Joao

On Tue, Apr 24, 2018 at 10:04 PM, Joao De Almeida Pereira  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.jo...@enterprisedb.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 
>> 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.jo...@enterprisedb.com> wrote:
>>>


 On Mon, Apr 23, 2018 at 1:30 PM, Dave Page  wrote:

> Hi
>
> On Thu, Apr 19, 2018 at 6:56 PM, Anthony Emengo 
> 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.jo...@enterprisedb.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>*

>>>
>>>
>>
>>
>> 

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

[Joao De Almeida Pereira]

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.

Our recommendations for this change are:
- Name the variables with comprehensive names
- Extract functions where we can and try to wrap some tests around them
(ex: the javascript disabled functions)
- 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.

Thanks
Victoria & Joao


On Tue, Apr 24, 2018 at 10:13 AM Akshay Joshi 
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 
> 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.jo...@enterprisedb.com> wrote:
>>
>>>
>>>
>>> On Mon, Apr 23, 2018 at 1:30 PM, Dave Page  wrote:
>>>
 Hi

 On Thu, Apr 19, 2018 at 6:56 PM, Anthony Emengo 
 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.jo...@enterprisedb.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>*
>

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

[Akshay Joshi]

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  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.jo...@enterprisedb.com> wrote:
>
>>
>>
>> On Mon, Apr 23, 2018 at 1:30 PM, Dave Page  wrote:
>>
>>> Hi
>>>
>>> On Thu, Apr 19, 2018 at 6:56 PM, Anthony Emengo 
>>> 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.jo...@enterprisedb.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-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*
>>
>
>


-- 
*Akshay Joshi*

*Sr. Software Architect *



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


SSH_Tunnel_v2.patch
Description: Binary data

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

[Anthony Emengo]

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  wrote:

>
>
> On Mon, Apr 23, 2018 at 1:30 PM, Dave Page  wrote:
>
>> Hi
>>
>> On Thu, Apr 19, 2018 at 6:56 PM, Anthony Emengo 
>> 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.jo...@enterprisedb.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-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*
>

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

[Akshay Joshi]

On Mon, Apr 23, 2018 at 1:30 PM, Dave Page  wrote:

> Hi
>
> On Thu, Apr 19, 2018 at 6:56 PM, Anthony Emengo 
> 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.jo...@enterprisedb.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-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*

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

[Dave Page]

Hi

On Thu, Apr 19, 2018 at 6:56 PM, Anthony Emengo  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).


>
> Anthony and Victoria
>
> On Thu, Apr 19, 2018 at 1:48 AM, Akshay Joshi <
> akshay.jo...@enterprisedb.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-9517Mobile: +91 976-788-8246*
>>
>
>


-- 
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

[Anthony Emengo]

Hey Akshay

This patch passed our test pipelines.

Anthony and Victoria

On Thu, Apr 19, 2018 at 1:48 AM, Akshay Joshi  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-9517Mobile: +91 976-788-8246*
>

[pgAdmin4][Patch] Feature #1447 SSH Tunnel

[Akshay Joshi]

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-9517Mobile: +91 976-788-8246*


SSH_Tunnel_v1.patch
Description: Binary data