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

Use new RequestLifecycle api #1527

Merged
merged 9 commits into from
Dec 15, 2022
Merged

Use new RequestLifecycle api #1527

merged 9 commits into from
Dec 15, 2022

Conversation

yawkat
Copy link
Member

@yawkat yawkat commented Dec 9, 2022

Untested, can't get the snapshot to run locally. Most of the code that I removed seems to be identical with RouteExecutor (or, now, RequestLifecycle), however I may have missed some differences.

Untested, can't get the snapshot to run locally. Most of the code that I removed seems to be identical with RouteExecutor (or, now, RequestLifecycle), however I may have missed some differences.
@graemerocher
Copy link
Contributor

@yawkat maybe speak to @melix as to why you can't get the snapshots working. Would prefer that these were verified as passing before merge

@melix
Copy link
Contributor

melix commented Dec 9, 2022

What do you mean by "can't get the snapshots working locally"? From the failing tests it seems to be because we don't have a version of Micronaut Mongodb compatible with Micronaut 4 yet: https://ge.micronaut.io/s/detg73tpbbbcq/tests/:function-aws:test/io.micronaut.function.aws.SquareHandlerSpec/test%20using%20detached%20mock?focused-exception-line=0-0&top-execution=1

@yawkat
Copy link
Member Author

yawkat commented Dec 9, 2022

@melix i can't get gradle to use the local artifact even if i use mavenLocal(), and if i use the code from here: https://github.com/micronaut-projects/micronaut-core/wiki/Gradle-Build-Tips#building-a-module-against-a-local-version-of-micronaut-core , it fails in the checkBom task before the tests run

@melix
Copy link
Contributor

melix commented Dec 9, 2022

So you mean using a local version of Micronaut core?

@yawkat
Copy link
Member Author

yawkat commented Dec 9, 2022

yes. this pr uses new apis

@melix
Copy link
Contributor

melix commented Dec 9, 2022

@yawkat I have updated your PR to use the latest build plugins. By doing this, it will use the snapshot versions of Micronaut from Sonatype snapshot repo.

If I apply this diff:

diff --git a/buildSrc/src/main/groovy/io.micronaut.build.internal.aws-base.gradle b/buildSrc/src/main/groovy/io.micronaut.build.internal.aws-base.gradle
index d69a20fc8..a5026b45d 100644
--- a/buildSrc/src/main/groovy/io.micronaut.build.internal.aws-base.gradle
+++ b/buildSrc/src/main/groovy/io.micronaut.build.internal.aws-base.gradle
@@ -1,4 +1,5 @@
 repositories {
+    mavenLocal()
     mavenCentral()
     maven { url "https://s01.oss.sonatype.org/content/repositories/snapshots/" }
 }

Then it uses my locally built version.

@yawkat
Copy link
Member Author

yawkat commented Dec 9, 2022

thanks. i had also put the mavenLocal in a different place, so that might have been a problem :)

@yawkat
Copy link
Member Author

yawkat commented Dec 9, 2022

yea there are still test failures. i will look on monday

melix and others added 7 commits December 9, 2022 19:15
This test relied on method order before: When no version was supplied, it would pick the first available route. The use of RequestLifecycle changes this to an error (ambiguous route), which is the correct behavior. I've patched the test so that the route choice is unambiguous.
@yawkat
Copy link
Member Author

yawkat commented Dec 10, 2022

There was one bug I fixed here: b71ecdb

And one I fixed in the main route executor PR: micronaut-projects/micronaut-core#8463 (comment)

I've adjusted one failing test in 29ea99d because the behavior is intended.

There are still test failures outside the function-aws-api-proxy module, but they seem to be general micronaut core 4.0 compatibility issues, I don't want to fix them all in this PR.

@yawkat
Copy link
Member Author

yawkat commented Dec 15, 2022

im going to merge this into 4.0.x and continue work in #1499

@yawkat yawkat marked this pull request as ready for review December 15, 2022 09:23
@yawkat yawkat merged commit 44e6446 into 4.0.x Dec 15, 2022
@yawkat yawkat deleted the route-refactor branch December 15, 2022 09:23
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.

None yet

3 participants