Skip to content
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.

Sign up for GitHub

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

Jump to bottom

listupdate,oci: instance show read-only annotations and CompressionAlgorithmNames #2040

Merged
merged 1 commit into from
Jul 17, 2023
Merged

listupdate,oci: instance show read-only annotations and CompressionAlgorithmNames #2040

merged 1 commit into from
Jul 17, 2023

Conversation

flouthoc
Copy link
Contributor

@flouthoc flouthoc commented Jul 14, 2023 •
edited
Loading

There is a need to read annotations of a particular instance to get its compression. Expose Annotations as a read-only field.

Needed By: #1987
Annotations will be used by prepareInstanceCopies of above PR to detect compression on the instance.

@flouthoc
Copy link
Contributor Author

@mtrmac @vrothberg PTAL

mtrmac
mtrmac reviewed Jul 14, 2023
View reviewed changes
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.

For #1987 : the manifest format handlers implement mapping algorithms to annotations, so they should also implement mapping annotations to algorithms.

I.e. add a CompressionAlgorithms field instead. Or rather, maybe CompressionAlgorithmNames instead, so that internal/manifest does not depend on pkg/compression.AlgorithmByName and does not bring all of c/storage/pkg/chunked/compressor to every metadata-only user.

(Adding annotations as well wouldn’t hurt but I think isn‘t immediately relevant.)

@flouthoc flouthoc force-pushed the readonly-annotations branch from e0910b9 to 9d5e193 Compare July 15, 2023 08:34
@flouthoc flouthoc changed the title listupdate,oci: instance show read-only annotations listupdate,oci: instance show read-only annotations and CompressionAlgorithmNames Jul 15, 2023
@flouthoc flouthoc requested a review from mtrmac July 15, 2023 08:35
mtrmac
mtrmac reviewed Jul 15, 2023
View reviewed changes
internal/manifest/list.go Outdated Show resolved Hide resolved
internal/manifest/oci_index.go Outdated Show resolved Hide resolved
@flouthoc flouthoc force-pushed the readonly-annotations branch from 9d5e193 to c650bd4 Compare July 16, 2023 03:12
@flouthoc flouthoc requested a review from mtrmac July 17, 2023 05:55
vrothberg
vrothberg reviewed Jul 17, 2023
View reviewed changes
internal/manifest/list.go Outdated
Digest digest.Digest
Size int64
MediaType string
Platform *imgspecv1.Platform // read-only field: may be set by Instance(), ignored by UpdateInstance()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider moving the read-only fields into an inner struct:

type ListUpdate struct {
    [...]
    struct ReadOnly {
        Platform string
        [...]
    }
}

This way, all users have to access it via (ListUpdate).ReadOnly.Platform which is quite expressive.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice Idea, Done

internal/manifest/oci_index.go Outdated Show resolved Hide resolved
internal/manifest/oci_index_test.go Outdated Show resolved Hide resolved
@flouthoc flouthoc force-pushed the readonly-annotations branch from c650bd4 to 54c1a97 Compare July 17, 2023 11:01
@flouthoc flouthoc requested a review from vrothberg July 17, 2023 11:01
@flouthoc flouthoc mentioned this pull request Jul 17, 2023
Merged
vrothberg
vrothberg approved these changes Jul 17, 2023
View reviewed changes
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

mtrmac
mtrmac reviewed Jul 17, 2023
View reviewed changes
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.

One last nit please

internal/manifest/oci_index.go Outdated Show resolved Hide resolved
internal/manifest/oci_index.go Outdated Show resolved Hide resolved
@flouthoc flouthoc force-pushed the readonly-annotations branch from 54c1a97 to 83a64f5 Compare July 17, 2023 14:59
@flouthoc flouthoc requested a review from mtrmac July 17, 2023 15:00
mtrmac
mtrmac approved these changes Jul 17, 2023
View reviewed changes
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.

Thanks!

@flouthoc @mtrmac
listupdate,oci: instance show read-only annotations and CompressionAl…
d9fc9d5 
…gorithmNames

There is a need to read annotations of a particular instance to get its
compression. Expose `Annotations` as a read-only field.

Needed By: containers#1987

Signed-off-by: Aditya R <[email protected]>
@mtrmac mtrmac force-pushed the readonly-annotations branch from 83a64f5 to d9fc9d5 Compare July 17, 2023 15:18
@flouthoc
Copy link
Contributor Author

@mtrmac Thanks for the rebase :) ready for merge

@flouthoc flouthoc requested a review from mtrmac July 17, 2023 15:51
@mtrmac mtrmac merged commit 8032cef into containers:main Jul 17, 2023
@flouthoc flouthoc mentioned this pull request Aug 4, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vrothberg vrothberg vrothberg approved these changes

@mtrmac mtrmac Awaiting requested review from mtrmac

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@flouthoc @mtrmac @vrothberg