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

rkt: exit with status 1 on image rm failure #1486

Merged
merged 2 commits into from Oct 2, 2015

Conversation

blixtra
Copy link
Collaborator

@blixtra blixtra commented Sep 25, 2015

When rkt image rm cannot remove one or more of the images that it is passed, it should return an exit status of 1. Up to now an exit status of 0 has been returned whether all, some or no images could be removed.

Closes #1483

@yifan-gu
Copy link
Contributor

@blixtra I think we can squash the commits into one as it's a small patch.
LGTM after squash.

@yifan-gu
Copy link
Contributor

Well, semaphore is not green. seems in pod_manifest_test, it fails to rm images.

@blixtra
Copy link
Collaborator Author

blixtra commented Sep 26, 2015

@yifan-gu I think there's a problem in that test. I'll look into it.

@blixtra
Copy link
Collaborator Author

blixtra commented Sep 28, 2015

That test is failing because it was doing a rkt image rm on referenced images and was relying on the return status for reporting the errors which, with this commit, it now gets..

However, I'd suggest we wait till #1240 is merged. That change makes it so that rkt image rm will not fail with existing references as the reference will be to the tree store, not the images. Thus, the issue in the failing test should not be an issue.

@iaguis
Copy link
Member

iaguis commented Sep 28, 2015

#1240 is merged now

@jonboulle
Copy link
Contributor

ping

If `rkt image rm` is not able to remove one or more of the images it
should exit with a status 1.
The "Multiple apps" was using the exact same image to run the test but
expecting different hashes to be returned. Because the same hash was
being returned and a `rkt image rm` command was being issued for each
hash, the second attempt to remove the image was failing.

This change applies a simple name patch to each image to force a unique
hash.
@blixtra blixtra force-pushed the blixtra/rkt-image-rm-exit-status branch from 620a96e to feaa5ed Compare October 2, 2015 21:47
@blixtra
Copy link
Collaborator Author

blixtra commented Oct 2, 2015

@jonboulle pong ;-) This should pass.

@jonboulle
Copy link
Contributor

thanks!

jonboulle added a commit that referenced this pull request Oct 2, 2015
rkt: exit with status 1 on image rm failure
@jonboulle jonboulle merged commit d6c8a33 into rkt:master Oct 2, 2015
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.

None yet

4 participants