-
Notifications
You must be signed in to change notification settings - Fork 1
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
Implement solution to write mapped data to storage #51
Conversation
sb-2011
commented
Apr 3, 2024
- Added functionality to write post mapped data to disk
- Introduced landing zone for location to write this data
- Added tests including a test for uploading mapped data to storage using minio
6a3efb2
to
79fd4bf
Compare
.gitignore
Outdated
@@ -182,3 +182,6 @@ node_modules/ | |||
|
|||
# Mac Desktop Services Store | |||
*.DS_Store | |||
|
|||
# Landing zone data storage | |||
# /data/landing_zone/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure whether we're intending to keep data/landing_zone/
under version control given that this is commented out. Do we want to keep the directory and the README.md under VC and ignore everything else in the folder? If so I think we can accomplish that as such in the .gitignore file:
data/landing_zone/*
!data/landing_zone/README.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes good call, I'll make the changes.
@@ -68,11 +65,13 @@ def create_column_map_repository(self): | |||
def create_logger(self): | |||
return BasicLogger(__name__, logging.DEBUG) | |||
|
|||
def create_storage(self): | |||
@staticmethod |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the create_storage
method here and in the other ApplicationContext
classes necessarily need to be static? Just curious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made it static because I need to use a storage class instance in the task_queue.py file and I don't want to create an application context just for that use case. I wasn't able to pass the application context to the celery task like we discussed before due to serialization so this was a work around.
There was a problem hiding this 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 few comments. Also the CI job is failing FWIW.
79fd4bf
to
45a7e56
Compare