-
Notifications
You must be signed in to change notification settings - Fork 366
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
failing build #707
Comments
as with reducing the minimum requirement for os4j to 1.7; for now a PR for 1.7 LGTM |
Hey team, Personally I think its fair to the community to support Current JDK - 1 version. After hearing a lot of feedback I think thats fair. When JDK9 is officially release we could then drop JDK7. It'd be nice to set the tone with the community and how we decide on JDK versions. We're very close as you mentioned to workaround the JDK7 bug and offer support for both JDK's. As far as betamax vs sorting I almost feel the patch should be in betamax? I could fork it on ContainX so we can control the relationship between OS4J and Betamax. It seems like parameter ordering may be a larger refactor but maybe I'm wrong. How do you feel about this? |
@gondor, I am afraid that forking dependencies for rather trivial fixes might be a slippery slope as you and up maintaining outdated forks with modifications you depend on (making it harder to switch to the upstream again). If I am not mistaken, the order of query parameters can generally have a meaning for the target application so I guess betamax would like to respect the order. Anyway, thanks for looking at JDK7 support. I will try to look at the remaining failures. |
My attempt to fix the travis build while running against both versions of java: #710 |
#710 looks very good to me. As Betamax offers multiple ways to do find out the correct response to a request from a tape there might be a way to exclude the query parameters for now. Makes sense? |
LGTM |
With help of @dhague we figured out what's currently causing the failing build after #693.
Most of the problems got fixed after correcting the path to the tapes-folder by #704, the remaining 3 failures where caused by the different way parameters are appended to a uri using jdk 1.7 and 1.8 .
Apparently Betamax just compares the strings. -.-
For example:
http://devstack.openstack.stack:5000/v3/users?name=foobar&domain_id=default
vshttp://devstack.openstack.stack:5000/v3/users?domain_id=default&name=foobar
.If there's no exact match we get the NPE as seen in the build logs, because betamax is unable to find a response to play back .
After correcting that in the tapes, and still skipping JDK-7157360-affected controllers using
mvn clean verify -pl '!org.pacesys.openstack4j.connectors:openstack4j-http-connector,!org.pacesys.openstack4j.connectors:openstack4j-jersey2,!org.pacesys:it-jersey2'
we get OS4j back to green using JDK 1.7 .Just to be clear: It would still fail using JDK 1.8 as it is sorting parameters differently.
So if we want to get the integration-tests back in both java versions we could either try to fix that in betamax or try to somehow fix the ordering of parameters in os4j itself.
For now I could follow up with a PR which works for 1.7 or 1.8 .
What do you guys think makes most sense here?
@gondor, @dhague, @vinodborole
/cc @olivergondza
The text was updated successfully, but these errors were encountered: