search

claudyus / LXC-Web-Panel

LXC Web Panel improved for lxc 1.0+
http://claudyus.github.io/LXC-Web-Panel/
Other
221 stars 68 forks source link

Overinstall new debian package resets user database #10

Open robvdl opened 10 years ago

robvdl commented 10 years ago

Since the user database is put directly in the .deb package under /srv/lwp/lwp.db, reinstalling the package, or installing a new version is going to reset the user database each time.

claudyus commented 10 years ago

Hi robvdl, this is a well know problem inherited by original project.

The dirty solution could be to install the database under /etc/lwp but this break any schema changes, for example when API token was introduced any user should create the appropriate tables in it own sqlite db.

Another solution is to migrate from a binary db to plain text configuration inside lwp.conf, as you can see this is yet a task for milestone 0.8.

Any contribute is welcome :+1:

robvdl commented 10 years ago

From a software design perspective, saving users to the DB as it currently is doing seems like to correct solution to me (other than LDAP off course). Saving users to the config file just doesn't seem like the right thing to do I think.

Generally this database (if sticking to sqlite) should go in /var, not /etc which is more for configuration. I remember a few years back I was working at a company where we worked with embedded systems a lot, and it was a problem for example when certain daemons would try to write temporary files to /etc, which we had made readonly to reduce writes to the SD card. My point is, /var is more for variable data and /etc is more for config.

Anyway, wouldn't it be better that the database gets created the first time the app starts, maybe use SQLAlchemy so other databases can be supported as well like Postgres. SQLAlchemy can create the tables when the application starts up for the first time and the database doesn't exist yet. If you want to support schema updates (migrations), Alembic can be used for that.

I wouldn't mind doing this work but lwp.py seems to be getting quite big and it would be nice if things can be refactored a bit, at least move the db/model code to it's own file would be a good start.

robvdl commented 10 years ago

While on the topic of refactoring.

Having both a module called lwp and a lwp.py is a bit of a hack and can be a bit confusing as they have the same namespace, then inside the module lwp/init.py it adds the folder above "../" to sys.path which is the part that is a bit of a hack.

I am going to try something a bit radical at least in a branch on my own repo and move everything into the lwp module instead, then install it using dh_python rather than using the debian/install file, this is the way python modules are normally installed on a debian system, but it does mean the app won't install under /srv anymore but under /usr/share/pyshared

robvdl commented 10 years ago

Check it out: https://github.com/robvdl/LXC-Web-Panel/commit/56913708790f9e597dcef1279d741f10b8386186

This is on a branch on my repo, it's a pretty big/disruptive change though, but it all works.

It's going a little off-topic of the original ticket though and the change is pretty big.

claudyus commented 10 years ago

:+1: I will check it asap!

robvdl commented 10 years ago

Normally you would make the binary pretty small (lwp/bin) with most of the code residing in the lwp module.

Also notice I picked up a missing exception class that wasn't defined, I left a comment in my commit about it. I use PyCharm and it picks up these little issues like that. The other thing I see a bit is "possible variable could not be initialised" that can generally be fixed by a bit of refactoring, making the logic a bit more solid/sound.

robvdl commented 10 years ago

Also one more thing I did, is take out the chdir statement in my upstart script and make a few minor changes so it could, I basically made it so /usr/bin/lwp can be executed from any directory.

Normally the upstart script will kick it off, but "sudo lwp" should also work.

In any way, if you use the upstart script, the logging is pretty cool.

robvdl commented 10 years ago

Ah, and the from future import absolute_import statements are there to make imports behave like Python 3, it helps migration to Python 3 easier later.

Other useful things that can be imported from future are print_function, division, etc.

claudyus commented 10 years ago

Hello Rob, I just push the app-reword branch with relevant commits, please use this branch for any future commits on this topics :)

On the rework itself, I like the idea of SQLAlchemy to support schema migration as well as migration to upstart, but in this case I'm not sure about debian compatibility. Actually I'm using lwp only on Ubuntu 12.04 and 14.04 but drop the support for debian per a logging system is not a great idea.

claudyus commented 10 years ago

Rob, I study your work that is in "app-rework" branch and I realize that such structural reorganization would break compatibility with any other lwp forks around. I would like to cherry-pick some other code around before reorganize the repo as you suggest :D

I'm next to push a "cherry-features" branch with some code soon.