GNIP-14
20 views
Skip to first unread message
Ian Schneider
Oct 17, 2012, 1:59:56 PM10/17/12
to GeoNode Development
Hi everyone,
After much work, there is a potential new search implementation for
your review and comments:
https://github.com/GeoNode/geonode/wiki/GNIP-14---New-Search-Backend/
For the impatient, the PR is here:
https://github.com/GeoNode/geonode/pull/425 (also noted in the GNIP).
Let's keep high-level discussion here and code-specifics on the PR.
Cheers,
--
Ian Schneider
OpenGeo - http://opengeo.org
Enterprise support for open source geospatial.
After much work, there is a potential new search implementation for
your review and comments:
https://github.com/GeoNode/geonode/wiki/GNIP-14---New-Search-Backend/
For the impatient, the PR is here:
https://github.com/GeoNode/geonode/pull/425 (also noted in the GNIP).
Let's keep high-level discussion here and code-specifics on the PR.
Cheers,
--
Ian Schneider
OpenGeo - http://opengeo.org
Enterprise support for open source geospatial.
Jeffrey Johnson
Oct 17, 2012, 2:07:41 PM10/17/12
to Ian Schneider, GeoNode Development
After already having reviewed this extensively, Im +1 on pulling it as
is. I will spend some time to add a haystack backend eventually, but
would rather just pull this in now rather than wait for that.
Great work Ian!
Jeff
is. I will spend some time to add a haystack backend eventually, but
would rather just pull this in now rather than wait for that.
Great work Ian!
Jeff
Matthew Hanson
Oct 17, 2012, 2:16:37 PM10/17/12
to Jeffrey Johnson, Ian Schneider, GeoNode Development
This looks great to me, nice job!
Travis build is still failing due to a timeout from the build taking too long. I assume it passed the tests and integration_tests? If so I'll throw in my +1 to pull it right away.
matt
Jeffrey Johnson
Oct 17, 2012, 2:20:06 PM10/17/12
to Matthew Hanson, Ian Schneider, GeoNode Development
I've got no idea why travis is timing out, but maybe something we can
try to take up with them?? I see no way to increase the timeout, and
not sure why it started timing out now when it didnt before.
I suppose this PR is big enough that we should leave it open for a
week to get any -1s, but up to everyone else as a group here.
try to take up with them?? I see no way to increase the timeout, and
not sure why it started timing out now when it didnt before.
I suppose this PR is big enough that we should leave it open for a
week to get any -1s, but up to everyone else as a group here.
Rob Marianski
Oct 17, 2012, 2:37:15 PM10/17/12
to Jeffrey Johnson, Matthew Hanson, Ian Schneider, GeoNode Development
Great work Ian. +1
Ian Schneider
Oct 17, 2012, 2:38:11 PM10/17/12
to Matthew Hanson, Jeffrey Johnson, GeoNode Development
On Wed, Oct 17, 2012 at 12:16 PM, Matthew Hanson
<mha...@appliedgeosolutions.com> wrote:
> This looks great to me, nice job!
>
> Travis build is still failing due to a timeout from the build taking too
> long. I assume it passed the tests and integration_tests? If so I'll
> throw in my +1 to pull it right away.
Thanks for the feedback. Unfortunately, the dev unit tests currently
<mha...@appliedgeosolutions.com> wrote:
> This looks great to me, nice job!
>
> Travis build is still failing due to a timeout from the build taking too
> long. I assume it passed the tests and integration_tests? If so I'll
> throw in my +1 to pull it right away.
fail due to unrelated changes.
Regarding the integration tests, thanks for the reminder :) Will follow up.
Ian Schneider
Oct 17, 2012, 4:20:06 PM10/17/12
to Rob Marianski, GeoNode Development, Marc Pfister
On Wed, Oct 17, 2012 at 12:37 PM, Rob Marianski <rmari...@opengeo.org> wrote:
> Great work Ian. +1
Thanks Rob.
As you know, I should have thanked you and Marc Pfister first for your
efforts in writing the unit tests for this :)
> Great work Ian. +1
Thanks Rob.
As you know, I should have thanked you and Marc Pfister first for your
efforts in writing the unit tests for this :)
Ian Schneider
Oct 17, 2012, 4:34:22 PM10/17/12
to GeoNode Development
integration tests is actually dealt with in the unit tests (and done
so more thoroughly). Except one item: the new search API does not
actually return permissions information in the results. Would the new
UI even do anything with the permissions? FWIW, security trimming
(removing results that aren't viewable by the searching user) is done
optimally in this implementation.
Unless anyone objects, it seems the integration tests can be trimmed
once this is pulled (or as part of it).
Thoughts?
Ariel Nunez
Oct 17, 2012, 4:36:37 PM10/17/12
to Ian Schneider, GeoNode Development
Since the search is backed by the database I agree unit tests are better and the integration ones are not needed.
-a
PS: I will take care of the failing tests in dev this afternoon when I get back home. My apologies for breaking them yesterday.
-a
PS: I will take care of the failing tests in dev this afternoon when I get back home. My apologies for breaking them yesterday.
Tom Kralidis
Oct 17, 2012, 7:57:13 PM10/17/12
to Ian Schneider, GeoNode Development
> Date: Wed, 17 Oct 2012 11:59:56 -0600
> Subject: GNIP-14
> From: ischn...@opengeo.org
> To: geono...@opengeo.org
> Subject: GNIP-14
> From: ischn...@opengeo.org
> To: geono...@opengeo.org
>
> Hi everyone,
>
> After much work, there is a potential new search implementation for
> your review and comments:
>
> https://github.com/GeoNode/geonode/wiki/GNIP-14---New-Search-Backend/
>
> For the impatient, the PR is here:
> https://github.com/GeoNode/geonode/pull/425 (also noted in the GNIP).
>
> Let's keep high-level discussion here and code-specifics on the PR.
>
+1, glad to see this value added search in GeoNode! I will take a look once this is in dev to figure out if it's viable (or makes sense) to bind the CSW search to this GNIP's underlying logic (my initial thoughts are no, given the scope of this search is greater than CSW's per se).
Ariel Nunez
Oct 17, 2012, 9:46:12 PM10/17/12
to Tom Kralidis, Ian Schneider, GeoNode Development
I read over the pull request's code and liked what I saw.
However, since this is the only search implementation. I would vote for putting it in geonode.silage or just calling it geonode.search.
Since search is really a sort of pluggable app and not a 'module' the way cws and perhaps ows in the future may be. I think we can get away with having: geonode.search or geonode.silage have the same urls and api as a potential geonode_haystack that may or may not live under our source tree.
I will now try the code and report the finding on the pull request.
-a
However, since this is the only search implementation. I would vote for putting it in geonode.silage or just calling it geonode.search.
Since search is really a sort of pluggable app and not a 'module' the way cws and perhaps ows in the future may be. I think we can get away with having: geonode.search or geonode.silage have the same urls and api as a potential geonode_haystack that may or may not live under our source tree.
I will now try the code and report the finding on the pull request.
-a
Simone Dalmasso
Oct 18, 2012, 3:28:04 AM10/18/12
to geono...@opengeo.org
Jeffrey Johnson
Oct 19, 2012, 11:20:05 AM10/19/12
to Simone Dalmasso, geono...@opengeo.org
Fwiw, this is now pulled to dev. We should update the GNIP to reflect
that and move it down to the section "Completed and/or Scheduled for
Release". Ian, I can do that if you like, or if you want to add any
final notes, go for it.
Great work! Thanks a bunch!
Jeff
that and move it down to the section "Completed and/or Scheduled for
Release". Ian, I can do that if you like, or if you want to add any
final notes, go for it.
Great work! Thanks a bunch!
Jeff
Reply all
Reply to author
Forward
0 new messages