8000 stage0: list|status --format=json panics: RuntimeApp.Mounts.AppVolume is optional by fabiokung · Pull Request #3699 · rkt/rkt · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
This repository was archived by the owner on Feb 24, 2020. It is now read-only.

stage0: list|status --format=json panics: RuntimeApp.Mounts.AppVolume is optional #3699

Merged
merged 1 commit into from
Jun 12, 2017

Conversation

fabiokung
Copy link
Contributor

When it is nil, the Volume info at the Pod level (with the same name)
should be used. Without this patch rkt list --format=json panics on a
nil pointer when Apps reference Volumes from the Pod level.

Signed-off-by: Fabio Kung fabio.kung@gmail.com

@ghost
Copy link
ghost commented Jun 7, 2017

Can one of the admins verify this patch?

@euank
Copy link
Member
euank commented Jun 8, 2017

ok to test

Copy link
Member
@lucab lucab left a comment

Choose a reason for hiding this comment

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

Thanks, bugfix looks fine, just two minor style comments.

lib/app.go Outdated
var (
podVols = podManifest.Volumes
podVolsByName = make(map[types.ACName]types.Volume, len(podVols))
)
Copy link
Member

Choose a reason for hiding this comment

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

I would avoid this inline var( ... ) block and just go for two plain := assignments.

lib/app.go Outdated
for _, mnt := range ra.Mounts {
var (
hostPath string
readOnly bool // false by default, since it is optional
Copy link
Member

Choose a reason for hiding this comment

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

Same here, let's just make it explicit as readOnly := false.

When it is nil, the Volume info at the Pod level (with the same name)
should be used. Without this patch `rkt list --format=json` panics on a
nil pointer when Apps reference Volumes from the Pod level.

Signed-off-by: Fabio Kung <fabio.kung@gmail.com>
@fabiokung
Copy link
Contributor Author

@lucab I made the changes you suggested. PTAL

Copy link
Member
@euank euank left a comment
8000

Choose a reason for hiding this comment

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

LGTM.

A regression test, either here or as a followup, would be nice as well.

Copy link
Member
@lucab lucab left a comment

Choose a reason for hiding this comment

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

LGTM

@lucab lucab merged commit 5f42e6b into rkt:master Jun 12, 2017
@fabiokung fabiokung deleted the panic-json branch June 14, 2017 23:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0