Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtp (Exim 4.84_2) (envelope-from ) id 1d81xo-0005u2-CT for pgadmin-hackers@arkaria.postgresql.org; Tue, 09 May 2017 10:01:52 +0000 Received: from localhost ([127.0.0.1] helo=postgresql.org) by malur.postgresql.org with smtp (Exim 4.84_2) (envelope-from ) id 1d81xn-0008Or-VT for pgadmin-hackers@arkaria.postgresql.org; Tue, 09 May 2017 10:01:52 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.84_2) (envelope-from ) id 1d81xY-00080E-FF for pgadmin-hackers@postgresql.org; Tue, 09 May 2017 10:01:36 +0000 Received: from mail-pg0-x235.google.com ([2607:f8b0:400e:c05::235]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.84_2) (envelope-from ) id 1d81xT-0001TF-MY for pgadmin-hackers@postgresql.org; Tue, 09 May 2017 10:01:35 +0000 Received: by mail-pg0-x235.google.com with SMTP id u28so38009665pgn.1 for ; Tue, 09 May 2017 03:01:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=enterprisedb-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=gxFDDBR1RzzxEUJFT5FLNlOBlzpx8H/plTkGjEc4Z6M=; b=GBncVEhwm0OjNfrgvMBa++/iJoU6P7zGo6eLns9cx8E18wpLLk4vTEYG/c9QuuOeOJ cGaijwCiZBl8RrLiCigyLCV2nMfw2mXzJJ+xeNh8S/x4WBAV+3U7jq/rt1tYxlzkaac6 SNTSUCyHAbuXyEPV1p4y2mH4YEUTJWIFhNLbyDjeIlgn2ZsGD7zgwQbC0T/39B2XuvNZ ZBcu/5/sbT+DKGEnROsLDXxiWt+em+XhdMs95zNxoN5IqPE7BHF40zLLOW5Q2XVw/Ly1 Ov3qHF01FwhVPvm0Aqv1iyoJIfVkabpZUtt4PFeXqUNXz5qkOYM3D44LFtcRoUT762s/ FU+A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=gxFDDBR1RzzxEUJFT5FLNlOBlzpx8H/plTkGjEc4Z6M=; b=mgFCmKTXBiIWXugetQ4Wptku4bnycoO5QCV6X3ZFF9/5PJZeQcLCLBg10xlO67UI9Z XDC5UoLdQiaoGnzSk1dKUtZ6zeyrbYrPf3Amatt+fcVugB9BIlvRzXzV4Oc1bAeE1EhH prx6xHRnAyFavR+bK+0qY/jWMPncXbVJ4PfMvM3BmjD0YhGpQFchJl28GdIQivaAnqpg CIFzSIJf9wu/ISGht1Tbfau1XjVRl8pUkvIfeErcNghehMrnsaKFpyTw/nNgLCwGZewD CXhfJQh9WZvcZyqRyzrpsQY91vIO1qO6VFCNUtL7Bbl7cFB8Y5stF9Qfrj9n9I+QvS7X Pnww== X-Gm-Message-State: AN3rC/50tJPJinyiIacpIuQzRWZuPcKmRLRbuERdAcvrU1TYQcEzuGqI rzYvmz9CGOl0meDeE3xaC9oCh07m4YPwafo= X-Received: by 10.98.147.145 with SMTP id r17mr23563507pfk.238.1494324089730; Tue, 09 May 2017 03:01:29 -0700 (PDT) MIME-Version: 1.0 Received: by 10.100.170.75 with HTTP; Tue, 9 May 2017 03:01:29 -0700 (PDT) In-Reply-To: References: From: Akshay Joshi Date: Tue, 9 May 2017 15:31:29 +0530 Message-ID: Subject: Re: [patch] Dependents and Dependencies in GreenPlum To: Sarah McAlear Cc: Dave Page , Robert Eckhardt , Joao Pedro De Almeida Pereira , Ashesh Vashi , pgadmin-hackers , Matthew Kleiman Content-Type: multipart/alternative; boundary=94eb2c0de80a8c4e12054f1470b3 X-Pg-Spam-Score: -1.9 (-) List-Archive: List-Help: List-ID: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: X-Mailing-List: pgadmin-hackers Precedence: bulk Sender: pgadmin-hackers-owner@postgresql.org --94eb2c0de80a8c4e12054f1470b3 Content-Type: text/plain; charset=UTF-8 Hi Sarah Below are my review comments on the latest patch: - Some test file names ended with "*_sql_template.py*" do we need to add that string ? - Files "test_column_acl_sql_template.py" and "test_column_properties_sql _template.py" should be moved from tables->tests to tables->column->tests. As it's related to column. - Files "test_trigger_get_oid_sql_template.py" and "test_trigger_nodes_ sql_template.py" should be moved from tables->tests to tables->triggers->tests. As its related to triggers. - Following test cases are failing for PG 9.6 and PG 10.0 with error "*name 'long' is not defined". *I am using python 3.5 - TestRoleDependenciesSqlTemplate - TestTablesPropertiesSqlTemplate - TestTablesNodeSqlTemplate On Mon, May 8, 2017 at 9:44 PM, Sarah McAlear wrote: > Hi! > > This patch has the tests moved to the test directories of the parent. > > Thanks, > Joao & Sarah > > On Mon, May 8, 2017 at 7:28 AM, Dave Page wrote: > >> Hi >> >> On Mon, May 8, 2017 at 12:24 PM, Akshay Joshi < >> akshay.joshi@enterprisedb.com> wrote: >> >>> Hi All >>> >>> I have reviewed the code and have following questions: >>> >>> - Why do we have empty __init__.py file in sql and >>> templates/ folder. In this patch we have couple of occurrences one >>> of them is web/pgadmin/browser/server_groups/servers/databases/ >>> schemas/table/templates/trigger and web/pgadmin/browser/serve >>> r_groups/servers/databases/schemas/table/templates/trigger/sql >>> >>> That was already answered up-thread: We added the __init__.py files >> into templates to convert them into packages so that the tests inside of >> them can be found by the test runner. >> >>> >>> - Why do we have tests folder inside sql folder as we already have >>> tests folder in respective module. For example we have tests folder in >>> *web/pgadmin/browser/server_groups/servers/databases/schemas/table/column *then >>> why do we need it in >>> *web/pgadmin/browser/server_groups/servers/databases/schemas/table/templates/column/sql/tests* >>> >>> That does seem like a valid concern to me. >> >> >>> Apart from above code looks good to me. >>> >>> On Mon, May 8, 2017 at 2:20 PM, Akshay Joshi < >>> akshay.joshi@enterprisedb.com> wrote: >>> >>>> >>>> >>>> On Mon, May 8, 2017 at 1:46 PM, Dave Page wrote: >>>> >>>>> Akshay, as Ashesh is unavailable today, can you please review/commit >>>>> this ASAP? >>>>> >>>> >>>> Sure. >>>> >>>>> >>>>> Thanks. >>>>> >>>>> On Fri, May 5, 2017 at 1:18 PM, Dave Page wrote: >>>>> >>>>>> Can you get this polished off on Monday please Ashesh? >>>>>> >>>>>> On Thu, May 4, 2017 at 12:51 PM, Robert Eckhardt < >>>>>> reckhardt@pivotal.io> wrote: >>>>>> >>>>>>> All, >>>>>>> >>>>>>> This change in the xss testing is preventing our CI from going green >>>>>>> and is also preventing the Dependents and Dependencies tabs in Greenplum >>>>>>> from being useful. >>>>>>> >>>>>>> Can we either merge this or provide feedback as to what needs to >>>>>>> change so that it can be merged. >>>>>>> >>>>>>> Thank you >>>>>>> Rob >>>>>>> >>>>>>> On Tue, May 2, 2017 at 5:17 PM, Sarah McAlear >>>>>>> wrote: >>>>>>> >>>>>>>> Hi Hackers & Ashesh! >>>>>>>> >>>>>>>> Is there anything else we can do for this? >>>>>>>> >>>>>>>> Thanks! >>>>>>>> Matt & Sarah >>>>>>>> >>>>>>>> On Thu, Apr 27, 2017 at 10:37 AM, Joao Pedro De Almeida Pereira < >>>>>>>> jdealmeidapereira@pivotal.io> wrote: >>>>>>>> >>>>>>>>> Thanks for reviewing, Ashesh. >>>>>>>>> >>>>>>>>> We have updated the patch. The headers are all consistent and we >>>>>>>>> removed the __init__.py files in directories containing only .sql. >>>>>>>>> >>>>>>>>> Thanks! >>>>>>>>> Joao & Matt >>>>>>>>> >>>>>>>>> On Wed, Apr 26, 2017 at 11:22 AM, Joao Pedro De Almeida Pereira < >>>>>>>>> jdealmeidapereira@pivotal.io> wrote: >>>>>>>>> >>>>>>>>>> Hello Ashesh, >>>>>>>>>> >>>>>>>>>> Thanks for reviewing the patch. >>>>>>>>>> >>>>>>>>>> We added the __init__.py files into templates to convert them >>>>>>>>>> into packages so that the tests inside of them can be found by the test >>>>>>>>>> runner. >>>>>>>>>> >>>>>>>>>> Thanks! >>>>>>>>>> Joao & Sarah >>>>>>>>>> >>>>>>>>>> On Wed, Apr 26, 2017 at 1:26 AM, Ashesh Vashi < >>>>>>>>>> ashesh.vashi@enterprisedb.com> wrote: >>>>>>>>>> >>>>>>>>>>> On Mon, Apr 24, 2017 at 4:43 PM, Dave Page >>>>>>>>>>> wrote: >>>>>>>>>>> >>>>>>>>>>>> Ashesh, can you review/commit this please? >>>>>>>>>>>> >>>>>>>>>>>> On Fri, Apr 21, 2017 at 8:42 PM, Joao Pedro De Almeida Pereira >>>>>>>>>>>> wrote: >>>>>>>>>>>> >>>>>>>>>>>>> Hi Hackers, >>>>>>>>>>>>> >>>>>>>>>>>>> We found out that when you are connected to a GreenPlum >>>>>>>>>>>>> database and try to get Dependents and Dependencies of an object the >>>>>>>>>>>>> application was returning a SQL error. >>>>>>>>>>>>> >>>>>>>>>>>>> This patch splits the SQL query used to retrieve the >>>>>>>>>>>>> Dependents, Dependencies, and Roles SQL file into multiple versioned files. >>>>>>>>>>>>> Add Unit Tests for each file. >>>>>>>>>>>>> Also added __init__.py files to other test directories to run >>>>>>>>>>>>> the tests in them. >>>>>>>>>>>>> >>>>>>>>>>>> Hi Joao & Sarah, >>>>>>>>>>> >>>>>>>>>>> Why do we need to add __init__.py in the template directory? >>>>>>>>>>> I didn't understand the purpose of the adding __init__.py files >>>>>>>>>>> in the template directories. >>>>>>>>>>> >>>>>>>>>>> NOTE: The headers in those files are not consistent with the >>>>>>>>>>> other project files. >>>>>>>>>>> >>>>>>>>>>> -- >>>>>>>>>>> >>>>>>>>>>> Thanks & Regards, >>>>>>>>>>> >>>>>>>>>>> Ashesh Vashi >>>>>>>>>>> EnterpriseDB INDIA: Enterprise PostgreSQL Company >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> *http://www.linkedin.com/in/asheshvashi* >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>>> Add ORDER BY into Copy Selection Feature test to ensure the >>>>>>>>>>>>> results are retrieved always in the same order >>>>>>>>>>>>> Renamed the Scenario of the xss_checks_pgadmin_debugger_test >>>>>>>>>>>>> and skip it for versions less than 9.1 >>>>>>>>>>>>> >>>>>>>>>>>>> Thanks >>>>>>>>>>>>> >>>>>>>>>>>>> Joao & Sarah >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> -- >>>>>>>>>>>>> Sent via pgadmin-hackers mailing list ( >>>>>>>>>>>>> pgadmin-hackers@postgresql.org) >>>>>>>>>>>>> To make changes to your subscription: >>>>>>>>>>>>> http://www.postgresql.org/mailpref/pgadmin-hackers >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> -- >>>>>>>>>>>> Dave Page >>>>>>>>>>>> Blog: http://pgsnake.blogspot.com >>>>>>>>>>>> Twitter: @pgsnake >>>>>>>>>>>> >>>>>>>>>>>> EnterpriseDB UK: http://www.enterprisedb.com >>>>>>>>>>>> The Enterprise PostgreSQL Company >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> -- >>>>>>>>> Sent via pgadmin-hackers mailing list ( >>>>>>>>> pgadmin-hackers@postgresql.org) >>>>>>>>> To make changes to your subscription: >>>>>>>>> http://www.postgresql.org/mailpref/pgadmin-hackers >>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> Dave Page >>>>>> Blog: http://pgsnake.blogspot.com >>>>>> Twitter: @pgsnake >>>>>> >>>>>> EnterpriseDB UK: http://www.enterprisedb.com >>>>>> The Enterprise PostgreSQL Company >>>>>> >>>>> >>>>> >>>>> >>>>> -- >>>>> Dave Page >>>>> Blog: http://pgsnake.blogspot.com >>>>> Twitter: @pgsnake >>>>> >>>>> EnterpriseDB UK: http://www.enterprisedb.com >>>>> The Enterprise PostgreSQL Company >>>>> >>>> >>>> >>>> >>>> -- >>>> *Akshay Joshi* >>>> *Principal Software Engineer * >>>> >>>> >>>> >>>> *Phone: +91 20-3058-9517 <+91%2020%203058%209517>Mobile: +91 >>>> 976-788-8246 <+91%2097678%2088246>* >>>> >>> >>> >>> >>> -- >>> *Akshay Joshi* >>> *Principal Software Engineer * >>> >>> >>> >>> *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* *Principal Software Engineer * *Phone: +91 20-3058-9517Mobile: +91 976-788-8246* --94eb2c0de80a8c4e12054f1470b3 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
Hi Sarah

