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.

Sign up for GitHub

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 rec catch exception #118

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fix rec catch exception #118

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
53dbd2f
added runtime exception catch block
iqbalrohail Oct 29, 2022
cacb31e
Removed Rec-catch-exception block from spotbugs-excludes
iqbalrohail Oct 29, 2022
f7a543f
added reference to the exception
iqbalrohail Oct 29, 2022
89aac28
Merge branch 'master' into fix-REC_CATCH_EXCEPTION
iqbalrohail Nov 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/main/java/jenkins/plugins/http_request/auth/CertificateAuthentication.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,10 @@ public CloseableHttpClient authenticate(HttpClientBuilder clientBuilder,
credentials.getPassword().getPlainText().toCharArray()).build());

return clientBuilder.build();
} catch (Exception e) {
}catch(RuntimeException e){
throw new RuntimeException(e);
}
catch (Exception e) {
Comment on lines +38 to +41
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see the benefit of this change in reducing the likelihood of issues. Can you explain further or point me to a location where I can read more about the benefit of this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As spotbug spots this issue of REC_CATCH_EXCEPTION so rather than ignoring that bug in excludes file i think it's a good approach to make a code change to solve that issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding this will catch the runtime exceptions and make the issue solve for reference you can check Alluxio/alluxio#10459

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MarkEWaite doesn't it seems good to have this change?

throw new IOException(e);
}
}
Expand Down
6 changes: 0 additions & 6 deletions src/spotbugs/spotbugs-excludes.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,6 @@
<Field name="step"/>
</Match>

<Match>
<Bug pattern="REC_CATCH_EXCEPTION"/>
<Class name="jenkins.plugins.http_request.auth.CertificateAuthentication"/>
<Method name="authenticate"/>
</Match>

<Match>
<Bug pattern="SE_NO_SERIALVERSIONID"/>
<Class name="jenkins.plugins.http_request.auth.CertificateAuthentication"/>
Expand Down