Skip to content

Conversation

@ePaul
Copy link
Contributor

@ePaul ePaul commented Mar 28, 2017

PR checklist

  • Read the contribution guidelines.
  • Ran the shell/batch script under ./bin/ to update Petstore sample so that CIs can verify the change.
    I ran bin/run-all-petstore, but before committing I omitted some changes listed below.
  • Filed the PR against the correct branch: master for non-breaking changes.

Description of the PR

No code changes, just updating the samples.

All the changes here come from #5232, which forgot to update the samples.
I omitted the changes covered by #5242, #5241, #5212, as well as changes in typescript-angular2/npm and typescript-jquery/npm, which have the date in their artifact ID (see #3084).

Interesting in the diff might be the several cases which replaced 8080 → 80.

@ePaul
Copy link
Contributor Author

ePaul commented Mar 28, 2017

@wing328 some tests now fail, at least one of the tests for feign. I'll have a look at this later, if you don't want to fix this first.

Failed tests:   testApiClient(io.swagger.client.api.PetApiTest): expected:<.../petstore.swagger.io[]/v2> but was:<.../petstore.swagger.io[:80]/v2>

@ePaul
Copy link
Contributor Author

ePaul commented Mar 28, 2017

I'm on it for the Java-based tests (because I can easily run them locally). Will push the result then for CI.

@ePaul
Copy link
Contributor Author

ePaul commented Mar 28, 2017

These now try to start a server at port 80 (and have no permissions to do so on my computer):

  • samples/server/petstore/jaxrs/jersey1
  • samples/server/petstore/jaxrs/jersey2

<stopWait>10</stopWait>
<httpConnector>
<port>8080</port>
<port>80</port>
Copy link
Contributor Author

@ePaul ePaul Mar 28, 2017

Choose a reason for hiding this comment

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

This change makes the server try to start at port 80, which (at least on normal Linux systems) is not permitted for non-privileged processes.

Same for the jersey2 variant of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #5246 for a possible fix for that.

@ePaul
Copy link
Contributor Author

ePaul commented Mar 28, 2017

I merged the branch of #5246 into this one, so that should be merged first, if it is deemed a plausible solution. (I can rebase this one afterwards, or revert the changes if not deemed okay.)

@wing328
Copy link
Contributor

wing328 commented Mar 29, 2017

@ePaul thanks for fixing all those test cases.

@ePaul ePaul deleted the update-all-samples branch March 29, 2017 17:43
davidgri pushed a commit to davidgri/swagger-codegen that referenced this pull request May 11, 2017
* Updating samples after swagger-api#5232.

* Fix tests after swagger-api#5232.

* Fix Javascript client tests.

* JaxRS server: set serverPort only when not given from outside.

* Update JaxRS sample creator scripts to fix serverPort.

* Preliminary test fix for JaxRS server generators.

* Updating samples for JaxRS with Jersey1/2.

* Updating JaxRS samples again.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants