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

✨ Add Controlled Access field to Genomic file entity #109

Merged
merged 3 commits into from
Feb 13, 2018

Conversation

parimalak
Copy link
Contributor

@parimalak parimalak commented Feb 7, 2018

Added Controlled Access field to Genomic file entity.
Renamed file_type to data_type.
Updated dummy data generation script,migrations and tests

@parimalak parimalak added the data model Changes to the underlying data representation label Feb 7, 2018
@parimalak parimalak added this to the CHOP Sprint 2 milestone Feb 7, 2018
@parimalak parimalak self-assigned this Feb 7, 2018
@parimalak parimalak changed the title ✨ Add Controlled Access field to Genomic file entity :Sparkles: Add Controlled Access field to Genomic file entity Feb 7, 2018
@parimalak parimalak changed the title :Sparkles: Add Controlled Access field to Genomic file entity ✨ Add Controlled Access field to Genomic file entity Feb 7, 2018

# revision identifiers, used by Alembic.
revision = 'f80726457a72'
down_revision = '0cc35a21afc2'
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this revision may need to be modified because of the new version added in the last merge

Copy link
Contributor

Choose a reason for hiding this comment

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

If you change this to b7852f8aab0a I think you can get rid of the merge version.

@parimalak parimalak force-pushed the genomic-file-afield branch from e4d57a7 to 3c69932 Compare February 8, 2018 18:55
🗃️ Remove postgres id and use kf_id as primary key

🗃️ Add migration for new primary key
@parimalak parimalak force-pushed the genomic-file-afield branch from 3c69932 to b269108 Compare February 8, 2018 19:03
@znatty22 znatty22 changed the title ✨ Add Controlled Access field to Genomic file entity ✨ Add Controlled Access field to Genomic file entity Feb 8, 2018
@parimalak parimalak changed the title ✨ Add Controlled Access field to Genomic file entity ✨ WIP Add Controlled Access field to Genomic file entity Feb 11, 2018
@parimalak parimalak changed the title ✨ WIP Add Controlled Access field to Genomic file entity ✨ Add Controlled Access field to Genomic file entity Feb 12, 2018
@@ -0,0 +1,30 @@
"""rename file_type to data_type for genomic_file
Copy link
Member

Choose a reason for hiding this comment

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

I know this is nit picking but can you change to:

"""
Rename file_type to data_type for genomic_file

Revision ID: 1c790b83f611
Revises: f80726457a72
Create Date: 2018-02-12 15:33:27.365798
"""

@@ -0,0 +1,28 @@
"""Add Controlled_access field to genomic_file
Copy link
Member

Choose a reason for hiding this comment

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

Similar request as above, can you change to:

"""
Add Controlled_access field to genomic_file

Revision ID: f80726457a72
Revises: b7852f8aab0a
Create Date: 2018-02-07 16:12:38.279955
"""

@@ -0,0 +1,28 @@
"""Add Controlled_access field to genomic_file

Revision ID: f80726457a72
Copy link
Member

@znatty22 znatty22 Feb 12, 2018

Choose a reason for hiding this comment

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

I think you should be able to combine these two migration script into one. On your branch, you could do:

git reset origin/master migrations
rm migrations/versions/1c790b83f611_.py migrations/versions/f80726457a72_.py
# Make sure your dataservice containers are running, and if not run:
docker-compose up -d
# Generate new migration script
flask db migrate

That should generate a new migration script. Make sure you update the docstring to include both the field 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.

👍

@parimalak
Copy link
Contributor Author

✨ Update fields(controlled_access, data_type) in Genomic file entity

@parimalak parimalak merged commit 151cbc5 into master Feb 13, 2018
@dankolbman dankolbman deleted the genomic-file-afield branch February 13, 2018 19:58
@dankolbman dankolbman mentioned this pull request Apr 2, 2018
alubneuski pushed a commit that referenced this pull request Oct 11, 2024
✨ Update fields(controlled_access, data_type)  in Genomic file entity
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data model Changes to the underlying data representation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants