feat(beetmover): Support expiration tag for GCS artifacts#1037
feat(beetmover): Support expiration tag for GCS artifacts#1037
Conversation
hneiva
force-pushed
the
hneiva/beetmover-expiry-alternative
branch
5 times, most recently
from
0b9ffe6 to
6c6dedb
Compare
bhearsum
left a comment
bhearsum
left a comment
There was a problem hiding this comment.
Nice work here! I know it was a struggle to find a good solution to avoid having custom_time set for the files in releases.
This patch looks fine, but it needs a bit more in the way of tests. specifically, tests that with some asserts that:
- custom_time is passed when an expiry is present
- custom_time is not passed when an expiry is not present
- Both of these cases mixed together in the same artifact map
- custom_time is not set when moving artifacts
I'd also like to see a staging release run through push-to-releases so that we can inspect the state of uploads run with this in a real bucket.
I'm also wondering if we need or want to support this for other schemas? maven_beetmover_task_schema.json and import-from-gcs-to-artifact-registry stand out as places where we might want to. (Even if we do, this is probably best handled as a follow-up.)
| # We need to set the data payload with some information so the metadata is NOT copied over. | ||
| # This prevents custom_time metadata from being copied unintentionally | ||
| # https://cloud.google.com/storage/docs/json_api/v1/objects/rewrite#request-body | ||
| dest_blob._properties["name"] = destination |
There was a problem hiding this comment.
Woohoo! I'm glad this ended up working out.
| return resp | ||
|
|
||
|
|
||
| @pytest.mark.asyncio |
There was a problem hiding this comment.
Thanks for the fix-up (these are not tests, so they don't need this mark AIUI).
|
Yeah adding more tests is a great idea. I've already tested this in a staging release: treeherder, Example task, gcloud bucket files I don't think we have a use case for other schemas yet; but if we do, adding it would be quite easy. Agree we should do it on a follow-up. |
Thanks! I will take a look there.
I agree we don't need to do it now unless we have specific plans for cleanup of artifacts that are uploaded through those methods already. |
hneiva
force-pushed
the
hneiva/beetmover-expiry-alternative
branch
from
4dfd66b to
0adf120
Compare
|
|
hneiva
force-pushed
the
hneiva/beetmover-expiry-alternative
branch
from
0adf120 to
db3b63b
Compare
bhearsum
left a comment
bhearsum
left a comment
There was a problem hiding this comment.
Thank you for the test improvements! I looked at your staging release as well, and it appears to have done all the right things with the artifacts on ftp.stage.mozaws.net.
2 participants
No description provided.