preinstall: Return warnings as errors if RunChecks is already going to return an error

[chrisccoulson]

If RunChecks returns one or more errors, any warnings that have been
collected and which would normally be returned via the CheckResults
structure will be discarded. In the case where RunChecks is already
going to return one or more errors, it should also return any warnings
that occurred as errors as well.

[pedronis]

@chrisccoulson I'm not entirely sure I understand this change, some warnings are because the user has permitted something but if they are turned again into errors it would seem like the user/caller didn't permit things they did

[chrisccoulson]

@chrisccoulson I'm not entirely sure I understand this change, some warnings are because the user has permitted something but if they are turned again into errors it would seem like the user/caller didn't permit things they did

Yes, that's a good point - it does introduce a little bit of confusion.

I've been trying to keep warnings distinct from errors and to not blur the lines between them so much that snapd feels like it needs to do anything with warnings. This is why the returned warnings do not get converted to WithKindAndActionsError errors and aren't suitable to export from snapd's API. A warning is essentially just an error that doesn't prevent us from creating a valid profile, and all I'm expecting from snapd wrt to warnings is that they get logged somewhere because they could be useful in debugging future issues install / boot issues.

The problem is that when we return one or more errors, we don't return a CheckResult instance and so we lose any warnings because that's the object that carries the warnings. The idea here was to bundle warnings with the errors, regardless of the input flags because some of those warnings might be contributing to us failing to create a valid profile.

I don't mind dropping it if it causes too much confusion, but if we genuinely aren't able to create a valid PCR profile configuration, I'd like to be able to expose the warnings in some way.

[pedronis]

@chrisccoulson

I don't mind dropping it if it causes too much confusion, but if we genuinely aren't able to create a valid PCR profile configuration, I'd like to be able to expose the warnings in some way.

I think it does causes confusion and I would drop this change unless there would be a way to get from the error that they are actually only warnings and snapd could filter them out? but then again if we allow that then it's unclear whether it's bad to drop them as we do now

[ernestl]

Surfacing warnings as errors seems useful if we provide the user [UI] with the opportunity to make configuration changes that will remove warnings, at install time using recovery actions.

If we do, then it makes sense to treat it similar to errors. One option will be to have non-fatal error-kinds at a level that the installer can distinguish. In this case rather remove warnings from CheckResult altogether, for consistent experience.
We can still log non-fatal error-kinds, same as we do warnings today.

[chrisccoulson]

I'll skip this one for now - I've rebased the next PR (#405) on master