Skip to content

[OSGi] Enable Swagger#56

Merged
asfgit merged 2 commits intoapache:masterfrom
neykov:osgi/swagger
Mar 30, 2016
Merged

[OSGi] Enable Swagger#56
asfgit merged 2 commits intoapache:masterfrom
neykov:osgi/swagger

Conversation

@neykov
Copy link
Member

@neykov neykov commented Mar 10, 2016

Note that this PR bumps the guava version, so might need wider community discussion!

@ahgittin
Copy link
Contributor

Apart from guava and felix version bumps this looks good. Felix version bump seems fine. Guava however...

Have you checked what from 16 (jclouds) and 17 (brooklyn) is incompatible with 18 (swagger) -- or even 19 (latest) ?

Alternatively it looks like the deps in the rest-swagger/pom.xml pointing at brooklyn are never used. So maybe we could remove them altogether and in the OSGi build allow it to take the guava version it wants even if different to the version brooklyn uses. (This strategy is probably more important for jclouds -- and if we had jclouds getting the guava bundle version it wants I'd be relaxed about Brooklyn taking any version.)

@ahgittin
Copy link
Contributor

test failure looks related:

2016-03-11 08:40:51,463 INFO  TESTNG FAILED: "Surefire test" - org.apache.brooklyn.core.mgmt.osgi.OsgiVersionMoreEntityTest.testMoreEntitiesV2FailsWithoutBasicTestOsgiEntitiesBundle() finished in 47 ms
java.lang.AssertionError: Missing expected text in error: org.apache.brooklyn.util.exceptions.CompoundRuntimeException: Unable to instantiate org.apache.brooklyn.test.osgi.entities.more.MoreEntity; 2 errors including: Transformer for java-type-name gave an error creating this plan: Unable to load org.apache.brooklyn.test.osgi.entities.more.MoreEntity from [OSGi:org.apache.brooklyn.test.osgi.entities.more.MoreEntity:0.1.0[[CatalogBundleDto{symbolicName=null, version=null, url=classpath:/brooklyn/osgi/brooklyn-test-osgi-more-entities_0.2.0.jar}]]]: Unable to resolve class org.apache.brooklyn.test.osgi.entities.more.MoreEntity in CatalogBundleDto{symbolicName=null, version=null, url=classpath:/brooklyn/osgi/brooklyn-test-osgi-more-entities_0.2.0.jar}: ClassNotFoundException: org.apache.brooklyn.test.osgi.entities.more.MoreEntity: BundleException: Unable to resolve org.apache.brooklyn.test.resources.osgi.brooklyn-test-osgi-more-entities [44](R 44.0): missing requirement [org.apache.brooklyn.test.resources.osgi.brooklyn-test-osgi-more-entities [44](R 44.0)] osgi.wiring.package; (osgi.wiring.package=org.apache.brooklyn.test.osgi.entities) Unresolved requirements: [[org.apache.brooklyn.test.resources.osgi.brooklyn-test-osgi-more-entities [44](R 44.0)] osgi.wiring.package; (osgi.wiring.package=org.apache.brooklyn.test.osgi.entities)]: ClassNotFoundException: org.apache.brooklyn.test.osgi.entities.more.MoreEntity expected [true] but found [false]
    at org.apache.brooklyn.core.mgmt.osgi.OsgiVersionMoreEntityTest.testMoreEntitiesV2FailsWithoutBasicTestOsgiEntitiesBundle(OsgiVersionMoreEntityTest.java:285)

@ahgittin
Copy link
Contributor

simple to fix, i guess the felix error messages have changed.

change OsgiVersionMoreEntityTest:282 from

    try {
        Entity me = addItemFromCatalog(c2);
        Assert.fail("Should have failed, with unresolved dependency; instead got "+me);
    } catch (Exception e) {
        Assert.assertTrue(e.toString().toLowerCase().contains("unresolved constraint"), "Missing expected text in error: "+e);
        Assert.assertTrue(e.toString().toLowerCase().contains("wiring.package"), "Missing expected text in error: "+e);
        Assert.assertTrue(e.toString().toLowerCase().contains("org.apache.brooklyn.test.osgi.entities"), "Missing expected text in error: "+e);
    }

to

    try {
        Entity me = addItemFromCatalog(c2);
        Asserts.shouldHaveFailedPreviously("Should have failed, with unresolved dependency; instead got: "+me);
    } catch (Exception e) {
        Asserts.expectedFailureContainsIgnoreCase(e, "unresolved", "wiring.package", "org.apache.brooklyn.test.osgi.entities");
    }

@neykov
Copy link
Member Author

neykov commented Mar 11, 2016

Had the same failure this morning, but after pulling latest changes and rebuilding it went away.

@neykov
Copy link
Member Author

neykov commented Mar 11, 2016

Forcing a rebuild.

@neykov neykov force-pushed the osgi/swagger branch 3 times, most recently from 2e49021 to b102ed2 Compare March 17, 2016 10:17
@neykov
Copy link
Member Author

neykov commented Mar 17, 2016

Re guava version change - haven't looked at the specific API changes between versions, but would expect that they would lead to a compile error if something used by Brooklyn changed. Will take a look at the release notes to confirm.

I suggest that we follow the jclouds lead and stick to the least common denominator API between guava versions. Then we can leave the v17 dependency in the pom, but load 16-18 in Karaf, as we see fit.

@neykov
Copy link
Member Author

neykov commented Mar 17, 2016

Loading both guava 17 & guava 18 worked out fine, sticking to this solution.

Leaving brooklyn to depend on 17.

</execution>
</executions>
</plugin>-->
<plugin>
Copy link
Contributor

Choose a reason for hiding this comment

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

nice that this can be removed now

@ahgittin
Copy link
Contributor

is guava 18 included in the osgi build? maybe worth a check it's not being too smart and trying to fetch that from the internet if not in the local maven cache.

alternatively does the swagger stuff work w the same version as brooklyn?

apart from that looks good

ahgittin added a commit to ahgittin/brooklyn-server that referenced this pull request Mar 28, 2016
Conflicts:
	rest/rest-resources/src/main/resources/OSGI-INF/blueprint/service.xml
	rest/rest-server-jersey/pom.xml

Both simple to resolve -- uncomment apidoc and remove the duplicates there, and keep both changes in other conflicting places.
@ahgittin
Copy link
Contributor

this conflicts w #55 but it's easy to resolve; done so in ahgittin@b49bb07 (which i will use when we merge this)

@neykov
Copy link
Member Author

neykov commented Mar 28, 2016

Yes, guava is included and swagger works with the newer version.

@asfgit asfgit merged commit 17044e8 into apache:master Mar 30, 2016
@neykov neykov deleted the osgi/swagger branch March 30, 2016 14:56
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.

3 participants