Refactor error handling #303
Conversation
obbardc
force-pushed
the
wip/obbardc/cleanup-error-handling
branch
2 times, most recently
from
4d0f172
to
9d2b107
Compare
obbardc
force-pushed
the
wip/obbardc/cleanup-error-handling
branch
from
9d2b107
to
6687aa2
Compare
obbardc
force-pushed
the
wip/obbardc/cleanup-error-handling
branch
from
6687aa2
to
de7d9b2
Compare
obbardc
force-pushed
the
wip/obbardc/cleanup-error-handling
branch
from
de7d9b2
to
b75eafc
Compare
|
The |
|
Any updates on this? As mentioned above - the |
It's on my TODO list for Jan. |
|
If you can add some tests, that would be great. The MR from Ed had a recent amount IMHO. |
obbardc
force-pushed
the
wip/obbardc/cleanup-error-handling
branch
from
b75eafc
to
0516571
Compare
|
@obbardc please fix the CI errors |
Sure, this also needs further rework so I have marked it as a draft. |
sjoerdsimons
force-pushed
the
wip/obbardc/cleanup-error-handling
branch
from
0516571
to
a25cb9a
Compare
obbardc
force-pushed
the
wip/obbardc/cleanup-error-handling
branch
3 times, most recently
from
3f91bbe
to
32e7464
Compare
obbardc
force-pushed
the
wip/obbardc/cleanup-error-handling
branch
from
32e7464
to
2810962
Compare
|
Humble poke? |
Rather than panicing on error, bubble up an error so that the execution may recover correctly and the error shown to the user. Fixes: go-debos#413 Signed-off-by: Christopher Obbard <[email protected]>
Currently the debos error handling sets an exitcode during the run, then exits with that code once execution has finished. This paradigm is quite fragile, so let's switch around the error handling to use context.State to track errors and check the state of this variable after the run has completed. While we are here, rename the checkError function to handleError to better describe the function's intent and make it return a boolean to show that an error has been handled. Rework the do_run function to also return a boolean to show that an error occurred during the run. Signed-off-by: Christopher Obbard <[email protected]>
Signed-off-by: Christopher Obbard <[email protected]>
Rework the error handling to show where an error occured during the run. Also print all error messages to the log rather than stdout. Signed-off-by: Christopher Obbard <[email protected]>
obbardc
force-pushed
the
wip/obbardc/cleanup-error-handling
branch
from
2810962
to
aa1e5a0
Compare
There was a problem hiding this comment.
You mention "this paradigm is quite fragile" but all this change seems to do is rather then having a local variable track the error state (in the form of error code) you moved it to marking it in the context. I'm not really sure why that's less fragile ;)
it does make the code a big more clear though so seems reasonable
| strings.Contains(value, "127.0.0.1") || | ||
| strings.Contains(value, "::1") { | ||
| strings.Contains(value, "127.0.0.1") || | ||
| strings.Contains(value, "::1") { |
There was a problem hiding this comment.
these don't look like typo fixes
| PrintRecipe bool `long:"print-recipe" description:"Print final recipe"` | ||
| DryRun bool `long:"dry-run" description:"Compose final recipe to build but without any real work started"` | ||
| DisableFakeMachine bool `long:"disable-fakemachine" description:"Do not use fakemachine."` | ||
| Backend string `short:"b" long:"fakemachine-backend" description:"Fakemachine backend to use" default:"auto"` |
There was a problem hiding this comment.
more spurious changes that aren't typo fixes
Currently the debos error handling sets an exitcode
during running, then exits with that code. Since we
only ever exit with a 1, unless an error occurs while
running fakemachine, let's switch around the error
handling to use the context.State and check the state
after running.
Signed-off-by: Christopher Obbard [email protected]
See-also the Fakemachine improvements: go-debos/fakemachine#74