Skip to content

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

Merged
merged 1 commit into from
Jun 27, 2023

Conversation

flouthoc
Copy link
Contributor

Adding instance to list should automatically add special annotations if any.

@flouthoc
Copy link
Contributor Author

@mtrmac PTAL

Copy link
Collaborator

@mtrmac mtrmac left a 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.

@flouthoc flouthoc force-pushed the list-add-annotations branch 2 times, most recently from 7d54792 to 1bf94cf Compare June 12, 2023 17:39
@flouthoc flouthoc requested a review from mtrmac June 12, 2023 17:39
@flouthoc flouthoc force-pushed the list-add-annotations branch 2 times, most recently from 7b98156 to e1652f5 Compare June 13, 2023 04:28
@flouthoc flouthoc force-pushed the list-add-annotations branch from e1652f5 to a18bee5 Compare June 14, 2023 09:11
@flouthoc flouthoc requested a review from mtrmac June 14, 2023 09:13
@rhatdan
Copy link
Member

rhatdan commented Jun 14, 2023

@vrothberg @mtrmac PTAL

@flouthoc flouthoc force-pushed the list-add-annotations branch 2 times, most recently from 17e4957 to d3ad950 Compare June 15, 2023 06:14
@flouthoc flouthoc requested a review from mtrmac June 15, 2023 11:14
@flouthoc
Copy link
Contributor Author

@mtrmac PTAL

@flouthoc
Copy link
Contributor Author

@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 ?

Copy link
Collaborator

@mtrmac mtrmac left a 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>
@flouthoc flouthoc force-pushed the list-add-annotations branch from d3ad950 to 4cf082a Compare June 27, 2023 08:03
@flouthoc flouthoc requested a review from mtrmac June 27, 2023 08:10
Copy link
Collaborator

@mtrmac mtrmac left a 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 {
Copy link
Collaborator

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.

@mtrmac mtrmac merged commit 35341f8 into containers:main Jun 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants