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

fix(job_attachments): pass original exception to AssetSyncError #285

Merged
merged 1 commit into from
Apr 3, 2024

Conversation

gahyusuh
Copy link
Contributor

@gahyusuh gahyusuh commented Apr 3, 2024

What was the problem/requirement? (What/Why)

In certain cases, when raising AssetSyncError from another exception using

except Exception as e:
    raise AssetSyncError from e

the error message was not being displayed correctly. This makes it hard to debug and identify the root cause of the issue.

What was the solution? (How)

Change raise AssetSyncError from e to raise AssetSyncError(e) from e, passing the original exception instance e to the AssetSyncError, so that we can ensure that the error message is preserved and displayed correctly.

What is the impact of this change?

Improves the overall debugging experience and make it easier to understand the root cause of issues.

How was this change tested?

  • Made sure all unit tests passed by running hatch run test
  • Made sure all integration tests passed by runninghatch run integ:test
  • Manually tested a scenario where the [WinError 183] Cannot create a file when that file already exists error occurred when running the output download command, deadline job download-output < resource ids > --output json --conflict-resolution CREATE_COPY, on windows. Before this change, the download would fail silently without any error message. After the change, the error message was correctly displayed.

Was this change documented?

No.

Is this a breaking change?

No.


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@gahyusuh gahyusuh marked this pull request as ready for review April 3, 2024 19:28
@gahyusuh gahyusuh requested a review from a team as a code owner April 3, 2024 19:28
Copy link
Contributor

@marofke marofke left a comment

Choose a reason for hiding this comment

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

Good find! This should help surface errors to users much better!

🚢 🦈

Signed-off-by: Gahyun Suh <132245153+gahyusuh@users.noreply.github.com>
@gahyusuh gahyusuh force-pushed the gahyusuh/fix_exception_wrapping branch from d60f1ba to d912a00 Compare April 3, 2024 19:41
@gahyusuh gahyusuh merged commit c1707b3 into mainline Apr 3, 2024
15 checks passed
@gahyusuh gahyusuh deleted the gahyusuh/fix_exception_wrapping branch April 3, 2024 19:45
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