-
Notifications
You must be signed in to change notification settings - Fork 748
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
Distributed error reporting: Setting up the database that stores all the errors #12250
Distributed error reporting: Setting up the database that stores all the errors #12250
Conversation
Build Artifacts
|
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.
Setup looks correct to me! manual tests checkout! @rtibbles any other thoughts?
![Screenshot 2024-06-06 at 21 32 40](https://private-user-images.githubusercontent.com/5203639/337392251-496765c7-e366-4600-9fd8-203623e42b23.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzk1NTg2MDIsIm5iZiI6MTczOTU1ODMwMiwicGF0aCI6Ii81MjAzNjM5LzMzNzM5MjI1MS00OTY3NjVjNy1lMzY2LTQ2MDAtOWZkOC0yMDM2MjNlNDJiMjMucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI1MDIxNCUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNTAyMTRUMTgzODIyWiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9MjkyNTc0NmM2NTJmYWFlYjUzNTZiM2ZmM2VjYjA2YTQ3YWYwMjM4NjI5N2UxZTMyMjAzZjBjNTFlNTJiZjU4NyZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QifQ.S5HHlX6Ox80Nt0EmjITGxTD7gTrUu9zxuCuFtu0Jyyc)
kolibri/core/errorreports/apps.py
Outdated
label = "errorreports" | ||
verbose_name = "Kolibri ErrorReports" | ||
|
||
def ready(self): |
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.
There's probably no use case for this method. However if there is, then its worth changing its body from ...
to pass
that is more explicit
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.
I thought both were the same thing!
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.
Changed to pass!
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.
Pass is more explicit. ... could mean a host of things.
It goes back to my earlier question. Do we really need the ready method?
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.
Not now, some apps inside core had it defined this way with pass. So I just copy-pasted
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.
I thinks it's best we remove it as it won't really be necessary.
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.
Hi @thesujai! Just 1 blocking change, but we should be good for a merge once update is made!
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. Thanks @thesujai!
e5f2b4c
into
learningequality:distributed-error-reporting
Summary
errorreports
app and add it to INSTALLED_APPSerrorreports
database name to the additional sqlite database…
References
Fixes #12244
…
Reviewer guidance
I created a TestModel in errorreports app, which I then migrated and tested in a shell that the creation of objects with that model is written to errorreports.sqlite3 and not any other databases.
…
Testing checklist
PR process
Reviewer checklist
yarn
andpip
)