Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtp (Exim 4.84_2) (envelope-from ) id 1dKQHM-0002bO-GE for pgadmin-hackers@arkaria.postgresql.org; Mon, 12 Jun 2017 14:25:16 +0000 Received: from localhost ([127.0.0.1] helo=postgresql.org) by malur.postgresql.org with smtp (Exim 4.84_2) (envelope-from ) id 1dKQHM-0002jg-2s for pgadmin-hackers@arkaria.postgresql.org; Mon, 12 Jun 2017 14:25:16 +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 1dKQHL-0002jL-43 for pgadmin-hackers@postgresql.org; Mon, 12 Jun 2017 14:25:15 +0000 Received: from mail-io0-x22e.google.com ([2607:f8b0:4001:c06::22e]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.84_2) (envelope-from ) id 1dKQHE-0001Tq-UR for pgadmin-hackers@postgresql.org; Mon, 12 Jun 2017 14:25:13 +0000 Received: by mail-io0-x22e.google.com with SMTP id t87so36122810ioe.0 for ; Mon, 12 Jun 2017 07:25:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pivotal-io.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=ejH8HqjqdWEl3wQ4O7EswvdRlAKFEmv8+Crk6CgH7bQ=; b=B5ke0IxEvz+44wGL84BTyA+1WG+2lB/VRMxYDi6j6Dr2xrWZieHMrW8CQmhIopO2i8 qj/AJK3DU1xU7yTeEf2CZ8rkmYerLvwTWqdJWDRzqw1mFDdYvJ1eya9ghhJQPaT6ZCU5 F8/Inep8DE1CKhAN4B2B3E/Z9KS9d49PvhPC1p44nJ+IfAkuu2K29LdO8OJ15vAEGmyv PJ+5L0tdIIIXWEJVCIkHb5vTRd27bJF2kGPBJu3S/9FRvHcX1l3W6h6Lfq/RNuor44G9 ekRvngUCfNBhFz0GxhWpMT7uamRCrKcpMXV5ogpqFb+j0Rz/RQNK4TGjcNvjbtb1B2q9 B/aw== 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=ejH8HqjqdWEl3wQ4O7EswvdRlAKFEmv8+Crk6CgH7bQ=; b=ElkfcMsJ0JQodOk67hz+oZxS9Ok0mCFaRR5gnvtO9Cn5Cq8hTkLRflE0a9yOnPsZN0 434aeGN9TwrETIakUyKdlvbDVoLMUAvqc75IxhVVgPJ1iULj/ZaRD2QkRq6skkMRSOxd tbYx+MSfEbLlZx3eVXvzM3epiuDuHtyENrwrPi0wLOUjxByE88rYVsODdnkiOrDpeVko sCOjB/DHWq/VqTpQEcp32ihMdvgOaNl//szTOqCyVU5m4YALO3CHnIYOjdgDKXgDyVcK GCDm5Nk//cyfMoOt8HTWlYyxqbRjegOflYgA4JJkRYfEMigtFlC42NKS8dk7izOjW6FZ 6lyA== X-Gm-Message-State: AODbwcChGCbezLvlE7aS0P7eFCNZCTUZl32PKvjfIWvUu+R0bEWjiUtd vDaaKeJXo82z0W+6vL9qnhAKdUHthwP7 X-Received: by 10.107.201.213 with SMTP id z204mr32708946iof.160.1497277505688; Mon, 12 Jun 2017 07:25:05 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Shruti B Iyer Date: Mon, 12 Jun 2017 14:24:54 +0000 Message-ID: Subject: Re: [pgAdmin4] [PATCH] History Tab rewrite in React To: Dave Page Cc: Surinder Kumar , Joao Pedro De Almeida Pereira , Murtuza Zabuawala , pgadmin-hackers Content-Type: multipart/alternative; boundary="94eb2c0b8828db712d0551c41563" 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 --94eb2c0b8828db712d0551c41563 Content-Type: text/plain; charset="UTF-8" Hello Dave, Thanks for making those fixes and sharing them with us. We tried applying the patch and it looks like there are some missing file changes from your patch that were present in ours, like the Make.bat file changes. But we will add them when we send you the new patches. While trying to generate the new patches we realized some tests are failing in master branch due to an internal server error: 2017-06-12 10:04:11,938: INFO werkzeug: 127.0.0.1 - - [12/Jun/2017 10:04:11] "GET /browser/table/sql/1/1/12669/2200/81920 HTTP/1.1" 500 - Traceback (most recent call last): File "/Users/pivotal/.pyenv/versions/2.7.10/envs/pgadmin/lib/python2.7/site-packages/flask/app.py", line 2000, in __call__ return self.wsgi_app(environ, start_response) File "/Users/pivotal/.pyenv/versions/2.7.10/envs/pgadmin/lib/python2.7/site-packages/flask/app.py", line 1991, in wsgi_app response = self.make_response(self.handle_exception(e)) File "/Users/pivotal/.pyenv/versions/2.7.10/envs/pgadmin/lib/python2.7/site-packages/flask/app.py", line 1567, in handle_exception reraise(exc_type, exc_value, tb) File "/Users/pivotal/.pyenv/versions/2.7.10/envs/pgadmin/lib/python2.7/site-packages/flask/app.py", line 1988, in wsgi_app response = self.full_dispatch_request() File "/Users/pivotal/.pyenv/versions/2.7.10/envs/pgadmin/lib/python2.7/site-packages/flask/app.py", line 1641, in full_dispatch_request rv = self.handle_user_exception(e) File "/Users/pivotal/.pyenv/versions/2.7.10/envs/pgadmin/lib/python2.7/site-packages/flask/app.py", line 1544, in handle_user_exception reraise(exc_type, exc_value, tb) File "/Users/pivotal/.pyenv/versions/2.7.10/envs/pgadmin/lib/python2.7/site-packages/flask/app.py", line 1639, in full_dispatch_request rv = self.dispatch_request() File "/Users/pivotal/.pyenv/versions/2.7.10/envs/pgadmin/lib/python2.7/site-packages/flask/app.py", line 1625, in dispatch_request return self.view_functions[rule.endpoint](**req.view_args) File "/Users/pivotal/.pyenv/versions/2.7.10/envs/pgadmin/lib/python2.7/site-packages/flask/views.py", line 84, in view return self.dispatch_request(*args, **kwargs) File "/Users/pivotal/workspace/pgadmin4/web/pgadmin/browser/utils.py", line 235, in dispatch_request return method(*args, **kwargs) File "/Users/pivotal/workspace/pgadmin4/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/__init__.py", line 315, in wrap return f(*args, **kwargs) File "/Users/pivotal/workspace/pgadmin4/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/__init__.py", line 2555, in sql data = self._formatter(did, scid, tid, data) File "/Users/pivotal/workspace/pgadmin4/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/__init__.py", line 1081, in _formatter data = self._columns_formatter(tid, data) File "/Users/pivotal/workspace/pgadmin4/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/__init__.py", line 663, in _columns_formatter column['attlen'] = matchObj.group(1) AttributeError: 'NoneType' object has no attribute 'group' Was this introduced in a previous patch? We will recreate the patches and send them ASAP. Thanks Shruti & Joao On Mon, Jun 12, 2017 at 6:59 AM Dave Page wrote: > Hi > > OK, so Ashesh and I spend much of the morning on this. > > Patch 01 - Applied. > Patch 02: > > - karma.conf.js wouldn't patch; I've manually handled that. > - test-main.js wouldn't patch. The diff looked like it was trying to > empty it; I have removed it instead. > - The imports in pgAdmin4.py need to be made after the app root is > added to the path. > - The JS bundler should be in pgadmin/utils, not pgadmin/tools (which > is intended for plugin modules) > - The tests were failing following some changes Ashesh pushed earlier > to add a client-side url_for function. > - pgAdmin4.py was attempting to run the bundler on every startup. I've > wrapped those called in "if config.DEBUG:" conditionals, as typically > an installation for an end-user will be in a read-only directory. > > We've fixed all of that in the attached patch. I'm not sure why it's > so much bigger than yours. > > The following issues are outstanding; please take a look at them: > > - There is no update to the Windows installer generation code (needed > in 2 places unfortunately; Make.bat and Make-MinGW.bat). > > - The updates to the other packages call "yarn run webpacker" which is > an undefined target. > > I haven't looked at patch 03 yet, but Ashesh did tell me it won't > apply for him. Patch 4 is also untested at this stage. > > If the issues above can be fixed, we can get patch 2 applied then move > on from there. > > I'll hold off on Harshal's patch for the Query Tool's load on demand > to give you a chance to get this done. > > Thanks. > > On Sat, Jun 10, 2017 at 2:52 AM, George Gelashvili > wrote: > > Hi Dave, > > > > Our patch touches code also changed by the patches that were recently > > committed. > > That's likely what's causing this issue. We've rebased on top of the new > > state of master. > > > > We had initially kept the yarn.lock .gitignored, but ran into an issue > > rather early on where an upgraded dependency introduced a regression. > > Checking-in the yarn.lock provides the "know your dependency version" > > benefit of vendorizing code without vendorization's drawback of having to > > manually manage your dependencies. > > > > It is safe to delete a yarn.lock before applying a patch, as you are > > authoring master. It provides a history of the versions of each > dependency > > that were working at the point in time of the commit. By contrast, > > package.json provides approximate versions and leaves room for > > fixes/improvements by the dependency authors to be pulled in as they > become > > available. > > > > To run linter and tests: > > > > The tasks that Grunt used to manage are now defined as a set of scripts > in > > the package.json > > After applying the patches--which may require deleting yarn.lock for the > > first patch--you should cd web && yarn install > > > > Then yarn test will run the linter, jasmine specs, and python tests > > including feature tests, in that order, exiting early if there are > > failures/errors. > > At the moment, the CheckForViewData test is failing on master as well as > in > > each of these patches; that should be resolved as RM2477. > > > > Thanks! > > George and Matt > > > > > > On Thu, Jun 8, 2017 at 9:15 AM, Dave Page wrote: > >> > >> Hi George > >> > >> On Wed, Jun 7, 2017 at 10:21 PM, George Gelashvili > >> wrote: > >> > Hi Dave, > >> > > >> > I split the linting out into an intermediate commit, and rebased on > top > >> > of > >> > master. > >> > >> Unfortunately, it still doesn't apply: > >> > >> error: patch failed: web/regression/javascript/test-main.js:1 > >> error: removal patch leaves file contents > >> error: web/regression/javascript/test-main.js: patch does not apply > >> Checking patch web/regression/requirements.txt... > >> Checking patch web/webpack.config.js... > >> Checking patch web/webpack.test.config.js... > >> Checking patch web/yarn.lock... > >> error: web/yarn.lock: already exists in working directory > >> Applied patch .gitignore cleanly. > >> Applied patch Make.bat cleanly. > >> Applied patch README cleanly. > >> Applied patch pkg/mac/build.sh cleanly. > >> Applied patch pkg/pip/build.sh cleanly. > >> Applied patch pkg/src/build.sh cleanly. > >> Applied patch web/.eslintrc.js cleanly. > >> Applied patch web/karma.conf.js cleanly. > >> Applied patch web/package.json cleanly. > >> Applied patch web/pgAdmin4.py cleanly. > >> Applied patch web/pgadmin/static/jsx/components.jsx cleanly. > >> Applied patch web/pgadmin/tools/javascript/__init__.py cleanly. > >> Applied patch web/pgadmin/tools/javascript/javascript_bundler.py > cleanly. > >> Applied patch web/pgadmin/tools/javascript/tests/__init__.py cleanly. > >> Applied patch > >> web/pgadmin/tools/javascript/tests/test_javascript_bundler.py > >> cleanly. > >> Applied patch web/regression/README cleanly. > >> Applied patch > >> web/regression/javascript/jasmine_capture_warnings_beforeall.js > >> cleanly. > >> Applied patch web/regression/requirements.txt cleanly. > >> Applied patch web/webpack.config.js cleanly. > >> Applied patch web/webpack.test.config.js cleanly. > >> > >> The second (lint update) patch is even worse, with significant number > >> change that just don't want to apply. > >> > >> Clearly yarn.lock needs to be removed from there repo. > >> > >> Once I can apply a version of this, how should I be running the linter > >> and the unit tests? > >> > >> -- > >> 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 > > -- > Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgadmin-hackers > --94eb2c0b8828db712d0551c41563 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable

Hello Dave,

Thanks for making those= fixes and sharing them with us. We tried applying the patch and it looks l= ike there are some missing file changes from your patch that were present i= n ours, like the Make.bat file changes. But we will add them when we send y= ou the new patches.

While trying to generate the n= ew patches we realized some tests are failing in m= aster branch due to an internal server error:

2017-06-12 10:04:11,938: INFO werkzeug: 127.0.0.1 - - [12/Jun= /2017 10:04:11] "GET /browser/table/sql/1/1/12669/2200/81920 HTTP/1.1&= quot; 500 -
Traceback (most recen= t call last):
=C2=A0 File "/= Users/pivotal/.pyenv/versions/2.7.10/envs/pgadmin/lib/python2.7/site-packag= es/flask/app.py", line 2000, in __call__
=C2=A0 =C2=A0 return self.wsgi_app(environ, start_response)<= /font>
=C2=A0 File "/Users/pivotal/= .pyenv/versions/2.7.10/envs/pgadmin/lib/python2.7/site-packages/flask/app.p= y", line 1991, in wsgi_app
= =C2=A0 =C2=A0 response =3D self.make_response(self.handle_exception(e))
=C2=A0 File "/Users/pivotal/.py= env/versions/2.7.10/envs/pgadmin/lib/python2.7/site-packages/flask/app.py&q= uot;, line 1567, in handle_exception
=C2=A0 =C2=A0 reraise(exc_type, exc_value, tb)
=C2=A0 File "/Users/pivotal/.pyenv/versions/2.7.10/e= nvs/pgadmin/lib/python2.7/site-packages/flask/app.py", line 1988, in w= sgi_app
=C2=A0 =C2=A0 response = =3D self.full_dispatch_request()
= =C2=A0 File "/Users/pivotal/.pyenv/versions/2.7.10/envs/pgadmin/lib/py= thon2.7/site-packages/flask/app.py", line 1641, in full_dispatch_reque= st
=C2=A0 =C2=A0 rv =3D self.hand= le_user_exception(e)
=C2=A0 File = "/Users/pivotal/.pyenv/versions/2.7.10/envs/pgadmin/lib/python2.7/site= -packages/flask/app.py", line 1544, in handle_user_exception
=C2=A0 =C2=A0 reraise(exc_type, exc_value,= tb)
=C2=A0 File "/Users/piv= otal/.pyenv/versions/2.7.10/envs/pgadmin/lib/python2.7/site-packages/flask/= app.py", line 1639, in full_dispatch_request
=C2=A0 =C2=A0 rv =3D self.dispatch_request()
<= div>=C2=A0 File "/Users/pivotal/.pyenv/versio= ns/2.7.10/envs/pgadmin/lib/python2.7/site-packages/flask/app.py", line= 1625, in dispatch_request
=C2=A0= =C2=A0 return self.view_functions[rule.endpoint](**req.view_args)
=C2=A0 File "/Users/pivotal/.pyenv/v= ersions/2.7.10/envs/pgadmin/lib/python2.7/site-packages/flask/views.py"= ;, line 84, in view
=C2=A0 =C2=A0= return self.dispatch_request(*args, **kwargs)
=C2=A0 File "/Users/pivotal/workspace/pgadmin4/web/pgad= min/browser/utils.py", line 235, in dispatch_request
= =C2=A0 =C2=A0 return method(*args, **kwargs)
=C2=A0 File "/Users/pivotal/works= pace/pgadmin4/web/pgadmin/browser/server_groups/servers/databases/schemas/t= ables/__init__.py", line 315, in wrap
=C2=A0 =C2=A0 return f(*args, **kwargs)
=C2=A0 File "/Users/pivotal/workspace/pgadmin4/web/pg= admin/browser/server_groups/servers/databases/schemas/tables/__init__.py&qu= ot;, line 2555, in sql
=C2=A0 =C2= =A0 data =3D self._formatter(did, scid, tid, data)
=C2=A0 File "/Users/pivotal/workspace/pgadmin4/web/p= gadmin/browser/server_groups/servers/databases/schemas/tables/__init__.py&q= uot;, line 1081, in _formatter
= =C2=A0 =C2=A0 data =3D self._columns_formatter(tid, data)
= =C2=A0 File "/Users/pivotal/workspace/pgadmin= 4/web/pgadmin/browser/server_groups/servers/databases/schemas/tables/__init= __.py", line 663, in _columns_formatter
=C2=A0 =C2=A0 column['attlen'] =3D matchObj.group(1)
AttributeError: 'NoneType' = object has no attribute 'group'

Was this introduced in a previous patch?

We wil= l recreate the patches and send them ASAP.

Thanks<= /div>
Shruti & Joao

On Mon, Jun 12, 2017 at 6:59 AM Dave Page <dpage@pgadmin.org> wrote:
Hi

OK, so Ashesh and I spend much of the morning on this.

Patch 01 - Applied.
Patch 02:

- karma.conf.js wouldn't patch; I've manually handled that.
- test-main.js wouldn't patch. The diff looked like it was trying to empty it; I have removed it instead.
- The imports in pgAdmin4.py need to be made after the app root is
added to the path.
- The JS bundler should be in pgadmin/utils, not pgadmin/tools (which
is intended for plugin modules)
- The tests were failing following some changes Ashesh pushed earlier
to add a client-side url_for function.
- pgAdmin4.py was attempting to run the bundler on every startup. I've<= br> wrapped those called in "if config.DEBUG:" conditionals, as typic= ally
an installation for an end-user will be in a read-only directory.

We've fixed all of that in the attached patch. I'm not sure why it&= #39;s
so much bigger than yours.

The following issues are outstanding; please take a look at them:

- There is no update to the Windows installer generation code (needed
in 2 places unfortunately; Make.bat and Make-MinGW.bat).

- The updates to the other packages call "yarn run webpacker" whi= ch is
an undefined target.

