codecov/patch flagged the change at 75% of diff hit. The uncovered lines
were the two the existing test could not reach: the fallthrough when no
metadata key matches, and the HeadObject error return.
Both are worth pinning regardless of the coverage gate.
packageIndexByHash calls ReadLink on a path it has only just confirmed
exists and treats an error as "leave it alone", so "this is not a link"
and "I could not find out" have to stay distinguishable: the first is an
empty string with a nil error, the second is an error.
ReadLink goes to 100%.
S3 lowercases user metadata keys. SymLink writes "SymLink" and ReadLink
looks up output.Metadata["SymLink"], but the value comes back under
"symlink", so the lookup misses and ReadLink returns an empty string with
a nil error. To the caller that reads as "this object is not a link"
rather than as a failure.
deb.packageIndexByHash uses ReadLink to find the physical file its .old
pointer refers to, and removes it before rotating:
linkTarget, err = publishedStorage.ReadLink(oldIndexPath)
if err == nil {
_ = publishedStorage.Remove(linkTarget)
}
With "" returned the Remove is a no-op and its error is discarded, so the
superseded by-hash index file is never deleted. The pointers rotate
correctly and the physical files accumulate: one per index file per hash
algorithm per publish, indefinitely. Filesystem backends are unaffected
because they have real symlinks.
Measured on a live R2-backed archive publishing three suites a few times
a day: dists/ held 711 objects, of which 672 were by-hash and only 39
were current indexes, growing by about 48 objects per publish since
Acquire-By-Hash was enabled.
The existing TestSymLink cannot catch this, because it ends in
c.Skip("copy not available in s3test") and never runs. The regression
test added here writes the metadata with PutObject instead, so it needs
no CopyObject support, and fails before this change with
obtained string = "".
When a publishing uses a publish prefix, instead of listing the contents
of the whole bucket under the storage prefix, only list the contents of
the bucket under the storage prefix and publish prefix, and cache it by
publish prefix.
This speeds up publish operations under a prefix.
DELETE requests, both for temporary files and no longer referenced
packages, lacked the configured path prefix and therefor were not
removed if a prefix is configured.
Original PR: #621Fixes: #619
I've added unit-test to Martyn's PR.
Without this fix, if `prefix` is set on S3 publish endpoint,
aptly would incorrectly build path cache and re-upload every object
on publish.
This is related to #506
As a first step, don't pass MD5 explicitly, pass checksum info object,
so that as a next step we can choose which hash to use.
There should be no functional changes so far.
Next step: stop returning explicit paths from public package pool.
Martin Simon
André Roth
Christoph Fiehe
Ludovico Cavedon
Sylvain Baubeau
boxjan
Andrey Smirnov
Oliver Sauder
Moritz Bechler
Chris Read