From 8cf9fcbdeaa67de9162d83c6816142ad6d2a3e58 Mon Sep 17 00:00:00 2001 From: Ethan Knox Date: Mon, 11 Feb 2019 16:33:29 -0500 Subject: [PATCH 1/7] initial --- config/core_project.yaml | 3 ++- core/helpers/configuration_mocker.py | 4 +--- core/models/configuration.py | 19 ++++++++++++++++--- tests/unit/test_helpers.py | 10 ++++++++++ 4 files changed, 29 insertions(+), 7 deletions(-) diff --git a/config/core_project.yaml b/config/core_project.yaml index 8305deac..95f42a0a 100644 --- a/config/core_project.yaml +++ b/config/core_project.yaml @@ -12,4 +12,5 @@ PROD_AWS_ACCOUNT: "687531504312" AWS_ACCOUNT: "265991248033" AWS_REGION: us-east-1 AWS_BATCH_TEST_JOB_QUEUE: core -LOGGING_CONFIG: logging.yaml \ No newline at end of file +LOGGING_CONFIG: logging.yaml +DEV_CONFIGURATION_APPLICATION_CONN_STRING: "postgresql://configurator:configurator@localhost/configuration_application" diff --git a/core/helpers/configuration_mocker.py b/core/helpers/configuration_mocker.py index baaf293a..ec6470be 100644 --- a/core/helpers/configuration_mocker.py +++ b/core/helpers/configuration_mocker.py @@ -10,9 +10,7 @@ class ConfigurationMocker(LoggerMixin): ''' def __init__(self)-> None: - # TODO: migrate to local postgres instance - engine = create_engine('sqlite://') - #engine = config.GenerateEngine(env='dev', local=True).get_engine() + engine = config.GenerateEngine(env='dev', local=True).get_engine() # this instansiates the in-memory sqlite instance config.Base.metadata.create_all(engine) diff --git a/core/models/configuration.py b/core/models/configuration.py index 74bd5ff3..c61d3f5e 100644 --- a/core/models/configuration.py +++ b/core/models/configuration.py @@ -1,7 +1,8 @@ from sqlalchemy import engine, create_engine, Column, Integer, String, BOOLEAN, TIMESTAMP, text, ForeignKey, func from sqlalchemy.ext.declarative import declarative_base from sqlalchemy.orm import session, sessionmaker, relationship - +from core.constants import DEV_CONFIGURATION_APPLICATION_CONN_STRING +from core.secret import Secret Base = declarative_base() @@ -32,11 +33,23 @@ def get_engine(self) -> engine.base.Engine: return self.engine def _local_engine(self) -> engine.base.Engine: - return create_engine('postgresql://configurator:configurator@localhost/configuration_application') + return create_engine(DEV_CONFIGURATION_APPLICATION_CONN_STRING) def _secret_defined_engine(self) -> engine.base.Engine: # TODO: in DC-57 update this to use secret - pass + secret = Secret(env=ENVIRONMENT, + name='configuration_application', + type_of='database', + mode='read' + ) + if secret.rdbms == "postgres": + conn_string = f"postgresql://{secret.user}:{secret.password}@{host}/{database}" + else: + m = "Only postgres databases are supported for configuration_application at this time." + logger.critical(m) + raise NotImplementedError(m) + return conn_string + return create_engine() """Mixins diff --git a/tests/unit/test_helpers.py b/tests/unit/test_helpers.py index e79a70b7..70db6ab1 100644 --- a/tests/unit/test_helpers.py +++ b/tests/unit/test_helpers.py @@ -150,3 +150,13 @@ def test_get_file_type(paramiko_trans,paramiko_sftp): ft = fm.get_file_type(test_file, fd) assert ft, "dont_move" +## SessionHelper + +def test_session_helper_prod(): + pass + +def test_secret_helper_uat(): + pass + +def test_secret_helper_dev(): + pass From 34e28edf4dbe3613d1b0819ecfdc53d9773f3a87 Mon Sep 17 00:00:00 2001 From: Ethan Knox Date: Tue, 12 Feb 2019 14:31:44 -0500 Subject: [PATCH 2/7] test coverage and fixed UAT --- config/core_project.yaml | 2 +- core/helpers/configuration_mocker.py | 4 ++-- core/helpers/session_helper.py | 10 +++++--- core/models/configuration.py | 20 ++++++++++------ tests/unit/test_helpers.py | 34 +++++++++++++++++++++------- 5 files changed, 49 insertions(+), 21 deletions(-) diff --git a/config/core_project.yaml b/config/core_project.yaml index 95f42a0a..a1eab99f 100644 --- a/config/core_project.yaml +++ b/config/core_project.yaml @@ -13,4 +13,4 @@ AWS_ACCOUNT: "265991248033" AWS_REGION: us-east-1 AWS_BATCH_TEST_JOB_QUEUE: core LOGGING_CONFIG: logging.yaml -DEV_CONFIGURATION_APPLICATION_CONN_STRING: "postgresql://configurator:configurator@localhost/configuration_application" +DEV_CONFIGURATION_APPLICATION_CONN_STRING: "postgresql://configurator:configurator@configurationpg/configuration_application" diff --git a/core/helpers/configuration_mocker.py b/core/helpers/configuration_mocker.py index ec6470be..3a2599e9 100644 --- a/core/helpers/configuration_mocker.py +++ b/core/helpers/configuration_mocker.py @@ -9,8 +9,8 @@ class ConfigurationMocker(LoggerMixin): optionally gives you a bunch of mocked data. ''' - def __init__(self)-> None: - engine = config.GenerateEngine(env='dev', local=True).get_engine() + def __init__(self, in_memory:bool =True, local:bool =True)-> None: + engine = config.GenerateEngine(in_memory=in_memory, local=local).get_engine() # this instansiates the in-memory sqlite instance config.Base.metadata.create_all(engine) diff --git a/core/helpers/session_helper.py b/core/helpers/session_helper.py index d9995db7..339007ec 100644 --- a/core/helpers/session_helper.py +++ b/core/helpers/session_helper.py @@ -1,10 +1,12 @@ from core.constants import ENVIRONMENT -from core.helpers.configuration_helper import ConfigurationHelper as CMock +from core.helpers.configuration_mocker import ConfigurationMocker as CMock +import core.models.configuration as config from sqlalchemy.orm.session import Session class SessionHelper: """ Gets the correct env-based configuration secret, returns a session to the right configuration db. + For the dev env it pre-populates the database with helper seed data. """ def __init__(self): self._session = None @@ -12,8 +14,10 @@ def __init__(self): cmock = CMock() cmock.generate_mocks() self._session == cmock.get_session() - if ENVIRONMENT == "prod": - pass + if ENVIRONMENT in ("prod", "uat"): + engine = config.GenerateEngine().get_engine() + session = config.Session(engine) + self._session = session.get_session() @property def session(self)-> Session: diff --git a/core/models/configuration.py b/core/models/configuration.py index c61d3f5e..d0f2b448 100644 --- a/core/models/configuration.py +++ b/core/models/configuration.py @@ -21,10 +21,12 @@ def get_session(self) -> session.Session: class GenerateEngine: - """ abstract defining connections here. Local assumes a psql instance on the metal. """ + """ abstract defining connections here. Local assumes a psql instance in a local docker container. """ - def __init__(self, env: str, local: bool = False) -> None: - if local: + def __init__(self, in_memory: bool = False, local: bool = False) -> None: + if in_memory: + self.engine = self._in_memory_engine() + elif local: self.engine = self._local_engine() else: self.engine = self._secret_defined_engine() @@ -32,12 +34,17 @@ def __init__(self, env: str, local: bool = False) -> None: def get_engine(self) -> engine.base.Engine: return self.engine + def _in_memory_engine(self): + """ For testing and other cases where you don't want the db to persist and don't care about triggers etc, this is the way to go.""" + return create_engine('sqlite://') + def _local_engine(self) -> engine.base.Engine: + """ Local, persisting as long as the container lives. Great for testing real PG triggers etc.""" return create_engine(DEV_CONFIGURATION_APPLICATION_CONN_STRING) def _secret_defined_engine(self) -> engine.base.Engine: - # TODO: in DC-57 update this to use secret - secret = Secret(env=ENVIRONMENT, + """ creates a session connecting to the correct configuration_application db based on ENV.""" + secret = Secret(env=ENVIRONMENT, name='configuration_application', type_of='database', mode='read' @@ -48,8 +55,7 @@ def _secret_defined_engine(self) -> engine.base.Engine: m = "Only postgres databases are supported for configuration_application at this time." logger.critical(m) raise NotImplementedError(m) - return conn_string - return create_engine() + return create_engine(conn_string) """Mixins diff --git a/tests/unit/test_helpers.py b/tests/unit/test_helpers.py index 70db6ab1..631c4d01 100644 --- a/tests/unit/test_helpers.py +++ b/tests/unit/test_helpers.py @@ -4,6 +4,7 @@ from core.helpers.project_root import ProjectRoot from core.helpers.configuration_mocker import ConfigurationMocker as CMock import core.models.configuration as config +from core.helpers.session_helper import SessionHelper from core.helpers.s3_naming_helper import S3NamingHelper from core.helpers.file_mover import FileMover, FileDestination @@ -152,11 +153,28 @@ def test_get_file_type(paramiko_trans,paramiko_sftp): ## SessionHelper -def test_session_helper_prod(): - pass - -def test_secret_helper_uat(): - pass - -def test_secret_helper_dev(): - pass +@patch("core.helpers.session_helper.config") +@patch("core.helpers.session_helper.CMock") +@patch("core.helpers.session_helper.ENVIRONMENT","prod") +def test_session_helper_prod(mock_cmock, mock_config): + session = SessionHelper().session + assert mock_config.GenerateEngine.called + assert mock_config.Session.called + assert not mock_cmock.called + +@patch("core.helpers.session_helper.config") +@patch("core.helpers.session_helper.CMock") +@patch("core.helpers.session_helper.ENVIRONMENT","uat") +def test_session_helper_uat(mock_cmock, mock_config): + session = SessionHelper().session + assert mock_config.GenerateEngine.called + assert mock_config.Session.called + assert not mock_cmock.called + +@patch("core.helpers.session_helper.config") +@patch("core.helpers.session_helper.CMock") +@patch("core.helpers.session_helper.ENVIRONMENT","dev") +def test_session_helper_dev(mock_cmock, mock_config): + session = SessionHelper().session + assert not mock_config.GenerateEngine.called + assert mock_cmock.called From f3a46cfa5db8b554a44940ae214fbbf668f6354a Mon Sep 17 00:00:00 2001 From: Ethan Knox Date: Wed, 13 Feb 2019 12:59:29 -0500 Subject: [PATCH 3/7] pep8 --- core/helpers/configuration_mocker.py | 8 +++++--- core/helpers/session_helper.py | 4 +++- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/core/helpers/configuration_mocker.py b/core/helpers/configuration_mocker.py index b8fa2e6c..3de4c689 100644 --- a/core/helpers/configuration_mocker.py +++ b/core/helpers/configuration_mocker.py @@ -9,8 +9,9 @@ class ConfigurationMocker(LoggerMixin): optionally gives you a bunch of mocked data. ''' - def __init__(self, in_memory:bool =True, local:bool =True)-> None: - engine = config.GenerateEngine(in_memory=in_memory, local=local).get_engine() + def __init__(self, in_memory: bool = True, local: bool = True)-> None: + engine = config.GenerateEngine( + in_memory=in_memory, local=local).get_engine() # this instansiates the in-memory sqlite instance config.Base.metadata.create_all(engine) @@ -80,7 +81,8 @@ def _mock_pipelines(self)-> None: pipeline_type_id=1, run_frequency='daily'), p(id=2, name="bluth_profitability", brand_id=2, pipeline_type_id=2, run_frequency='hourly'), - p(id=3, name="temocil_profitablility", brand_id=1, pipeline_type_id=1, run_frequency='daily'), + p(id=3, name="temocil_profitablility", brand_id=1, + pipeline_type_id=1, run_frequency='daily'), p(id=500, name="bluth_banana_regression_deprecated", brand_id=2, pipeline_type_id=1, is_active=False, run_frequency='hourly')]) self.session.commit() diff --git a/core/helpers/session_helper.py b/core/helpers/session_helper.py index afa94614..af36357d 100644 --- a/core/helpers/session_helper.py +++ b/core/helpers/session_helper.py @@ -3,11 +3,13 @@ import core.models.configuration as config from sqlalchemy.orm.session import Session + class SessionHelper: """ Gets the correct env-based configuration secret, returns a session to the right configuration db. For the dev env it pre-populates the database with helper seed data. """ + def __init__(self): self._session = None if ENVIRONMENT == "dev": @@ -24,5 +26,5 @@ def session(self)-> Session: return self._session @session.setter - def session(self,session)->None: + def session(self, session)->None: raise ValueError("session cannot be explicitly set in session_helper.") From 7076ccd0578c54f1c4e327ecda0ddcb96ef0c9e7 Mon Sep 17 00:00:00 2001 From: Ethan Knox Date: Wed, 13 Feb 2019 14:46:02 -0500 Subject: [PATCH 4/7] refactored to clean up smells and simplify config url selection --- core/database/env.py | 26 +++--------------- core/helpers/configuration_mocker.py | 5 ++-- core/helpers/session_helper.py | 9 ++++--- core/models/configuration.py | 40 +++++++++++++++------------- 4 files changed, 32 insertions(+), 48 deletions(-) diff --git a/core/database/env.py b/core/database/env.py index 4f0d9e17..83334ee3 100644 --- a/core/database/env.py +++ b/core/database/env.py @@ -5,7 +5,7 @@ from sqlalchemy import create_engine from sqlalchemy import engine_from_config from sqlalchemy import pool -from core.models.configuration import Base +from core.models.configuration import Base, GenerateEngine from core.secret import Secret from core.constants import ENVIRONMENT from alembic import context @@ -29,25 +29,6 @@ # my_important_option = config.get_main_option("my_important_option") # ... etc. -def create_conn_string_from_secret(): - """ builds the appropriate db string based on ENV - specific secret. - For dev uses the value in alembic.ini.""" - if ENVIRONMENT == "dev": - return config.get_main_option("sqlalchemy.url") - - secret = Secret(env=ENVIRONMENT, - name='configuration_application', - type_of='database', - mode='read' - ) - if secret.rdbms == "postgres": - conn_string = f"postgresql://{secret.user}:{secret.password}@{host}/{database}" - else: - m = "Only postgres databases are supported for configuration_application at this time." - logger.critical(m) - raise NotImplementedError(m) - return conn_string - def run_migrations_offline(): """Run migrations in 'offline' mode. @@ -60,7 +41,7 @@ def run_migrations_offline(): script output. """ - url = create_conn_string_from_secret() + url = GenerateEngine().url context.configure( url=url, target_metadata=target_metadata, literal_binds=True ) @@ -76,8 +57,7 @@ def run_migrations_online(): and associate a connection with the context. """ - connectable = create_engine(create_conn_string_from_secret()) - + connectable = GenerateEngine().get_engine() with connectable.connect() as connection: context.configure( connection=connection, target_metadata=target_metadata diff --git a/core/helpers/configuration_mocker.py b/core/helpers/configuration_mocker.py index 3de4c689..514654a4 100644 --- a/core/helpers/configuration_mocker.py +++ b/core/helpers/configuration_mocker.py @@ -9,9 +9,8 @@ class ConfigurationMocker(LoggerMixin): optionally gives you a bunch of mocked data. ''' - def __init__(self, in_memory: bool = True, local: bool = True)-> None: - engine = config.GenerateEngine( - in_memory=in_memory, local=local).get_engine() + def __init__(self)-> None: + engine = config.GenerateEngine().get_engine() # this instansiates the in-memory sqlite instance config.Base.metadata.create_all(engine) diff --git a/core/helpers/session_helper.py b/core/helpers/session_helper.py index af36357d..0d75154a 100644 --- a/core/helpers/session_helper.py +++ b/core/helpers/session_helper.py @@ -2,9 +2,9 @@ from core.helpers.configuration_mocker import ConfigurationMocker as CMock import core.models.configuration as config from sqlalchemy.orm.session import Session +from core.logging import LoggerMixin - -class SessionHelper: +class SessionHelper(LoggerMixin): """ Gets the correct env-based configuration secret, returns a session to the right configuration db. For the dev env it pre-populates the database with helper seed data. @@ -12,14 +12,17 @@ class SessionHelper: def __init__(self): self._session = None + self.logger.info(f"Creating session for {ENVIRONMENT} environment...") if ENVIRONMENT == "dev": cmock = CMock() cmock.generate_mocks() - self._session == cmock.get_session() + self._session = cmock.get_session() + self.logger.info("Done. Created dev session with mock data.") if ENVIRONMENT in ("prod", "uat"): engine = config.GenerateEngine().get_engine() session = config.Session(engine) self._session = session.get_session() + self.logger.info(f"Done. Created {ENVIRONMENT} session.") @property def session(self)-> Session: diff --git a/core/models/configuration.py b/core/models/configuration.py index cf15af96..ff8b3ee4 100644 --- a/core/models/configuration.py +++ b/core/models/configuration.py @@ -1,7 +1,7 @@ from sqlalchemy import engine, create_engine, Column, Integer, String, Boolean, TIMESTAMP, text, ForeignKey, func from sqlalchemy.ext.declarative import declarative_base from sqlalchemy.orm import session, sessionmaker, relationship -from core.constants import DEV_CONFIGURATION_APPLICATION_CONN_STRING +from core.constants import DEV_CONFIGURATION_APPLICATION_CONN_STRING, ENVIRONMENT from core.secret import Secret Base = declarative_base() @@ -19,30 +19,32 @@ def __init__(self, engine: engine.base.Engine) -> None: def get_session(self) -> session.Session: return self.session - class GenerateEngine: """ abstract defining connections here. Local assumes a psql instance in a local docker container. """ - def __init__(self, in_memory: bool = False, local: bool = False) -> None: - if in_memory: - self.engine = self._in_memory_engine() - elif local: - self.engine = self._local_engine() + def __init__(self, in_memory:bool=True) -> None: + """ So the default development configuration database is an in-memory sqlite instance. + This is fast and easy and atomic (resets after every execution) but it is NOT exactly the same as + the prod PG instance. When we want an identical configuration environment, setting in_memory to False + gives you the local docker container PG. """ + + if ENVIRONMENT == "dev": + if in_memory: + self._url = "sqlite://" + else: + self._url = DEV_CONFIGURATION_APPLICATION_CONN_STRING else: - self.engine = self._secret_defined_engine() - - def get_engine(self) -> engine.base.Engine: - return self.engine + self._url = self._secret_defined_url() - def _in_memory_engine(self): - """ For testing and other cases where you don't want the db to persist and don't care about triggers etc, this is the way to go.""" - return create_engine('sqlite://') + @property + def url(self) -> str: + return self._url - def _local_engine(self) -> engine.base.Engine: - """ Local, persisting as long as the container lives. Great for testing real PG triggers etc.""" - return create_engine(DEV_CONFIGURATION_APPLICATION_CONN_STRING) + def get_engine(self) -> engine.base.Engine: + engine = create_engine(self._url) + return engine - def _secret_defined_engine(self) -> engine.base.Engine: + def _secret_defined_url(self) -> str: """ creates a session connecting to the correct configuration_application db based on ENV.""" secret = Secret(env=ENVIRONMENT, name='configuration_application', @@ -55,7 +57,7 @@ def _secret_defined_engine(self) -> engine.base.Engine: m = "Only postgres databases are supported for configuration_application at this time." logger.critical(m) raise NotImplementedError(m) - return create_engine(conn_string) + return conn_string """Mixins From ce0f96496b5629f337c06a5f83ad6a80c3048a79 Mon Sep 17 00:00:00 2001 From: Nate Sanford Date: Thu, 14 Feb 2019 09:48:26 -0500 Subject: [PATCH 5/7] use project root instead of . --- tests/unit/test_docker.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/unit/test_docker.py b/tests/unit/test_docker.py index 9af944a3..3dc48c45 100644 --- a/tests/unit/test_docker.py +++ b/tests/unit/test_docker.py @@ -1,10 +1,11 @@ import pytest from core.helpers import docker from git import Repo +from core.helpers.project_root import ProjectRoot class Test(): def setup(self): - self.branch_name = Repo('.').active_branch.name + self.branch_name = Repo(ProjectRoot().get_path()).active_branch.name def test_get_core_tag(self): self.setup() From 8a34855cf6d6728ee093f8cfc944e99e7a7ec799 Mon Sep 17 00:00:00 2001 From: Nate Sanford Date: Thu, 14 Feb 2019 10:02:43 -0500 Subject: [PATCH 6/7] add ci exception for git repo --- core/helpers/docker.py | 2 +- tests/unit/test_docker.py | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/core/helpers/docker.py b/core/helpers/docker.py index 22dec2c9..9cccd458 100644 --- a/core/helpers/docker.py +++ b/core/helpers/docker.py @@ -11,7 +11,7 @@ # commit hash, the work-around is to use the BRANCH_NAME env var that # Jenkins sets. def get_branch_name(): - repo = Repo('.') + repo = Repo(ProjectRoot().get_path()) try: return repo.active_branch.name except: diff --git a/tests/unit/test_docker.py b/tests/unit/test_docker.py index 3dc48c45..a687fc92 100644 --- a/tests/unit/test_docker.py +++ b/tests/unit/test_docker.py @@ -5,7 +5,11 @@ class Test(): def setup(self): - self.branch_name = Repo(ProjectRoot().get_path()).active_branch.name + repo = Repo(ProjectRoot().get_path()) + try: + self.branch_name = repo.active_branch.name + except: + self.branch_name = os.environ['BRANCH_NAME'] def test_get_core_tag(self): self.setup() From c78ed1695bb658c51724dc399a36aca2a5cc594d Mon Sep 17 00:00:00 2001 From: Nate Sanford Date: Thu, 14 Feb 2019 10:06:49 -0500 Subject: [PATCH 7/7] add missing os import --- tests/unit/test_docker.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/unit/test_docker.py b/tests/unit/test_docker.py index a687fc92..e4545b61 100644 --- a/tests/unit/test_docker.py +++ b/tests/unit/test_docker.py @@ -2,6 +2,7 @@ from core.helpers import docker from git import Repo from core.helpers.project_root import ProjectRoot +import os class Test(): def setup(self):