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