-
Notifications
You must be signed in to change notification settings - Fork 3
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
fix: Local development & add steps in README.md
#173
Conversation
059ffbc
to
6ffe76d
Compare
9db6969
to
4d56b88
Compare
README.md
WORKDIR /tmp/ | ||
COPY Cargo.toml Cargo.lock ./ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This step caches built dependencies in a docker layer, causing a slower first time build, but faster subsequent builds. We aren't caching docker images in CI, and dev builds don't take too long so this doesn't provide much benefit.
@@ -1,30 +1,16 @@ | |||
FROM rust:1.68 AS build | |||
|
|||
ARG CARGO_BUILD_MODE=release |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default to release builds, but set to debug when building via the local Docker Compose
@@ -74,13 +74,17 @@ const getMessagesFromStream = async <Message extends Record<string, string>>( | |||
return results?.[0].messages as StreamMessages<Message>; | |||
}; | |||
|
|||
const incrementStreamId = (id: string): string => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When lastProcessedId
was null
, this code would generate an invalid ID, causing errors in Redis.
It might also be good to highlight steps for running provisioning to get the databases set up too. |
@darunrs that's no longer required, the indexers will self provision. |
This PR contains various fixes to the local development environment, I've commented inline the reasoning for these changes. On top of this, I've expanded
README.md
to include steps for setting up.