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

GEODE-2014: Upgrade Swagger libraries #265

Closed
wants to merge 4 commits into from
Closed

GEODE-2014: Upgrade Swagger libraries #265

wants to merge 4 commits into from

Conversation

kjduling
Copy link

No description provided.

asfgit pushed a commit that referenced this pull request Oct 19, 2016
@@ -100,8 +100,8 @@ spring-tx.version = 4.3.2.RELEASE
springframework.version = 4.3.2.RELEASE
stephenc-findbugs.version = 1.3.9-1
spymemcached.version = 2.9.0
swagger.version = 1.3.2
swagger-springmvc.version = 0.8.2
swagger.version=1.3.13
Copy link
Contributor

Choose a reason for hiding this comment

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

Verified that the license is still ASL 2.0 so no changes to LICENSE. Also does not include a NOTICE file so we do not need to propagate anything in our NOTICE.

Copy link
Contributor

Choose a reason for hiding this comment

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

However, 1.3.13 is an old Swagger release. I don't think we should upgrade to an old version. If we're going to upgrade then let's upgrade to the latest version (1.5.10?).

@@ -94,7 +94,7 @@ public static void before() throws Exception {
@Test
public void testFunctions() throws Exception {
String json = "{\"@type\":\"double\",\"@value\":210}";

Thread.sleep(500000);
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a really long pause for an integration test. Is there another way to do this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think we should be very adamant about not adding sleeps. Please change this to an Awaitility call that's awaiting whatever condition this sleep is trying to let happen.

Copy link
Author

Choose a reason for hiding this comment

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

Whoops! That shouldn't have gone in. I needed that for hand-testing swagger.

Copy link
Contributor

@kirklund kirklund left a comment

Choose a reason for hiding this comment

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

The long thread sleep should be removed. If there needs to be a pause of some sort there then use Awaitility instead.

Also, 1.3.13 is an older version of Swagger. There are com.wordnik.swagger-core_2.10 (latest version is 1.3.13) and the newer io.swagger.swagger-core (latest version is 1.5.10). I think you should review both versions and determine what it will take to upgrade to io.swagger.

Kevin Duling added 2 commits October 19, 2016 10:38
Remove accidental checkin of sleep call used in testing
Upgrade to the Swagger2 Spec using SpringFox.
@kjduling
Copy link
Author

Added a test to see if Swagger is up and the splash page has the right Geode headers.
Upgraded to the Swagger2 specification
Removed the accidental checkin of the sleep.

assertThat(getCode(response), is(200));

// Check the JSON
response = doRequest(new HttpGet("/geode/v2/api-docs"), "", "");
Copy link
Member

Choose a reason for hiding this comment

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

Just curious, why this is geode/v2, but all the api is still geode/v1

Copy link
Author

Choose a reason for hiding this comment

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

"geode/v2/api-docs" is the endpoint for Swagger2 documentation. Swagger was "geode/api-docs". Geode's endpoints are all "geode/v1/*" and I've verified that if/when the REST API revision is bumped to "v2", there will be no conflict unless we create a new endpoint called "v2/api-docs".

This is supposed to be able to be overridden with the "springfox.documentation.swagger.v2.path" env property, but it doesn't seem to be honored. Or SwaggerConfig is being loaded too late in the lifecycle, which I doubt. At any rate, I am adding the structure to use this, but leaving the values set to the default ones.

@@ -93,8 +93,7 @@ protected String getRestApiVersion() {
response = void.class
)
@ApiResponses({
@ApiResponse(code = 200, message = "OK."),
@ApiResponse( code = 401, message = "Invalid Username or Password." ),
@ApiResponse(code = 200, message = "OK."), @ApiResponse(code = 401, message = "Invalid Username or Password."),
Copy link
Member

Choose a reason for hiding this comment

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

use the old fomat?

Copy link
Author

Choose a reason for hiding this comment

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

Apparently. Mine was from Sept. 9th and it was updated again on the 15th.

Provide framework to override Swagger2 paths
Reformatting
@asfgit asfgit closed this in 892d6d3 Oct 24, 2016
PurelyApplied pushed a commit to PurelyApplied/geode that referenced this pull request Mar 29, 2017
* this closes apache#265

(cherry picked from commit 892d6d3)

GEODE-2014: Upgrade Swagger libraries

* Updated expected jars list after swagger update
* this closes apache#270

(cherry picked from commit 15a5465)

GEODE-2014: Upgrade Swagger libraries

* resolve compiling errors

(cherry picked from commit 15a5465)

GEODE-2014: add the missing license header

(cherry picked from commit 7742981)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants