[Geoserver-devel] Ways to reduce further the amount of work needed to handle pull requests

Andrea_Aime4 · December 26, 2015, 6:45pm

Hi,
as you can see, pull requests are going “well”, we have quite a bit of them coming
in all the time, and compared to other projects, relatively small queues and wait times.
Yet, this continuous flow, coupled with the chronic lack of people dedicating
time to review them, makes me believe we need to do something to streamline
this further.

The first choice in a community project would be to throw more “meat” at the problem,
having more reviewers. This has been brought up before, and some unsolicited help
(people that we don’t need to call up by name to get some opinion)
showed up, but imho we are not nearly enough yet.

I am wondering, would it help if we added in the wiki a review guide? Would that
make more people show up to do reviews? If not that, what will?

Another issue is that some come out of the blue and propose
deep changes, or large refactors, making them very hard to review, as in, demanding
the reviewer to literally spend several hours of their spare time on a single pull request
just to handle aestetics/class structure changes, connect the dots and figure
out if the change is actually an improvement or not.
Shall we just add to the list of pull request criterias “keep changes to a minimum, be
mindful of those reviewing them”? Other ideas?

Moving on, there is a number of people making pull request with changes that are
so focused on their specific issue that it’s hard if not impossible to merge the changes,
because they don’t see the big picture and are causing regressions in other use
cases.
Again, maybe we should just write in docs in a more evident way to discuss before
proposing changes, in order to avoid the above? Or ask people to consider
all ways the code they are using can be leveraged, and not just their particular
use case? What else?

Finally, another source of pain is that the core developer doing the review is held responsible
for the changes being merged, while the person proposing the pull request should be instead.
Now, we cannot have these people contributing pull requests be responsible forever,
but could we setup some policy that if anything goes wrong with the pull request, they are supposed
to be around and help fixing the mess, for some amount of time (like, don’t know,
2 to 6 months?). If they are not, the commits just get reverted instead (assuming
it’s possible, sometimes it will not be).
Also, it would be great if we could ask them to sit on the user list to answer of their
changes to the user base, for the same amount of time (right now it’s pretty much
the core dev that merged the pull doing that, which is also unfair).
This would be mostly for new features/large changes, not for bug fixes, although
I would really love to have the responsible be grilled by the angry users when
a so called bug fix causes some important regression.

That’s what I have, any other idea is course more than welcomed

Cheers
Andrea

···

==
GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

Geosolutions’ Winter Holidays from 24/12 to 6/1

Ing. Andrea Aime

@geowolf
Technical Lead

GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

AVVERTENZE AI SENSI DEL D.Lgs. 196/2003

Le informazioni contenute in questo messaggio di posta elettronica e/o nel/i file/s allegato/i sono da considerarsi strettamente riservate. Il loro utilizzo è consentito esclusivamente al destinatario del messaggio, per le finalità indicate nel messaggio stesso. Qualora riceviate questo messaggio senza esserne il destinatario, Vi preghiamo cortesemente di darcene notizia via e-mail e di procedere alla distruzione del messaggio stesso, cancellandolo dal Vostro sistema. Conservare il messaggio stesso, divulgarlo anche in parte, distribuirlo ad altri soggetti, copiarlo, od utilizzarlo per finalità diverse, costituisce comportamento contrario ai principi dettati dal D.Lgs. 196/2003.

The information in this message and/or attachments, is intended solely for the attention and use of the named addressee(s) and may be confidential or proprietary in nature or covered by the provisions of privacy act (Legislative Decree June, 30 2003, no.196 - Italy’s New Data Protection Code).Any use not in accord with its purpose, any disclosure, reproduction, copying, distribution, or either dissemination, either whole or partial, is strictly forbidden except previous formal approval of the named addressee(s). If you are not the intended recipient, please contact immediately the sender by telephone, fax or e-mail and delete the information in this message that has been received in error. The sender does not give any warranty or accept liability as the content, accuracy or completeness of sent messages and accepts no responsibility for changes made after they were sent or for other risks which arise as a result of e-mail transmission, viruses, etc.

jive · December 30, 2015, 7:58pm

Thanks for the discussion Andrea.

We should also acknowledge the pain in the other direction - there is a category of “small fixes” where our response (although understandable give our limited resources) is viewed as out of line with respect to other open source projects.

It may be simply a difference in scale, although I understand your points about “focused” changes which break a wider story. Still we have examples such as the recent conversation about date representation where this has been handled well. I think we could collect examples like this to use in documentation.

While I wish we could somehow lighten up with respect to smaller changes - I feel we are managing a large codebase reasonably well (with respect to requiring a test-case and/or docs). Adding “discussion” to that list would be appropriate.

···

On 26 December 2015 at 10:45, Andrea Aime <andrea.aime@anonymised.com> wrote:

Hi,
as you can see, pull requests are going “well”, we have quite a bit of them coming
in all the time, and compared to other projects, relatively small queues and wait times.
Yet, this continuous flow, coupled with the chronic lack of people dedicating
time to review them, makes me believe we need to do something to streamline
this further.

The first choice in a community project would be to throw more “meat” at the problem,
having more reviewers. This has been brought up before, and some unsolicited help
(people that we don’t need to call up by name to get some opinion)
showed up, but imho we are not nearly enough yet.

I am wondering, would it help if we added in the wiki a review guide? Would that
make more people show up to do reviews? If not that, what will?

Another issue is that some come out of the blue and propose
deep changes, or large refactors, making them very hard to review, as in, demanding
the reviewer to literally spend several hours of their spare time on a single pull request
just to handle aestetics/class structure changes, connect the dots and figure
out if the change is actually an improvement or not.
Shall we just add to the list of pull request criterias “keep changes to a minimum, be
mindful of those reviewing them”? Other ideas?

Moving on, there is a number of people making pull request with changes that are
so focused on their specific issue that it’s hard if not impossible to merge the changes,
because they don’t see the big picture and are causing regressions in other use
cases.
Again, maybe we should just write in docs in a more evident way to discuss before
proposing changes, in order to avoid the above? Or ask people to consider
all ways the code they are using can be leveraged, and not just their particular
use case? What else?

Finally, another source of pain is that the core developer doing the review is held responsible
for the changes being merged, while the person proposing the pull request should be instead.
Now, we cannot have these people contributing pull requests be responsible forever,
but could we setup some policy that if anything goes wrong with the pull request, they are supposed
to be around and help fixing the mess, for some amount of time (like, don’t know,
2 to 6 months?). If they are not, the commits just get reverted instead (assuming
it’s possible, sometimes it will not be).
Also, it would be great if we could ask them to sit on the user list to answer of their
changes to the user base, for the same amount of time (right now it’s pretty much
the core dev that merged the pull doing that, which is also unfair).
This would be mostly for new features/large changes, not for bug fixes, although
I would really love to have the responsible be grilled by the angry users when
a so called bug fix causes some important regression.

That’s what I have, any other idea is course more than welcomed

Cheers
Andrea

–

==
GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

Geosolutions’ Winter Holidays from 24/12 to 6/1

Ing. Andrea Aime

@geowolf
Technical Lead

GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

AVVERTENZE AI SENSI DEL D.Lgs. 196/2003

Le informazioni contenute in questo messaggio di posta elettronica e/o nel/i file/s allegato/i sono da considerarsi strettamente riservate. Il loro utilizzo è consentito esclusivamente al destinatario del messaggio, per le finalità indicate nel messaggio stesso. Qualora riceviate questo messaggio senza esserne il destinatario, Vi preghiamo cortesemente di darcene notizia via e-mail e di procedere alla distruzione del messaggio stesso, cancellandolo dal Vostro sistema. Conservare il messaggio stesso, divulgarlo anche in parte, distribuirlo ad altri soggetti, copiarlo, od utilizzarlo per finalità diverse, costituisce comportamento contrario ai principi dettati dal D.Lgs. 196/2003.

The information in this message and/or attachments, is intended solely for the attention and use of the named addressee(s) and may be confidential or proprietary in nature or covered by the provisions of privacy act (Legislative Decree June, 30 2003, no.196 - Italy’s New Data Protection Code).Any use not in accord with its purpose, any disclosure, reproduction, copying, distribution, or either dissemination, either whole or partial, is strictly forbidden except previous formal approval of the named addressee(s). If you are not the intended recipient, please contact immediately the sender by telephone, fax or e-mail and delete the information in this message that has been received in error. The sender does not give any warranty or accept liability as the content, accuracy or completeness of sent messages and accepts no responsibility for changes made after they were sent or for other risks which arise as a result of e-mail transmission, viruses, etc.

Geoserver-devel mailing list
Geoserver-devel@anonymised.comsts.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel

–
Jody Garnett

Andrea_Aime4 · December 30, 2015, 8:07pm

On Wed, Dec 30, 2015 at 8:58 PM, Jody Garnett <jody.garnett@anonymised.com>
wrote:

Thanks for the discussion Andrea.

We should also acknowledge the pain in the other direction - there is a
category of "small fixes" where our response (although understandable give
our limited resources) is viewed as out of line with respect to other open
source projects.

Can you provide some examples? I don't work very well with general
statements.

It seems by your comment one way to handle this would be to increase the
resources, so that the reviewer of the day is not
so hard pressed to do things quickly and get back to work/family.

It may be simply a difference in scale, although I understand your points
about "focused" changes which break a wider story. Still we have examples
such as the recent conversation about date representation where this has
been handled well. I think we could collect examples like this to use in
documentation.

While I wish we could somehow lighten up with respect to smaller changes -
I feel we are managing a large codebase reasonably well (with respect to
requiring a test-case and/or docs). Adding "discussion" to that list would
be appropriate.

It depends on what you mean by lighten up.

If it's relaxing the amount of stuff to be changed in order
to require a contribution agreement, I don't see a problem, the current
practice has been set basically following
Locationtech advice, I don't think anyone around here wanted to make it so
strict.

If it's not requiring tests, then you have my unconditional -1, too many
people making too many chances
unaware of each other, if there are no tests you might as well not
contribute, the change is going to be broken
soon anyways.

Again, you're talking too much in generic terms for me, could you propose
something more specific?

Cheers
Andrea

--

GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

*Geosolutions' Winter Holidays from 24/12 to 6/1*

Ing. Andrea Aime
@geowolf
Technical Lead

GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

*AVVERTENZE AI SENSI DEL D.Lgs. 196/2003*

Le informazioni contenute in questo messaggio di posta elettronica e/o
nel/i file/s allegato/i sono da considerarsi strettamente riservate. Il
loro utilizzo è consentito esclusivamente al destinatario del messaggio,
per le finalità indicate nel messaggio stesso. Qualora riceviate questo
messaggio senza esserne il destinatario, Vi preghiamo cortesemente di
darcene notizia via e-mail e di procedere alla distruzione del messaggio
stesso, cancellandolo dal Vostro sistema. Conservare il messaggio stesso,
divulgarlo anche in parte, distribuirlo ad altri soggetti, copiarlo, od
utilizzarlo per finalità diverse, costituisce comportamento contrario ai
principi dettati dal D.Lgs. 196/2003.

The information in this message and/or attachments, is intended solely for
the attention and use of the named addressee(s) and may be confidential or
proprietary in nature or covered by the provisions of privacy act
(Legislative Decree June, 30 2003, no.196 - Italy's New Data Protection
Code).Any use not in accord with its purpose, any disclosure, reproduction,
copying, distribution, or either dissemination, either whole or partial, is
strictly forbidden except previous formal approval of the named
addressee(s). If you are not the intended recipient, please contact
immediately the sender by telephone, fax or e-mail and delete the
information in this message that has been received in error. The sender
does not give any warranty or accept liability as the content, accuracy or
completeness of sent messages and accepts no responsibility for changes
made after they were sent or for other risks which arise as a result of
e-mail transmission, viruses, etc.

-------------------------------------------------------

jive · December 30, 2015, 8:24pm

We should also acknowledge the pain in the other direction - there is a

category of "small fixes" where our response (although understandable give
our limited resources) is viewed as out of line with respect to other open
source projects.

Can you provide some examples? I don't work very well with general
statements.

Should of been more clear that I agree with everything you say, and was
trying to add the idea of how we look to potential contributors into the
mix. Each case I can think of (jdbc timezone fix that I rejected, wps pull
request that went from simple to insurmountable) has a danger of
distracting from the point - that developers expect to be able to
contribute a fix and walk away. Both of those examples had very clear
reasons why they were rejected - but the impression remains that our
codebase is hard to work with.

A review guide as you describe it, can help both those new to the geoserver
community conduct a review and feel confident in the result, and those
contributing to the project to know what to expect and that we are treating
them fairly/consistently.

It seems by your comment one way to handle this would be to increase the

resources, so that the reviewer of the day is not so hard pressed to do
things quickly and get back to work/family.

Yeah, the real strain here is lack of resources on the project.

If it's relaxing the amount of stuff to be changed in order
to require a contribution agreement, I don't see a problem, the current
practice has been set basically following Locationtech advice, I don't
think anyone around here wanted to make it so strict.

That was more apache advice when we were setting up OSGeo, but I do
understand your point.

If it's not requiring tests, then you have my unconditional -1, too many

people making too many chances unaware of each other, if there are no tests
you might as well not contribute, the change is going to be broken soon
anyways. Again, you're talking too much in generic terms for me, could you
propose something more specific?

A test case and documentation are often considered unreasonable. We do a
good job of communicating why they are required. And I agree with our
limited resources submitting a fix without a test case is wasted effort.

So for something more specific - at the end of a "review checklist" we
could include a paragraph inviting the reader to join geoserver-devel and
ask questions. Or at least acknowledge that this is hard work and the
checklist helps us share the work more evenly.

Andrea_Aime4 · December 31, 2015, 11:18am

On Wed, Dec 30, 2015 at 9:24 PM, Jody Garnett <jody.garnett@anonymised.com>
wrote:

We should also acknowledge the pain in the other direction - there is a

category of "small fixes" where our response (although understandable give
our limited resources) is viewed as out of line with respect to other open
source projects.

Can you provide some examples? I don't work very well with general
statements.

Should of been more clear that I agree with everything you say, and was
trying to add the idea of how we look to potential contributors into the
mix. Each case I can think of (jdbc timezone fix that I rejected, wps pull
request that went from simple to insurmountable) has a danger of
distracting from the point - that developers expect to be able to
contribute a fix and walk away. Both of those examples had very clear
reasons why they were rejected - but the impression remains that our
codebase is hard to work with.

Actually I believe in both cases the original developer in question just
walked away a yard before the victory llne.
In the case of the date one, at the end of the thread we offered a simple
solution to the problem (handle the timezone shift for dates as part of a
setting), in the second one I provided some feedback on how to address it
that surely made it go from simple (but wrong, was reporting the process as
started while it was actually still queued) to non trivial, but still a far
cry from insurmountable, took me less than a hour to cook up a pull request
while helping my daughter do her Christmas break homeworks. Full pull
request:
https://github.com/geoserver/geoserver/pull/1386
The actual fix:
https://github.com/geoserver/geoserver/commit/275bd9886e538e384cd2796facef5cc499ba6582
I've preserved Dave's test in a separate commit, and doh, just looked at it
forgot to add copyright headers, remove the tabs and so on, some more
cleanup needed on this part...

A review guide as you describe it, can help both those new to the
geoserver community conduct a review and feel confident in the result, and
those contributing to the project to know what to expect and that we are
treating them fairly/consistently.

All right, I'll try to cook up one.

Cheers
Andrea

--

GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

*Geosolutions' Winter Holidays from 24/12 to 6/1*

Ing. Andrea Aime
@geowolf
Technical Lead

GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

*AVVERTENZE AI SENSI DEL D.Lgs. 196/2003*

Le informazioni contenute in questo messaggio di posta elettronica e/o
nel/i file/s allegato/i sono da considerarsi strettamente riservate. Il
loro utilizzo è consentito esclusivamente al destinatario del messaggio,
per le finalità indicate nel messaggio stesso. Qualora riceviate questo
messaggio senza esserne il destinatario, Vi preghiamo cortesemente di
darcene notizia via e-mail e di procedere alla distruzione del messaggio
stesso, cancellandolo dal Vostro sistema. Conservare il messaggio stesso,
divulgarlo anche in parte, distribuirlo ad altri soggetti, copiarlo, od
utilizzarlo per finalità diverse, costituisce comportamento contrario ai
principi dettati dal D.Lgs. 196/2003.

The information in this message and/or attachments, is intended solely for
the attention and use of the named addressee(s) and may be confidential or
proprietary in nature or covered by the provisions of privacy act
(Legislative Decree June, 30 2003, no.196 - Italy's New Data Protection
Code).Any use not in accord with its purpose, any disclosure, reproduction,
copying, distribution, or either dissemination, either whole or partial, is
strictly forbidden except previous formal approval of the named
addressee(s). If you are not the intended recipient, please contact
immediately the sender by telephone, fax or e-mail and delete the
information in this message that has been received in error. The sender
does not give any warranty or accept liability as the content, accuracy or
completeness of sent messages and accepts no responsibility for changes
made after they were sent or for other risks which arise as a result of
e-mail transmission, viruses, etc.

-------------------------------------------------------

jive · December 31, 2015, 7:40pm

I was thinking of the one Travis was working on where we spent an afternoon sorting out what had been done - and then determining it was a hack that violated API contract (use of a dummy class as a flag to engage alternate logic in a converter).

···

On 31 December 2015 at 03:18, Andrea Aime <andrea.aime@anonymised.com> wrote:

–
Jody Garnett

On Wed, Dec 30, 2015 at 9:24 PM, Jody Garnett <jody.garnett@anonymised.com> wrote:

Actually I believe in both cases the original developer in question just walked away a yard before the victory llne.
In the case of the date one, at the end of the thread we offered a simple solution to the problem (handle the timezone shift for dates as part of a setting), in the second one I provided some feedback on how to address it that surely made it go from simple (but wrong, was reporting the process as started while it was actually still queued) to non trivial, but still a far cry from insurmountable, took me less than a hour to cook up a pull request while helping my daughter do her Christmas break homeworks. Full pull request:
https://github.com/geoserver/geoserver/pull/1386

The actual fix:
https://github.com/geoserver/geoserver/commit/275bd9886e538e384cd2796facef5cc499ba6582

I’ve preserved Dave’s test in a separate commit, and doh, just looked at it forgot to add copyright headers, remove the tabs and so on, some more cleanup needed on this part…

All right, I’ll try to cook up one.

Cheers

Andrea

–

==
GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

Geosolutions’ Winter Holidays from 24/12 to 6/1

Ing. Andrea Aime

@geowolf
Technical Lead

GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

AVVERTENZE AI SENSI DEL D.Lgs. 196/2003

Le informazioni contenute in questo messaggio di posta elettronica e/o nel/i file/s allegato/i sono da considerarsi strettamente riservate. Il loro utilizzo è consentito esclusivamente al destinatario del messaggio, per le finalità indicate nel messaggio stesso. Qualora riceviate questo messaggio senza esserne il destinatario, Vi preghiamo cortesemente di darcene notizia via e-mail e di procedere alla distruzione del messaggio stesso, cancellandolo dal Vostro sistema. Conservare il messaggio stesso, divulgarlo anche in parte, distribuirlo ad altri soggetti, copiarlo, od utilizzarlo per finalità diverse, costituisce comportamento contrario ai principi dettati dal D.Lgs. 196/2003.

The information in this message and/or attachments, is intended solely for the attention and use of the named addressee(s) and may be confidential or proprietary in nature or covered by the provisions of privacy act (Legislative Decree June, 30 2003, no.196 - Italy’s New Data Protection Code).Any use not in accord with its purpose, any disclosure, reproduction, copying, distribution, or either dissemination, either whole or partial, is strictly forbidden except previous formal approval of the named addressee(s). If you are not the intended recipient, please contact immediately the sender by telephone, fax or e-mail and delete the information in this message that has been received in error. The sender does not give any warranty or accept liability as the content, accuracy or completeness of sent messages and accepts no responsibility for changes made after they were sent or for other risks which arise as a result of e-mail transmission, viruses, etc.

Should of been more clear that I agree with everything you say, and was trying to add the idea of how we look to potential contributors into the mix. Each case I can think of (jdbc timezone fix that I rejected, wps pull request that went from simple to insurmountable) has a danger of distracting from the point - that developers expect to be able to contribute a fix and walk away. Both of those examples had very clear reasons why they were rejected - but the impression remains that our codebase is hard to work with.

We should also acknowledge the pain in the other direction - there is a category of “small fixes” where our response (although understandable give our limited resources) is viewed as out of line with respect to other open source projects.

Can you provide some examples? I don’t work very well with general statements.

A review guide as you describe it, can help both those new to the geoserver community conduct a review and feel confident in the result, and those contributing to the project to know what to expect and that we are treating them fairly/consistently.

Andrea_Aime4 · December 31, 2015, 11:55pm

On Thu, Dec 31, 2015 at 8:40 PM, Jody Garnett <jody.garnett@anonymised.com>
wrote:

I was thinking of the one Travis was working on where we spent an
afternoon sorting out what had been done - and then determining it was a
hack that violated API contract (use of a dummy class as a flag to engage
alternate logic in a converter).

Ah, yeah, I remember, that one was handling negative dates (BC) in
postgresql

Cheers
Andrea

--

GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

*Geosolutions' Winter Holidays from 24/12 to 6/1*

Ing. Andrea Aime
@geowolf
Technical Lead

GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

*AVVERTENZE AI SENSI DEL D.Lgs. 196/2003*

Le informazioni contenute in questo messaggio di posta elettronica e/o
nel/i file/s allegato/i sono da considerarsi strettamente riservate. Il
loro utilizzo è consentito esclusivamente al destinatario del messaggio,
per le finalità indicate nel messaggio stesso. Qualora riceviate questo
messaggio senza esserne il destinatario, Vi preghiamo cortesemente di
darcene notizia via e-mail e di procedere alla distruzione del messaggio
stesso, cancellandolo dal Vostro sistema. Conservare il messaggio stesso,
divulgarlo anche in parte, distribuirlo ad altri soggetti, copiarlo, od
utilizzarlo per finalità diverse, costituisce comportamento contrario ai
principi dettati dal D.Lgs. 196/2003.

The information in this message and/or attachments, is intended solely for
the attention and use of the named addressee(s) and may be confidential or
proprietary in nature or covered by the provisions of privacy act
(Legislative Decree June, 30 2003, no.196 - Italy's New Data Protection
Code).Any use not in accord with its purpose, any disclosure, reproduction,
copying, distribution, or either dissemination, either whole or partial, is
strictly forbidden except previous formal approval of the named
addressee(s). If you are not the intended recipient, please contact
immediately the sender by telephone, fax or e-mail and delete the
information in this message that has been received in error. The sender
does not give any warranty or accept liability as the content, accuracy or
completeness of sent messages and accepts no responsibility for changes
made after they were sent or for other risks which arise as a result of
e-mail transmission, viruses, etc.

-------------------------------------------------------

Andrea_Aime4 · January 2, 2016, 10:59am

On Wed, Dec 30, 2015 at 9:24 PM, Jody Garnett <jody.garnett@anonymised.com>
wrote:

A review guide as you describe it, can help both those new to the
geoserver community conduct a review and feel confident in the result, and
those contributing to the project to know what to expect and that we are
treating them fairly/consistently.

Hi Jody,
I've put down in writing what I normally check during a full pull request
review. It's still a draft of course, additions/corrections (or discussion
about the contents) are welcomed:
https://github.com/geoserver/geoserver/wiki/Pull-request-review-guide

Cheers
Andrea

--

GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

*Geosolutions' Winter Holidays from 24/12 to 6/1*

Ing. Andrea Aime
@geowolf
Technical Lead

GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

*AVVERTENZE AI SENSI DEL D.Lgs. 196/2003*

Le informazioni contenute in questo messaggio di posta elettronica e/o
nel/i file/s allegato/i sono da considerarsi strettamente riservate. Il
loro utilizzo è consentito esclusivamente al destinatario del messaggio,
per le finalità indicate nel messaggio stesso. Qualora riceviate questo
messaggio senza esserne il destinatario, Vi preghiamo cortesemente di
darcene notizia via e-mail e di procedere alla distruzione del messaggio
stesso, cancellandolo dal Vostro sistema. Conservare il messaggio stesso,
divulgarlo anche in parte, distribuirlo ad altri soggetti, copiarlo, od
utilizzarlo per finalità diverse, costituisce comportamento contrario ai
principi dettati dal D.Lgs. 196/2003.

The information in this message and/or attachments, is intended solely for
the attention and use of the named addressee(s) and may be confidential or
proprietary in nature or covered by the provisions of privacy act
(Legislative Decree June, 30 2003, no.196 - Italy's New Data Protection
Code).Any use not in accord with its purpose, any disclosure, reproduction,
copying, distribution, or either dissemination, either whole or partial, is
strictly forbidden except previous formal approval of the named
addressee(s). If you are not the intended recipient, please contact
immediately the sender by telephone, fax or e-mail and delete the
information in this message that has been received in error. The sender
does not give any warranty or accept liability as the content, accuracy or
completeness of sent messages and accepts no responsibility for changes
made after they were sent or for other risks which arise as a result of
e-mail transmission, viruses, etc.

