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

Check for Image in config object #6

Merged
merged 1 commit into from Apr 11, 2017

Conversation

jhadvig
Copy link
Member

@jhadvig jhadvig commented Apr 11, 2017

There was a bug opened that Identifier value is not set in the console.
screenshot_from_2017-04-10_17-58-37

Was talking to @mfojtik and he said that we are not setting the Image attribute in the ImageConfig since 1.4
@stefwalter PTAL

@@ -19,8 +19,8 @@
<dd ng-if="!labels['build-date'] && !layers[0].v1Compatibility.created && image.dockerImageMetadata.Created" title="{{image.dockerImageMetadata.Created}}">{{image.dockerImageMetadata.Created | dateRelative}}</dd>
<dt translate>Digest</dt>
<dd><tt>{{ image.metadata.name }}</tt></dd>
<dt translate>Identifier</dt>
<dd><tt>{{ config.Image }}</tt></dd>
<dt ng-if="config.Image" translate>Identifier</dt>
Copy link
Member

Choose a reason for hiding this comment

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

use ng-if-start and ng-if-end instead of duplicating the if check

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, was thinking about it, but we use the same (dup.) logic on other places in this file so want sure which one to use.
Will update

@mfojtik
Copy link
Member

mfojtik commented Apr 11, 2017

@miminar @legionus PTAL please. (config.Image maps to the DockerConfig struct that we don't have for v2 images AFAIK).

@jhadvig
Copy link
Member Author

jhadvig commented Apr 11, 2017

@jwforres @stefwalter updated

@jwforres
Copy link
Member

jwforres commented Apr 11, 2017

change LGTM, not sure if @stefwalter had a merge queue or tests set up for this repo, guessing no?

@stefwalter
Copy link
Contributor

stefwalter commented Apr 11, 2017

Smoke tested this manually by editing tests/image.jsonp. Does what it says on the tin.

So @jwforres is Origin including even less information about Docker images now? I guess that'll affect the registry console and display of images in OWC, correct?

@jhadvig Could you point to the exact change in origin that this is a result of?

@stefwalter stefwalter merged commit 838001c into openshift:master Apr 11, 2017
@jhadvig
Copy link
Member Author

jhadvig commented Apr 11, 2017

@stefwalter I guess @mfojtik will have more info about that

@mfojtik
Copy link
Member

mfojtik commented Apr 12, 2017

@stefwalter the manifest removal is here: openshift/origin#11925

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.

None yet

4 participants