Below are my review comments o= n the latest patch:
  • Some test file names ended with "_sql_template.py" do we need to add = that string ?=C2=A0
  • Files "test_column_acl_sql_template.py" and "test_column_properties_sql_template.py" should be m= oved from tables->tests to tables->column->tests. As it's rela= ted to column.
  • Files "test_trigger_get_oid_sql_template.py" and &qu= ot;test_trigger_nodes_sql_template.py" should = be moved from tables->tests to tables->triggers->tests. As its rel= ated to triggers.
  • Following test cases are failing = for PG 9.6 and PG 10.0 with error "name 'long'= ; is not defined".=C2=A0I am using python 3.5
    • TestRoleDependenciesSqlTemplate
    • <= font face=3D"arial, helvetica, sans-serif">TestTablesPropertiesSqlTemplate
    • TestTablesNodeSqlTemplate

On Mon, May 8, 2017 at 9:44 PM, Sarah McAlear <= ;smcalear@pivotal.= io> wrote:
Hi!

This patch has the tests moved to the test director= ies of the parent.=C2=A0

Thanks,
Joao &a= mp; Sarah

On Mon, May 8, 2017 at 7:28 AM,= Dave Page <dpage@pgadmin.org> wrote:
Hi

On Mon, May 8, 2017 at 12:24 PM, Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:
Hi All
=
I have reviewed the code and have following questions:
=
  • Why do we have empty __init__.py file in sql and templates/<mo= dule> folder. In this patch we have couple of occurrences one of them is= web/pgadmin/browser/server_groups/servers/dat= abases/schemas/table/templates/trigger and= =C2=A0=C2=A0web/pgadmin/browser/server_gro= ups/servers/databases/schemas/table/templates/= trigger/sql