-------------------------------------------------------

Bart_van_den_Eijnde1 · January 2, 2016, 10:13pm

On 02/01/16 23:59, Andrea Aime wrote:

I've put down in writing what I normally check during a full pull request
review. It's still a draft of course, additions/corrections (or discussion
about the contents) are welcomed:
https://github.com/geoserver/geoserver/wiki/Pull-request-review-guide

Andrea, that is very good. Will this document be moved to the developer guide?

Other things I like to watch for:

- Code style/formatting, no tabs, use of modern Java idioms including generics, foreach, and try-with-resources (which should reduce use of finally). I prefer JUnit 4 over JUnit 3.

- Resource leaks: any time a close method is used outside a finally or try-with-resources block it attracts my attention.

- Malicious or dangerous code from untrusted submitters.

- static instances / singletons are a great way of introducing concurrency issues; you have this in your "Thread safety" section already but it might be worth mentioning that static instances are a red flag. (gt-app-schema DataAccessRegistry I am looking at you.)

Do you use or recommend any linting tools that support differential operation, that is, reporting new suspect code practices introduced by a commit? Do you think such tools are helpful for this project?

I also think that we need a pull request submitter guide. Reviews are less work when they are prepared with review in mind. Should pull request submitter guidelines be part of the same document? A few points I would like to see:

- Architectural or widespread changes *must* first be discussed on the mailing lists.

- Successful pull requests do more than just change code: they communicate intent to people. Pull requests are a conversation not a just a diff. Make your pull request as clear as possible to maximise your chances of acceptance.

- Do not mix renaming/moving files or bulk reformatting with substantive changes as this can make pull requests very hard to understand. Put renaming/moving/bulk-reformatting in separate commits and let the reviewer know that you have done so. Do not squash commits if doing so mixes renaming/moving/bulk-reformatting with substantive changes.

- Pull requests enable formal review by project leaders but also (1) make you learn more about your code by explaining it, and (2) let you teach the reviewer about your code. Review by a peer or junior or rubber duck is also valuable (although rubber ducks are not known for asking hard questions). Everyone will be able to see your pull request. Use this opportunity.

Kind regards,

--
Ben Caradoc-Davies <ben@anonymised.com>
Director
Transient Software Limited <http://transient.nz/>
New Zealand

Andrea_Aime4 · January 3, 2016, 12:13pm

On Sat, Jan 2, 2016 at 11:13 PM, Ben Caradoc-Davies <ben@anonymised.com>
wrote:

On 02/01/16 23:59, Andrea Aime wrote:

I've put down in writing what I normally check during a full pull request
review. It's still a draft of course, additions/corrections (or discussion
about the contents) are welcomed:
https://github.com/geoserver/geoserver/wiki/Pull-request-review-guide

Andrea, that is very good. Will this document be moved to the developer
guide?

For the moment I suggest to keep it in the wiki, it's quicker to modify,
but once the dust settles
down some, why not, I believe it's the second time someone asks that very
same question (to move
it to the dev guide).

Other things I like to watch for:

- Code style/formatting, no tabs, use of modern Java idioms including
generics, foreach, and try-with-resources (which should reduce use of
finally). I prefer JUnit 4 over JUnit 3.

- Resource leaks: any time a close method is used outside a finally or
try-with-resources block it attracts my attention.

- Malicious or dangerous code from untrusted submitters.

- static instances / singletons are a great way of introducing concurrency
issues; you have this in your "Thread safety" section already but it might
be worth mentioning that static instances are a red flag. (gt-app-schema
DataAccessRegistry I am looking at you.)

I've tried to integrate the above in the guide.

Do you use or recommend any linting tools that support differential
operation, that is, reporting new suspect code practices introduced by a
commit? Do you think such tools are helpful for this project?

I'm not aware of open source tools for Java that can be used in a
differential manner, then ones I've seen are all or nothing, so you have to
start
with a "full clean" in order for them to point out misbehavior.
Many of them are configurable, so if there is interest in working this, one
could start by looking at just one rule (thinking FindBugs here), clean it
up fully in
the current code, and enable a build that would fail on subsequent
violations.
Normally these checks are pretty expensive to perform, so I'd advise to run
them in a separate build (I've experience on some projects
running PMD in each module and the build times are quite a bit higher, to
the point that a "full build" is sort of a last resort measure, even on a
fast machine).

I also think that we need a pull request submitter guide. Reviews are less
work when they are prepared with review in mind. Should pull request
submitter guidelines be part of the same document? A few points I would
like to see:

We already have one, which is also presented automatically by Gitbub when
making a pull request, and thus
referenceable as "Something you should have looked at before making a pull
request":

https://github.com/geoserver/geoserver/blob/master/CONTRIBUTING.md

Just a though, maybe instead of the dev guide we could do a REVIEWING.md
document right besides the CONTRIBUTING.md one, and then link them back to
back.

- Architectural or widespread changes *must* first be discussed on the
mailing lists.

- Successful pull requests do more than just change code: they communicate
intent to people. Pull requests are a conversation not a just a diff. Make
your pull request as clear as possible to maximise your chances of
acceptance.

- Do not mix renaming/moving files or bulk reformatting with substantive
changes as this can make pull requests very hard to understand. Put
renaming/moving/bulk-reformatting in separate commits and let the reviewer
know that you have done so. Do not squash commits if doing so mixes
renaming/moving/bulk-reformatting with substantive changes.

- Pull requests enable formal review by project leaders but also (1) make
you learn more about your code by explaining it, and (2) let you teach the
reviewer about your code. Review by a peer or junior or rubber duck is also
valuable (although rubber ducks are not known for asking hard questions).
Everyone will be able to see your pull request. Use this opportunity.

Good suggestions, want to add them to the above document?

Cheers
Andrea

--

GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

*Geosolutions' Winter Holidays from 24/12 to 6/1*

Ing. Andrea Aime
@geowolf
Technical Lead

GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

*AVVERTENZE AI SENSI DEL D.Lgs. 196/2003*

Le informazioni contenute in questo messaggio di posta elettronica e/o
nel/i file/s allegato/i sono da considerarsi strettamente riservate. Il
loro utilizzo è consentito esclusivamente al destinatario del messaggio,
per le finalità indicate nel messaggio stesso. Qualora riceviate questo
messaggio senza esserne il destinatario, Vi preghiamo cortesemente di
darcene notizia via e-mail e di procedere alla distruzione del messaggio
stesso, cancellandolo dal Vostro sistema. Conservare il messaggio stesso,
divulgarlo anche in parte, distribuirlo ad altri soggetti, copiarlo, od
utilizzarlo per finalità diverse, costituisce comportamento contrario ai
principi dettati dal D.Lgs. 196/2003.

The information in this message and/or attachments, is intended solely for
the attention and use of the named addressee(s) and may be confidential or
proprietary in nature or covered by the provisions of privacy act
(Legislative Decree June, 30 2003, no.196 - Italy's New Data Protection
Code).Any use not in accord with its purpose, any disclosure, reproduction,
copying, distribution, or either dissemination, either whole or partial, is
strictly forbidden except previous formal approval of the named
addressee(s). If you are not the intended recipient, please contact
immediately the sender by telephone, fax or e-mail and delete the
information in this message that has been received in error. The sender
does not give any warranty or accept liability as the content, accuracy or
completeness of sent messages and accepts no responsibility for changes
made after they were sent or for other risks which arise as a result of
e-mail transmission, viruses, etc.

-------------------------------------------------------

jive · January 4, 2016, 5:14pm

We already have one, which is also presented automatically by Gitbub when
making a pull request, and thus
referenceable as "Something you should have looked at before making a pull
request":

https://github.com/geoserver/geoserver/blob/master/CONTRIBUTING.md

Just a though, maybe instead of the dev guide we could do a REVIEWING.md
document right besides the CONTRIBUTING.md one, and then link them back to
back.

Not a bad idea, I am also not against adding two headings to the
CONTRIBUTING.md document (one for contributors and one for reviewing).

I also hate duplication and would not be against having the developers
guide link to the CONTRIBUTING.md document to reduce confusion. I imagine
the developers guide could have examples and the CONTRIUTING.md document
could have the short-form content.

jive · January 4, 2016, 5:15pm

Would like to Echo Ben’s feedback - thanks!

···

Andrea, that is very good. Will this document be moved to the developer guide?

Agree we should sort things out in the wiki and then move contents to developers guide.

Other things I like to watch for:

