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

Added https, statistics and request log support #3

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

tomdz
Copy link

@tomdz tomdz commented Sep 15, 2011

I also updated to jetty 6.1.26 which is the most recent jetty 6 version.

@stormbrew
Copy link
Owner

Hey, @tomdz. Thanks for the patch. I've done a somewhat cleaned up (as in removed intermediary commits) version as the branch upgrade-jetty-and-add-ssl, could you take a look at it and make sure it's still correct as far as you're concerned?

There's a previous patch in that just adds SSL, but I like the other things this one does. I'll get it in asap and push out a new gem once pending an opportunity for the other submitter to give some feedback.

I don't suppose you'd be willing to add some specs for the features added, btw? It'd be nice to have this gem properly regression tested, and you'll probably be able to do a better job of it than me.

@stormbrew
Copy link
Owner

Oh, direct link to the branch: https://github.com/stormbrew/rack-jetty/tree/upgrade-jetty-and-add-ssl

@tomdz
Copy link
Author

tomdz commented Sep 25, 2011

Looks good :) #2 has some additional things though for the trust store, not sure if @lupine needs that.

@lupine
Copy link

lupine commented Sep 27, 2011

Hi,

As it happens, we're not using the truststore at all - I just added it at the time for completeness.

The patch looks fine for me, and would meet our needs perfectly well. Personally, I'd default the stats to being off, to preserve the previous behaviour - otherwise, people who upgrade are going to find a surprise extra feature being turned on, which would be a bit weird. Other than that, I'd be happy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants