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 additional codeql flagged items #2437

Merged
merged 6 commits into from
Feb 18, 2025
Merged

Fix additional codeql flagged items #2437

merged 6 commits into from
Feb 18, 2025

Conversation

awharn
Copy link
Member

@awharn awharn commented Feb 18, 2025

What It Does

Resolves some of the remaining CodeQL flagged items.

How to Test

Review Checklist
I certify that I have:

Additional Comments

Signed-off-by: Andrew W. Harn <[email protected]>
Copy link

github-actions bot commented Feb 18, 2025

📅 Suggested merge-by date: 3/4/2025

Copy link

codecov bot commented Feb 18, 2025

Codecov Report

Attention: Patch coverage is 95.45455% with 1 line in your changes missing coverage. Please review.

Project coverage is 91.41%. Comparing base (3331611) to head (c0247b4).
Report is 7 commits behind head on master.

Files with missing lines Patch % Lines
.../cli/src/workflows/create/Create.common.handler.ts 0.00% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

Signed-off-by: Andrew W. Harn <[email protected]>
Copy link
Member

@traeok traeok left a 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!

Copy link
Member

@zFernand0 zFernand0 left a 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! 🥳

Comment on lines 75 to 80
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;
}
Copy link
Member

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...

Suggested change
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;

Copy link
Member Author

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;

Copy link
Member

@zFernand0 zFernand0 left a comment

Choose a reason for hiding this comment

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

Thanks for simplifying 😋

@zFernand0 zFernand0 marked this pull request as draft February 18, 2025 16:53
Signed-off-by: Andrew W. Harn <[email protected]>
@awharn awharn marked this pull request as ready for review February 18, 2025 17:30
@awharn awharn requested review from traeok and zFernand0 February 18, 2025 18:22
Copy link
Member

@t1m0thyj t1m0thyj left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @awharn!

@awharn awharn merged commit 85288de into master Feb 18, 2025
26 checks passed
@awharn awharn deleted the codeql-cleanup branch February 18, 2025 19:36
@awharn awharn added the release-current Indicates that there is no new functionality being delivered label Feb 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog release-current Indicates that there is no new functionality being delivered
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

4 participants