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.

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

Images are themselves manifests, and reference a config blob #79

Merged

Conversation

smarterclayton
Copy link
Contributor

@smarterclayton smarterclayton commented Jul 28, 2018

The naming of the nested field was wrong (this known as the image "config") and we need to correct it. An OpenShift image is always a manifest. This is naming only.

Second change: Allow the API to return info about images whose references are not resolvable

Administrators may delete an image intentionally or accidentally, and the layers API needs to be able to inform the registry that the image is referenced, but no layer data is available, so that the registry can more accurately return an error to the client.

@liggitt as discussed. The first is an error @bparees and I found when updating api.ci. The second is our discussion about references.

@openshift-ci-robot openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jul 28, 2018
@smarterclayton smarterclayton force-pushed the correct_image_layer_name branch from 8ddd177 to df28b03 Compare July 28, 2018 20:00
// imagesWithoutMetadata contains information about all images referenced by an image
// stream whose metadata could not be loaded. This can be due to an administrator or
// other automated process deleting the image (in which case the image cannot be pulled)
// or be a temporary condition because the image has not yet been uploaded.
Copy link
Contributor

Choose a reason for hiding this comment

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

@smarterclayton when can we have a reference to the image that has not yet been uploaded?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If someone writes an integration that uses the API but does so without verifying the references. We don't allow it directly, but I want to leave this open so that a consumer of the API knows that this is possible even if we're confident none of our existing code paths allow it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can change the godoc if you'd like, but I do want to call out that this API is dependent on both the object references and the object metadata.

Copy link
Contributor

@liggitt liggitt Aug 6, 2018

Choose a reason for hiding this comment

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

is it better to have this as a separate field, or have a missing attribute of ImageBlobReferences that can only be true if config and layers are unset? looking up an image in two places is a pain

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could do either.

Copy link
Contributor

Choose a reason for hiding this comment

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

@deads2k opinion on ^?

Copy link
Contributor

Choose a reason for hiding this comment

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

@liggitt @smarterclayton I prefer a missing attribute on ImageBlobReferences. I don't care for parallel maps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@smarterclayton
Copy link
Contributor Author

@deads2k since liggitt is out - this is a must for 3.11 (since the field is new). Jordan and I had verbal agreement on a separate map for tracking missing references, but we didn't talk naming or structure.

@liggitt
Copy link
Contributor

liggitt commented Aug 6, 2018

one question on the separate map vs additional field on existing ImageBlobReferences type. LGTM otherwise

The naming of the nested field was wrong (this known as the image "config")
and we need to correct it. An OpenShift image is always a manifest.
@smarterclayton smarterclayton force-pushed the correct_image_layer_name branch from df28b03 to a6b8d79 Compare August 14, 2018 17:42
@smarterclayton
Copy link
Contributor Author

Updated

@deads2k
Copy link
Contributor

deads2k commented Aug 14, 2018

Need to regenerate proto. We've been encouraging https://github.com/openshift/api#pull-request-process. I don't think this will cause problems, but something to keep in mind.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 14, 2018
@smarterclayton
Copy link
Contributor Author

Ok, will do it now. Didn't see that before.

Communicates when an image stream references an image api object that has been
deleted by an administrator.
@smarterclayton smarterclayton force-pushed the correct_image_layer_name branch from a6b8d79 to 1ba94e5 Compare August 14, 2018 18:41
@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 14, 2018
@smarterclayton smarterclayton added the lgtm Indicates that a PR is ready to be merged. label Aug 14, 2018
@smarterclayton smarterclayton merged commit 072d197 into openshift:master Aug 14, 2018
@smarterclayton
Copy link
Contributor Author

openshift/origin#20643

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants