User Management backend - Base - DB support #38
Conversation
Added config file so the app will auto configured once the flask app starts.
Added triggers to Vagrantfile for up and destroy commands. After vagrant up is executed an attempt to restore db data accures. Before vagrant destroy is executed a save to db data accures.
|
Unfortunately GitHub makes it close to impossible to properly review PRs that depend on other PRs because it shows all the changes mixed together instead of letting you see only the changes in the relevant PR. So we'll need to merge the first PR before we can review the ones that depend on it (This is why I don't personally consider GitHub to be a serious code review platform). |
edison/config.py
Outdated
| # Iterating through all config.py members | ||
| for name, obj in inspect.getmembers(sys.modules[__name__]): | ||
| # We're interested only with the derived classes of the Config class | ||
| if inspect.isclass(obj) and name != "Config": |
There was a problem hiding this comment.
Do you care that it is a class inherited from Config, or any class as long as its not Config?
If you want to know if it inherits from Config you can use:
issubclass(obj, Config)
Note this will return true for Config itself so you still need your code and name != "Config"
There was a problem hiding this comment.
Since the addition of a DB and an ORM layer is complex enough, I'd move the Token and User models to another PR (You can keep a stub implemetation of the user mode for testing purposes, but move the majority of the code elsewhere).
Also I'd like to see some tests that verify that the DB and the ORM work - think about how you would work with a DB in a test scenario while maintaining isolation between tests. I estimate you would probably need a fixture function that initialized and destroys a test DB schema.
| Vagrant.configure("2") do |config| | ||
| config.trigger.before :destroy do |trigger| |
There was a problem hiding this comment.
I'm not sure it really makes sense to keep the database data if you destroy the environment. And it also looks like not doing that would enable you to remove alot of code...
You do probably need to make sure that if you run vagrant up after you did vagrant halt (without destroy), you don't end up clearing the DB by mistake...
There was a problem hiding this comment.
I'm not sure it really makes sense to keep the database data if you destroy the environment. And it also looks like not doing that would enable you to remove alot of code...
Don't we want to be on the safe side by saving the DB data ? what if by mistake someone performs vagrant destroy ?
You do probably need to make sure that if you run vagrant up after you did vagrant halt (without destroy), you don't end up clearing the DB by mistake...
I tested it and the data inside the DB remained unchanged.
I also removed db.sql content to see if it's doing anything, then (after vagrant halt) I executed vagrant up and still the data remained.
There was a problem hiding this comment.
Don't we want to be on the safe side by saving the db data ? what if by mistake someone perform
vagrant destroy?
not really - Vagrant is your development environment, I don't think being too careful there is worth the extra effort and code. Also consider that if you're going to run tests with the database, you'll probably want it to be empty to begin with so the tests get a predictable initial state.
I tested it and the data inside the DB remained unchanged.
I also removed db.sql content to see if it's doing anything, then (aftervagrant halt) I executedvagrant upand still the data remained.
ok, you need to make sure this remains the case if/when you remove the backup and restore code.
There was a problem hiding this comment.
not really - Vagrant is your development environment, I don't think being too careful there is worth the extra effort and code. Also consider that if you're going to run tests with the database, you'll probably want it to be empty to begin with so the tests get a predictable initial state.
Make sense. We'll remove that code :)
ok, you need to make sure this remains the case if/when you remove the backup and restore code.
OK
There was a problem hiding this comment.
Also consider that if you're going to run tests with the database, you'll probably want it to be empty to begin with so the tests get a predictable initial state.
@ifireball
I agree that we don't gain much for saving the DB before 'vagrant destroy', but I don't understand how does it affect the tests?
I thought we need to create separate DB for the tests
There was a problem hiding this comment.
@ifireball
I agree that we don't gain much for saving the DB before 'vagrant destroy', but I don't understand how does it affect the tests?
I thought we need to create separate DB for the tests
Well, using a separate DB is probably the safest way to use a DB with tests, but:
- It takes a considerable amount of work to make a test fixture that properly handles creating and destroying of DB schemas
- It takes a lot of discipline to ensure all the App code lets the tests determine which DB is used
- Creating and destroying DB schemas before/after each test can have a significant performance impact on the tests.
Some often a "close approximation" where you get ~80% of the benefit with ~20% of the effort is worthwhile. So its possible to have the development DB be shared between the tests as long as the initial conditions are controlled and the tests know how to cleanup after themselves.
Deleted Token model and some of the logic in User model 👍 |
fixes #27.
This PR depends on PR #37.
Added Flask-SQLAlchemy to the project.
Added vagrant triggers to Vagrantfile for up and destroy commands.
@ahinoamta @RoniRush