Open
Conversation
Also updates other Scala dependencies to latest versions. To enable this, we drop Scanamo and switch to a custom codec implementation. TODO: write error handling into the dynamodecs inferface
Tidies up a buunch of tech debt and switches to a more rigorously effect-managed approach using the Typelevel ecosystem. Sticks with Scanamo, since it provides an out-the-box integration with CE3.
There was a problem hiding this comment.
Pull Request Overview
This PR migrates the PokerDot application from ZIO to Cats Effect 3 and upgrades to Scala 3. The migration involves:
- Updating build.properties to sbt 1.10.11
- Refactoring lambda handlers to use Cats Effect IO instead of ZIO
- Converting all integration tests to use AsyncIOSpec and IO-based effects
- Updating services and persistence layer to use Cats Effect typeclasses
Reviewed Changes
Copilot reviewed 57 out of 59 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| project/build.properties | Updates sbt version from 1.9.8 to 1.10.11 |
| lambda/src/main/scala/io/adamnfish/pokerdot/Tracing.scala | Creates new Tracing trait for CE3 with implementation |
| lambda/src/main/scala/io/adamnfish/pokerdot/Lambda.scala | Migrates AWS Lambda handler from ZIO to Cats Effect IO |
| lambda/src/main/scala/io/adamnfish/pokerdot/AwsMessaging.scala | Converts messaging service to use Cats Effect |
| integration/src/test/scala/io/adamnfish/pokerdot/integration/*.scala | Migrates all integration tests to AsyncIOSpec |
| devserver/src/main/scala/io/adamnfish/pokerdot/*.scala | Updates dev server components for CE3 compatibility |
| core/src/test/scala/io/adamnfish/pokerdot/*.scala | Updates core tests to use Cats Effect and munit |
| frontend/package.json | Updates Parcel and related dependencies |
Comment on lines
+57
to
+62
| apiGatewayManagementApiClient <- Resource.make(IO { | ||
| ApiGatewayManagementApiClient.builder() | ||
| .region(Region.EU_WEST_1) | ||
| .httpClient(UrlConnectionHttpClient.create()) | ||
| .credentialsProvider(EnvironmentVariableCredentialsProvider.create()) | ||
| .endpointOverride(URI.create(Properties.envOrElse("APIGATEWAY_ENDPOINT", "http://localhost:3001"))) |
There was a problem hiding this comment.
Using a hardcoded localhost endpoint as a default is problematic for production deployments. Consider making this an error if the environment variable is not set, or use a more appropriate default that won't cause connection issues.
Comment on lines
+49
to
+59
| // AWS clients | ||
| dynamoDbClient <- Resource.make(IO { | ||
| DynamoDbAsyncClient.builder() | ||
| .region(Region.EU_WEST_1) | ||
| .httpClient(crtAsyncHttpClient) | ||
| .credentialsProvider(EnvironmentVariableCredentialsProvider.create()) | ||
| .build() | ||
| })(client => IO(client.close())) | ||
| apiGatewayManagementApiClient <- Resource.make(IO { | ||
| ApiGatewayManagementApiClient.builder() | ||
| .region(Region.EU_WEST_1) |
There was a problem hiding this comment.
The AWS region is hardcoded to EU_WEST_1. This should be configurable via environment variables to support deployments in different regions.
| "bet" as 10, | ||
| "pot" as 0, | ||
| ) | ||
| _ =playerDbsPreFlop.get(p3Welcome.playerId).value should have( |
There was a problem hiding this comment.
Missing space between underscore and variable name: should be _ = playerDbsPreFlop
Suggested change
| _ =playerDbsPreFlop.get(p3Welcome.playerId).value should have( | |
| _ = playerDbsPreFlop.get(p3Welcome.playerId).value should have( |
Comment on lines
+25
to
+29
| private val client = DynamoDbAsyncClient.builder() | ||
| .endpointOverride(URI.create("http://localhost:8042")) | ||
| .region(Region.US_EAST_1) // not used for local dynamodb, but required | ||
| .credentialsProvider(StaticCredentialsProvider.create( | ||
| AwsBasicCredentials.create("dummykey", "dummysecret"))) |
There was a problem hiding this comment.
Hardcoded dummy credentials in test code could potentially be copied to production code. Consider using constants or a more obvious test marker.
Suggested change
| private val client = DynamoDbAsyncClient.builder() | |
| .endpointOverride(URI.create("http://localhost:8042")) | |
| .region(Region.US_EAST_1) // not used for local dynamodb, but required | |
| .credentialsProvider(StaticCredentialsProvider.create( | |
| AwsBasicCredentials.create("dummykey", "dummysecret"))) | |
| private val TEST_AWS_ACCESS_KEY_ID = "dummykey" | |
| private val TEST_AWS_SECRET_ACCESS_KEY = "dummysecret" | |
| private val client = DynamoDbAsyncClient.builder() | |
| .endpointOverride(URI.create("http://localhost:8042")) | |
| .region(Region.US_EAST_1) // not used for local dynamodb, but required | |
| .credentialsProvider(StaticCredentialsProvider.create( | |
| AwsBasicCredentials.create(TEST_AWS_ACCESS_KEY_ID, TEST_AWS_SECRET_ACCESS_KEY))) |
| })) | ||
| } | ||
| } | ||
| //package io.adamnfish.pokerdot |
There was a problem hiding this comment.
This entire file is commented out. If it's no longer needed, it should be deleted rather than left as commented code.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Long overdue, will get released when I've a chunk of time to properly test it.
Switch to Scala 3 and CE3.