Implement semi-atomic push #2689
Conversation
Make Package.push fail if the destination already contains a package with a top hash that's different from the current package's parent. TODO: Fix unit tests
Codecov Report
@@ Coverage Diff @@
## master #2689 +/- ##
==========================================
+ Coverage 41.45% 41.54% +0.09%
==========================================
Files 536 536
Lines 24791 24831 +40
Branches 3380 3380
==========================================
+ Hits 10276 10315 +39
- Misses 13742 13743 +1
Partials 773 773
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
api/python/quilt3/packages.py
Outdated
| raise | ||
|
|
||
| if self._origin is None or latest_hash != self._origin.top_hash: | ||
| raise QuiltException( |
There was a problem hiding this comment.
Might make sense to add specific subclass of QuiltException for that.
api/python/quilt3/packages.py
Outdated
|
|
||
| if self._origin is None or latest_hash != self._origin.top_hash: | ||
| raise QuiltException( | ||
| f"Package with an unexpected hash {latest_hash} already exists at the destination; " |
There was a problem hiding this comment.
Might make sense to specify both expected and actual hashes.
| @@ -462,7 +474,7 @@ def add_pkg_file(pkg, lk, filename, data, *, version): | |||
| ) | |||
| with patch('time.time', return_value=timestamp1), \ | |||
| patch('quilt3.data_transfer.MAX_CONCURRENCY', 1): | |||
| remote_pkg = new_pkg.push(pkg_name, registry) | |||
| remote_pkg = new_pkg.push(pkg_name, registry, force=True) | |||
There was a problem hiding this comment.
Why do we need force=True here?
There was a problem hiding this comment.
Because otherwise, it makes extra S3 calls, so I'd need to mock those. I guess setup_s3_stubber_push_manifest could be updated to mock them by default, but force=True plus a new unit test for push conflicts seemed simpler.
There was a problem hiding this comment.
but force=True plus a new unit test for push conflicts seemed simpler
Well, yes, it's simpler, but are we going to use force=True in every new test?
Make Package.push fail if the destination already contains a package with a top hash that's different from the current package's parent.