User Management backend - Base #26

DoRTaL94 · 2020-04-11T16:52:23Z

fixes #27.
We created the models we are going to work with and the database handler.
We're going to send 3 more PRs that will include: login, logout and signup logic.
@ahinoamta @RoniRush

… the Restful API

backend/__init__.py

setup.sh

backend/models/response_user.py

setup.sh

simzacks · 2020-04-12T08:54:39Z

please resolve the conflicts in the conflicting files by pulling the latest master, rebasing and fixing

- Added to_json function to User model that exposes less information to the client. - Added types to methods parameters - Added id column to token_blacklist table

lmilbaum

There is not issue linked to this RP.
There are unresolved conversations. Please verify the reviewers resolve them.
This PR is huge. Can you break it to smaller ones?

ahinoamta · 2020-04-15T15:31:33Z

@lioramilbaum

There is not issue linked to this RP - ack.
There are unresolved conversations. Please verify the reviewers resolve them - **We are waiting for @simzacks review 😄 **.
This PR is huge. Can you break it to smaller ones? - The PR already reviewed by Sim, there are few changes according to the CR that require last review. We will be glad if this PR will be approved, and for the next PRs we will pay more attention about the PR size.
@DoRTaL94 @RoniRush

requirements.txt

setup.sh

.gitignore

simzacks · 2020-04-20T06:29:57Z

Vagrantfile

 Vagrant.configure("2") do |config|
  config.vm.box = "ubuntu/bionic64"
  config.vm.provision :shell, path: "setup.sh"
-  config.vm.network :forwarded_port, guest: FLASK_PORT, host: FLASK_PORT
- config.vm.network :forwarded_port, guest: POSTGRESQL_PORT, host: POSTGRESQL_PORT
+  config.vm.network "public_network"


IIUC, this means that the Vagrant machine will get a DHCP IP address when it is started and you will access the app directly from that IP address.
How are you planning on keeping track/publishing of that IP address? Or are you planning on using DYNDNS?

When vagrant start up it shows its public IP inside the console. Should I make its public IP static?

You really shouldn't be connecting your development VM to the public network. It exposes it to the network outside your development machine which is not something you generally want and can cause issues for other developers down the line because the way the machine works can now be affected by the network the developer's machine happens to be connected to...

Oh I see. So making it a private network and set a static IP will be okay?

Setting a static IP can also cause issues if it will happen to clash with an IP in someone's home network. The best approach is to not make any hard assumptions and just let Vagrant allocate an internal dynamic IP automatically.

@ifireball I changed the network definition to private, executed vagrant up and everything went smooth. But when I executed vagrant destroy and vagrant up again I got this issue hashicorp/vagrant#3083 .
The solution I found that worked for me was hashicorp/vagrant#8878 (comment) .
Should I add this to the Vagrantfile ?

Edit 1: To be more exact, after the second vagrant up this command aborted and I got this message:

A host only network interface you're attempting to configure via DHCP already has a conflicting host only adapter with DHCP enabled. The DHCP on this adapter is incompatible with the DHCP settings. Two host only network interfaces are not allowed to overlap, and each host only network interface can have only one DHCP server. Please reconfigure your host only network or remove the virtual machine using the other host only network.

Edit 2: I found another solution for this problem that not need any modification to Vagrantfile - https://www.gitmemory.com/issue/hashicorp/vagrant/3083/502713875 .

I'd really appreciate your advice on how to proceed.

Either of those 2 solutions work, but whichever you do, you should have a comment that references the problem and the solution

Ok. I think the easiest solution, that also won't require the team to fix it manually every time, would be the Vagrantfile solution.

backend/__init__.py

backend/models/token.py

setup.sh

ifireball · 2020-04-21T04:46:57Z

setup.sh

-echo "running flask_init.py"
-export FLASK_APP=/vagrant/flask_init.py
-python3 -m flask run --host=0.0.0.0 >> /vagrant/log.log 2>&1 &
+echo "configuring database"


Executing this kind of SQL manually during the setup stage means you get a new database every time you bring up the system - this might work out in development but not in staging or production.

The technique for managing database configuration over time with Git is called "database migrations" here is a post introducing this concept:
https://rollout.io/blog/database-migration/
The examples there are for Ruby-on-Rails but the same concepts apply to Flask and SQLAlchemy

Here is a DB migration framework you can use with SQLAlchemy:
https://alembic.sqlalchemy.org/en/latest/

Executing this kind of SQL manually during the setup stage means you get a new database every time you bring up the system

Should we create the database manually ? But on the next vagrant up we'll need to create it again... That is why I added those lines to setup.sh so we wouldn't need to do it over and over again.

The technique for managing database configuration over time with Git is called "database migrations"

Doesn't it migrate only the tables schema without the data ? and if so on vagrant destroy all the data will be gone. How does it different from creating a new database on every vagrant up ?

Edit: I've added Flask-Migrate extension to the project that handles SQLAlchemy database migrations for Flask applications using Alembic. It super easy to use and it configures Alembic in the proper way to work with our Flask and SQLAlchemy application.
Regarding of the question I asked:

Doesn't it migrate only the tables schema without the data ? and if so on vagrant destroy all the data will be gone.

I solved this issue of the data being destroyed by saving it inside the vagrant synced folder.
I used vagrant triggers so just before vagrant destroy is executed I save the data and
after vagrant up is executed I restore the data.
But I still need the SQL commands inside setup.sh to be executed so the database will be created.

I squashed some commits so this file is no longer reachable through this commit.
This file could be found here.

Not sure what vagrant workflow you're aiming for, but I think the current solution looks too complex and may not be needed.

They way I think of it, your workflow should include several commands:

A command that brings up a dev environment from scratch on a new machine (Typically the first time you run vagrant up)

A command that resyncs your code changes to the dev environment (May not be needed if your intended workflow is to develop directly on the Vagrant machine). It should probably also do things like:

Run any DB migrations that were not yet applied to the DB

Restart the application server or otherwise have some other way to make it reload the code if it does not do that automatically

A command that shuts down the environment without destroying the data in it (Typically vagrant halt)

