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

osbuild-worker: rework the workerClientErrorFrom() error #4254

Merged
merged 1 commit into from
Aug 1, 2024

Conversation

mvo5
Copy link
Contributor

@mvo5 mvo5 commented Jul 9, 2024

The workerClientErrorFrom() was returning an *clienterrors.Error and an error (if something with the conversation goes wrong.

But the calling code was expecting that even if an error is returned the *clienterrors.Error is still valid. The caller would then just log the error. As returning a valid value even when there is an error is an unexpected pattern this commit changes the code to always return a *clienterrors.Error and log any issue via the logger.

Copy link
Member

@thozza thozza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added one comment, but the PR technically LGTM. 👍

cmd/osbuild-worker/jobimpl-depsolve.go Outdated Show resolved Hide resolved
@mvo5 mvo5 force-pushed the clienterrors-workerClientErrorFrom branch from 86f4503 to 36fac07 Compare July 12, 2024 07:14
@mvo5 mvo5 requested a review from thozza July 12, 2024 07:28
thozza
thozza previously approved these changes Jul 12, 2024
Copy link
Member

@thozza thozza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks 👍

@mvo5 mvo5 requested a review from croissanne July 30, 2024 07:47
@mvo5 mvo5 force-pushed the clienterrors-workerClientErrorFrom branch from 36fac07 to 43908d5 Compare July 30, 2024 07:48
@mvo5 mvo5 force-pushed the clienterrors-workerClientErrorFrom branch 2 times, most recently from 2d97926 to 45003b8 Compare July 31, 2024 10:59
The workerClientErrorFrom() was returning an `*clienterrors.Error` and
an `error` (if something with the conversation goes wrong.

But the calling code was expecting that even if an `error` is returned
the `*clienterrors.Error` is still valid. The caller would then just
log the error. As returning a valid `value` even when there is an
`error` is an unexpected pattern this commit changes the code to
always return a `*clienterrors.Error` and log any issue via the
logger.
@mvo5 mvo5 force-pushed the clienterrors-workerClientErrorFrom branch from 45003b8 to 96633ff Compare August 1, 2024 09:45
@mvo5
Copy link
Contributor Author

mvo5 commented Aug 1, 2024

LGTM, thanks 👍

Sorry, had to rebase because of conflicts, could you please re-review?

@mvo5 mvo5 requested a review from thozza August 1, 2024 09:46
Copy link
Member

@thozza thozza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still looks good.

I realized that we should eventually replace dnf-json in the error strings with osbuild-depsolve-dnf...

@mvo5 mvo5 enabled auto-merge (rebase) August 1, 2024 12:51
@mvo5 mvo5 merged commit 1d0232f into osbuild:main Aug 1, 2024
43 checks passed
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