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

[Java:vertx] Initialize router in init method and re-use router member to create SwaggerRouter #7234

Merged
merged 3 commits into from
Jan 7, 2018

Conversation

ccozzolino
Copy link
Contributor

@ccozzolino ccozzolino commented Dec 20, 2017

…waggerRouter

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: 3.0.0 branch for changes related to OpenAPI spec 3.0. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

Modify MainApiVerticle.mustache template to properly initialize main router and re-use the router to create SwaggerRouter - fix for #7230

@ccozzolino
Copy link
Contributor Author

No technical committee member is listed for Vertx, tagging @phiz71 since he committed initial Vertx templates.

@ccozzolino ccozzolino changed the title Initialize router in init method and re-use router member to create S… [Java:vertx] Initialize router in init method and re-use router member to create S… Dec 21, 2017
@ccozzolino
Copy link
Contributor Author

ccozzolino commented Dec 22, 2017

Originally copied @phiz71 since he was initial contributor of Vertx support (and no technical committee member listed for Vertx), also copying larger Java technical committee: @bbdouglas (2017/07) @JFCote (2017/08) @sreeshas (2017/08) @jfiala (2017/08) @lukoyanov (2017/09) @cbornet (2017/09)

@ccozzolino
Copy link
Contributor Author

@wing328 - any recommendation on a proper resource to review these changes?

@wing328
Copy link
Contributor

wing328 commented Dec 27, 2017

Thanks for the PR but your commit (as shown in the Commits tab) is not linked to your Github account, which means this PR won't count as your contribution in https://github.com/swagger-api/swagger-codegen/graphs/contributors.

Let me know if you need help fixing it.

Ref: https://github.com/swagger-api/swagger-codegen/wiki/FAQ#how-can-i-update-commits-that-are-not-linked-to-my-github-account

@ccozzolino ccozzolino reopened this Dec 27, 2017
@ccozzolino
Copy link
Contributor Author

ccozzolino commented Dec 27, 2017

@wing328 - Thanks for catching that my account was not linked. I've adjusted my PR and the account should now be linked correctly.

It's not clear to me why the circleci check is failing, can you please advise?

@wing328 wing328 closed this Dec 28, 2017
@wing328 wing328 reopened this Dec 28, 2017
@wing328
Copy link
Contributor

wing328 commented Dec 28, 2017

That's the error:

Warning: Permanently added 'github.com,192.30.253.113' (RSA) to the list of known hosts.

fatal: Could not parse object 'f767d456131c29e558e660593b26159ce2d6a844'.

Could not find commit f767d456131c29e558e660593b26159ce2d6a844 in the repo

It could be an issue with CircleCI. I'll restart it later today.


@Override
public void init(Vertx vertx, Context context) {
super.init(vertx, context);
Copy link
Contributor

Choose a reason for hiding this comment

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

Align with 4 spaces

super.init(vertx, context);
router = Router.router(vertx);
}

@Override
public void start(Future<Void> startFuture) throws Exception {
Json.mapper.registerModule(new JavaTimeModule());
FileSystem vertxFileSystem = vertx.fileSystem();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you realign this ?

@ccozzolino
Copy link
Contributor Author

@wing328 - looks like circleci issue is resolved. Thanks!

@ccozzolino
Copy link
Contributor Author

@wing328 - are we okay to merge this now?

@wing328
Copy link
Contributor

wing328 commented Jan 5, 2018

@ccozzolino thanks for the PR.

Does this PR need to urgently go into 2.3.1 which will be released next week, or it's ok to wait till 2.4.0 release?

@ccozzolino
Copy link
Contributor Author

@wing328 - I'd really like to get this into 2.3.1; however, if that complicates your release schedule, then delaying it to 2.4.0 is not a problem.

Thanks again for all your work on this.

@wing328 wing328 modified the milestones: v2.4.0, v2.3.1 Jan 7, 2018
@wing328 wing328 merged commit d974596 into swagger-api:master Jan 7, 2018
@wing328
Copy link
Contributor

wing328 commented Jan 7, 2018

@ccozzolino thanks for the PR, which has been merged into master (2.3.1).

I need some help from you to look into the following errors:

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.3:compile (default-compile) on project swagger-java-vertx-server: Compilation failure: Compilation failure:
[ERROR] /private/tmp/swagger-codegen/samples/server/petstore/java-vertx/async/src/main/java/io/swagger/server/api/verticle/UserApiVerticle.java:[30,27] cannot find symbol
[ERROR] symbol:   class UserApiImpl
[ERROR] location: class io.swagger.server.api.verticle.UserApiVerticle
[ERROR] /private/tmp/swagger-codegen/samples/server/petstore/java-vertx/async/src/main/java/io/swagger/server/api/verticle/StoreApiVerticle.java:[26,28] cannot find symbol
[ERROR] symbol:   class StoreApiImpl
[ERROR] location: class io.swagger.server.api.verticle.StoreApiVerticle
[ERROR] /private/tmp/swagger-codegen/samples/server/petstore/java-vertx/async/src/main/java/io/swagger/server/api/verticle/PetApiVerticle.java:[32,26] cannot find symbol
[ERROR] symbol:   class PetApiImpl
[ERROR] location: class io.swagger.server.api.verticle.PetApiVerticle
[ERROR] -> [Help 1]
[ERROR] 

I got these when running "mvn test" under "samples/server/petstore/java-vertx/async"

@wing328 wing328 changed the title [Java:vertx] Initialize router in init method and re-use router member to create S… [Java:vertx] Initialize router in init method and re-use router member to create SwaggerRouter Jan 7, 2018
@ccozzolino
Copy link
Contributor Author

@wing328 - I see the issue. This looks like an issue unrelated to my changes. I incorrectly assumed that the server build ran the individual sample tests which is why I didn't catch this. I'll fix it and submit a new PR.

@wing328
Copy link
Contributor

wing328 commented Jan 8, 2018

@ccozzolino yup, the issue has nothing to do with your change as it was present in master before merging this PR. Thanks for offering help to fix this.

jimschubert added a commit to jimschubert/swagger-codegen that referenced this pull request Jan 10, 2018
* master: (26 commits)
  [Scala] Fix async helper methods when body is optional (swagger-api#7274)
  [Rust] Recommend style based on 'rustfmt' defaults (swagger-api#7335)
  [Java:vertx] Initialize router in init method and re-use router member to create S… (swagger-api#7234)
  [Scala] Fix missing json4s import (swagger-api#7271)
  deploy snapshot version 2.3.1
  [Ada] Add Ada support for server code generator swagger-api#6680 (swagger-api#7256)
  add shijinkui to scala technical committee
  Generate swagger yaml for go client (swagger-api#7281)
  use openjdk7 in travis to ensure it works with jdk7
  docs(readme): update link to contributing guid (swagger-api#7332)
  Fix a regression bug that was introduce in a recent commit. Removed the tabs that were causing error in Play Framework (swagger-api#7241)
  Fix issue swagger-api#7262 with the parameter name in the path. The problem was that camelCase naming was forced only in this part of the code when everywhere else it is configurable. (swagger-api#7313)
  Java8 fix (swagger-api#7260)
  update to 2.3.1-SNAPSHOT
  fix typo, update 2017 to 2018
  [Doc] add huawei cloud to companies list swagger-api#7308 (swagger-api#7309)
  Adding Peatio opensource as reference project (swagger-api#7267)
  Update README.md (swagger-api#7298)
  Update README.md (swagger-api#7299)
  [all] sys props in CodegenConstants
  ...
@ccozzolino
Copy link
Contributor Author

"mvn test" issues addressed in #7359

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.

3 participants