A command that brings an existing environment back up (Typically vagrant up when its not the first time you've ran it)

A command that clears up the environment so it can be created from scratch (Typically vagrant destroy)

You can use different combinations of Vagrant commands and scripts to achieve the commands above, but you should have them available because they are all needed in a typical development workflow.

simzacks

LGTM. Just some minor comments.
I'm not familiar with the migration stuff so maybe @ifireball can have a look at that.

.gitignore

Vagrantfile

edison/config.py

All files inside edison/migrations were generated automatically by the extension.

- Deleted forwarded port from Vagrantfile. - Set Vagrantfile network definition to private - 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. - Fixed dhcp configuration conflict of the private network.

ifireball

This PR is becoming too big and it contains many different things:

Restructuring of the Vagrant workflow
An addition a DB service
The addition of an ORM and a Migrations framework
The addition of unit tests
(Finally) the addition of some new models

You should probably think up ways to split it apart....

ifireball · 2020-04-23T02:52:14Z

setup.sh

-echo "running flask_init.py"
-export FLASK_APP=/vagrant/flask_init.py
-python3 -m flask run --host=0.0.0.0 >> /vagrant/log.log 2>&1 &
+echo "configuring database"


Not sure what vagrant workflow you're aiming for, but I think the current solution looks too complex and may not be needed.

They way I think of it, your workflow should include several commands:

A command that brings up a dev environment from scratch on a new machine (Typically the first time you run vagrant up)

A command that resyncs your code changes to the dev environment (May not be needed if your intended workflow is to develop directly on the Vagrant machine). It should probably also do things like:

Run any DB migrations that were not yet applied to the DB

Restart the application server or otherwise have some other way to make it reload the code if it does not do that automatically

A command that shuts down the environment without destroying the data in it (Typically vagrant halt)

A command that brings an existing environment back up (Typically vagrant up when its not the first time you've ran it)

A command that clears up the environment so it can be created from scratch (Typically vagrant destroy)

You can use different combinations of Vagrant commands and scripts to achieve the commands above, but you should have them available because they are all needed in a typical development workflow.

ifireball · 2020-04-23T02:55:07Z

edison/migrations/env.py

@@ -0,0 +1,96 @@
+from __future__ import with_statement


was this file and others like it auto-created? you should document the processes for creating/updating them and what they are used for.

Yes, all files inside edison/migrations folder was auto created by Flask-Migrate extension.
I'll document what you suggested inside the README file in the migrations folder.

ifireball · 2020-04-23T02:55:55Z

edison/migrations/README

@@ -0,0 +1 @@
+Generic single-database configuration.


you probably need to add more information here so your peers can learn what is the migratins directory used for and how to use/update it.

ifireball · 2020-04-23T02:57:36Z

edison/config.py

+    JWT_BLACKLIST_TOKEN_CHECKS = ['access', 'refresh']
+    SQLALCHEMY_DATABASE_URI = 'postgresql://postgres:[email protected]/edison'
+
+class ProductionConfig(Config):


you should probably include a comment here that at last the connection string should be update once an actual production environment is established.

ifireball · 2020-04-23T03:04:21Z

setup.sh

-export FLASK_APP=/vagrant/flask_init.py
-python3 -m flask run --host=0.0.0.0 >> /vagrant/log.log 2>&1 &
+echo "configuring database"
+sudo su postgres <<POSTGRESQL_COMMANDS


IIR Postgeas has some command-line shortcuts for DB and user creation, could be cleaner to use those then inlined SQL.

ifireball · 2020-04-23T03:07:30Z

tests/init_test.py

+
+class Tester(unittest.TestCase):
+	def test_home(self):
+		response = requests.get("http://127.0.0.1:5000")


Is this meant to run on the Vagrant VM or on the developer machine? If its the latter, 127.0.0.1 is probably not the right address to connect to. If its the former, there should be instructions somewhere to tell oyur peers how to run test.

Its not a file that we created as part of this PR.
In fact this file is approved as part of issue #7.
I just changed the name of the folder containing it.

ifireball · 2020-04-23T03:08:06Z

tests/init_test.py

@@ -0,0 +1,11 @@
+import unittest


Id recommend using pytest is waaay less cumbersome then PyUnit.

As I said in the conversation above its not a file that we created as part of this PR.
But in our future tests we sure will use pytest.

DoRTaL94 · 2020-04-25T16:57:06Z

This PR is splitted into muiltiple PRs.
The following PRs are now fixing issue #27:
#36, #37, #38, #39, #40.

DoRTaL94 and others added 6 commits April 10, 2020 15:08

Created db models and configured app

ee7e3ef

adding .gitignore

1b7aaf8

editing the models User, Token and ResponseUser

345daf4

adding db_hanlder

1bf76b8

Updating db handler

92482f1

Added required packages to setup.sh and forwarded additional port for…

5bde4d5

… the Restful API

DoRTaL94 requested a review from simzacks April 11, 2020 16:53

simzacks requested changes Apr 12, 2020

View reviewed changes

added new line

daf8484

ahinoamta force-pushed the user-management branch from d280909 to daf8484 Compare April 12, 2020 11:39

ahinoamta and others added 3 commits April 12, 2020 14:42

Merge branch 'master' into user-management

82e7381

added necessary packages to requirements.txt file

4a1e0ce

CR fixes

ba95634

ahinoamta requested review from ahinoamta and simzacks April 12, 2020 12:21

Modified user, db_handler and token files

aeac940

- Added to_json function to User model that exposes less information to the client. - Added types to methods parameters - Added id column to token_blacklist table

DoRTaL94 force-pushed the user-management branch from 122c07a to aeac940 Compare April 12, 2020 19:35

DoRTaL94 changed the title ~~User Management backend #12~~ WIP User Management backend Apr 12, 2020

added abstract base class BaseStorageHandler that DbHandler derives from

9580175

DoRTaL94 changed the title ~~WIP User Management backend~~ User Management backend Apr 13, 2020

DoRTaL94 added 2 commits April 14, 2020 13:52

Fixed token query

21dad4f

Modified generic dbhandler

e605ac5

lmilbaum self-requested a review April 15, 2020 13:13

lmilbaum assigned DoRTaL94, ahinoamta and RoniRush Apr 15, 2020

lmilbaum suggested changes Apr 15, 2020

View reviewed changes

requirements.txt Outdated Show resolved Hide resolved

setup.sh Outdated Show resolved Hide resolved

DoRTaL94 requested a review from ifireball April 16, 2020 14:13

DoRTaL94 force-pushed the user-management branch from 0904387 to 2dd0d61 Compare April 16, 2020 15:39

simzacks requested changes Apr 20, 2020

View reviewed changes

DoRTaL94 requested a review from simzacks April 20, 2020 11:41

DoRTaL94 changed the title ~~User Management backend~~ User Management backend - Base Apr 20, 2020

DoRTaL94 force-pushed the user-management branch from 7f7db5d to 197a78b Compare April 20, 2020 19:12

ifireball requested changes Apr 21, 2020

View reviewed changes

This was referenced Apr 21, 2020

WIP- User management -user resource #33

Draft

WIP -User management - users resource #34

Draft

DoRTaL94 requested a review from ifireball April 21, 2020 14:59

DoRTaL94 force-pushed the user-management branch 4 times, most recently from be4d16e to 0367edc Compare April 21, 2020 21:37

simzacks reviewed Apr 22, 2020

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

Vagrantfile Outdated Show resolved Hide resolved

edison/config.py Show resolved Hide resolved

DoRTaL94 force-pushed the user-management branch from d0a8fba to 80b7023 Compare April 22, 2020 18:58

DoRTaL94 and others added 9 commits April 22, 2020 22:08

Added config file

54500cc

Fixed token model repr function

f6c312f

Updated setup.sh and changed app file name

61e4b1c

created test that verifies application is running

7deba85

Restructured project layout

9937b84

Merged both flask servers into one

922a891

Added use of Flask-Migrate extension

2f2d231

All files inside edison/migrations were generated automatically by the extension.

Updated gitignore

93122d9

DoRTaL94 force-pushed the user-management branch from 80b7023 to 93122d9 Compare April 22, 2020 19:12

ifireball requested changes Apr 23, 2020

View reviewed changes

DoRTaL94 closed this Apr 25, 2020

DoRTaL94 mentioned this pull request Apr 26, 2020

User Management backend - Base - Project restructure #36

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

User Management backend - Base #26

User Management backend - Base #26

DoRTaL94 commented Apr 11, 2020 •

edited

Loading

simzacks commented Apr 12, 2020

lmilbaum left a comment

ahinoamta commented Apr 15, 2020

simzacks Apr 20, 2020

DoRTaL94 Apr 20, 2020

ifireball Apr 20, 2020

DoRTaL94 Apr 20, 2020

ifireball Apr 20, 2020 •

edited

Loading

DoRTaL94 Apr 20, 2020

DoRTaL94 Apr 20, 2020 •

edited

Loading

simzacks Apr 22, 2020

DoRTaL94 Apr 22, 2020

ifireball Apr 21, 2020

DoRTaL94 Apr 21, 2020 •

edited

Loading

DoRTaL94 Apr 22, 2020

ifireball Apr 23, 2020

simzacks left a comment

ifireball left a comment

ifireball Apr 23, 2020

ifireball Apr 23, 2020

DoRTaL94 Apr 23, 2020

ifireball Apr 23, 2020

DoRTaL94 Apr 23, 2020

ifireball Apr 23, 2020

DoRTaL94 Apr 23, 2020

ifireball Apr 23, 2020

ifireball Apr 23, 2020

DoRTaL94 Apr 23, 2020

ifireball Apr 23, 2020

DoRTaL94 Apr 23, 2020 •

edited

Loading

DoRTaL94 commented Apr 25, 2020

User Management backend - Base #26

User Management backend - Base #26

Conversation

DoRTaL94 commented Apr 11, 2020 • edited Loading

simzacks commented Apr 12, 2020

lmilbaum left a comment

Choose a reason for hiding this comment

ahinoamta commented Apr 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ifireball Apr 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DoRTaL94 Apr 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DoRTaL94 Apr 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simzacks left a comment

Choose a reason for hiding this comment

ifireball left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DoRTaL94 Apr 23, 2020 • edited Loading

Choose a reason for hiding this comment

DoRTaL94 commented Apr 25, 2020

DoRTaL94 commented Apr 11, 2020 •

edited

Loading

ifireball Apr 20, 2020 •

edited

Loading

DoRTaL94 Apr 20, 2020 •

edited

Loading

DoRTaL94 Apr 21, 2020 •

edited

Loading

DoRTaL94 Apr 23, 2020 •

edited

Loading