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

Conversation

@iaguis
Copy link
Member

@iaguis iaguis commented Sep 18, 2017

This can happen if the pod dies but we don't have time to register
the app exit code.

Let's be more flexible here and continue even then.

// after a pod dies) let's continue instead of erroring out, the exit code
// will be -1 and we'll let callers handle it.
code, _ := pod.AppExitCode(app.Name)
exitCode := int32(code)
Copy link
Member

Choose a reason for hiding this comment

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

Exit codes should be in the 0-255 range though (u8). I've no idea if this would confuse any consumer, but it looks a bit sketchy. I suggest to handle the error instead and fallback to either 1 (generic error code) or 254 (rkt error code).

Copy link
Member Author

Choose a reason for hiding this comment

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

That sounds reasonable. Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

I went with a generic error code since we don't know what really happened.

This can happen if the pod dies but we don't have time to register
the app exit code.

Let's be more flexible here and continue even then.
@iaguis iaguis force-pushed the iaguis/relax-exit-code branch from bb9a61d to 374d8ba Compare September 19, 2017 09:30
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 added this to the 1.29.0 milestone Sep 19, 2017
@lucab lucab merged commit 5e83d91 into rkt:master Sep 20, 2017
@iaguis iaguis deleted the iaguis/relax-exit-code branch January 9, 2018 12:05
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.

2 participants