-
Notifications
You must be signed in to change notification settings - Fork 20
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
indexer management client with db connection and cost server #43
Conversation
e796ba0
to
ea26a77
Compare
ea26a77
to
56d2c51
Compare
Pull Request Test Coverage Report for Build 6053871226
💛 - Coveralls |
6bb57b8
to
e30e5cc
Compare
e30e5cc
to
05ca6c0
Compare
05ca6c0
to
3ccc617
Compare
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.
LGTM overall, just a few little things (see code comments).
Things that could be explored later:
- It'd be nice to have some tests on the graphql server, with request/response test vectors.
- Perhaps some migrations could be added only for the purposes of testing. I couldn't build on my machine (sqlx complaining) and had to run a PG instance and create the table myself. Though I'm confused why the CI didn't run into that problem.
- Related to the point above, we could set-up the CI such that there is a PG instance available, to avoid ignoring all the tests using sqlx.
6d33a3d
to
ad3ed6c
Compare
Thanks for the review @aasseman !
Agreed and added an item on #15
Would it be because those tests had
Agreed as well, it would just need to run |
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.
One last thing, perhaps adding a migrations/README.md
that explains that the migrations are there only for testing?
FYI #44 |
ad3ed6c
to
478182b
Compare
Good idea! added in 478182b |
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.
Resolves #19 and #3
some quick notes