Code style/formatting, no tabs, use of modern Java idioms including generics, foreach, and try-with-resources (which should reduce use of finally). I prefer JUnit 4 over JUnit 3.

When moving to the developers guide I would like to link to any style guide, rather than duplicate.

Resource leaks: any time a close method is used outside a finally or try-with-resources block it attracts my attention.

I do think we can update the developers guide with this kind of information, even if it is just “these things are probably wrong and will slow down your code review”.

Malicious or dangerous code from untrusted submitters.

static instances / singletons are a great way of introducing concurrency issues; you have this in your “Thread safety” section already but it might be worth mentioning that static instances are a red flag. (gt-app-schema DataAccessRegistry I am looking at you.)

Do you use or recommend any linting tools that support differential operation, that is, reporting new suspect code practices introduced by a commit? Do you think such tools are helpful for this project?

I use FindBugz when reviewing.

I also think that we need a pull request submitter guide. Reviews are less work when they are prepared with review in mind. Should pull request submitter guidelines be part of the same document?

Ideally I would like this to be the same document to avoid having the two drift and be clear about expectations.

Andrea_Aime4 · January 4, 2016, 5:22pm

On Mon, Jan 4, 2016 at 6:15 PM, Jody Garnett <jody.garnett@anonymised.com> wrote:

Do you use or recommend any linting tools that support differential

operation, that is, reporting new suspect code practices introduced by a
commit? Do you think such tools are helpful for this project?

I use FindBugz when reviewing.

Interesting... is there a way to make it find bugs only in the modified
sections of code, or at least modified files?
Any way to automate it? Findbugs in desktop mode (IDE integration) requires
actually fetching the pull request, something that I do for
large ones, for simple ones it seems overkill.

Also... having findbugs run in IDE all time sounds like it would slow down
the IDE a lot, how do you deal with that?

Cheers
Andrea

--

GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

*Geosolutions' Winter Holidays from 24/12 to 6/1*

Ing. Andrea Aime
@geowolf
Technical Lead

GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

*AVVERTENZE AI SENSI DEL D.Lgs. 196/2003*

Le informazioni contenute in questo messaggio di posta elettronica e/o
nel/i file/s allegato/i sono da considerarsi strettamente riservate. Il
loro utilizzo è consentito esclusivamente al destinatario del messaggio,
per le finalità indicate nel messaggio stesso. Qualora riceviate questo
messaggio senza esserne il destinatario, Vi preghiamo cortesemente di
darcene notizia via e-mail e di procedere alla distruzione del messaggio
stesso, cancellandolo dal Vostro sistema. Conservare il messaggio stesso,
divulgarlo anche in parte, distribuirlo ad altri soggetti, copiarlo, od
utilizzarlo per finalità diverse, costituisce comportamento contrario ai
principi dettati dal D.Lgs. 196/2003.

The information in this message and/or attachments, is intended solely for
the attention and use of the named addressee(s) and may be confidential or
proprietary in nature or covered by the provisions of privacy act
(Legislative Decree June, 30 2003, no.196 - Italy's New Data Protection
Code).Any use not in accord with its purpose, any disclosure, reproduction,
copying, distribution, or either dissemination, either whole or partial, is
strictly forbidden except previous formal approval of the named
addressee(s). If you are not the intended recipient, please contact
immediately the sender by telephone, fax or e-mail and delete the
information in this message that has been received in error. The sender
does not give any warranty or accept liability as the content, accuracy or
completeness of sent messages and accepts no responsibility for changes
made after they were sent or for other risks which arise as a result of
e-mail transmission, viruses, etc.

-------------------------------------------------------

jive · January 4, 2016, 7:07pm

I use FindBugz when reviewing.

Interesting... is there a way to make it find bugs only in the modified
sections of code, or at least modified files?

I right click on the file and say "find bugz" You can also filter in
eclipse to only show modified files.

You can use eclipse git staging view to show only things that have been
modified (ie by your pulling in the external pull request)

Any way to automate it? Findbugs in desktop mode (IDE integration)
requires actually fetching the pull request, something that I do for large
ones, for simple ones it seems overkill.

I was not automating, just treating it as the first step of my manual
review.

Also... having findbugs run in IDE all time sounds like it would slow down

the IDE a lot, how do you deal with that?

I do not have the "builder" turned on all the time, only use to run
manually.

Cheers
Andrea

--

GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

*Geosolutions' Winter Holidays from 24/12 to 6/1*

Ing. Andrea Aime
@geowolf
Technical Lead

GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

*AVVERTENZE AI SENSI DEL D.Lgs. 196/2003*

Le informazioni contenute in questo messaggio di posta elettronica e/o
nel/i file/s allegato/i sono da considerarsi strettamente riservate. Il
loro utilizzo è consentito esclusivamente al destinatario del messaggio,
per le finalità indicate nel messaggio stesso. Qualora riceviate questo
messaggio senza esserne il destinatario, Vi preghiamo cortesemente di
darcene notizia via e-mail e di procedere alla distruzione del messaggio
stesso, cancellandolo dal Vostro sistema. Conservare il messaggio stesso,
divulgarlo anche in parte, distribuirlo ad altri soggetti, copiarlo, od
utilizzarlo per finalità diverse, costituisce comportamento contrario ai
principi dettati dal D.Lgs. 196/2003.

The information in this message and/or attachments, is intended solely for
the attention and use of the named addressee(s) and may be confidential or
proprietary in nature or covered by the provisions of privacy act
(Legislative Decree June, 30 2003, no.196 - Italy's New Data Protection
Code).Any use not in accord with its purpose, any disclosure, reproduction,
copying, distribution, or either dissemination, either whole or partial, is
strictly forbidden except previous formal approval of the named
addressee(s). If you are not the intended recipient, please contact
immediately the sender by telephone, fax or e-mail and delete the
information in this message that has been received in error. The sender
does not give any warranty or accept liability as the content, accuracy or
completeness of sent messages and accepts no responsibility for changes
made after they were sent or for other risks which arise as a result of
e-mail transmission, viruses, etc.

-------------------------------------------------------

tbarsballe · January 4, 2016, 8:36pm

I feel like adding a review guide is an excellent idea.

A single file with 2 headings may be better than seperate CONTRIBUTING.md and REVIEWING.md files, especially given the amount of duplicated information between the two proceedures.

