-
Notifications
You must be signed in to change notification settings - Fork 8
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
Database Support #107
base: main
Are you sure you want to change the base?
Database Support #107
Conversation
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.
Alright so I looked through everything and it looks good to me. I have not used aerich yet so I didn't want to review those until I fully understand aerich.
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.
Looks pretty good, I'm definitely a fan.
@@ -18,4 +18,4 @@ COPY . . | |||
# install the package using pep 517 | |||
RUN pip install . --no-deps --use-feature=in-tree-build | |||
|
|||
CMD ["python", "-m", "modmail"] | |||
CMD ["sh", "-c", "aerich upgrade && python -m modmail"] |
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 seems like it could be run before the entry point command.
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 is also this option to be able to run aerich: https://github.com/tortoise/aerich#use-aerich-in-application
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 would like to run it manually before running the bot, as you don't always want to run aerich
when you are running the bot. Entry point commands are run when you are building the image and will only be executed once, if the DB gets changed it won't be running again.. so I ran it as CMD
.
```psql | ||
CREATE USER voting WITH SUPERUSER PASSWORD 'modmail'; | ||
CREATE DATABASE modmail WITH OWNER modmail; | ||
``` |
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.
Rather than using the same name for all of them, to make it slightly more clear which is which, could the names be different?
Also, what is voting?
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.
Oops, I got these docs from my personal project... forgot to update them
@@ -27,6 +27,16 @@ | |||
emojis_and_stickers=True, | |||
) | |||
|
|||
TORTOISE_ORM = { |
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.
perhaps this can be exported to a seperate file? This possibly could be in the configuration file, since it is a configuration value.
I don't really like this in bot.py
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.
It is like a constant, not really a configuration file as it is not modifiable.. so not sure. I can insert it directly in Tortoise.init
if you want but I wouldn't be a fan of that.
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 think its a constant which is useful outside of this file, but this doesn't seem like a great place that other files should import that from, which is why I think it should be in a different file.
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.
Useful outside of this file, could you give some examples?
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.
Plugins may want to be able to see the connections and apps.
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.
Plugins would be using self.bot.db
, they would never be interacting with variables, at least I don't see the need to.
This function is ran before the discord modmail bot is run so that the bot is not ran unnecessarily even if the DB is not responding. It creates a local session which can be used throughout the bot and then tests if the db is responding. If the DB is not responding it would log the error with the exception and then exit.
Co-authored-by: Jake <[email protected]>
The changes didn't deserve a different commit
e831686
to
8cf9455
Compare
related_name="tickets", | ||
to_field="id", | ||
) | ||
thread_id = fields.BigIntField(unique=True) |
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.
reference to thread?
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 is no thread model, should it reference to the channel model you asked me to create?
from .stickers import Stickers | ||
|
||
|
||
class Messages(Model): |
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.
channel field.
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.
Why do messages need a channel field? It is already referencing to the modmail ticket it was sent in.
from .tickets import Tickets | ||
|
||
|
||
__all__ = ( |
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.
need a channel model
Will hold classes that implement both discord.abc.Messageable and discord.GuildChannel
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.
Why do we need a channels DB model?
Closes #55
Continuation of #65
This PR adds an async PostgreSQL setup with the help of
tortoise-orm
andaerich
The following models have been added to the DB:TODO