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

chore(deps): Updated Kotlin from v1.7.0 to v1.9.10, Gradle from v6.7.1 to v8.3 and upgraded linter #1343

Conversation

daniel-lopes-optimizely
Copy link
Contributor

This PR upgrades:

  • the Gradle version - the build files were rewritten in Kotlin for type safety
  • the Kotlin version, from 1.7.x to 1.9.x
  • the Kotlin coroutine library version, from release candidate to the latest stable version
  • the code linting gradle plugin and ktlint version, which prompted a code reformat

Please have a look and let me know how I can further support this PR.

Developer Checklist ℹ️

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Appropriate docs were updated (if necessary)

Fixes #1277 🦕

@@ -3207,11 +3011,10 @@ class LookerSDK(authSession: AuthSession) : APIMethods(authSession) {
*/
fun create_content_favorite(
body: WriteContentFavorite
) : SDKResponse {
): SDKResponse {
Copy link
Member

Choose a reason for hiding this comment

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

This all LGTM but want to get @drstrangelooker's opinion on formatting the autogen files.

When the SDK is regenerated for the next Looker release we'll probably need to rerun the Kotlin formatter. Is it possible to make that part of the gen task?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have no idea about formatting. Sorry

@daniel-lopes-optimizely daniel-lopes-optimizely force-pushed the kotlin-gradle-upgrade branch 4 times, most recently from c887d38 to e3e1463 Compare August 24, 2023 08:18
@daniel-lopes-optimizely daniel-lopes-optimizely changed the title chore(deps): Updated Kotlin from v1.7.0 to v1.9.0, Gradle from v6.7.1 to v8.2.1 and upgraded linter chore(deps): Updated Kotlin from v1.7.0 to v1.9.10, Gradle from v6.7.1 to v8.3 and upgraded linter Aug 24, 2023
@tjbanghart
Copy link
Member

Thanks for the PR @daniel-lopes-optimizely!

I am seeing the same issue described in #1291 (comment) when trying to build in both Linux and Mac OS. I believe this is due to changes with how gradle locates JVM toolchains.

It seems like the org.gradle.toolchains.foojay-resolver-convention plugin should help with this but I have not had luck adding it to the settings. I opened gradle/foojay-toolchains#48 to ask for some assistance.

In #1345 I was able to change the gradle version with no problem so perhaps some of the other changes are causing this.

kotlin/build.gradle.kts Outdated Show resolved Hide resolved
@daniel-lopes-optimizely
Copy link
Contributor Author

Hi @tjbanghart , I wasn't having those issues on my M1 mac. Just in case I added the foojay-resolver-convention plugin to the settings and according to the support response, there's no need to do anything else other than applying the plugin.

I also bumped the jvm toolchain version to 17 to keep up with updates, but if you feel that we need 11 to keep backwards compatibility, that's fine too.

tjbanghart
tjbanghart previously approved these changes Sep 25, 2023
Copy link
Member

@tjbanghart tjbanghart left a comment

Choose a reason for hiding this comment

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

Thank you! LGTM

@daniel-lopes-optimizely
Copy link
Contributor Author

Hi @tjbanghart would you know when this PR can be merged?

@drstrangelooker drstrangelooker changed the base branch from main to kotlin-gradle-upgrade October 12, 2023 17:33
@drstrangelooker drstrangelooker merged commit f21fdd2 into looker-open-source:kotlin-gradle-upgrade Oct 12, 2023
11 of 12 checks passed
@drstrangelooker
Copy link
Collaborator

The checks can't run from a forked repository. I'm merging it to a branch in this repo, and then we will reun the checks locally.

drstrangelooker pushed a commit that referenced this pull request Oct 12, 2023
…1 to v8.3 and upgraded linter (#1343)

* chore(deps): Updated Kotlin from v1.7.0 to v1.9.10, Gradle from v6.7.1 to v8.3 and upgraded linter

* chore(deps): upgraded kotlin linter formatted code

* chore(deps): Updated Kotlin from v1.7.0 to v1.9.10, Gradle from v6.7.1 to v8.3 and upgraded linter

adding foojay-resolver-convention gradle plugin

* chore(deps): Updated Kotlin from v1.7.0 to v1.9.10, Gradle from v6.7.1 to v8.3 and upgraded linter

setting gradle jvm toolchain version from 8 to 17

* chore(deps): upgraded kotlin linter formatted code
drstrangelooker added a commit that referenced this pull request Oct 23, 2023
#1379)

…1 to v8.3 and upgraded linter (#1343)

* chore(deps): Updated Kotlin from v1.7.0 to v1.9.10, Gradle from v6.7.1
to v8.3 and upgraded linter

* chore(deps): upgraded kotlin linter formatted code

* chore(deps): Updated Kotlin from v1.7.0 to v1.9.10, Gradle from v6.7.1
to v8.3 and upgraded linter

adding foojay-resolver-convention gradle plugin

* chore(deps): Updated Kotlin from v1.7.0 to v1.9.10, Gradle from v6.7.1
to v8.3 and upgraded linter

setting gradle jvm toolchain version from 8 to 17

* chore(deps): upgraded kotlin linter formatted code

from #1343
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade Kotlin, Koroutines, Gradle, Formatting
3 participants