Skip to content

[Kotlin][Multiplatform] Fix Kotlin Multiplatform Test #10900

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.

Sign up for GitHub

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

Jump to bottom
Merged
wing328 merged 3 commits into from
Nov 20, 2021
Merged

[Kotlin][Multiplatform] Fix Kotlin Multiplatform Test #10900

wing328 merged 3 commits into from
Nov 20, 2021

Conversation

@tkirshboim
Copy link
Contributor

@tkirshboim tkirshboim commented Nov 19, 2021

PR Description

Fixes #10898

  • Remove obsolete GRADLE_METADATA Gradle preview flag from Kotlin Multiplatform settings.gradle.kts.mustache template file.
  • Update Kotlin Version to 1.5.31.
  • Update coroutines, serialization and ktor versions to the versions recommended for usage with Kotlin 1.5.31 (see https://kotlinlang.org/docs/releases.html#release-details).
  • Add a clean task to the Kotlin Multiplatform template pom.xml file to allow to easily clean the samples folder from previous build outputs.
  • Update the Kotlin Multiplatform sample to reflect the changes to the template.

Testing

To validate that this PR fixes the issue, run the Kotlin Multiplatform test from the project's root folder and see that the test completes successfully.

./mvnw clean integration-test -f samples/client/petstore/kotlin-multiplatform/pom.xml

CCs

Kotlin technical committee:
@jimschubert @dr4ke616 @karismann @Zomzog @andrewemery @4brunu @yutaka0m

People working on previous change #10468 : @shadowsheep1

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.3.0), 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.

@tkirshboim
Fix Kotlin Multiplatform Test
9d89838 
- Remove obsolete 'GRADLE_METADATA' Gradle preview flag from
Kotlin Multiplatform settings.gradle template file.
- Update Kotlin Version to 1.5.31
- Update coroutines, serialization and Ktor versions to the versions
recommended for usage with Kotlin 1.5.31
see https://kotlinlang.org/docs/releases.html#release-details .

Fixes OpenAPITools#10898
4brunu
4brunu reviewed Nov 19, 2021
View reviewed changes
...generator/src/main/resources/kotlin-client/libraries/multiplatform/build.gradle.kts.mustache
val coroutines_version = "1.5.0"
val serialization_version = "1.2.1"
val ktor_version = "1.6.0"
val kotlin_version = "1.5.31"
Copy link
Contributor

@4brunu 4brunu Nov 19, 2021

Choose a reason for hiding this comment

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

@tkirshboim Why not updating to 1.6.0?

Copy link
Contributor Author

@tkirshboim tkirshboim Nov 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@4brunu Jetpack Compose is not yet compatible with Kotlin 1.6.0. Using 1.6.0 would mean that KMM projects that use Compose might have issues using the multiplatform template. Since Compose is a popular UI library on Android, I thought to use 1.5.31, until Compose catches up - something that usually happens quite promptly.

Copy link
Contributor

@4brunu 4brunu Nov 19, 2021

Choose a reason for hiding this comment

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

You are right 👍
Could you just update the serialization_version to 1.3.0, so that we only use stable versions?

4brunu
4brunu reviewed Nov 19, 2021
View reviewed changes
...generator/src/main/resources/kotlin-client/libraries/multiplatform/build.gradle.kts.mustache Outdated
val ktor_version = "1.6.0"
val kotlin_version = "1.5.31"
val coroutines_version = "1.5.2"
val serialization_version = "1.3.0-RC"
Copy link
Contributor

@4brunu 4brunu Nov 19, 2021

Choose a reason for hiding this comment

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

@tkirshboim could you please update this to the stable version 1.3.1?

Copy link
Contributor Author

@tkirshboim tkirshboim Nov 19, 2021

Choose a reason for hiding this comment

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

@4brunu I can do that, but I chose this version because it is the recommended version to use with Kotlin 1.5.31 .

Copy link
Contributor

@4brunu 4brunu Nov 19, 2021

Choose a reason for hiding this comment

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

I think we should use here the stable version 1.3.0.

Copy link
Contributor Author

@tkirshboim tkirshboim Nov 19, 2021

Choose a reason for hiding this comment

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

✅ Done.

4brunu
4brunu reviewed Nov 19, 2021
View reviewed changes
...generator/src/main/resources/kotlin-client/libraries/multiplatform/build.gradle.kts.mustache
val kotlin_version = "1.5.31"
val coroutines_version = "1.5.2"
val serialization_version = "1.3.0-RC"
val ktor_version = "1.6.3"
Copy link
Contributor

@4brunu 4brunu Nov 19, 2021

Choose a reason for hiding this comment

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

@tkirshboim Why not updating to 1.6.5?

Copy link
Contributor Author

@tkirshboim tkirshboim Nov 19, 2021

Choose a reason for hiding this comment

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

@4brunu I chose this version because it is the recommended version to use with Kotlin 1.5.31 .

Copy link
Contributor

@4brunu 4brunu Nov 19, 2021

Choose a reason for hiding this comment

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

Let's leave it the way it is.

@4brunu
Copy link
Contributor

4brunu commented Nov 19, 2021

@tkirshboim Hi, first of all, thanks for contributing with this PR 👍
I left some comments on some possible changes, that I would like to have your feedback.
Thanks 🙂

4brunu
4brunu approved these changes Nov 19, 2021
View reviewed changes
Copy link
Contributor

@4brunu 4brunu left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me 👍

@wing328 wing328 added this to the 5.3.1 milestone Nov 20, 2021
@wing328 wing328 merged commit eeb00e2 into OpenAPITools:master Nov 20, 2021
@tkirshboim tkirshboim deleted the fix-multiplatform-test branch November 20, 2021 07:07
@tkirshboim tkirshboim mentioned this pull request Dec 2, 2021
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@4brunu 4brunu 4brunu approved these changes

Assignees

No one assigned

Labels

Client: Kotlin Enhancement: CI/Test

Projects

None yet

Milestone

5.3.1

Development

Successfully merging this pull request may close these issues.

[BUG][multiplatform] Test fails

3 participants

@tkirshboim @4brunu @wing328