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: Present Error Handling Job #1204

Merged
merged 3 commits into from
Jun 21, 2024
Merged

Conversation

CryptoKnightIOG
Copy link
Contributor

Signed-off-by: Bassam Riman <bassam.riman@iohk.io>
Copy link
Contributor

github-actions bot commented Jun 20, 2024

Unit Test Results

 96 files  ±0   96 suites  ±0   21m 20s ⏱️ -4s
826 tests ±0  818 ✅ +1  8 💤 ±0  0 ❌  - 1 
833 runs  ±0  825 ✅ +1  8 💤 ±0  0 ❌  - 1 

Results for commit 73516b2. ± Comparison against base commit e272d70.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@FabioPinheiro FabioPinheiro left a comment

Choose a reason for hiding this comment

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

Everything that I look seems good

Can we merge this quickly because I wanted to see if it helps with compilation. Or I can also just continue my work in top of the PR while you finalize it.

.tapError({
(error: PresentationError | DIDSecretStorageError | BackgroundJobError | CredentialServiceError |
CastorDIDResolutionError | GetManagedDIDError | TransportError) =>
ZIO.logErrorCause(
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems we have error log in several places.
Just make sure we are not double logging

// case te: TransportError => new RuntimeException(te)
case ex: IOException => ex
case ex: Throwable => ex

sealed trait MercuryError
Copy link
Contributor

Choose a reason for hiding this comment

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

I extended org.hyperledger.identus.shared.models.Failure on my PR.
Or are you also doing that on your next PR?

Copy link
Contributor

@bvoiturier bvoiturier left a comment

Choose a reason for hiding this comment

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

LGTM, step-by-step... 👌

@mineme0110
Copy link
Contributor

LGTM I have tested the end to end scenarios

Signed-off-by: Bassam Riman <bassam.riman@iohk.io>
@CryptoKnightIOG CryptoKnightIOG enabled auto-merge (squash) June 21, 2024 00:19
Copy link
Contributor

Integration Test Results

14 files  14 suites   2s ⏱️
29 tests 29 ✅ 0 💤 0 ❌
42 runs  42 ✅ 0 💤 0 ❌

Results for commit 73516b2.

@CryptoKnightIOG CryptoKnightIOG merged commit 3191d8b into main Jun 21, 2024
11 checks passed
@CryptoKnightIOG CryptoKnightIOG deleted the ATL-7264-Present-Error-Handling branch June 21, 2024 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants