On Thu, May 3, 2018 at 10:06 AM, Akshay Joshi <akshay.jo...@enterprisedb.com > wrote:
> Hi > > On Thu, May 3, 2018 at 2:20 PM, Dave Page <dp...@pgadmin.org> 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 <vhe...@pivotal.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. > 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 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* >>>>> >>>> >>>> >>> >>> >>> -- >>> *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* > -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
