Reverse the exitcode initialization value #278

eds-collabora · 2021-08-09T12:39:52Z

No description provided.

cmd/debos/debos.go

eds-collabora · 2021-08-09T20:43:53Z

Summary of comments on this code to date:

evelikov · 2021-08-11T17:46:32Z

In case it matters, the PR is:
Reviewed-by: Emil Velikov <[email protected]>

If it were me, I would squash all the patches, adding a small note at the bottom for the extra changes wrt the original. Not a huge deal either way. Example:

...
This commit reverses the logic of the exitcode: it is initialized to 1
(i.e. failure), and it is set to 0 (i.e. success) only if Debos has
reached the end of the things it has to do (or for the help message).

Signed-off-by: Gaël PORTAY <[email protected]>

[Ed Smith]
 - Merge lines …
 - Fix exit behaviour when using fakemachine …
Signed-off-by: ... < if the project needs it, not sure

Edit: and you've done it already, but the time I wrote this. Nice

evelikov · 2021-08-11T17:48:37Z

Btw would be nice to hear from @sjoerdsimons what testing he had in plan in the earlier PR #198.

Perhaps even close the old/superseded PR.

obbardc · 2021-08-13T12:48:05Z

Btw would be nice to hear from @sjoerdsimons what testing he had in plan in the earlier PR #198.

the comment was:

testing of the various failure and test cases and with both kvm and non-fakemachine runs

docker/exitcode-test.yaml

tests/exit_test/bad.yaml

tests/exit_test/good.yaml

evelikov · 2021-09-09T12:29:25Z

With a bunch of tests in place (thanks Ed), can we get this trivial PR merged?

obbardc · 2022-01-07T11:47:18Z

I took another look at this and wasn't happy; decided it would be cleaner to rework the panics in the code to bubbled-up errors: go-debos/fakemachine#74

Fakemachine is subject to panic and causes Debos to exit success due to the current logic of the exitcode. For example, the fakemachine function CopyFileTo() panics if the file is missing. In the case of a panic, the function never returns. Thus, the exitcode cannot be set to 1 and Debos exits with 0. This commit reverses the logic of the exitcode: it is initialized to 1 (i.e. failure), and it is set to 0 (i.e. success) only if Debos has reached the end of the things it has to do (or for the help message). Co-authored-by: Gaël PORTAY <[email protected]> Co-authored-by: Santosh Mahto <[email protected]> Signed-off-by: Gaël PORTAY <[email protected]>

sjoerdsimons · 2024-01-13T16:19:14Z

@obbardc would probably be worth picking up the tests that were added here even if you ended up doing the error handlign improvements in another way?

obbardc · 2024-01-15T10:43:47Z

@obbardc would probably be worth picking up the tests that were added here even if you ended up doing the error handlign improvements in another way?

Yeah, I will pick the tests up from this PR and cherry pick onto !303. Once that is done I will close this PR.

obbardc requested changes Aug 9, 2021

View reviewed changes

cmd/debos/debos.go Outdated Show resolved Hide resolved

cmd/debos/debos.go Show resolved Hide resolved

obbardc reviewed Aug 9, 2021

View reviewed changes

cmd/debos/debos.go Show resolved Hide resolved

eds-collabora marked this pull request as ready for review August 11, 2021 17:35

eds-collabora force-pushed the eds/exitcode_fix2 branch from 257601c to 5df60ff Compare August 11, 2021 17:42

obbardc mentioned this pull request Aug 13, 2021

Reverse the exitcode initialization value #198

Closed