Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Duplicated error messages #727

Open
schuellerf opened this issue Jun 4, 2024 · 8 comments
Open

Duplicated error messages #727

schuellerf opened this issue Jun 4, 2024 · 8 comments

Comments

@schuellerf
Copy link
Contributor

Now as we see the .Reason of errors in the UI it is now duplicated in case of "DNF DepsolveError"
(due to osbuild/image-builder-frontend#2066 )
Screenshot_20240604_192337

I think the duplication comes from:
https://github.com/osbuild/images/blob/main/pkg/dnfjson/dnfjson.go#L716
writing the .Reason into the string while
https://github.com/osbuild/osbuild-composer/blob/main/cmd/osbuild-worker/jobimpl-depsolve.go#L114
uses .Error() and adds .Reason

Possible fix:
@achilleas-k @mvo5 Do you agree that pkg/dnfjson/dnfjson.go should not add the Reason to Error()? or some other solution?

@achilleas-k
Copy link
Member

achilleas-k commented Jun 4, 2024

@achilleas-k @mvo5 Do you agree that pkg/dnfjson/dnfjson.go should not add the Reason to Error()? or some other solution?

I think osbuild-composer (jobimpl-depsolve) should either "trust" the Error() method from dnfjson's error type or construct one itself when it's a dnfjson.Error type, but not do both.

@achilleas-k
Copy link
Member

achilleas-k commented Jun 4, 2024

To expand on that a bit: IMO, the Error() method should be a stringification of the error, and consumers of that error can expect that stringifying it will give them enough information. If they feel like type-converting the error to deconstruct it, it's alright to make a new string out of the exported fields, but I think combining exported fields with Error() will, in many cases, produce duplicate parts.

I think it makes sense for osbuild-composer to reconstruct the string, if it doesn't like the DNF error occurred: part for example. Then again, if that's the reason it's reconstructing it, let's just make osbuild/images/pkg/dnfjson.Error have an Error() method that osbuild-composer likes.

But I wouldn't make dnfjson.Error.Error() return a string that doesn't include the reason and expect consumers to pull out the exported fields to figure out what's going on.

@supakeen
Copy link
Member

supakeen commented Jun 5, 2024

Is this a usecase for errors.Join (which is my favorite)?

mvo5 added a commit to mvo5/osbuild-composer that referenced this issue Jun 5, 2024
Tiny commit to extract a helper from DepsolveJobImpl.Run() that
can then be unit tested.

This should help with osbuild/images#727
@mvo5
Copy link
Contributor

mvo5 commented Jun 5, 2024

Thanks for finding this! I think we should use the opportunity to refactor the code in jobimpl-depsolve.go a tiny bit and add a test, I started this in osbuild/osbuild-composer#4188 so that we have a baseline to work with.

@schuellerf
Copy link
Contributor Author

maybe also create a second function as the text "DNF error occured" is explanatory and good, and should stay in dnfjson.go IMHO. maybe:

func (err Error) Error() string {
	return fmt.Sprintf("DNF error occurred: %s: %s", err.Kind, err.Reason)
}

func (err Error) ErrorHeader() string {
	return fmt.Sprintf("DNF error occurred: %s", err.Kind)
}

@mvo5
Copy link
Contributor

mvo5 commented Jun 5, 2024

maybe also create a second function as the text "DNF error occured" is explanatory and good, and should stay in dnfjson.go IMHO. maybe:

func (err Error) Error() string {
	return fmt.Sprintf("DNF error occurred: %s: %s", err.Kind, err.Reason)
}

func (err Error) ErrorHeader() string {
	return fmt.Sprintf("DNF error occurred: %s", err.Kind)
}

I really like orthogonal APIs, i.e. where every exported function or data member supports the others without "overlapping" (hope this picture makes sense). It seems we already have the exported err.Kind so adding this helper wouldn't be very orthogonal. It seems the caller (osbuild-composer) can already construct this header from the public interface so maybe best to do it on the caller side? Or is there a downside to that approach?

(fwiw, this also hints again that clienterror.Error might need some tweaks, the idea behing "reason" and "details" is great but in practise seems to be less clear (but that is a topic for another day :)

@achilleas-k
Copy link
Member

achilleas-k commented Jun 5, 2024

It seems we already have the exported err.Kind so adding this helper wouldn't be very orthogonal. It seems the caller (osbuild-composer) can already construct this header from the public interface so maybe best to do it on the caller side? Or is there a downside to that approach?

I had the same thought. Header() isn't very useful when Kind is accessible. Also, I don't see any use for it currently (unless I'm missing some places in osbuild-composer that might need it).

@schuellerf
Copy link
Contributor Author

The idea of ErrorHeader was that the explanatory string "DNF error occured" should not be duplicated/implemented on every "consumer side" of the error object, IMHO.
Obviously those functions should be nested to not even duplicate the string in images. But I get the point - let's push implementing the tests and decide then where the details/header string (the thing before err.Kind in the current Error()) should reside.

schuellerf pushed a commit to mvo5/osbuild-composer that referenced this issue Jun 6, 2024
Tiny commit to extract a helper from DepsolveJobImpl.Run() that
can then be unit tested.

This should help with osbuild/images#727
mvo5 added a commit to mvo5/osbuild-composer that referenced this issue Jun 7, 2024
Tiny commit to extract a helper from DepsolveJobImpl.Run() that
can then be unit tested.

This should help with osbuild/images#727
bcl pushed a commit to mvo5/osbuild-composer that referenced this issue Jun 10, 2024
Tiny commit to extract a helper from DepsolveJobImpl.Run() that
can then be unit tested.

This should help with osbuild/images#727
achilleas-k pushed a commit to osbuild/osbuild-composer that referenced this issue Jun 11, 2024
Tiny commit to extract a helper from DepsolveJobImpl.Run() that
can then be unit tested.

This should help with osbuild/images#727
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

No branches or pull requests

4 participants