Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1lgPLH-0006yX-Or for pgadmin-hackers@arkaria.postgresql.org; Tue, 11 May 2021 10:10:20 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1lgPLG-0007fI-Lw for pgadmin-hackers@arkaria.postgresql.org; Tue, 11 May 2021 10:10:18 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1lgPLG-0007fA-Dv for pgadmin-hackers@lists.postgresql.org; Tue, 11 May 2021 10:10:18 +0000 Received: from mail-ed1-x52d.google.com ([2a00:1450:4864:20::52d]) by magus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1lgPLD-0003bF-PV for pgadmin-hackers@postgresql.org; Tue, 11 May 2021 10:10:18 +0000 Received: by mail-ed1-x52d.google.com with SMTP id l7so22206901edb.1 for ; Tue, 11 May 2021 03:10:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pgadmin.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=ptolr5lsB1SaUuJ2PllphbXiLreXM8ecO7Y08rKTnOY=; b=XG/OR4GHb7qNG5z4QQsewbXge/5Z3X6FtAfzKfWV/CEoGoCAdl/77J70QR2boJ9lDU VWPL3pDrMifYFa0V1ZS5FW4y5YeFFVuwqYtkKeU2uO8ZsTeqmh2LnyMwzx7ycoJiguHr R657uMEjKIbJBhT2PGDMskTjdQfX7GEou0akCWsIFkXzh5lH+TBBj8PuJJ1328IZj2Vv VvSk/Wf0vhZTFxTyhdMR6ejNKPn24T2qxaB4ZT02MYqk/UEywqJbLBbIi+xjMBQ9KQjW lZtUwP7wBpaeB4gWsyw6elkYqPZImIFmZdNzQ/dk8RxKJ3w0YuG459KacYsntIv8VMX+ TMNg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=ptolr5lsB1SaUuJ2PllphbXiLreXM8ecO7Y08rKTnOY=; b=CSO4Rif9cs2DGof+RuWUpqeIqhb+FWI1iuShYCp8rnQqjXUcpZ/e6o21hSh28SbO4V FN5TKF5EE8HI0UYJ683aiqbCRejK0fGCPtfkdli7mOa6qydZc6YedZkAKMPr7L8ah5kS clY8mH70IW4vQmsVF2N5adYUjpxo+YbQTim8WrdJh6zE6n6z57xpOuB9GO6WgGOl9nue CSvZyWJaGugvNtcD/a6IrpShFC9ecuDcAOFyfTpNDMsB/SjknMndU0Pz6/aj4zw0cZbC 1aiVFY1ciCRaGQZZJx8bqPY9VI85GlVCtTBhnFc7mB1Xq0yuW8KZp0DdTdm2AKKtjmWE vQQw== X-Gm-Message-State: AOAM533WlYqrDLZCftrAgnQI2hPaAEuVzC3suoKz3wabUXKHpcmnOUhw JFyTZ6Qx7l2Ypjz0CVgSwzpanUpKuOlIqGmzs9wVXw== X-Google-Smtp-Source: ABdhPJzyGs5+TG2hpaVSDEXg4WWHsY1v81xZZM001+ztkMIAzMurwwudKonkXuJpVjgFfHfhSysUUrGQgydJYXcAhIw= X-Received: by 2002:aa7:cad4:: with SMTP id l20mr4829541edt.382.1620727814664; Tue, 11 May 2021 03:10:14 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Dave Page Date: Tue, 11 May 2021 11:10:03 +0100 Message-ID: Subject: Re: [pgAdmin][RM-2341]: Add menu option for starting PSQL To: Akshay Joshi Cc: Nikhil Mohite , pgadmin-hackers Content-Type: multipart/alternative; boundary="000000000000abc33405c20b1911" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --000000000000abc33405c20b1911 Content-Type: text/plain; charset="UTF-8" Hi On Tue, May 11, 2021 at 9:02 AM Akshay Joshi wrote: > Hi Nikhil > > Following are the review comments: > > *GUI specific*: > > - We need a panel icon for PSQL like query tool, we can also add that > on the browser tree toolbar. > - PSQL Tool menu should be visible for all the child nodes of the > database node. Follow the same as Query Tool. > - PSQL tab title should be only database server name as the user can > change the database/user from PSQL command, so it's been difficult to > update the tab title. > - PSQL connection is still open even if we disconnect the database > server from the browser tree. > > *Code specific:* > > - Remove an extra space from requirements.txt and package.json > - Documentation needs to be updated to let the user know from where > the PSQL tool will open and on which node it is applicable. > - psql/__init__.py check there are so many unused imports please > remove them. > - We are not using cheroot so it should be removed from > requirements.txt and also remove the import statement from pgAdmin4.py > - Test cases are showing successful but actually, there are some > routing errors please check. > > A few other things I noticed: - I was prompted to enter a password. This should be passed in the environment to psql as it is for pg_dump etc. - There seems to be an issue with terminal compatibility (which I didn't have on my prototype): ml=# select * from pg_class; WARNING: terminal is not fully functional -[ RECORD 1 ]-------+---------------------------------------------- oid | 79354 relname | housing ... - The panel should honour the styleguide. I'm running in dark mode, and see a jet black background. I would expect to see the same background/foreground colours as the treeview. - I spotted at least one print() statement that shouldn't be there (debug output should go through the logger) - psql/__init__.py:235 - This seems suspect - why would there be a password in a connection string we've built? And why would it be xxx? if 'password=xxx' in conn_attr: conn_attr = conn_attr.replace('password=xxx', '') - There's a thick white line at the bottom of the panel, where a horizontal scrollbar might be if there was one. - The trailing semi-colon should be removed from: "ERROR: Shell commands are disabled in psql for security;" Once we're happy with the patch in general, I'll do a string review before committing. In particular, I want to be sure the text in config.py is appropriately worded. This is shaping up nicely! Good work. > > On Mon, May 10, 2021 at 7:32 PM Nikhil Mohite < > nikhil.mohite@enterprisedb.com> wrote: > >> Hi Dave/ Team, >> >> PFA updated patch, sorry for the inconvenience, while cleanup I removed >> the unwanted libraries but forgot to remove the code related to them. >> >> On Mon, May 10, 2021 at 7:10 PM Dave Page wrote: >> >>> Hi >>> >>> On Mon, May 10, 2021 at 1:45 PM Nikhil Mohite < >>> nikhil.mohite@enterprisedb.com> wrote: >>> >>>> Hi Hackers, >>>> >>>> Please find the attached patch for RM-2341 >>>> : Add Menu option for >>>> starting PSQL. >>>> 1. Added new Option PSQL Tool in Tools menu. >>>> 2. Added the same option for Server and Database nodes from the tree >>>> view. >>>> >>> >>> Unfortunately there's a trailing comma in package.json that makes it >>> invalid. If I fix that, then I get the error below, so I'm guessing the >>> intention was to actually include another package there? >>> >>> ERROR in ./pgadmin/tools/psql/static/js/psql_module.js 23:50-82 >>> Module not found: Error: Can't resolve 'local-echo-controller' in >>> '/Users/dpage/git/pgadmin4/web/pgadmin/tools/psql/static/js' >>> resolve 'local-echo-controller' in >>> '/Users/dpage/git/pgadmin4/web/pgadmin/tools/psql/static/js' >>> Parsed request is a module >>> using description file: /Users/dpage/git/pgadmin4/web/package.json >>> (relative path: ./pgadmin/tools/psql/static/js) >>> aliased with mapping 'local-echo-controller': >>> '/Users/dpage/git/pgadmin4/web/node_modules/local-echo' to >>> '/Users/dpage/git/pgadmin4/web/node_modules/local-echo' >>> using description file: /Users/dpage/git/pgadmin4/web/package.json >>> (relative path: ./pgadmin/tools/psql/static/js) >>> Field 'browser' doesn't contain a valid alias configuration >>> root path /Users/dpage/git/pgadmin4/web >>> using description file: >>> /Users/dpage/git/pgadmin4/web/package.json (relative path: >>> ./Users/dpage/git/pgadmin4/web/node_modules/local-echo) >>> no extension >>> Field 'browser' doesn't contain a valid alias configuration >>> >>> /Users/dpage/git/pgadmin4/web/Users/dpage/git/pgadmin4/web/node_modules/local-echo >>> doesn't exist >>> .js >>> Field 'browser' doesn't contain a valid alias configuration >>> >>> /Users/dpage/git/pgadmin4/web/Users/dpage/git/pgadmin4/web/node_modules/local-echo.js >>> doesn't exist >>> .jsx >>> Field 'browser' doesn't contain a valid alias configuration >>> >>> /Users/dpage/git/pgadmin4/web/Users/dpage/git/pgadmin4/web/node_modules/local-echo.jsx >>> doesn't exist >>> as directory >>> >>> /Users/dpage/git/pgadmin4/web/Users/dpage/git/pgadmin4/web/node_modules/local-echo >>> doesn't exist >>> using description file: >>> /Users/dpage/git/pgadmin4/web/package.json (relative path: >>> ./node_modules/local-echo) >>> no extension >>> Field 'browser' doesn't contain a valid alias configuration >>> /Users/dpage/git/pgadmin4/web/node_modules/local-echo >>> doesn't exist >>> .js >>> Field 'browser' doesn't contain a valid alias configuration >>> /Users/dpage/git/pgadmin4/web/node_modules/local-echo.js >>> doesn't exist >>> .jsx >>> Field 'browser' doesn't contain a valid alias configuration >>> /Users/dpage/git/pgadmin4/web/node_modules/local-echo.jsx >>> doesn't exist >>> as directory >>> /Users/dpage/git/pgadmin4/web/node_modules/local-echo >>> doesn't exist >>> @ ./pgadmin/tools/psql/static/js/index.js 17:19-43 >>> >>> 2021-05-10 14:38:37: webpack 5.21.2 compiled with 1 error in 60041 ms >>> >>> -- >>> Dave Page >>> Blog: https://pgsnake.blogspot.com >>> Twitter: @pgsnake >>> >>> EDB: https://www.enterprisedb.com >>> >> >> Regards, >> Nikhil Mohite >> > > > -- > *Thanks & Regards* > *Akshay Joshi* > *pgAdmin Hacker | Principal Software Architect* > *EDB Postgres * > > *Mobile: +91 976-788-8246* > -- Dave Page Blog: https://pgsnake.blogspot.com Twitter: @pgsnake EDB: https://www.enterprisedb.com --000000000000abc33405c20b1911 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi

On Tue, May 11, 2021 at 9:02 AM Akshay Joshi <akshay.joshi@enterprisedb.com>= ; wrote:
Hi Nikhil=C2=A0
Following are the review comments:

= GUI specific:
  • We need a panel icon for PSQL like quer= y tool, we can also add that on the browser tree toolbar.
  • PSQL Tool= menu should be visible for all the child nodes of the database node. Follo= w the same as Query Tool.
  • PSQL tab title should be only database se= rver name as the user can change the database/user from PSQL command, so it= 's been difficult to update the tab title.
  • PSQL connection is s= till open even if we disconnect the database server from the browser tree.<= /li>
Code specific:
  • Remove an extra= space from requirements.txt and package.json
  • Documentation needs t= o be updated to let the user know from where the PSQL tool will open and on= which node it is applicable.
  • psql/__init__.py check there are so m= any unused imports please remove them.
  • We are not using cheroot so = it should be removed from requirements.txt and also remove the import state= ment from pgAdmin4.py=C2=A0
  • Test cases are showing=C2=A0success= ful but actually, there are some routing errors please check.
A few other things I noticed:

- I was prompted to enter a password. This should be passed in the en= vironment to psql as it is for pg_dump etc.
- There seems to be a= n issue with terminal compatibility (which I didn't have on my prototyp= e):

ml=3D# select * from pg_class;
= WARNING: terminal is not fully functional
-[ RECORD 1 ]-------+--= --------------------------------------------
oid =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 | 79354
relname =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 | housing
...
=
- The panel should honour the styleguide. I'm running in= dark mode, and see a jet black background. I would expect to see the same = background/foreground colours as the treeview.
- I spotted at lea= st one print() statement that shouldn't be there (debug output should g= o through the logger) - psql/__init__.py:235
- This seems suspect= - why would there be a password in a connection string we've built? An= d why would it be xxx?

=C2=A0 =C2=A0 if '= password=3Dxxx' in conn_attr:
=C2=A0 =C2=A0 =C2=A0 =C2=A0 con= n_attr =3D conn_attr.replace('password=3Dxxx', '')

- There's a thick white line at the bottom of th= e panel, where a horizontal scrollbar might be if there was one.
= - The trailing semi-colon=C2=A0should be removed from: "ERROR: Shell c= ommands are disabled in psql for security;"

O= nce we're happy with the patch in general, I'll do a string review = before committing. In particular, I want to be sure the text in config.py i= s appropriately worded.

This is shaping up nicely!= Good work.
=C2=A0

On Mon, May 10, 2021 at 7:32= PM Nikhil Mohite <nikhil.mohite@enterprisedb.com> wrote:
Hi Dave/ Team,

P= FA updated patch, sorry for the inconvenience, while cleanup I removed the = unwanted libraries but forgot to remove the code related to them.

=
On Mon, Ma= y 10, 2021 at 7:10 PM Dave Page <dpage@pgadmin.org> wrote:
=
Hi

On Mon, May 10, 2021= at 1:45 PM Nikhil Mohite <nikhil.mohite@enterprisedb.com> wrote:
Hi Hackers,

Please = find the attached patch for RM-2341:=C2=A0Add Menu option for starting PS= QL.
1. Added new Option PSQL Tool in Tools menu.
2. Add= ed the same option for Server and Database nodes from the tree view.
<= /div>

Unfortunately=C2=A0there's a trai= ling comma in package.json that makes it invalid. If I fix that, then I get= the error below, so I'm guessing the intention was to actually include= another package there?

ERROR in ./pgadmin/to= ols/psql/static/js/psql_module.js 23:50-82
Module not found: Erro= r: Can't resolve 'local-echo-controller' in '/Users/dpage/g= it/pgadmin4/web/pgadmin/tools/psql/static/js'
resolve 'lo= cal-echo-controller' in '/Users/dpage/git/pgadmin4/web/pgadmin/tool= s/psql/static/js'
=C2=A0 Parsed request is a module
=C2=A0 using description file: /Users/dpage/git/pgadmin4/web/package.json = (relative path: ./pgadmin/tools/psql/static/js)
=C2=A0 =C2=A0 ali= ased with mapping 'local-echo-controller': '/Users/dpage/git/pg= admin4/web/node_modules/local-echo' to '/Users/dpage/git/pgadmin4/w= eb/node_modules/local-echo'
=C2=A0 =C2=A0 =C2=A0 using descri= ption file: /Users/dpage/git/pgadmin4/web/package.json (relative path: ./pg= admin/tools/psql/static/js)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 Field = 9;browser' doesn't contain a valid alias configuration
= =C2=A0 =C2=A0 =C2=A0 =C2=A0 root path /Users/dpage/git/pgadmin4/web
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 using description file: /Users/dpage/= git/pgadmin4/web/package.json (relative path: ./Users/dpage/git/pgadmin4/we= b/node_modules/local-echo)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 no extension
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= Field 'browser' doesn't contain a valid alias configuration
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /Users/dpage/git/p= gadmin4/web/Users/dpage/git/pgadmin4/web/node_modules/local-echo doesn'= t exist
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 .js
= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Field 'browser' do= esn't contain a valid alias configuration
=C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /Users/dpage/git/pgadmin4/web/Users/dpage/g= it/pgadmin4/web/node_modules/local-echo.js doesn't exist
=C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 .jsx
=C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 Field 'browser' doesn't contain a v= alid alias configuration
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 /Users/dpage/git/pgadmin4/web/Users/dpage/git/pgadmin4/web/node_= modules/local-echo.jsx doesn't exist
=C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 as directory
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 /Users/dpage/git/pgadmin4/web/Users/dpage/git/pgadmin4/web/n= ode_modules/local-echo doesn't exist
=C2=A0 =C2=A0 =C2=A0 =C2= =A0 using description file: /Users/dpage/git/pgadmin4/web/package.json (rel= ative path: ./node_modules/local-echo)
=C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 no extension
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= Field 'browser' doesn't contain a valid alias configuration
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /Users/dpage/git/pgadmin4= /web/node_modules/local-echo doesn't exist
=C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 .js
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 F= ield 'browser' doesn't contain a valid alias configuration
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /Users/dpage/git/pgadmin4/w= eb/node_modules/local-echo.js doesn't exist
=C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 .jsx
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = Field 'browser' doesn't contain a valid alias configuration
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /Users/dpage/git/pgadmin4/= web/node_modules/local-echo.jsx doesn't exist
=C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 as directory
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 /Users/dpage/git/pgadmin4/web/node_modules/local-echo doesn'= t exist
=C2=A0@ ./pgadmin/tools/psql/static/js/index.js 17:19-43<= /div>

2021-05-10 14:38:37: webpack 5.21.2 compiled with = 1 error in 60041 ms
=C2=A0
--
Dave Page
Blog: https://pgsnake.blogspot.com
Twitter: @pgsnak= e

EDB: ht= tps://www.enterprisedb.com
Regards,
Nikhil Mohite=C2=A0


--
Thank= s & Regards
Akshay Joshi
pgAdmin Hacker | Principal Softw= are Architect
EDB Po= stgres
Mobile: +91 976-788-8246



--
<= /div>
--000000000000abc33405c20b1911--