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. > >> 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
