fix(weaver): improper exception handling #2803
Conversation
Yogesh01000100
requested review from
petermetz,
takeutak,
izuru0,
jagpreetsinghsasan,
VRamakrishna and
sandeepnRES
as code owners
There was a problem hiding this comment.
@Yogesh01000100 There is no contextual information added at all. Please read my point 2) from the issue comments! The idea is that in the logs it gets explained what was being done when the crash happened. That way whoever is reading the log can have a better chance of figuring out why the exception was thrown in the first place.
|
@Yogesh01000100 Hi, I don't get the logic behind this PR, you catch it and throw it again? @VRamakrishna you wrote this code, I'll suggest you to comment on this, and approve if this is good. |
|
I have made the requested modifications to include contextual information and followed the suggestion by @petermetz to re-throw the error to the getConfig function. Please inform me if any additional adjustments are needed. added these : |
|
@Yogesh01000100 There's 16 commits in the PR right now, please reduce that to one by rebasing onto upstream/main and squashing the commits together. |
I think while updating branch, @Yogesh01000100 by mistake duplicated commits from main branch. Don't squash these, instead fix this so that duplicate commits don't appear here. What I'd do is delete all these duplicate commits and retain only yours, and then merge You should always update your fork with rebase option. |
.../sdks/corda/src/main/kotlin/org/hyperledger/cacti/weaver/sdk/corda/CredentialsExtractor.java
Outdated
Show resolved
Hide resolved
|
@Yogesh01000100 Just fix above issues, otherwise it looks good to me. |
There was a problem hiding this comment.
@Yogesh01000100 Please do an interactive rebase and squash the 3 separate commits together into a single one.
If you need any help there are some links in the contributing.md document in the project root. If that isn't enough just let me know and I can help too.
@petermetz Thank you for your support. I am a beginner in Git workflow, and I have tried my best to follow your instructions. I have squashed the three separate commits into a single one. Please check my changes and let me know if you have any feedback. |
There was a problem hiding this comment.
@Yogesh01000100 Please do an interactive rebase and squash the 3 separate commits together into a single one. If you need any help there are some links in the contributing.md document in the project root. If that isn't enough just let me know and I can help too.
@petermetz Thank you for your support. I am a beginner in Git workflow, and I have tried my best to follow your instructions. I have squashed the three separate commits into a single one.
Please check my changes and let me know if you have any feedback.
@Yogesh01000100 No worries, being a beginner with git can be a little confusing sometimes so it's not a problem if we need a few iterations until the conventions are all adhered to. What you just did was a big step in the right direction, I just have a few more things before we are ready for merging it, please bear with me and we'll get it done!
The changes I ask:
- What you have in the pull request description at the moment is quite good, it just misses one small thing: you need to declare in there with a specific syntax that this commit is going to fix/resolve a specific issue (the one that you already mentioned in the description). For the exact syntax please this: https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue and you can also see other pull requests to observe how it works in action.
- Once you've updated the PR description according to my point above, then you can do a commit message amendment and set the commit message body to the same message you have in the pull request description. Once you've amended the commit message locally, you can do a force push with-lease: https://stackoverflow.com/questions/52823692/git-push-force-with-lease-vs-force
Other than these administrative changes, it is looking good to me.
@petermetz can you check the changes now? Included the fixes keyword, in the PR description, and previously because of the squashing of the commits, there were multiple commit messages, now I have amended the message body, currently it has one message. |
There was a problem hiding this comment.
@Yogesh01000100 We are getting there, slowly but surely! You forgot to set the commit message body to what's in the PR description, but otherwise good job on de-duplication!
yes done with adding the PR description in the commit message body. |
|
I have added the changes mentioned earlier, do you think this PR can be merged now? |
There was a problem hiding this comment.
@Yogesh01000100 Almost there! Please fix the commit message linter errors that are making the check fail. Pasting them below in case you don't know where to see them:
Error: You have commit messages with errors
⧗ input: fix(weaver): improper exception handling
This pull request addresses the issue of improper exception handling. The need is to wrap the expected exceptions in a try-catch block and handle them explicitly.
Changes:
Enclosed the existing code within a try-catch block to capture exceptions.
Added contextual information in the logs, and the exception is re-thrown within the getConfig() function, as part of the exception propagation process.
fixes #2767
Signed-off-by: D.Yogesh [email protected]
✖ body's lines must not be longer than 100 characters [body-max-line-length]
✖ found 1 problems, 0 warnings
ⓘ Get help: https://github.com/conventional-changelog/commitlint/#what-is-commitlint
|
@Yogesh01000100 looks good to me. Please address the above review comments from @petermetz |
|
Based on the feedback, I've made the changes to the PR. I've also addressed all of the comments in the discussion. |
|
@Yogesh01000100 Sorry about the slow reviews, I'll ping the rest of the maintainers to check on this. @VRamakrishna @sandeepnRES Do you have a take on this PR? |
Address improper exception handling by wrapping expected exceptions in a try-catch block and managing them explicitly. Changes: - Code is now enclosed within a try-catch to capture exceptions. - Logs include contextual information for clarity. - Exceptions re-thrown in getConfig() as part of propagation. fixes #2767 Signed-off-by: D.Yogesh <[email protected]>
Description:
Fixes: #2767
This pull request addresses the issue of improper exception handling. The need is to wrap the expected exceptions in a try-catch block and handle them explicitly.
Changes: