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.

Sign up for GitHub

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

Jump to bottom

🐘 Postgres #78

Merged
merged 7 commits into from
Feb 5, 2018
Merged

🐘 Postgres #78

merged 7 commits into from
Feb 5, 2018

Conversation

dankolbman
Copy link
Contributor

@dankolbman dankolbman commented Jan 30, 2018 •
edited
Loading

Reworks configuration, Jenkins pipeline, CircleCI, error handling, and secret management for operation with Postgres.

Fixes #28,
Fixes #29
Fixes #76

  • Use postgres in local environments
  • Change CicrleCI to use postgres
  • Change Jenkins to use postgres
  • Integrate vault to use postgres on aws

@dankolbman dankolbman added feature New functionality devops Involves features of the deployment data model Changes to the underlying data representation labels Jan 30, 2018
@dankolbman dankolbman self-assigned this Jan 30, 2018
@dankolbman dankolbman force-pushed the postgres branch 26 times, most recently from 67d8930 to ebc2afb Compare January 31, 2018 17:44
znatty22
znatty22 reviewed Feb 2, 2018
View reviewed changes
README.md Outdated
@@ -58,6 +72,9 @@ Unit tests and pep8 linting is run via `flask test`
```
# Install test dependencies
pip install -r dev-requirements.txt
# Setup test database
docker run --name dataservice-pg -p 5432:5432 -d postgres
docker exec dataservice-pg psql -U postgres -c "CREATE DATABASE dev;"
Copy link
Member

Choose a reason for hiding this comment

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

This should be "CREATE DATABASE test;" right?

znatty22
znatty22 reviewed Feb 2, 2018
View reviewed changes
README.md Outdated
@@ -21,12 +21,18 @@ pip install -r requirements.txt
# Configure the flask application
export FLASK_APP=manage
# Setup the database
flask db init
docker run --name dataservice-pg -p 5432:5432 -d postgres:9.5
docker exec dataservice-pg psql -U postgres -c "CREATE DATABASE dev;"
flask db upgrade
Copy link
Member

Choose a reason for hiding this comment

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

Need to add a line above here with flask db migrate, otherwise the relations don't exist on the first time you try to run the app

znatty22
znatty22 reviewed Feb 4, 2018
View reviewed changes
Copy link
Member

@znatty22 znatty22 left a comment

Choose a reason for hiding this comment

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

Looks good! Just a couple minor comments on test_errors.py and the README.md

tests/test_errors.py Outdated
assert message in response['_status']['message']


def test_unique(self, client):
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this should be called test_unique_demographic since its specific to the demographic and we could have other one-to-one entity tests in this class.

tests/test_errors.py Outdated


def test_unique(self, client):
""" Test integrity errors trying to break one-many with many-many """
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be

""" Test integrity errors trying to break one-to-one with one-to-many """

since a participant can only have on demographic and a demographic can only be related to one participant?

@dankolbman dankolbman force-pushed the postgres branch 2 times, most recently from fb1b48c to d2330d9 Compare February 5, 2018 13:32
@dankolbman dankolbman requested a review from parimalak February 5, 2018 13:45
@dankolbman dankolbman mentioned this pull request Feb 5, 2018
Closed
@dankolbman dankolbman force-pushed the postgres branch 2 times, most recently from dda1101 to f3c74aa Compare February 5, 2018 15:45
znatty22
znatty22 reviewed Feb 5, 2018
View reviewed changes
bin/initdb.sh Outdated
@@ -0,0 +1,3 @@
#!/bin/bash
flask db upgrade
flask db migrate
Copy link
Member

Choose a reason for hiding this comment

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

I think it should be migrate first then upgrade?

bin/run.sh Outdated
#!/bin/bash
source venv/bin/activate
flask db upgrade
flask db migrate
Copy link
Member

Choose a reason for hiding this comment

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

Same here, I think migrate first then upgrade

@dankolbman dankolbman force-pushed the postgres branch 2 times, most recently from 5b0b6bc to b34ba26 Compare February 5, 2018 17:14
znatty22
znatty22 reviewed Feb 5, 2018
View reviewed changes
docker-compose.yml
version: '3'

services:
pg:
Copy link
Member

Choose a reason for hiding this comment

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

Can we expose the pg port so that people can use other apps like pgadmin to connect

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd rather not to avoid any conflict with local instances. Someone familiar enough to be connecting to the db should be able to figure out how to expose the port here.

Copy link
Member

Choose a reason for hiding this comment

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

👍

znatty22
znatty22 approved these changes Feb 5, 2018
View reviewed changes
Copy link
Member

@znatty22 znatty22 left a comment

Choose a reason for hiding this comment

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

Looks great!

@dankolbman dankolbman merged commit 221d29f into master Feb 5, 2018
@dankolbman dankolbman deleted the postgres branch February 5, 2018 18:27
@dankolbman dankolbman changed the title 🐘 Postgres 🐘 Postgres Mar 9, 2018
@dankolbman dankolbman mentioned this pull request Apr 2, 2018
Merged
alubneuski pushed a commit that referenced this pull request Oct 11, 2024
@dankolbman
Merge pull request #78 from kids-first/postgres
da1100d 
 🐘 Postgres
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@znatty22 znatty22 znatty22 approved these changes

@parimalak parimalak parimalak approved these changes

Assignees

@dankolbman dankolbman

Labels
data model Changes to the underlying data representation devops Involves features of the deployment feature New functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dankolbman @znatty22 @parimalak