Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #322 +/- ##
==========================================
- Coverage 94.54% 88.00% -6.54%
==========================================
Files 63 28 -35
Lines 1594 642 -952
==========================================
- Hits 1507 565 -942
+ Misses 87 77 -10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Here are the last edits:
The test suite is considerably faster than before 😅 Suggestions for review considering the size of the PR (most of the diff is from the poetry.lock though):
Things to add to prevent feature reduction compared to the previous version:
|
frgfm
left a comment
frgfm
left a comment
There was a problem hiding this comment.
Thanks @RonanMorgan! Not sure about the sleep in the make though? I thought we implemented healthcheck and add the --wait so that we don't need to wait after that
Or do you still see persisting problems?
| docker compose -f docker-compose.dev.yml down | ||
| -poetry export -f requirements.txt --without-hashes --with test --output requirements.txt | ||
| -docker compose -f docker-compose.dev.yml up -d --build --wait | ||
| -sleep 30 |
There was a problem hiding this comment.
Isn't the whole point of the --wait and the healthcheck that we don't need to sleep? 😄
There was a problem hiding this comment.
I still have many issues with the database initialization by alembic if I don't add the sleep :/
There was a problem hiding this comment.
it's not consistent, it happens one time out of two
There was a problem hiding this comment.
really? I think we need to investigate that in another PR, sleep is at best a flaky solution :/
There was a problem hiding this comment.
yes or we can push a solution and find a better one later :)
since it seems that there is not so much volunteer currently we can revert if you want but it seems to me that it's better to have something which works even if it not super clean
There was a problem hiding this comment.
My debugging mind feels like I'd actually like to have people with problems so we can find the source. But I agree that I didn't expect you to have any, I consistently get everything up & running from the get go 😅
Feel free to open a PR after that to fix that bug (I imagine you have it on the other commands as well then?)
| async def get_by(self, field_name: str, val: Union[str, int], strict: bool = False) -> Union[ModelType, None]: | ||
| statement: Select = select(self.model).where(getattr(self.model, field_name) == val) | ||
| results = await self.session.execute(statement=statement) | ||
| results = await self.session.exec(statement=statement) # type: ignore |
There was a problem hiding this comment.
Also let's no ignore all typing issues and be specific if we ignore any
Not sure why it was worth changing this line, since we're now ignore mypy on this line? Perhaps safer to address this in a more specific PR (if the only CI problem is with mypy temporarily, I think it's quite acceptable)
There was a problem hiding this comment.
oh yes sorry for that I forgot th precise the mypy error to ignore
| docker compose -f docker-compose.dev.yml up -d --build --wait | ||
| docker compose exec -T backend pytest --cov=app | ||
| docker compose -f docker-compose.dev.yml down | ||
| -poetry export -f requirements.txt --without-hashes --with test --output requirements.txt |
There was a problem hiding this comment.
btw, why are there leading hyphens here?
There was a problem hiding this comment.
leading hyphens are really useful to launch the "docker compose down" even if there are any issue in the commands before
There was a problem hiding this comment.
Oh thanks for teaching me, I felt like I didn't know something there!
Mind opening a PR after that one to adjust this on all relevant make commands?
|
cf #304 |
3 participants
Following up on #304, this PR introduces the following modifications:
data model
update all routes
update dependencies, the end to end test, the CI
simplify docker orchestration by using localstack by default
noticeable changes: the camera aren't "users" anymore, but an admin can create an unexpiring token for cameras (that means they can use a created token, but don't have credentials to log in). This reduces API calls & problems with creds
The only things missing are:
In the following PRs, this is what will be added:
Any feedback is welcome!