-
Notifications
You must be signed in to change notification settings - Fork 87
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 additional codeql flagged items #2437
Conversation
Signed-off-by: Andrew W. Harn <andrew.harn@broadcom.com>
📅 Suggested merge-by date: 3/4/2025 |
packages/cli/__tests__/daemon/__integration__/cli.zowe.daemon.integration.suite.subtest.ts
Fixed
Show fixed
Hide fixed
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2437 +/- ##
==========================================
+ Coverage 91.39% 91.41% +0.01%
==========================================
Files 639 639
Lines 18275 18267 -8
Branches 3919 3953 +34
==========================================
- Hits 16703 16699 -4
+ Misses 1570 1566 -4
Partials 2 2 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Andrew W. Harn <andrew.harn@broadcom.com>
Signed-off-by: Andrew W. Harn <andrew.harn@broadcom.com>
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.
LGTM, thanks Andrew for the technical currency!
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.
LGTM! 😋
Thanks for fixing these! 🙏
Left a small comment but I'm fine if this gets merged as-is! 🥳
packages/zostso/src/IssueTso.ts
Outdated
if (e.message?.includes("status 404")) { | ||
// Set useNewApi to false to handle fallback logic | ||
useNewApi = false; | ||
// Continue to the old API behavior | ||
} else { | ||
// Re-throw for other exceptions | ||
throw e; | ||
} |
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.
can we simplify this to...
if (e.message?.includes("status 404")) { | |
// Set useNewApi to false to handle fallback logic | |
useNewApi = false; | |
// Continue to the old API behavior | |
} else { | |
// Re-throw for other exceptions | |
throw e; | |
} | |
if (e.message && !e.message.includes("status 404")) throw e; |
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.
Please look over the change. I made a small modification to keep the logic the same.
I implemented:
if (e.message == null || !e.message.includes("status 404")) throw e;
Signed-off-by: Andrew W. Harn <andrew.harn@broadcom.com>
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.
Thanks for simplifying 😋
Signed-off-by: Andrew W. Harn <andrew.harn@broadcom.com>
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.
LGTM, thanks @awharn!
What It Does
Resolves some of the remaining CodeQL flagged items.
How to Test
Review Checklist
I certify that I have:
Additional Comments