=
That was already answered up-thread:=C2=A0We added the __init__.py files into templates to convert them i= nto packages so that the tests inside of them can be found by the test runn= er.
  • Why do we have tests folder inside sql folder as we already have tests folder in respective module.= For example we have tests folder in=C2=A0web/pgadmin/browser/server_groups/servers/databases/schemas/table/column=C2=A0then why do we need it in=C2=A0web/pgadmin/browser/server_groups/servers/databases/= schemas/table/templates/column/
    =C2=A0
    Apart from above code looks good to me.

    On Mon, May 8, 2017 = at 2:20 PM, Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:
    <= div dir=3D"ltr">

    On Mon, May 8, 2017 at 1:46 PM, Dave Page <dpage@pgadmin.org&= gt; wrote:
    Akshay, as Ashesh is unavailable today, can you please review/= commit this ASAP?

    =C2=A0 =C2= =A0 Sure.=C2=A0

    Thanks.

    On Fri, May 5, 2017 = at 1:18 PM, Dave Page <dpage@pgadmin.org> wrote:
    Can you get this= polished off on Monday please Ashesh?

    On Thu,= May 4, 2017 at 12:51 PM, Robert Eckhardt <reckhardt@pivotal.io>= wrote:
    All,=C2=A0

    This change in the xss testing is p= reventing our CI from going green and is also preventing the=C2=A0Dependent= s and Dependencies tabs in Greenplum from being useful.=C2=A0
    Can we either merge this or provide feedback as to what needs t= o change so that it can be merged.=C2=A0

    Thank you=
    Rob

    On Tue, May 2, 2017 at 5:17 PM, Sarah McAlear <smcal= ear@pivotal.io> wrote:
    Hi Hackers & Ashesh!

    Is the= re anything else we can do for this?

    Thanks!
    Ma= tt & Sarah

    On Thu, Apr 27= , 2017 at 10:37 AM, Joao Pedro De Almeida Pereira <jdealmeidape= reira@pivotal.io> wrote:
    Thanks for reviewing, Ashesh.

    We have updated the patch. The headers are all consistent and we re= moved the __init__.py files in d= irectories containing only .sql.=

    Thanks!
    Joao & Matt

    On= Wed, Apr 26, 2017 at 11:22 AM, Joao Pedro De Almeida Pereira <= jdealmeidapereira@pivotal.io> wrote:
    Hello Ashesh,

    Th= anks for reviewing the patch.

    We added the __init_= _.py files into templates to convert them into packages so that the tests i= nside of them can be found by the test runner.

    Thanks!
    Joao & Sarah

    On Wed, Apr 26, 2017 at 1:26 AM, Ashesh Vashi = <ashe= sh.vashi@enterprisedb.com> wrote:
    On Mon, Apr 24, 2017 at 4:43 PM, Dave Pa= ge <dpage@pgadmin.org> wrote:
    Ashesh, can you review/commit this = please?

    On Fri, Apr 21, 2017 at 8:42 PM, J= oao Pedro De Almeida Pereira <jdealmeidapereira@pivotal.io&= gt; wrote:
    Hi Ha= ckers,

    We found out that when you are connected to a Gre= enPlum database and try to get Dependents and Dependencies of an object the= application was returning a SQL error.

    This patch= splits the SQL query used to retrieve the Dependents, Dependencies, and Ro= les SQL file into multiple versioned files.
    Add Unit Tests for ea= ch file.
    Also added __init__.py files to other test directories t= o run the tests in them.
    Hi Joao & Sarah,

    Why do we need to= add __init__.py in the template directory?
    I didn't understa= nd the purpose of the adding __init__.py files in the template directories.=

    NOTE: The headers in those files are not consiste= nt with the other project files.

    --=

    Thanks & Regards,

    Ashesh Vashi
    EnterpriseDB INDIA:=C2=A0Enterprise PostgreSQL C= ompany


    http://www.linkedin.= com/in/asheshvashi

    =
    Add = ORDER BY into Copy Selection Feature test to ensure the results are retriev= ed always in the same order
    Renamed the Scenario of the=C2=A0xss_= checks_pgadmin_debugger_test and skip it for versions less than 9.1

    Thanks

    Joao & Sarah


    --
    Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org) To make changes to your subscription:
    http://www.postgresql.org/mailpref/pgadmin-ha= ckers




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

    EnterpriseDB = UK: http://www.en= terprisedb.com
    The Enterprise PostgreSQL Company





    --
    Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org) To make changes to your subscription:
    http://www.postgresql.org/mailpref/pgadmin-ha= ckers






--
=
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake
<= br>EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
=



--
=
Dave P= age
Blog: http= ://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterpri= sedb.com
The Enterprise PostgreSQL Company



--
Akshay Joshi
Principal Software Engi= neer=C2=A0

=




--
Akshay Joshi
Principal Software Engineer=C2=A0

<= font color=3D"#3333FF">
=



--
Dave Page
Blog: = http://pgsnake.bl= ogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com<= br>The Enterprise PostgreSQL Company




--
=
Akshay = Joshi
Principal Software Engineer=C2=A0
=
=
Phone: +91 20-3058-9517
Mobil= e: +91 976-788-8246
--94eb2c0de80a8c4e12054f1470b3--