Fix data sync request model; Add find_filesystem_boundary utility #15
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
njmei
force-pushed
the
fix-data-sync-request-model
branch
from
35e76ff to
d491a85
Compare
rpmcginty
reviewed
Nov 5, 2024
njmei
force-pushed
the
fix-data-sync-request-model
branch
2 times, most recently
from
5cd42a0 to
dca355c
Compare
rpmcginty
reviewed
Nov 6, 2024
njmei
force-pushed
the
fix-data-sync-request-model
branch
2 times, most recently
from
3a507fd to
57dba3e
Compare
The previous version of this model returned `config` and `task` properties that did not follow typing properly. This commit attempts to fix things by making the `config` and `task` properties return the correct "types". It also adds in missing fields in the `DataSyncConfig` model. This commit also adds a new field to the `DataSyncConfig` model called `remote_to_local_config` which allows more fine-grained controlled over remote (S3) to local filesystem transfers. Specifically, the temporary location where data sync operations download individual objects. Fix
njmei
force-pushed
the
fix-data-sync-request-model
branch
from
57dba3e to
2e58893
Compare
rpmcginty
approved these changes
Nov 7, 2024
| @@ -104,17 +121,33 @@ class DataSyncConfig(SchemaModel): | |||
| force: bool = custom_field(default=False, mm_field=BooleanField()) | |||
| size_only: bool = custom_field(default=False, mm_field=BooleanField()) | |||
| fail_if_missing: bool = custom_field(default=True, mm_field=BooleanField()) | |||
| remote_to_local_config: RemoteToLocalConfig = custom_field( | |||
There was a problem hiding this comment.
nit: do you think for the sake of making clean serialized content, we make this config optional? and just having a getter to return a default object instead if not specified?
There was a problem hiding this comment.
I just worry about the size of the serialized object if we do this.
There was a problem hiding this comment.
The RemoteToLocalConfig is not that big. If we do end up hitting an issue in the future with this, bring up this thread and I will own up to it!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See commit messages for details