Verify if a manifest already exists locally when syncing a remote repo. #1133
Conversation
decko
force-pushed
the
check_for_existing_manifests_1047
branch
from
2e618b6
to
47476d6
Compare
decko
force-pushed
the
check_for_existing_manifests_1047
branch
6 times, most recently
from
94e8c78
to
e3002da
Compare
decko
force-pushed
the
check_for_existing_manifests_1047
branch
from
e3002da
to
fa44324
Compare
| digest = dl_res.artifact_attributes["sha256"] | ||
| validate_manifest(content_data, media_type, digest) | ||
| for artifact in asyncio.as_completed(to_download_artifact): | ||
| saved_artifact, content_data, raw_data, response = await artifact |
There was a problem hiding this comment.
consider returning here just raw_data and on the line after do json.load for the content_data
| response.artifact_attributes["file"] = response.path | ||
|
|
||
| saved_artifact = await sync_to_async(_save_artifact_blocking)(response.artifact_attributes) | ||
| content_data = json.loads(raw_data) |
There was a problem hiding this comment.
not sure, you probably don't need to return content_data here. just raw_data should suffice
| raw_data = saved_artifact.file | ||
| content_data = json.load(raw_data) | ||
| saved_artifact.file.close() | ||
| return saved_artifact, content_data, raw_data |
There was a problem hiding this comment.
how about we return just just raw_data and later on where it is needed we can json.load it. I am trying to avoid to carry both content_data and raw_data
There was a problem hiding this comment.
I tried it, but there are some places where I need to use _get_content_data_blocking and _download_and_save_artifact_data. Specially in the create_listed_manifest, where there are statements under the else branch that need content_data. I will end calling json.load more times than if left where they are now. 😭
There was a problem hiding this comment.
I am also in favour of removing content_data from the return parameters. You will call json.load two times instead of one. Or not? Still, the number of parameters will decrease all around the code.
There was a problem hiding this comment.
it will be written in the code twice - under if and under else branch but in the end of the day called it will be once.
decko
force-pushed
the
check_for_existing_manifests_1047
branch
from
fa44324
to
e2e8b13
Compare
decko
force-pushed
the
check_for_existing_manifests_1047
branch
3 times, most recently
from
d5b525a
to
f860076
Compare
decko
force-pushed
the
check_for_existing_manifests_1047
branch
3 times, most recently
from
d21e24c
to
a029df5
Compare
| saved_artifact = manifest.contentartifact_set.first().artifact | ||
| raw_data = saved_artifact.file.read() | ||
| content_data = json.loads(raw_data) | ||
| saved_artifact.file.close() |
There was a problem hiding this comment.
you can use context manager here
decko
force-pushed
the
check_for_existing_manifests_1047
branch
from
a029df5
to
709f27b
Compare
Closes #1047