-
Notifications
You must be signed in to change notification settings - Fork 403
list,oci_index: automatically add inbuilt annotations on add #1992
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@mtrmac PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK.
Another part (that can be this PR or a separate one) is to ensure that skopeo copy --format zstd
, and a plain skopeo copy
that uses zstd (e.g. because it reuses an existing blob)) also adds that annotation.
7d54792
to
1bf94cf
Compare
7b98156
to
e1652f5
Compare
e1652f5
to
a18bee5
Compare
@vrothberg @mtrmac PTAL |
17e4957
to
d3ad950
Compare
@mtrmac PTAL |
@mtrmac Is Anything left pending in this PR ? I have reverted some of the changes, so we can revisit them later once we have minimal working model in place. WDYT ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall.
Some unit tests would be nice but not blocking.
Adding instance to list should automatically add special annotations if any. Signed-off-by: Aditya R <arajan@redhat.com>
d3ad950
to
4cf082a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
default: | ||
return fmt.Errorf("internal error: invalid operation: %d", editInstance.ListOperation) | ||
} | ||
} | ||
if len(addedEntries) != 0 { | ||
index.Manifests = append(index.Manifests, addedEntries...) | ||
} | ||
if len(addedEntries) != 0 || updatedAnnotations { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch!
Note for the record: Arguably we should re-sort only on compression annotation change.
But then, we are not computing compression annotations precisely right now (only adding, not deleting), so… we can’t exactly guarantee specific behavior. The two boundary cases (UpdateInstances
with no annotation input, and EditInstances
adding Zstd) both work correctly.
Adding instance to list should automatically add special annotations if any.