Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SLD package extension [GEOS-6837] #757

Merged
merged 7 commits into from Feb 22, 2015
Merged

Conversation

josegar74
Copy link
Contributor

A extension to upload with the REST API a zip package with SLD file and related resources (images) to Geoserver.

Code to review.

https://jira.codehaus.org/browse/GEOS-6837

@jdeolive
Copy link
Contributor

Thanks Jose. I wonder though if this could be rolled directly into the core rest interface rather than a community module?

@josegar74
Copy link
Contributor Author

Will manage tomorrow about this, updating the pull request.

@simboss
Copy link
Member

simboss commented Sep 22, 2014

Maybe also adding a few tests ;)

@jdeolive
Copy link
Contributor

Thanks Jose. Sorry if I wasn't clear before but I was thinking we would roll this into the existing styles resource, rather than break out a new url for it. So basically just updated StyleResource to accept a zip file as input on PUT/POST.

@josegar74
Copy link
Contributor Author

Thanks for the feedback, I'll check to update StyleResource and add unit testing.

@aaime
Copy link
Member

aaime commented Oct 5, 2014

Any progress on this one?

@josegar74
Copy link
Contributor Author

Quite busy with other projects and could not manage yet to complete the work on this, hope in coming weeks can have it ready.

@jodygarnett
Copy link
Member

This would be cool, let us know how it goes @josegar74

@aaime
Copy link
Member

aaime commented Dec 8, 2014

Any update? :)

@jodygarnett
Copy link
Member

Without an update this won't make it into the GeoServer 2.7 release...

@mbarto
Copy link
Contributor

mbarto commented Jan 15, 2015

I can try to review the PR and add some tests tomorrow, so that it can be merged before the freeze.

@mbarto
Copy link
Contributor

mbarto commented Jan 16, 2015

Hi created a new pull request for this (#894) with some tests and basic documentation.
I also created a JIRA (http://jira.codehaus.org/browse/GEOS-6837) for it.

@josegar74
Copy link
Contributor Author

Hi

Apologies, as I have been busy with other stuff and could not manage about this.

I started with Justin suggestions, adding some unit testing and refactoring the code in this branch: https://github.com/josegar74/geoserver/commits/sld_package_styleresource , but requires still some checks.

Looks like @mbarto looks has done some improvements of the original code, so I guess can be fine.

@aaime
Copy link
Member

aaime commented Jan 17, 2015

See the thread on geoserver-devel, it appears that Mauro's work does not yet match Justin's original request (it still creates a new url to hit)

@josegar74
Copy link
Contributor Author

I see, I'll check next week to find some time to finish this. Apologies for delays in providing it.

@jdeolive
Copy link
Contributor

Fwiw I don't want to have my feedback be a blocker, if there is a big push
to get it in now as is go for it.

On Saturday, January 17, 2015, josegar74 notifications@github.com wrote:

I see, I'll check next week to find some time to finish this. Apologies
for delays in providing it.


Reply to this email directly or view it on GitHub
#757 (comment).

@aaime
Copy link
Member

aaime commented Jan 17, 2015

Eh, at this point I believe we might be too late, Jody is going to cut 2.7-beta Monday and the feature freeze will start.

@jodygarnett
Copy link
Member

I also noted the new "sld" endpoint and found it a but rough (especially as
we now support multiple styles for css). I am happy to wait until after the
code freeze and back port this REST API change when it is ready.

Jody Garnett

On 17 January 2015 at 09:34, Justin Deoliveira notifications@github.com
wrote:

Fwiw I don't want to have my feedback be a blocker, if there is a big push
to get it in now as is go for it.

On Saturday, January 17, 2015, josegar74 notifications@github.com
wrote:

I see, I'll check next week to find some time to finish this. Apologies
for delays in providing it.


Reply to this email directly or view it on GitHub
#757 (comment).


Reply to this email directly or view it on GitHub
#757 (comment).

<entry>
<key><value>/sld/{workspace}/{style}</value></key>
<value>stylePackageFinder</value>
</entry>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this is the new endpoint, can we do anything more generic (as sld is not the only thing in the mix - see StyleHandler interface).

@jodygarnett
Copy link
Member

Looks like we should move conversation over to #894 and mark this request as closed?

@jodygarnett jodygarnett changed the title SLD package extension SLD package extension [GEOS-6837] Jan 19, 2015
@josegar74
Copy link
Contributor Author

Updated the code to use the the style resource url, added unit tests and documentation.

@@ -94,7 +95,7 @@ Controls a given style.
* - GET
- Return style ``s``
- 200
- SLD, HTML, XML, JSON
- SLD, HTML, XML, JSON, ZIP
- HTML
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think ZIP (SLDPackage) is supported for GET: am I wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SLDPackageHandler.encode manages about this. But maybe is better to just delegate to sldHandler.encode and return the SLD raw file. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, is the GET with application/zip content-type just zipping the sld file? We don't have the images in the file I suppose. Probably it's not worth having this if we are not creating a "package" for GET too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now yes, only the sld. Changed to return the SLD instead. In the future can be improved to return a full package with all related resources if that is useful.

@mbarto
Copy link
Contributor

mbarto commented Feb 20, 2015

Looks good to me with latest changes.

@aaime
Copy link
Member

aaime commented Feb 20, 2015

Nice. We just need to wait the freeze to be over and then we can merge on master (GeoServer 2.8.x)

@jodygarnett
Copy link
Member

I think the feature freeze is over, there is now a master and a 13.x branch
(we only have build problems with the 13.x jobs).

Jody Garnett

On 20 February 2015 at 03:24, Andrea Aime notifications@github.com wrote:

Nice. We just need to wait the freeze to be over and then we can merge on
master (GeoServer 2.8.x)


Reply to this email directly or view it on GitHub
#757 (comment).

@aaime
Copy link
Member

aaime commented Feb 21, 2015

You mean a 2.7.x branch: https://github.com/geoserver/geoserver/tree/2.7.x ;-)

@aaime
Copy link
Member

aaime commented Feb 22, 2015

All right, let's merge then

aaime added a commit that referenced this pull request Feb 22, 2015
SLD package extension [GEOS-6837]
@aaime aaime merged commit 4f0ba02 into geoserver:master Feb 22, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants