-
Notifications
You must be signed in to change notification settings - Fork 182
Return failed step in the reason #993
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
Conversation
39dc02b to
a19c51a
Compare
c753ef9 to
f7dc5a5
Compare
packages/orchestrator/internal/template/build/phases/finalize/builder.go
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we have it as a string in db and not json?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
forgot about that, thx, will add a migration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am mainly asking about why we don't save it to DB as a json, why do we do the parsing ourself
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed the column type to jsonb + added migration
Co-authored-by: jakub.dobry <[email protected]>
508a8cb to
b60a9a8
Compare
|
@cursor review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Bugbot reviewed your changes and found no bugs!
Comment @cursor review or bugbot run to trigger another review on this PR
This is a breaking change, but in the new SDK, so it's acceptable. The old Template SDK versions will show
[object Object]instead of the reason, this will be fixed in the new SDK release.Return failed step in the reason for the build. This changes the original reason format to:
The step value can be either:
base3)finalizeThe reason is saved in the DB as serialized JSON instead of just a string message (the string reason was never actually saved, there was a bug).
This PR also reduce error wrapping for main paths where the error source is obvious.