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

solver: include vertex Description in OpError #5108

Merged
merged 1 commit into from
Jul 2, 2024

Conversation

sipsma
Copy link
Collaborator

@sipsma sipsma commented Jul 2, 2024

These descriptions can be useful for callers to identify which op failed and plumb any custom metadata through to those failures.

Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

Iiuc OpError is intermediate error that gets converted to SolveError (that is a serializable error unlike OpError). Isn't that the one that should be updated? (There is also VertexError for good measure.)

@sipsma
Copy link
Collaborator Author

sipsma commented Jul 2, 2024

Iiuc OpError is intermediate error that gets converted to SolveError (that is a serializable error unlike OpError). Isn't that the one that should be updated? (There is also VertexError for good measure.)

Ah for Dagger's use-case we're all in-process with the solver so we use OpError directly. Can update SolveError either way too though.

VertexError seems to only support the digest atm. I guess it could be updated with the Description inside of OpError. Either would work for us, I'll go with adding to SolveError for now but can update if there's a preference.

@sipsma
Copy link
Collaborator Author

sipsma commented Jul 2, 2024

Pushed an update that puts it in SolveError too

These descriptions can be useful for callers to identify which op failed
and plumb any custom metadata through to those failures.

Signed-off-by: Erik Sipsma <[email protected]>
@sipsma sipsma merged commit 981d4fc into moby:master Jul 2, 2024
75 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.

2 participants