-
Notifications
You must be signed in to change notification settings - Fork 108
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
Rework database modules and class names #3191
Rework database modules and class names #3191
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.
I only skimmed really quickly, but a few offhand comments...
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 OK to me, for the most part. I would prefer that "metadata" and "server_config" not acquire trailing s
's, and I don't think we need the comments justifying the plural-ness of the table names (and I have a suggestion for ridding us of a Black-ism), but nothing I would stop the merge for.
Now, just so you know, I did not request review on this PR yet. I still have to create a migration and do some additional verification. |
Sorry...the description seemed to imply that we could review this once the previous PR had landed...which is has. 🙂 As long as you don't edit the existing commits, it won't be a problem.... 😉 |
32f6b06
to
7af9dae
Compare
b8a1a45
to
1e2d0e1
Compare
d11537e
to
2181b2d
Compare
2181b2d
to
5472b50
Compare
7031480
to
dfb770b
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.
No major issues; a few comments. One of which you've already resolved in the middle of my review. Ha.
Flakey agent legacy test killed the CI job. |
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.
Wow. That was a lot of change. And, I'm not sure that we got much value out of it.
I have two concerns, both centering on items whose names differ only in the trailing s
. Unfortunately, I don't have a good solution for resolving ServerSetting
vs. ServerSettings
(short of renaming one of them back to "config" or similar, although we could change the latter to ServerSettingAPI
or ServerSettingResource
). However, in the case of the other, auth_tokens
, I think maybe the rename was incomplete -- naming both of them consistently would remove the hazard.
Other than those, I just hit a few nits.
The main goal for this work is consistency in naming surrounding the database models. Most of the code refers to "auth tokens" in various contexts. As such, "active token(s)" names are renamed to "auth token(s)". Related to that, we update other database modules so that they follow the same pattern of singluar module name and plural table names. The plural table names are more natural when constructing SQL queries. We also change the database module model names to follow their table names. One notable change is that the `database.model.server_configs` module has been renamed to `database.model.server_settings`, with the ORM class name renamed from `ServerConfig` to `ServerSetting`. We are making both changes in this commit so that we can provide one database migration for the changes.
129e773
to
87e821c
Compare
Commit message for this PR:
This is work pulled from a series of commits in PR #3114:
The work is not a simple repeat of those commits but a more thorough consideration of them.