Notably, the “Fast formal checks” section of the draft review guide closely mirrors the “Pull Request Guidelines / Commit guidelines” in CONTRIBUTING.md (and in many ways is better - having some justification for certain requirements that may seem arbitrary on the surface is a definite plus. Given the similarity of these two sections, perhaps they could be a combined section for both contributing and reviewing?

Slightly off topic, but concerning the note about community modules - is there any checks we can enforce when a community module gets promoted to a regular module? I get the feeling that this has caused some issues with lower quality code making its way into geoserver without a proper review (Am I right in thinking that whenever a community module does get promoted to core, it is usually sufficiently large that it is hard to enforce such guidlines whithout substantial work from both the reviewer(s) and the contributor(s)).

Torben

···

On Mon, Jan 4, 2016 at 11:07 AM, Jody Garnett <jody.garnett@anonymised.com> wrote:

Geoserver-devel mailing list
Geoserver-devel@anonymised.comsts.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel

I right click on the file and say “find bugz” You can also filter in eclipse to only show modified files.

You can use eclipse git staging view to show only things that have been modified (ie by your pulling in the external pull request)

I was not automating, just treating it as the first step of my manual review.

I do not have the “builder” turned on all the time, only use to run manually.

Interesting… is there a way to make it find bugs only in the modified sections of code, or at least modified files?

I use FindBugz when reviewing.

Any way to automate it? Findbugs in desktop mode (IDE integration) requires actually fetching the pull request, something that I do for large ones, for simple ones it seems overkill.

Also… having findbugs run in IDE all time sounds like it would slow down the IDE a lot, how do you deal with that?

Cheers

Andrea

–

==
GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

Geosolutions’ Winter Holidays from 24/12 to 6/1

Ing. Andrea Aime

@geowolf
Technical Lead

GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

AVVERTENZE AI SENSI DEL D.Lgs. 196/2003

Le informazioni contenute in questo messaggio di posta elettronica e/o nel/i file/s allegato/i sono da considerarsi strettamente riservate. Il loro utilizzo è consentito esclusivamente al destinatario del messaggio, per le finalità indicate nel messaggio stesso. Qualora riceviate questo messaggio senza esserne il destinatario, Vi preghiamo cortesemente di darcene notizia via e-mail e di procedere alla distruzione del messaggio stesso, cancellandolo dal Vostro sistema. Conservare il messaggio stesso, divulgarlo anche in parte, distribuirlo ad altri soggetti, copiarlo, od utilizzarlo per finalità diverse, costituisce comportamento contrario ai principi dettati dal D.Lgs. 196/2003.

The information in this message and/or attachments, is intended solely for the attention and use of the named addressee(s) and may be confidential or proprietary in nature or covered by the provisions of privacy act (Legislative Decree June, 30 2003, no.196 - Italy’s New Data Protection Code).Any use not in accord with its purpose, any disclosure, reproduction, copying, distribution, or either dissemination, either whole or partial, is strictly forbidden except previous formal approval of the named addressee(s). If you are not the intended recipient, please contact immediately the sender by telephone, fax or e-mail and delete the information in this message that has been received in error. The sender does not give any warranty or accept liability as the content, accuracy or completeness of sent messages and accepts no responsibility for changes made after they were sent or for other risks which arise as a result of e-mail transmission, viruses, etc.

Andrea_Aime4 · January 5, 2016, 8:07am

Hi Jody,
makes sense. So I’m wondering, are you proposing that a reviewer should use Findbugs, or
just suggesting it as a step for those that want to go the extra mile?
Also, Findbugs is pretty configurable, are you running with the defaults?

As said, seems a bit overkill for small pull requests

Cheers
Andrea

···

On Mon, Jan 4, 2016 at 8:07 PM, Jody Garnett <jody.garnett@anonymised.com> wrote:

–

I right click on the file and say “find bugz” You can also filter in eclipse to only show modified files.

You can use eclipse git staging view to show only things that have been modified (ie by your pulling in the external pull request)

I was not automating, just treating it as the first step of my manual review.

I do not have the “builder” turned on all the time, only use to run manually.

Interesting… is there a way to make it find bugs only in the modified sections of code, or at least modified files?

I use FindBugz when reviewing.

Any way to automate it? Findbugs in desktop mode (IDE integration) requires actually fetching the pull request, something that I do for large ones, for simple ones it seems overkill.

Also… having findbugs run in IDE all time sounds like it would slow down the IDE a lot, how do you deal with that?

Cheers

Andrea

–

==
GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

Geosolutions’ Winter Holidays from 24/12 to 6/1

Ing. Andrea Aime

@geowolf
Technical Lead

GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

AVVERTENZE AI SENSI DEL D.Lgs. 196/2003

Le informazioni contenute in questo messaggio di posta elettronica e/o nel/i file/s allegato/i sono da considerarsi strettamente riservate. Il loro utilizzo è consentito esclusivamente al destinatario del messaggio, per le finalità indicate nel messaggio stesso. Qualora riceviate questo messaggio senza esserne il destinatario, Vi preghiamo cortesemente di darcene notizia via e-mail e di procedere alla distruzione del messaggio stesso, cancellandolo dal Vostro sistema. Conservare il messaggio stesso, divulgarlo anche in parte, distribuirlo ad altri soggetti, copiarlo, od utilizzarlo per finalità diverse, costituisce comportamento contrario ai principi dettati dal D.Lgs. 196/2003.

The information in this message and/or attachments, is intended solely for the attention and use of the named addressee(s) and may be confidential or proprietary in nature or covered by the provisions of privacy act (Legislative Decree June, 30 2003, no.196 - Italy’s New Data Protection Code).Any use not in accord with its purpose, any disclosure, reproduction, copying, distribution, or either dissemination, either whole or partial, is strictly forbidden except previous formal approval of the named addressee(s). If you are not the intended recipient, please contact immediately the sender by telephone, fax or e-mail and delete the information in this message that has been received in error. The sender does not give any warranty or accept liability as the content, accuracy or completeness of sent messages and accepts no responsibility for changes made after they were sent or for other risks which arise as a result of e-mail transmission, viruses, etc.

==
GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

Geosolutions’ Winter Holidays from 24/12 to 6/1

Ing. Andrea Aime

@geowolf
Technical Lead

GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

AVVERTENZE AI SENSI DEL D.Lgs. 196/2003

Le informazioni contenute in questo messaggio di posta elettronica e/o nel/i file/s allegato/i sono da considerarsi strettamente riservate. Il loro utilizzo è consentito esclusivamente al destinatario del messaggio, per le finalità indicate nel messaggio stesso. Qualora riceviate questo messaggio senza esserne il destinatario, Vi preghiamo cortesemente di darcene notizia via e-mail e di procedere alla distruzione del messaggio stesso, cancellandolo dal Vostro sistema. Conservare il messaggio stesso, divulgarlo anche in parte, distribuirlo ad altri soggetti, copiarlo, od utilizzarlo per finalità diverse, costituisce comportamento contrario ai principi dettati dal D.Lgs. 196/2003.

The information in this message and/or attachments, is intended solely for the attention and use of the named addressee(s) and may be confidential or proprietary in nature or covered by the provisions of privacy act (Legislative Decree June, 30 2003, no.196 - Italy’s New Data Protection Code).Any use not in accord with its purpose, any disclosure, reproduction, copying, distribution, or either dissemination, either whole or partial, is strictly forbidden except previous formal approval of the named addressee(s). If you are not the intended recipient, please contact immediately the sender by telephone, fax or e-mail and delete the information in this message that has been received in error. The sender does not give any warranty or accept liability as the content, accuracy or completeness of sent messages and accepts no responsibility for changes made after they were sent or for other risks which arise as a result of e-mail transmission, viruses, etc.

Andrea_Aime4 · January 5, 2016, 8:19am

On Mon, Jan 4, 2016 at 9:36 PM, Torben Barsballe <
tbarsballe@anonymised.com> wrote:

I feel like adding a review guide is an excellent idea.

A single file with 2 headings may be better than seperate CONTRIBUTING.md
and REVIEWING.md files, especially given the amount of duplicated
information between the two proceedures.

I guess we could have everything in the CONTRIBUTING.md one (which has a
special treatment in github:
https://github.com/blog/1184-contributing-guidelines)
but others keep on asking to have it in the developer guide.
If we want to avoid duplication, then CONTRIBUTING.md should link to the
dev guide... I'm not a fan of indirections honestly, but let's hear what
people want to do.

Notably, the "Fast formal checks" section of the draft review guide
closely mirrors the "Pull Request Guidelines / Commit guidelines" in
CONTRIBUTING.md (and in many ways is better - having some justification for
certain requirements that may seem arbitrary on the surface is a definite
plus. Given the similarity of these two sections, perhaps they could be a
combined section for both contributing and reviewing?

Why not.

Slightly off topic, but concerning the note about community modules - is
there any checks we can enforce when a community module gets promoted to a
regular module? I get the feeling that this has caused some issues with
lower quality code making its way into geoserver without a proper review
(Am I right in thinking that whenever a community module does get promoted
to core, it is usually sufficiently large that it is hard to enforce such
guidlines whithout substantial work from both the reviewer(s) and the
contributor(s)).

That's a good point, maybe we should add this as graduation criteria,
having someone do a review according to
the reviewing guide?
Or even better, ask the module owners to first do a self-review based on
the list of topics in the review guide,
and when they think they are ready, have someone else check and confirm?

Cheers
Andrea

--

GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

*Geosolutions' Winter Holidays from 24/12 to 6/1*

Ing. Andrea Aime
@geowolf
Technical Lead

GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

*AVVERTENZE AI SENSI DEL D.Lgs. 196/2003*

Le informazioni contenute in questo messaggio di posta elettronica e/o
nel/i file/s allegato/i sono da considerarsi strettamente riservate. Il
loro utilizzo è consentito esclusivamente al destinatario del messaggio,
per le finalità indicate nel messaggio stesso. Qualora riceviate questo
messaggio senza esserne il destinatario, Vi preghiamo cortesemente di
darcene notizia via e-mail e di procedere alla distruzione del messaggio
stesso, cancellandolo dal Vostro sistema. Conservare il messaggio stesso,
divulgarlo anche in parte, distribuirlo ad altri soggetti, copiarlo, od
utilizzarlo per finalità diverse, costituisce comportamento contrario ai
principi dettati dal D.Lgs. 196/2003.

The information in this message and/or attachments, is intended solely for
the attention and use of the named addressee(s) and may be confidential or
proprietary in nature or covered by the provisions of privacy act
(Legislative Decree June, 30 2003, no.196 - Italy's New Data Protection
Code).Any use not in accord with its purpose, any disclosure, reproduction,
copying, distribution, or either dissemination, either whole or partial, is
strictly forbidden except previous formal approval of the named
addressee(s). If you are not the intended recipient, please contact
immediately the sender by telephone, fax or e-mail and delete the
information in this message that has been received in error. The sender
does not give any warranty or accept liability as the content, accuracy or
completeness of sent messages and accepts no responsibility for changes
made after they were sent or for other risks which arise as a result of
e-mail transmission, viruses, etc.

-------------------------------------------------------

Andrea_Aime4 · January 5, 2016, 8:21am

Hi folks,
I’m writing to ask if anyone else want to grab the baton and complete this one?
I believe I have done my fair share of effort, but more work is needed to update
the contribution guide, find a good place to both documents (or single shared document) and so on.

Any takers?

Cheers
Andrea

···

On Sat, Dec 26, 2015 at 7:45 PM, Andrea Aime <andrea.aime@anonymised.com> wrote:

Hi,
as you can see, pull requests are going “well”, we have quite a bit of them coming
in all the time, and compared to other projects, relatively small queues and wait times.
Yet, this continuous flow, coupled with the chronic lack of people dedicating
time to review them, makes me believe we need to do something to streamline
this further.

The first choice in a community project would be to throw more “meat” at the problem,
having more reviewers. This has been brought up before, and some unsolicited help
(people that we don’t need to call up by name to get some opinion)
showed up, but imho we are not nearly enough yet.

I am wondering, would it help if we added in the wiki a review guide? Would that
make more people show up to do reviews? If not that, what will?

Another issue is that some come out of the blue and propose
deep changes, or large refactors, making them very hard to review, as in, demanding
the reviewer to literally spend several hours of their spare time on a single pull request
just to handle aestetics/class structure changes, connect the dots and figure
out if the change is actually an improvement or not.
Shall we just add to the list of pull request criterias “keep changes to a minimum, be
mindful of those reviewing them”? Other ideas?

Moving on, there is a number of people making pull request with changes that are
so focused on their specific issue that it’s hard if not impossible to merge the changes,
because they don’t see the big picture and are causing regressions in other use
cases.
Again, maybe we should just write in docs in a more evident way to discuss before
proposing changes, in order to avoid the above? Or ask people to consider
all ways the code they are using can be leveraged, and not just their particular
use case? What else?

Finally, another source of pain is that the core developer doing the review is held responsible
for the changes being merged, while the person proposing the pull request should be instead.
Now, we cannot have these people contributing pull requests be responsible forever,
but could we setup some policy that if anything goes wrong with the pull request, they are supposed
to be around and help fixing the mess, for some amount of time (like, don’t know,
2 to 6 months?). If they are not, the commits just get reverted instead (assuming
it’s possible, sometimes it will not be).
Also, it would be great if we could ask them to sit on the user list to answer of their
changes to the user base, for the same amount of time (right now it’s pretty much
the core dev that merged the pull doing that, which is also unfair).
This would be mostly for new features/large changes, not for bug fixes, although
I would really love to have the responsible be grilled by the angry users when
a so called bug fix causes some important regression.

That’s what I have, any other idea is course more than welcomed

Cheers
Andrea

–

==
GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

Geosolutions’ Winter Holidays from 24/12 to 6/1

Ing. Andrea Aime

@geowolf
Technical Lead

GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

AVVERTENZE AI SENSI DEL D.Lgs. 196/2003

Le informazioni contenute in questo messaggio di posta elettronica e/o nel/i file/s allegato/i sono da considerarsi strettamente riservate. Il loro utilizzo è consentito esclusivamente al destinatario del messaggio, per le finalità indicate nel messaggio stesso. Qualora riceviate questo messaggio senza esserne il destinatario, Vi preghiamo cortesemente di darcene notizia via e-mail e di procedere alla distruzione del messaggio stesso, cancellandolo dal Vostro sistema. Conservare il messaggio stesso, divulgarlo anche in parte, distribuirlo ad altri soggetti, copiarlo, od utilizzarlo per finalità diverse, costituisce comportamento contrario ai principi dettati dal D.Lgs. 196/2003.

The information in this message and/or attachments, is intended solely for the attention and use of the named addressee(s) and may be confidential or proprietary in nature or covered by the provisions of privacy act (Legislative Decree June, 30 2003, no.196 - Italy’s New Data Protection Code).Any use not in accord with its purpose, any disclosure, reproduction, copying, distribution, or either dissemination, either whole or partial, is strictly forbidden except previous formal approval of the named addressee(s). If you are not the intended recipient, please contact immediately the sender by telephone, fax or e-mail and delete the information in this message that has been received in error. The sender does not give any warranty or accept liability as the content, accuracy or completeness of sent messages and accepts no responsibility for changes made after they were sent or for other risks which arise as a result of e-mail transmission, viruses, etc.

–

==
GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

Geosolutions’ Winter Holidays from 24/12 to 6/1

Ing. Andrea Aime

@geowolf
Technical Lead

GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

AVVERTENZE AI SENSI DEL D.Lgs. 196/2003

Le informazioni contenute in questo messaggio di posta elettronica e/o nel/i file/s allegato/i sono da considerarsi strettamente riservate. Il loro utilizzo è consentito esclusivamente al destinatario del messaggio, per le finalità indicate nel messaggio stesso. Qualora riceviate questo messaggio senza esserne il destinatario, Vi preghiamo cortesemente di darcene notizia via e-mail e di procedere alla distruzione del messaggio stesso, cancellandolo dal Vostro sistema. Conservare il messaggio stesso, divulgarlo anche in parte, distribuirlo ad altri soggetti, copiarlo, od utilizzarlo per finalità diverse, costituisce comportamento contrario ai principi dettati dal D.Lgs. 196/2003.

The information in this message and/or attachments, is intended solely for the attention and use of the named addressee(s) and may be confidential or proprietary in nature or covered by the provisions of privacy act (Legislative Decree June, 30 2003, no.196 - Italy’s New Data Protection Code).Any use not in accord with its purpose, any disclosure, reproduction, copying, distribution, or either dissemination, either whole or partial, is strictly forbidden except previous formal approval of the named addressee(s). If you are not the intended recipient, please contact immediately the sender by telephone, fax or e-mail and delete the information in this message that has been received in error. The sender does not give any warranty or accept liability as the content, accuracy or completeness of sent messages and accepts no responsibility for changes made after they were sent or for other risks which arise as a result of e-mail transmission, viruses, etc.

jive · January 6, 2016, 11:20pm

I have no surprises in this list, small bits of feedback:

Is it worth grouping to allow the document to be scanned quickly?

compatibility: java version and library usage, backwards compatibility, standards conformance
regression: performance / leaks / thread safety
integration: fit with existing architecture, proper module usage
code health: ip checks, dangerous code

···

On 5 January 2016 at 00:21, Andrea Aime <andrea.aime@anonymised.com> wrote:

Hi folks,
I’m writing to ask if anyone else want to grab the baton and complete this one?
I believe I have done my fair share of effort, but more work is needed to update
the contribution guide, find a good place to both documents (or single shared document) and so on.

Any takers?

Cheers

Andrea

Geoserver-devel mailing list
Geoserver-devel@anonymised.comsts.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel

–
Jody Garnett

On Sat, Dec 26, 2015 at 7:45 PM, Andrea Aime <andrea.aime@anonymised.com> wrote:

Hi,
as you can see, pull requests are going “well”, we have quite a bit of them coming
in all the time, and compared to other projects, relatively small queues and wait times.
Yet, this continuous flow, coupled with the chronic lack of people dedicating
time to review them, makes me believe we need to do something to streamline
this further.

The first choice in a community project would be to throw more “meat” at the problem,
having more reviewers. This has been brought up before, and some unsolicited help
(people that we don’t need to call up by name to get some opinion)
showed up, but imho we are not nearly enough yet.

I am wondering, would it help if we added in the wiki a review guide? Would that
make more people show up to do reviews? If not that, what will?

Another issue is that some come out of the blue and propose
deep changes, or large refactors, making them very hard to review, as in, demanding
the reviewer to literally spend several hours of their spare time on a single pull request
just to handle aestetics/class structure changes, connect the dots and figure
out if the change is actually an improvement or not.
Shall we just add to the list of pull request criterias “keep changes to a minimum, be
mindful of those reviewing them”? Other ideas?

Moving on, there is a number of people making pull request with changes that are
so focused on their specific issue that it’s hard if not impossible to merge the changes,
because they don’t see the big picture and are causing regressions in other use
cases.
Again, maybe we should just write in docs in a more evident way to discuss before
proposing changes, in order to avoid the above? Or ask people to consider
all ways the code they are using can be leveraged, and not just their particular
use case? What else?

Finally, another source of pain is that the core developer doing the review is held responsible
for the changes being merged, while the person proposing the pull request should be instead.
Now, we cannot have these people contributing pull requests be responsible forever,
but could we setup some policy that if anything goes wrong with the pull request, they are supposed
to be around and help fixing the mess, for some amount of time (like, don’t know,
2 to 6 months?). If they are not, the commits just get reverted instead (assuming
it’s possible, sometimes it will not be).
Also, it would be great if we could ask them to sit on the user list to answer of their
changes to the user base, for the same amount of time (right now it’s pretty much
the core dev that merged the pull doing that, which is also unfair).
This would be mostly for new features/large changes, not for bug fixes, although
I would really love to have the responsible be grilled by the angry users when
a so called bug fix causes some important regression.

That’s what I have, any other idea is course more than welcomed

Cheers
Andrea

–

==
GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

Geosolutions’ Winter Holidays from 24/12 to 6/1

Ing. Andrea Aime

@geowolf
Technical Lead

GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

AVVERTENZE AI SENSI DEL D.Lgs. 196/2003

Le informazioni contenute in questo messaggio di posta elettronica e/o nel/i file/s allegato/i sono da considerarsi strettamente riservate. Il loro utilizzo è consentito esclusivamente al destinatario del messaggio, per le finalità indicate nel messaggio stesso. Qualora riceviate questo messaggio senza esserne il destinatario, Vi preghiamo cortesemente di darcene notizia via e-mail e di procedere alla distruzione del messaggio stesso, cancellandolo dal Vostro sistema. Conservare il messaggio stesso, divulgarlo anche in parte, distribuirlo ad altri soggetti, copiarlo, od utilizzarlo per finalità diverse, costituisce comportamento contrario ai principi dettati dal D.Lgs. 196/2003.

The information in this message and/or attachments, is intended solely for the attention and use of the named addressee(s) and may be confidential or proprietary in nature or covered by the provisions of privacy act (Legislative Decree June, 30 2003, no.196 - Italy’s New Data Protection Code).Any use not in accord with its purpose, any disclosure, reproduction, copying, distribution, or either dissemination, either whole or partial, is strictly forbidden except previous formal approval of the named addressee(s). If you are not the intended recipient, please contact immediately the sender by telephone, fax or e-mail and delete the information in this message that has been received in error. The sender does not give any warranty or accept liability as the content, accuracy or completeness of sent messages and accepts no responsibility for changes made after they were sent or for other risks which arise as a result of e-mail transmission, viruses, etc.

–

==
GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

Geosolutions’ Winter Holidays from 24/12 to 6/1

Ing. Andrea Aime

@geowolf
Technical Lead

GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

AVVERTENZE AI SENSI DEL D.Lgs. 196/2003

Le informazioni contenute in questo messaggio di posta elettronica e/o nel/i file/s allegato/i sono da considerarsi strettamente riservate. Il loro utilizzo è consentito esclusivamente al destinatario del messaggio, per le finalità indicate nel messaggio stesso. Qualora riceviate questo messaggio senza esserne il destinatario, Vi preghiamo cortesemente di darcene notizia via e-mail e di procedere alla distruzione del messaggio stesso, cancellandolo dal Vostro sistema. Conservare il messaggio stesso, divulgarlo anche in parte, distribuirlo ad altri soggetti, copiarlo, od utilizzarlo per finalità diverse, costituisce comportamento contrario ai principi dettati dal D.Lgs. 196/2003.

The information in this message and/or attachments, is intended solely for the attention and use of the named addressee(s) and may be confidential or proprietary in nature or covered by the provisions of privacy act (Legislative Decree June, 30 2003, no.196 - Italy’s New Data Protection Code).Any use not in accord with its purpose, any disclosure, reproduction, copying, distribution, or either dissemination, either whole or partial, is strictly forbidden except previous formal approval of the named addressee(s). If you are not the intended recipient, please contact immediately the sender by telephone, fax or e-mail and delete the information in this message that has been received in error. The sender does not give any warranty or accept liability as the content, accuracy or completeness of sent messages and accepts no responsibility for changes made after they were sent or for other risks which arise as a result of e-mail transmission, viruses, etc.

Andrea_Aime4 · January 7, 2016, 7:29am

On Thu, Jan 7, 2016 at 12:20 AM, Jody Garnett <jody.garnett@anonymised.com>
wrote:

I have no surprises in this list, small bits of feedback:

Is it worth grouping to allow the document to be scanned quickly?
- compatibility: java version and library usage, backwards compatibility,
standards conformance
- regression: performance / leaks / thread safety
- integration: fit with existing architecture, proper module usage
- code health: ip checks, dangerous code

I have no strong opinion either way. If you feel is better, go for it,
especially if
you want to get the baton (see my last mail in this thread).

Cheers
Andrea

--

GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

*Geosolutions' Winter Holidays from 24/12 to 6/1*

Ing. Andrea Aime
@geowolf
Technical Lead

GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

*AVVERTENZE AI SENSI DEL D.Lgs. 196/2003*

Le informazioni contenute in questo messaggio di posta elettronica e/o
nel/i file/s allegato/i sono da considerarsi strettamente riservate. Il
loro utilizzo è consentito esclusivamente al destinatario del messaggio,
per le finalità indicate nel messaggio stesso. Qualora riceviate questo
messaggio senza esserne il destinatario, Vi preghiamo cortesemente di
darcene notizia via e-mail e di procedere alla distruzione del messaggio
stesso, cancellandolo dal Vostro sistema. Conservare il messaggio stesso,
divulgarlo anche in parte, distribuirlo ad altri soggetti, copiarlo, od
utilizzarlo per finalità diverse, costituisce comportamento contrario ai
principi dettati dal D.Lgs. 196/2003.

The information in this message and/or attachments, is intended solely for
the attention and use of the named addressee(s) and may be confidential or
proprietary in nature or covered by the provisions of privacy act
(Legislative Decree June, 30 2003, no.196 - Italy's New Data Protection
Code).Any use not in accord with its purpose, any disclosure, reproduction,
copying, distribution, or either dissemination, either whole or partial, is
strictly forbidden except previous formal approval of the named
addressee(s). If you are not the intended recipient, please contact
immediately the sender by telephone, fax or e-mail and delete the
information in this message that has been received in error. The sender
does not give any warranty or accept liability as the content, accuracy or
completeness of sent messages and accepts no responsibility for changes
made after they were sent or for other risks which arise as a result of
e-mail transmission, viruses, etc.

-------------------------------------------------------

[Geoserver-devel] Ways to reduce further the amount of work needed to handle pull requests

== GeoServer Professional Services from the experts! Visit http://goo.gl/it488V for more information.

== GeoServer Professional Services from the experts! Visit http://goo.gl/it488V for more information.

--

GeoServer Professional Services from the experts! Visit http://goo.gl/it488V for more information.

--

GeoServer Professional Services from the experts! Visit http://goo.gl/it488V for more information.

== GeoServer Professional Services from the experts! Visit http://goo.gl/it488V for more information.

--

GeoServer Professional Services from the experts! Visit http://goo.gl/it488V for more information.

--

GeoServer Professional Services from the experts! Visit http://goo.gl/it488V for more information.

--

GeoServer Professional Services from the experts! Visit http://goo.gl/it488V for more information.

--

GeoServer Professional Services from the experts! Visit http://goo.gl/it488V for more information.

--

GeoServer Professional Services from the experts! Visit http://goo.gl/it488V for more information.

== GeoServer Professional Services from the experts! Visit http://goo.gl/it488V for more information.

== GeoServer Professional Services from the experts! Visit http://goo.gl/it488V for more information.

== GeoServer Professional Services from the experts! Visit http://goo.gl/it488V for more information.

--

GeoServer Professional Services from the experts! Visit http://goo.gl/it488V for more information.

== GeoServer Professional Services from the experts! Visit http://goo.gl/it488V for more information.

== GeoServer Professional Services from the experts! Visit http://goo.gl/it488V for more information.

== GeoServer Professional Services from the experts! Visit http://goo.gl/it488V for more information.

== GeoServer Professional Services from the experts! Visit http://goo.gl/it488V for more information.

--

GeoServer Professional Services from the experts! Visit http://goo.gl/it488V for more information.

==
GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

==
GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

==
GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

==
GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

==
GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

==
GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

==
GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

==
GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

==
GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

==
GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.