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

[FIX] DockerTestConfig #15

Merged
merged 4 commits into from
Sep 16, 2020
Merged

Conversation

jdkent
Copy link
Member

@jdkent jdkent commented Sep 10, 2020

fixes #14

This pull request suggests:

  • change DockerTestConfig to look for the environmental variable POSTGRES_PASSWORD
  • add a note in the readme on where to find the postgres password.

@codecov-commenter
Copy link

codecov-commenter commented Sep 15, 2020

Codecov Report

Merging #15 into master will decrease coverage by 0.18%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #15      +/-   ##
==========================================
- Coverage   63.50%   63.31%   -0.19%     
==========================================
  Files          22       22              
  Lines         685      687       +2     
==========================================
  Hits          435      435              
- Misses        250      252       +2     
Impacted Files Coverage Δ
neurostuff/example_config.py 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9ce336a...26cd210. Read the comment docs.

Copy link
Member

@adelavega adelavega left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The password is already pulled from the env variables in the main config, so you should just be able to change the db_name and re-set the URI to use the new variables

neurostuff/example_config.py Show resolved Hide resolved
neurostuff/example_config.py Outdated Show resolved Hide resolved
update DB_NAME and SQLALCHEMY_DATABASE_URI variables for DockerTestConfig

Co-authored-by: Alejandro de la Vega <aleph4@gmail.com>
@adelavega
Copy link
Member

oh crap, i guess the variable is not available. well then shoot lets revert that

@adelavega
Copy link
Member

yeah actually instead just add these thre lines to the DockerTestConfig since i'm not sure how to access inherited class attributes:

    POSTGRES_PASSWORD = os.environ.get('POSTGRES_PASSWORD', '')
    DB_NAME = 'scout_test'
    SQLALCHEMY_DATABASE_URI = "postgres://postgres:" \
        f"{POSTGRES_PASSWORD}@postgres:5432/{DB_NAME}"

@jdkent
Copy link
Member Author

jdkent commented Sep 15, 2020

Can we make an __init__ function for the class and assign these variables to self? Would that fix the issue of reassignment?

@adelavega
Copy link
Member

yes that would be another option.

you could set DB_NAME outside of self for each instance, and then create an __init__ method in the class Config (the parent) which based on DB_NAME and POSTGRES_PASSWORD creates self.SQLALCHEMY_DATABASE_URI.



class ProductionConfig(Config):
ENV = 'production'
def __init__(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I'm being super pedantic now so I apologize, but I think we can get away with only the self.SQLALCHEMY_DATABASE_URI variable being in the __init__ function on top, since all others are just plain variables.

Especially for these derivative classes, let's keep them simple just to simplify future maintenance.

Suggested change
def __init__(self):



class ProductionConfig(Config):
ENV = 'production'
def __init__(self):
super().__init__()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
super().__init__()

ENV = 'production'
def __init__(self):
super().__init__()
self.ENV = 'production'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
self.ENV = 'production'
ENV = 'production'

As an example, I think this should work, since the ENV variable for this derivative class will be set before the __init__ method is called.

That way, no need to mess around with super.

@jdkent
Copy link
Member Author

jdkent commented Sep 16, 2020

okay, I went back to my original solution and learned(?) about when variables are instantiated in a class when referencing it as Class versus Class().

@adelavega
Copy link
Member

Cool LGTM

@adelavega adelavega merged commit e192084 into neurostuff:master Sep 16, 2020
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.

running tests requires knowledge of the postgres password
3 participants