Skip to content
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

[feat] Migrating users from config file to database #261

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

haasal
Copy link
Contributor

@haasal haasal commented Aug 21, 2022

A CLI was added to make management of users possible/easier. Currently, it's not possible to manage users through the REST API as this poses a security risk.
This PR is simply a migration from the config file, and additions to the public REST API interface are not planned.
All previous commits but the last three can be ignored as they are part of another ongoing PR #250.
This should be kept as a Draft Request and repulled when #250 gets merged to avoid untidy commit history

@lgtm-com
Copy link

lgtm-com bot commented Aug 21, 2022

This pull request introduces 3 alerts when merging f13005e into bcbfdd0 - view on LGTM.com

new alerts:

  • 2 for Unused import
  • 1 for Except block handles 'BaseException'

@haasal haasal changed the title [feat] Migrating users from config fil to database [feat] Migrating users from config file to database Aug 22, 2022
@haasal
Copy link
Contributor Author

haasal commented Aug 22, 2022

This doesn't work yet with the REST service because for some reason the database object is used in a thread that it wasn't initialized in.

@lgtm-com
Copy link

lgtm-com bot commented Aug 22, 2022

This pull request introduces 2 alerts when merging 0235596 into bcbfdd0 - view on LGTM.com

new alerts:

  • 2 for Unused import

@maxfischer2781
Copy link
Member

This doesn't work yet with the REST service because for some reason the database object is used in a thread that it wasn't initialized in.

Such issues should hopefully go away with the new COBalD version. Everything using asyncio (i.e. practically all of TARDIS) is now initialised in the asyncio/main thread.

@haasal
Copy link
Contributor Author

haasal commented Sep 1, 2022

This doesn't work yet with the REST service because for some reason the database object is used in a thread that it wasn't initialized in.

Such issues should hopefully go away with the new COBalD version. Everything using asyncio (i.e. practically all of TARDIS) is now initialised in the asyncio/main thread.

I've already fixed the issue. I'm just not sure if my solution has a relevant performance overhead.

@maxfischer2781
Copy link
Member

Reconnecting to an SQLite DB for every execute should be fast enough. Also, your design encapsulates the DB enough so that refactoring it to cache a per-thread connection in the future would be possible. 👍

@@ -0,0 +1,88 @@
from importlib.resources import path

Check notice

Code scanning / CodeQL

Unused import

Import of 'path' is not used.
@@ -1,4 +1,5 @@
from .app.security import DatabaseUser
from .app.security import DatabaseUser, LoginUser

Check notice

Code scanning / CodeQL

Unused import

Import of 'LoginUser' is not used.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants