public inbox for [email protected]
help / color / mirror / Atom feedFrom: Dave Page <[email protected]>
To: Shruti B Iyer <[email protected]>
Cc: Surinder Kumar <[email protected]>
Cc: Joao Pedro De Almeida Pereira <[email protected]>
Cc: Murtuza Zabuawala <[email protected]>
Cc: pgadmin-hackers <[email protected]>
Subject: Re: [pgAdmin4] [PATCH] History Tab rewrite in React
Date: Mon, 12 Jun 2017 15:44:32 +0100
Message-ID: <CA+OCxowHBoAMBE9Np--JupNneYGe__=4Zwm2JMSPGD2U6=S2Lg@mail.gmail.com> (raw)
In-Reply-To: <CACrUwhJ_YXJuZGbBrMKrCTq5qQ3JJ8zrpTu_fauOC1KXcmADNg@mail.gmail.com>
References: <CAFS4TJYDRi6fs+wrhkj89gFe3xaFOkoApCK+AU2O4DfU2XfXog@mail.gmail.com>
<CAKKotZQSTtJVkBKMeFi9vTrPugBEbEE38dhbbn-hr+hpd0Qzbw@mail.gmail.com>
<CA+OCxowChMtZzaSwrG5Sganh4LiMQcgL7BpoqhTxZga30DqDXw@mail.gmail.com>
<CAKKotZRmvbScMdUtjMgUJdRe-EXaqDoDbYBxrMZrkbVDVR_iUg@mail.gmail.com>
<CA+OCxow3e=q9w_e=Kkw1ebZJ82EM8RyFmiTaXihdqhX6HbzZhw@mail.gmail.com>
<CAFS4TJapVubRP_BK01GwYjfN7zpaTbCMX+CROC4mV93aL08DOg@mail.gmail.com>
<CA+OCxozPDpP_5zdLjOSQhiGHgzE-QDsFv8-WyC8-XTxf5+uSJg@mail.gmail.com>
<CAE+jjakzXbzvtai0Z2q0bXfecMhDzZwtn+p8mQwV7kfRGPON_Q@mail.gmail.com>
<CA+OCxoxkM--KjFWt7FkJm+mrL5sA9jdZ59YN7=n=2ZMrkH8iSg@mail.gmail.com>
<CAFS4TJaZA0U_BtiUBdx1jT6Z5bQzMQbjhwrjimDLgfkTb=gWJg@mail.gmail.com>
<CAE+jjakrsf7f41LyCy6BWvjrsH9FyKK98koO0MgUkR0qmbk-uw@mail.gmail.com>
<CA+OCxoxH+5gAjw17rWQb-jqNBeQGgOs4Q=GZ7KEwdmGHOQrsBw@mail.gmail.com>
<CAHowoHZJtKn8F9XuiK2yFT=_OcgG7Ctx83iiFab17PwexajfDQ@mail.gmail.com>
<CA+OCxozz7pbMQi_h-USovgwA2XkVzpeAUuK-zcro4La-_KjgtA@mail.gmail.com>
<CAFS4TJYRuaRkHguU_BkMAa4vv7Xy_aa37B4M4HSbJ0h2QpmcFA@mail.gmail.com>
<CA+OCxozdD8_4uyu9f+CJTjtH0x8bcKoeNMtRDcSHXSKStAPwbQ@mail.gmail.com>
<CAE+jjakmryMDovhh+OMsw=FSTj5W62CuhbdQS4WpjcLSs9jO4g@mail.gmail.com>
<CAM5-9D-wS-9qwMwvpUCwc8s=KumY+k8pkM7tOt1eysuFvq3n-w@mail.gmail.com>
<CAFS4TJZtCDqoenHREPC7PEopwYVyPGUh0_9WtxQKNV2w_WLAjw@mail.gmail.com>
<CAM5-9D9cm88KaiNjAwyNQG7+0FNxcAEJ6xaWTS9+pJ-6uLLTWQ@mail.gmail.com>
<CAGRPzo-byoSsREPZxJ-bEbse2c2emFF5UMD=SGCJHWduk8kiCA@mail.gmail.com>
<CA+OCxox3vW4dpK5w9LuL=axXpp_xR5V14oLbGYoe4N9zSOP3dA@mail.gmail.com>
<CAHowoHa-xGTQgckQC431jusaHD2Eg3S0KCrn24yt9uiS0ow7bA@mail.gmail.com>
<CA+OCxoy3gVbGiPL3hi250Ya0qMOURYOERH5XxYrnrt2Co2v-xw@mail.gmail.com>
<CAHowoHZFkDQrMO0-e2gx3ffo3kB56Qwb3jT2S1M7Sc8Bpo0Usw@mail.gmail.com>
<CA+OCxoxtxBO-mWfUokBu1WG8VH4E6MrHj16mrKT=L0svYvKfGg@mail.gmail.com>
<CACrUwhJ_YXJuZGbBrMKrCTq5qQ3JJ8zrpTu_fauOC1KXcmADNg@mail.gmail.com>
List-Unsubscribe: <mailto:[email protected]?body=unsub%20pgadmin-hackers>
Hi Shruti
On Mon, Jun 12, 2017 at 3:24 PM, Shruti B Iyer <[email protected]> wrote:
>
> 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.
Hmm, I wonder if I missed them because I applied the patch in a sub directory.
> 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?
Yes, it looks like it. For some reason it's not failing on the Jenkins
server though. I'll ask Khushboo to fix it.
> We will recreate the patches and send them ASAP.
Thanks!
> Thanks
> Shruti & Joao
>
> On Mon, Jun 12, 2017 at 6:59 AM Dave Page <[email protected]> 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
>> <[email protected]> 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 <[email protected]> wrote:
>> >>
>> >> Hi George
>> >>
>> >> On Wed, Jun 7, 2017 at 10:21 PM, George Gelashvili
>> >> <[email protected]> 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 ([email protected])
>> 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 ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers
view thread (45+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: [pgAdmin4] [PATCH] History Tab rewrite in React
In-Reply-To: <CA+OCxowHBoAMBE9Np--JupNneYGe__=4Zwm2JMSPGD2U6=S2Lg@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox