Minor fixes and changes to warnings handling in option checkers #1086
Conversation
When the check was first added [1] it was the last in the function and was written to immediately return. New checks were added after the check, without changing the early return, which means invalid configurations were not caught. Append to warnings instead of returning. [1] 665a128
Always return the warnings list from checkOptions() so that future modifications or reshuffling of the checks doesn't drop any warnings collected before the return.
Always return the warnings list from checkOptions() so that future modifications or reshuffling of the checks doesn't drop any warnings collected before the return.
When collecting warnings in checkOptions() functions, don't `log.Print()` the warning. Let the caller decide whether to print them, suppress them, make them errors, format them, etc.
|
Rebased and resolved conflicts. |
achilleas-k
force-pushed
the
warnings-fiddling
branch
from
6072c24 to
db697f2
Compare
| @@ -293,31 +293,31 @@ func (t *imageType) checkOptions(bp *blueprint.Blueprint, options distro.ImageOp | |||
| var warnings []string | |||
|
|
|||
| if !t.rpmOstree && options.OSTree != nil { | |||
| return nil, fmt.Errorf("OSTree is not supported for %q", t.Name()) | |||
| return warnings, fmt.Errorf("OSTree is not supported for %q", t.Name()) | |||
There was a problem hiding this comment.
I'm not sure I follow here (for this commit and for commit 811bc5f). The way I see it is that the go convention is that for cases like this return nil, err is the norm, adding the warnings here seems to be unnecessary. I read in the commit message that it is to guard against code-shuffling but looking at this case here the only way that warnings would get dropped is if we chnage the code from a) erroring in this case and b) still returning early. That seems quite unlikely (and would hopefully get caught in code reviews).
I guess there is a argument that it might be nice to show the user both the error and the warnings so that they can fix both but that interpretation does not matches the commit message. Maybe I'm missing something?
what exactly would this protect against?
There was a problem hiding this comment.
I guess there is a argument that it might be nice to show the user both the error and the warnings so that they can fix both
That's the idea, yeah.
but that interpretation does not matches the commit message. Maybe I'm missing something?
The commit message mentioning reshuffling is because I was expecting (or assuming) an argument that it doesn't make sense to return warnings before there's any reason for it to be set. Warnings only really get set pretty late in the function. I suppose a more complete message would say something like:
Always return warnings so the caller gets both warnings and error messages for
the customizations and options. Return from the top even if no warnings could
possibly be generated to cover potential future additions or reordering.
Which makes me think, maybe we should be collecting all errors too and only returning them at the end, either as a list or Join()ed.
distro/fedora: don't return early from FIPS check in checkOptions()
When the check was first added [1] it was the last in the function and
was written to immediately return. New checks were added after the
check, without changing the early return, which means invalid
configurations were not caught.
Append to warnings instead of returning.
[1] 665a128
distro/fedora: always return warnings from checkOptions()
Always return the warnings list from checkOptions() so that future
modifications or reshuffling of the checks doesn't drop any warnings
collected before the return.
distro/rhel: always return warnings from checkOptions()
Always return the warnings list from checkOptions() so that future
modifications or reshuffling of the checks doesn't drop any warnings
collected before the return.
distro/rhel: don't print warnings in checkOptions()
When collecting warnings in checkOptions() functions, don't
log.Print()the warning. Let the caller decide whether to print them,suppress them, make them errors, format them, etc.