Skip to content

Conversation

@lzap
Copy link
Collaborator

@lzap lzap commented Sep 23, 2025

The exporter and importer types have a logger that is used for issuing warnings and errors. The idea was to continue conversion even if some parts of the blueprint were incorrect.

This patch changes that for the exporter to return errors instead of logging them. The importer will be changed in a follow-up patch.


I am taking a hybrid approach with errors - errors are collected in the two main functions (top-level exporting function and customization function) and joined using errors.Join. This means is when an error occurs, the process will continue and finish off the conversion and then the error(s) are returned. Also I make an effort to return data that are still valid in case something is going on, for example in exportUserCustomization when converting more than one ssh key (or keyboard layout).

I just cannot get rid of a feeling the convertor should still be a little bit of a "code compiler" experience - when it encounter one error, it does not mean it stops parsing. Modern compilers will go further and try to identify as many problems as possible as long as the error is not fatal (like a missing bracket).

Anyways, if you insist on a strict approach, and something is telling me you will, then I will simply change the code to just return single error and do normal Go error checking everywhere. This will probably render all "invalid" conversion testdata a bit irrelevant - it will always render to one error which is fine I guess.

@achilleas-k
Copy link
Member

I just cannot get rid of a feeling the convertor should still be a little bit of a "code compiler" experience - when it encounter one error, it does not mean it stops parsing. Modern compilers will go further and try to identify as many problems as possible as long as the error is not fatal (like a missing bracket).

I'm fine with this and I agree that it's a better experience to see all the problems in the source from the first run so one can fix everything before trying again. What I (and others) have pushed back on before was the idea of falling back to default values when something is invalid and only producing warnings.

But this is good. I like this.

@lzap lzap marked this pull request as ready for review October 22, 2025 14:26
@lzap lzap requested a review from a team as a code owner October 22, 2025 14:26
@lzap lzap requested review from bcl, mvo5 and supakeen October 22, 2025 14:26
The exporter and importer types have a logger that is used for issuing warnings
and errors. The idea was to continue conversion even if some parts of the
blueprint were incorrect.
@lzap lzap changed the title conv: use errors instead of logger for all problems for exporter conv: use errors instead of logger for convertors Oct 22, 2025
@supakeen
Copy link
Member

Joining the errors together like this is fine in my book; I like to see them all so I don't have to go back and forth all the time.

@achilleas-k achilleas-k enabled auto-merge October 27, 2025 13:57
@achilleas-k achilleas-k added this pull request to the merge queue Oct 28, 2025
Merged via the queue into osbuild:main with commit fea1a55 Oct 28, 2025
2 checks passed
@lzap lzap deleted the errors1 branch October 29, 2025 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants