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

Kotlin-Server: Optional metrics and upgrade to Ktor 1.5.4 #9358

Merged
merged 1 commit into from
Jun 20, 2021

Conversation

saschpe
Copy link
Contributor

@saschpe saschpe commented Apr 28, 2021

Use Gradle 6.9 and Kotlin 1.4.32. Generate Paths for other HTTP verbs (#828) and fix imports (#5640). Use 'object' when no parameters are used. Introduce 'featureMetrics' to control metrics plugin usage. Remove HOCON configuration parsing. This is provided by Application.environment.config already and removes a dependency.

Resolves #9087, resolves #828, resolves #5640
Relates-To #5346

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh
    ./bin/utils/export_docs_generators.sh
    
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    For Windows users, please run the script in Git BASH.
  • File the PR against the correct branch: master, 5.1.x, 6.0.x
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@jimschubert

@wing328
Copy link
Member

wing328 commented Apr 28, 2021

@saschpe thanks for the PR. Is this change similar to #9088?

Can you have a look at it to see if it meets your requirement as well?

@saschpe
Copy link
Contributor Author

saschpe commented Apr 28, 2021

@saschpe thanks for the PR. Is this change similar to #9088?

Can you have a look at it to see if it meets your requirement as well?

I didn't see this PR initially. It also improves quite a few things but it's not recent enough and misses a few things. Whenever that PR gets merged I'd be happy to rebase this one onto it and fix it.

@saschpe
Copy link
Contributor Author

saschpe commented Apr 28, 2021

Once both PRs are merged (i.e. after review and potential reworks), I will change the serialization mechanism to be configurable in order to allow using kotlinx.serialization.

@wing328
Copy link
Member

wing328 commented Apr 28, 2021

👌 I'll ping you here when #9088 gets merged.

@wing328
Copy link
Member

wing328 commented Apr 28, 2021

I've merged #9088 into 5.2.x branch. Please kindly rebase your work on that and target the PR for 5.2.x branch

@saschpe
Copy link
Contributor Author

saschpe commented Apr 28, 2021

Ok

@saschpe
Copy link
Contributor Author

saschpe commented Apr 28, 2021

Question: master or 5.2.x ?

@wing328
Copy link
Member

wing328 commented Apr 28, 2021

5.2.x (minor release - breaking changes with fallbacks) as that PR was merged into 5.2.x (instead of master)

Current master (patch release - no breaking changes) will be released soon.

@saschpe saschpe changed the title Kotlin-Server: Modernize and upgrade to Ktor 1.5.3 Kotlin-Server: Optional metrics and upgrade to Ktor 1.5.3 Apr 29, 2021
@saschpe saschpe changed the base branch from master to 5.2.x April 29, 2021 08:03
@saschpe
Copy link
Contributor Author

saschpe commented Apr 29, 2021

Rebased and re-targeted towards 5.2.x

@wing328
Copy link
Member

wing328 commented Apr 29, 2021

@saschpe thanks for the update.

cc @jimschubert (2017/09), @dr4ke616 (2018/08) @karismann (2019/03) @Zomzog (2019/04) @andrewemery (2019/10) @4brunu (2019/11) @yutaka0m (2020/03)

@saschpe saschpe changed the title Kotlin-Server: Optional metrics and upgrade to Ktor 1.5.3 Kotlin-Server: Optional metrics and upgrade to Ktor 1.5.4 Apr 30, 2021
@rsinukov
Copy link
Contributor

LGTM from Ktor side.
@wing328 feel free to ping me in the future if you have changes related to the Ktor.

@saschpe
Copy link
Contributor Author

saschpe commented May 4, 2021

The Travis-CI error looks unrelated but I lack the rights to re-build.

@4brunu
Copy link
Contributor

4brunu commented May 4, 2021

It looks unrelated.
To restart the CI you can close and reopen this PR.

@saschpe
Copy link
Contributor Author

saschpe commented May 4, 2021

Uhm, wouldn't it be enough for a project maintainer to press re-build on Travis-CI rather than creating confusion here?

@4brunu
Copy link
Contributor

4brunu commented May 4, 2021

Sure, I was just trying to show you a quick way to restart CI.

@wing328 wing328 modified the milestones: 5.1.1, 5.2.0 May 10, 2021
@saschpe
Copy link
Contributor Author

saschpe commented May 11, 2021

@jimschubert ping

@saschpe
Copy link
Contributor Author

saschpe commented May 20, 2021

Mhm, no real review progress here. Meanwhile, the 5.2.x Branch was merged into master. Shall I rebase this one onto master or keep waiting?

@wing328
Copy link
Member

wing328 commented May 20, 2021

Yes please rebase this one onto master.

I'll try to review tomorrow or this weekend.

@saschpe saschpe changed the base branch from 5.2.x to master May 21, 2021 06:16
@saschpe
Copy link
Contributor Author

saschpe commented May 21, 2021

The Travis-CI error around node_modules/@types/bluebird/index.d.ts(42,47): happens on other branches and merge requests as well.

@4brunu
Copy link
Contributor

4brunu commented May 21, 2021

Thanks, the remaining CI issues are not related to this PR 👍

@wing328
Copy link
Member

wing328 commented May 31, 2021

I got the following when running gradle test with the output:

> Task :compileKotlin
e: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/AppMain.kt: (50, 22): Unresolved reference: HttpClient
e: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/AppMain.kt: (50, 33): Unresolved reference: Apache
e: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/Configuration.kt: (55, 29): Unresolved reference: HttpMethod
e: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/Configuration.kt: (56, 24): Unresolved reference: settings
e: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/Configuration.kt: (57, 28): Unresolved reference: settings
e: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/PetApi.kt: (30, 5): Unresolved reference: authenticate
e: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/PetApi.kt: (32, 30): Unresolved reference: authentication
e: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/PetApi.kt: (32, 55): Unresolved reference: OAuthAccessTokenResponse
e: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/PetApi.kt: (61, 5): Unresolved reference: authenticate
e: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/PetApi.kt: (63, 30): Unresolved reference: authentication
e: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/PetApi.kt: (63, 55): Unresolved reference: OAuthAccessTokenResponse
e: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/PetApi.kt: (69, 5): Unresolved reference: authenticate
e: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/PetApi.kt: (71, 30): Unresolved reference: authentication
e: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/PetApi.kt: (71, 55): Unresolved reference: OAuthAccessTokenResponse
e: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/PetApi.kt: (100, 5): Unresolved reference: authenticate
e: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/PetApi.kt: (102, 30): Unresolved reference: authentication
e: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/PetApi.kt: (102, 55): Unresolved reference: OAuthAccessTokenResponse
e: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/PetApi.kt: (131, 5): Unresolved reference: authenticate
e: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/PetApi.kt: (133, 30): Unresolved reference: authentication
e: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/PetApi.kt: (162, 5): Unresolved reference: authenticate
e: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/PetApi.kt: (164, 30): Unresolved reference: authentication
e: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/PetApi.kt: (164, 55): Unresolved reference: OAuthAccessTokenResponse
e: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/PetApi.kt: (193, 5): Unresolved reference: authenticate
e: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/PetApi.kt: (195, 30): Unresolved reference: authentication
e: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/PetApi.kt: (195, 55): Unresolved reference: OAuthAccessTokenResponse
e: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/PetApi.kt: (201, 5): Unresolved reference: authenticate
e: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/PetApi.kt: (203, 30): Unresolved reference: authentication
e: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/PetApi.kt: (203, 55): Unresolved reference: OAuthAccessTokenResponse
e: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/StoreApi.kt: (33, 5): Unresolved reference: authenticate
e: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/StoreApi.kt: (35, 30): Unresolved reference: authentication
e: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/UserApi.kt: (29, 5): Unresolved reference: authenticate
e: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/UserApi.kt: (31, 30): Unresolved reference: authentication
e: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/UserApi.kt: (37, 5): Unresolved reference: authenticate
e: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/UserApi.kt: (39, 30): Unresolved reference: authentication
e: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/UserApi.kt: (45, 5): Unresolved reference: authenticate
e: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/UserApi.kt: (47, 30): Unresolved reference: authentication
e: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/UserApi.kt: (53, 5): Unresolved reference: authenticate
e: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/UserApi.kt: (55, 30): Unresolved reference: authentication
e: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/UserApi.kt: (85, 5): Unresolved reference: authenticate
e: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/UserApi.kt: (87, 30): Unresolved reference: authentication
e: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/UserApi.kt: (93, 5): Unresolved reference: authenticate
e: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/UserApi.kt: (95, 30): Unresolved reference: authentication

> Task :compileKotlin FAILED

Command to generate code:

java -jar modules/openapi-generator-cli/target/openapi-generator-cli.jar generate -g kotlin-server -i modules/openapi-generator/src/test/resources/3_0/petstore.yaml -o /tmp/kotlin-server

Can you please take a look when you've time?

@saschpe
Copy link
Contributor Author

saschpe commented May 31, 2021

I got the following when running gradle test with the output:
[...]
Can you please take a look when you've time?

Should be fixed.

Use Gradle 6.9 and Kotlin 1.4.32. Generate Paths for other HTTP verbs
(OpenAPITools#828) and fix imports (OpenAPITools#5640). Use 'object' when no parameters are
used. Introduce 'featureMetrics' to control metrics plugin usage. Remove
HOCON configuration parsing. This is provided by
`Application.environment.config already` and removes a dependency.

Resolves OpenAPITools#9087, resolves OpenAPITools#828, resolves OpenAPITools#5640
Relates-To OpenAPITools#5346
@saschpe
Copy link
Contributor Author

saschpe commented Jun 11, 2021

Anything left to do here?

@saschpe
Copy link
Contributor Author

saschpe commented Jun 18, 2021

ping

@wing328
Copy link
Member

wing328 commented Jun 20, 2021

Tested again and it's now built successfully:

$ gradle build
Starting a Gradle Daemon (subsequent builds will be faster)

> Task :compileKotlin
w: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/PetApi.kt: (33, 13): Variable 'principal' is never used
w: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/PetApi.kt: (64, 13): Variable 'principal' is never used
w: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/PetApi.kt: (72, 13): Variable 'principal' is never used
w: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/PetApi.kt: (103, 13): Variable 'principal' is never used
w: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/PetApi.kt: (134, 13): Variable 'principal' is never used
w: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/PetApi.kt: (165, 13): Variable 'principal' is never used
w: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/PetApi.kt: (196, 13): Variable 'principal' is never used
w: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/PetApi.kt: (204, 13): Variable 'principal' is never used
w: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/StoreApi.kt: (36, 13): Variable 'principal' is never used
w: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/UserApi.kt: (32, 13): Variable 'principal' is never used
w: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/UserApi.kt: (40, 13): Variable 'principal' is never used
w: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/UserApi.kt: (48, 13): Variable 'principal' is never used
w: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/UserApi.kt: (56, 13): Variable 'principal' is never used
w: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/UserApi.kt: (88, 13): Variable 'principal' is never used
w: /private/tmp/kotlin-server/src/main/kotlin/org/openapitools/server/apis/UserApi.kt: (96, 13): Variable 'principal' is never used

Deprecated Gradle features were used in this build, making it incompatible with Gradle 8.0.
Use '--warning-mode all' to show the individual deprecation warnings.
See https://docs.gradle.org/7.0.2/userguide/command_line_interface.html#sec:command_line_warnings

@wing328 wing328 merged commit 463d905 into OpenAPITools:master Jun 20, 2021
@saschpe saschpe deleted the kotlin-server-modernize branch June 25, 2021 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants