Remove existing file during build-disk

[frelon]

The first commit makes build-disk command behave like build-iso in that it will remove an existing raw disk before writing the new disk and log a warning.

Second commit changes the build-iso Makefile target to use the toolkit image instead of the target derivative image to be consistent with build-disk target

[codecov-commenter]

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (e732621) 75.35% compared to head (b779089) 75.29%.

Files	Patch %	Lines
pkg/action/build-disk.go	0.00%	5 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1857      +/-   ##
==========================================
- Coverage   75.35%   75.29%   -0.06%     
==========================================
  Files          67       67              
  Lines        6820     6825       +5     
==========================================
  Hits         5139     5139              
- Misses       1311     1315       +4     
- Partials      370      371       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[davidcassany]

With the exception of the second commit all good to me.

Regarding the second commit (c8b76f2) I have mixed feelings with it. My idea to solve that, was to add a new RunNoError (or similar) into the runner interface and in that interface simply log as warnings any error, but never return error. Which would still call it always but do not present it as an error in logs.

Note this error message is also present on install processes, which is also ugly. There the suggested approach here would still log the red ugly error.

So probably I'd be more in favor of log it as a warning|debug message. I have an implementation here. Probably we can leave that for a separate PR.

[frelon]

Regarding the second commit (c8b76f2) I have mixed feelings with it. My idea to solve that, was to add a new RunNoError (or similar) into the runner interface and in that interface simply log as warnings any error, but never return error. Which would still call it always but do not present it as an error in logs.

That feels so hacky though.. We could consider moving the logging out of the runner for this reason, but I do think that only running partx on files in /dev makes sense. Will have to try it with the install command also to see what happens!

[frelon]

For now I could remove the second commit to get the other changes merged!

[davidcassany]

Regarding the second commit (c8b76f2) I have mixed feelings with it. My idea to solve that, was to add a new RunNoError (or similar) into the runner interface and in that interface simply log as warnings any error, but never return error. Which would still call it always but do not present it as an error in logs.

That feels so hacky though.. We could consider moving the logging out of the runner for this reason, but I do think that only running partx on files in /dev makes sense. Will have to try it with the install command also to see what happens!

Well, I am not convinced either, but this isn't much different than calling Run and ignore the error. On exception based languages I'd code Run with a flag RaiseOnError defaulting to true. Anyway as long as I am aware this is only happening for partx that I am aware of and I'd like to apply the best effort run concept (swallow errors) for blkdeactivate calls, this way we can drop LVM requirement from the spec and simply keep it as a recommended tool.

Another option could be turning all runner logs into debug level.

Alternatively we could also have a quiet flag we set and unset on the call we want to ignore any kind of message.

I think having the logger within the runner is helpful for debugging purposes as we see the exact commands that are executed.