I haven't looked at patch 03 yet, but Ashesh did tell me it won't apply for him. Patch 4 is also untested at this stage.

If the issues above can be fixed, we can get patch 2 applied then move
on from there.

I'll hold off on Harshal's patch for the Query Tool's load on d= emand
to give you a chance to get this done.

Thanks.

On Sat, Jun 10, 2017 at 2:52 AM, George Gelashvili
<ggelashvili@pivotal.io> wrote:
> Hi Dave,
>
> Our patch touches code also changed by the patches that were recently<= br> > committed.
> That's likely what's causing this issue. We've rebased on = top of the new
> state of master.
>
> We had initially kept the yarn.lock .gitignored, but ran into an issue=
> rather early on where an upgraded dependency introduced a regression.<= br> > Checking-in the yarn.lock provides the "know your dependency vers= ion"
> benefit of vendorizing code without vendorization's drawback of ha= ving to
> manually manage your dependencies.
>
> It is safe to delete a yarn.lock before applying a patch, as you are > authoring master. It provides a history of the versions of each depend= ency
> that were working at the point in time of the commit. By contrast,
> package.json provides approximate versions and leaves room for
> fixes/improvements by the dependency authors to be pulled in as they b= ecome
> available.
>
> To run linter and tests:
>
> The tasks that Grunt used to manage are now defined as a set of script= s in
> the package.json
> After applying the patches--which may require deleting yarn.lock for t= he
> first patch--you should cd web && yarn install
>
> Then yarn test will run the linter, jasmine specs, and python tests > including feature tests, in that order, exiting early if there are
> failures/errors.
> At the moment, the CheckForViewData test is failing on master as well = as in
> each of these patches; that should be resolved as RM2477.
>
> Thanks!
> George and Matt
>
>
> On Thu, Jun 8, 2017 at 9:15 AM, Dave Page <dpage@pgadmin.org= > wrote:
>>
>> Hi George
>>
>> On Wed, Jun 7, 2017 at 10:21 PM, George Gelashvili
>> <ggelashvili@pivotal.io> wrote:
>> > Hi Dave,
>> >
>> > I split the linting out into an intermediate commit, and reba= sed on top
>> > of
>> > master.
>>
>> Unfortunately, it still doesn't apply:
>>
>> error: patch failed: web/regression/javascript/test-main.js:1
>> error: removal patch leaves file contents
>> error: web/regression/javascript/test-main.js: patch does not appl= y
>> Checking patch web/regression/requirements.txt...
>> Checking patch web/webpack.config.js...
>> Checking patch web/webpack.test.config.js...
>> Checking patch web/yarn.lock...
>> error: web/yarn.lock: already exists in working directory
>> Applied patch .gitignore cleanly.
>> Applied patch Make.bat cleanly.
>> Applied patch README cleanly.
>> Applied patch pkg/mac/build.sh cleanly.
>> Applied patch pkg/pip/build.sh cleanly.
>> Applied patch pkg/src/build.sh cleanly.
>> Applied patch web/.eslintrc.js cleanly.
>> Applied patch web/karma.conf.js cleanly.
>> Applied patch web/package.json cleanly.
>> Applied patch web/pgAdmin4.py cleanly.
>> Applied patch web/pgadmin/static/jsx/components.jsx cleanly.
>> Applied patch web/pgadmin/tools/javascript/__init__.py cleanly. >> Applied patch web/pgadmin/tools/javascript/javascript_bundler.py c= leanly.
>> Applied patch web/pgadmin/tools/javascript/tests/__init__.py clean= ly.
>> Applied patch
>> web/pgadmin/tools/javascript/tests/test_javascript_bundler.py
>> cleanly.
>> Applied patch web/regression/README cleanly.
>> Applied patch
>> web/regression/javascript/jasmine_capture_warnings_beforeall.js >> cleanly.
>> Applied patch web/regression/requirements.txt cleanly.
>> Applied patch web/webpack.config.js cleanly.
>> Applied patch web/webpack.test.config.js cleanly.
>>
>> The second (lint update) patch is even worse, with significant num= ber
>> change that just don't want to apply.
>>
>> Clearly yarn.lock needs to be removed from there repo.
>>
>> Once I can apply a version of this, how should I be running the li= nter
>> and the unit tests?
>>
>> --
>> 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

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