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

[Spring] Add apiFirst option #184

Merged
merged 6 commits into from
Jun 7, 2018
Merged

Conversation

cbornet
Copy link
Member

@cbornet cbornet commented May 30, 2018

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

This PR:

  • adds a apiFirst option that will not generate the API but will configure the openapi-generator-maven-plugin for API-first developpement instead.
  • removes usage of Springfox and serves the OAI spec directly if apiFirst or reactive is set (as Springfox doesn't support webflux yet)
  • removes some generated classes that have no use in Spring (ApiException, ApiOriginFilter, ApiResponseMessage, NotFoundException)
  • fixes configuration of ThreetenBP for Spring-MVC
  • configures the CORS globally (code to uncomment)

@cbornet
Copy link
Member Author

cbornet commented May 30, 2018

cc @bbdouglas (2017/07) @JFCote (2017/08) @sreeshas (2017/08) @jfiala (2017/08) @lukoyanov (2017/09) @cbornet (2017/09) @jeff9finger (2018/01)

@cbornet cbornet force-pushed the spring-api-first branch 3 times, most recently from ffc9c88 to 9c4c594 Compare May 30, 2018 16:30
@jmini
Copy link
Member

jmini commented May 31, 2018

Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.

Are you sure that you did it?

There is an error on Shippable

Shippable — Run 493 status is FAILED.

With this log:

UNCOMMITTED CHANGES ERROR
There are uncommitted changes in working tree after execution of 'bin/ensure-up-to-date'
HEAD detached at FETCH_HEAD
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

	modified:   samples/server/petstore/springboot-reactive/src/main/resources/openapi.yaml

no changes added to commit (use "git add" and/or "git commit -a")
Please run 'bin/ensure-up-to-date' locally and commit changes

The procedure should be, on your PR branch:

  • run mvn verify
  • run bin/springboot-petstore-server-reactive.sh
  • commit the changes

Can you try it (I did not investigate it myself yet).

@cbornet
Copy link
Member Author

cbornet commented May 31, 2018

@jmini No I'm sure I updated the samples. Can you have a look on your side ? I suspect code generated might be different depending on the machine on which the generator is run.

@jmini
Copy link
Member

jmini commented May 31, 2018

Same here. I do not have any changes.

git fetch origin pull/184/head:pull_184
git checkout pull_184
mvn clean verify
bin/springboot-petstore-server-reactive.sh
git status

Gives me this output:

On branch pull_184
nothing to commit, working tree clean

For investigation, can you maybe add a line with git diff after git status in the script:

if [ -n "$(git status --porcelain)" ]; then
echo "UNCOMMITTED CHANGES ERROR"
echo "There are uncommitted changes in working tree after execution of 'bin/ensure-up-to-date'"
git status
echo "Please run 'bin/ensure-up-to-date' locally and commit changes"
exit 1

This way we will see what is going on, on the CI Server.

@cbornet
Copy link
Member Author

cbornet commented May 31, 2018

So the code generation is indeed not fully predictable. See the shippable log. Not sure what to do from there...

@@ -26,7 +26,9 @@ sleep 5
./bin/openapi3/php-petstore.sh

# Check:
echo "Perform git diff"
Copy link
Member

Choose a reason for hiding this comment

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

I would have let this 3 lines (echo, git diff and git status) inside the if... We do not need them in the OK case.

@jmini
Copy link
Member

jmini commented Jun 1, 2018

Thank you for the addition.

Not having stable serialization is really bad. I guess io.swagger.v3.core.jackson.SchemaSerializer in swagger-parser should be improved. I will file an issue in Swagger-Parser.


In the mean time, I suggest that we remove spring from the list of checked samples. Remove this line:

./bin/spring-all-pestore.sh

I prefer the build to be green over additional checks.

@jmini
Copy link
Member

jmini commented Jun 6, 2018

I have proposed a way to make Yaml serialization deterministic with: #233.

I performed some tests locally (I have merged #233 into the branch of this PR). I think it is a solution.

@jmini
Copy link
Member

jmini commented Jun 6, 2018

@cbornet: Now that #233 is merged I wanted to update this PR. In order to do so you need to grant access:

When I try to push to your branch spring-api-first I get:

Repository `cbornet/openapi-generator' is disabled. Please ask the owner to check their account.

You can pick the commit from my pull_184 branch. Just rebase your spring-api-first on top of my pull_184 branch.

@jmini
Copy link
Member

jmini commented Jun 7, 2018

Shippable build is now OK. Thank you @cbornet.

@wing328 wing328 merged commit 7a1945e into OpenAPITools:master Jun 7, 2018
@cbornet cbornet deleted the spring-api-first branch June 7, 2018 06:58
jimschubert added a commit to jimschubert/openapi-generator that referenced this pull request Jun 8, 2018
* master:
  Add 'unblu inc.' to company list (OpenAPITools#246)
  put company list in alphabetical order (OpenAPITools#244)
  [jaxrs-spec] generate spec file (yaml) correctly (OpenAPITools#243)
  [C++] Adjust the names (script, sample folder, generator) to lang option (OpenAPITools#220)
  Add GMO Pepabo to company list (OpenAPITools#242)
  [Spring] Add apiFirst option (OpenAPITools#184)
  [cli] Write to stdout/stderr, allow redirection (OpenAPITools#207)
  [JAVA][Client] New object instead of null for empty POST request (OpenAPITools#98)
  Make yaml serialization deterministic (OpenAPITools#233)
  Add syntax highlighting to migration guide (OpenAPITools#237)
  Fix shippable badge (OpenAPITools#232)
  update company list (OpenAPITools#227)
  Fix OpenAPITools#210: [Ada] Update the code generator for required and optional parameters (OpenAPITools#211)
  Delete unused methods in DefaultCodegen (OpenAPITools#209)
  add note about maven plugins (OpenAPITools#216)
  add raiffeisen to company list (OpenAPITools#223)
  add a remark about homebrew installatio (OpenAPITools#217)
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.

None yet

3 participants