Skip to content
This repository was archived by the owner on Feb 24, 2020. It is now read-only.

rkt: get name from ImageManifest #1701

Merged
merged 3 commits into from
Nov 3, 2015
Merged

rkt: get name from ImageManifest #1701

merged 3 commits into from
Nov 3, 2015
+171 −35

Conversation

@blixtra
Copy link
Contributor

@blixtra blixtra commented Oct 30, 2015

Currently we are relying on an optional field in the PodManifest to get
the images name and version when running rkt list because, since one
has been able to remove referenced images, there was no guarantee that
the ImageManifest in the cas would be available.

However, little did we know, there is a copy of the ImageManifest of the
pod in the pod's appinfo directory. This commit makes use of that.

Should close #1659.

@blixtra
Copy link
Contributor Author

blixtra commented Oct 30, 2015

Oh yeah. It even has tests. :)

@yifan-gu
Copy link
Contributor

Good found @blixtra ! I will make the same change in the api service :)

yifan-gu
yifan-gu reviewed Oct 30, 2015
View reviewed changes
rkt/pods.go
Copy link
Contributor

Choose a reason for hiding this comment

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

Any particular reason for changing this?

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 changed this because earlier in the file, in getApps it was doing it this way so I want to make it perform the same task the same way. But your comment got me looking at it a bit closer and I've now removed the body of getAppCount completely and reuse getApps. Thanks!

@yifan-gu
Copy link
Contributor

LGTM except a small nit. Also it would be helpful to have something like func (p *pod) GetImageManifestForApp() so the code can be reusable.

@blixtra blixtra force-pushed the blixtra/rkt-get-name-from-imagemanifest branch 2 times, most recently from 1635617 to f5e1d16 Compare November 2, 2015 12:13
@blixtra
Copy link
Contributor Author

blixtra commented Nov 2, 2015

LGTM except a small nit. Also it would be helpful to have something like func (p *pod) GetImageManifestForApp() so the code can be reusable.

I've added a function func (p *pod) getAppsImageManifests() which returns a map with key of type ACName and a value of ImageManifest. This fits in better with the other pod methods.

yifan-gu
yifan-gu reviewed Nov 2, 2015
View reviewed changes
rkt/pods.go Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

How about use a pointer *schema.ImageManifest?

@blixtra
rkt: get image name from appinfo ImageManifest
a71e3f2 
Currently we are relying on an optional field in the PodManifest to get
the images name and version when running `rkt list` because, since one
has been able to remove referenced images, there was no guarantee that
the ImageManifest in the cas would be available.

However, little did we know, there is a copy of the ImageManifest of the
pod in the pod's appinfo directory. This commit makes use of that.

fixes rkt#1659
@blixtra
test: check that rkt list returns assumed fields
4fd5652 
Adds tests for `rkt list` to insure that assumed fields are displayed.
@blixtra blixtra force-pushed the blixtra/rkt-get-name-from-imagemanifest branch from f5e1d16 to 4fd5652 Compare November 2, 2015 19:30
@yifan-gu
Copy link
Contributor

yifan-gu commented Nov 2, 2015

LGTM

iaguis added a commit that referenced this pull request Nov 3, 2015
@iaguis
Merge pull request #1701 from kinvolk/blixtra/rkt-get-name-from-image…
0e2b9af 
…manifest

rkt: get name from ImageManifest
@iaguis iaguis merged commit 0e2b9af into rkt:master Nov 3, 2015
@iaguis iaguis mentioned this pull request Nov 3, 2015
Merged
yifan-gu
yifan-gu reviewed Jan 6, 2016
View reviewed changes
rkt/list.go
Copy link
Contributor

Choose a reason for hiding this comment

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

I was trying to reuse this function in #1936
Turns out getImageName is in a loop, so that getAppsImageManifests is called multiple times, while it actually only needs to be called once.
@blixtra Am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

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

can we actually have something like getAppImageManifest to get a single image manifest?

Copy link
Member

Choose a reason for hiding this comment

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

For the record, done in #1938

@blixtra
Copy link
Contributor Author

blixtra commented Jan 6, 2016

@yifan-gu I'll have a look.

@blixtra blixtra mentioned this pull request Jan 6, 2016
Merged
jonboulle added a commit to jonboulle/rkt that referenced this pull request Jan 7, 2016
@jonboulle
common: fix function name AppImageManifestPath
da6cc23 
Noticed in rkt#1701 that it appeared to have gained a superfluous modifier
and the docstring was out of sync with the function name
@jonboulle jonboulle mentioned this pull request Jan 7, 2016
Merged
blixtra pushed a commit to kinvolk/rkt that referenced this pull request Jan 8, 2016
@jonboulle @blixtra
common: fix function name AppImageManifestPath
8966b6b 
Noticed in rkt#1701 that it appeared to have gained a superfluous modifier
and the docstring was out of sync with the function name
blixtra pushed a commit to kinvolk/rkt that referenced this pull request Jan 8, 2016
@jonboulle @blixtra
common: fix function name AppImageManifestPath
537f0e8 
Noticed in rkt#1701 that it appeared to have gained a superfluous modifier
and the docstring was out of sync with the function name
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

priority/P1 reviewed/lgtm

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

rkt list: Retrieve Image name from treestore when Image has been removed.

4 participants

@blixtra @yifan-gu @alban @iaguis