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 resource leaks #119

Merged
merged 1 commit into from
Jan 6, 2025
Merged

Fix resource leaks #119

merged 1 commit into from
Jan 6, 2025

Conversation

al-niessner
Copy link
Contributor

@al-niessner al-niessner commented Jan 2, 2025

🗒️ Summary

Syntax sugar to make false positives go away.

⚙️ Test Data and/or Report

Code scanning does not list these anymore.

♻️ Related Issues

Closes #118

@al-niessner al-niessner self-assigned this Jan 2, 2025
Copy link

sonarqubecloud bot commented Jan 2, 2025

@al-niessner
Copy link
Contributor Author

@jordanpadams @nutjob4life

Your CI needs some love and attention. First, it marked an old issue that is on main from 3 weeks ago a new problem on this branch that was found 3 weeks ago.

Second, if we just leave the results float with the crap in them that are false positives like the log injection that failed 3 weeks ago, then we crowd out good information with floating junk. Can we mark them as false positives and get them out of the scanning results?

Third, something failed in the code scanning but the branch can still be merged with a review and the quality gate has passed with a failure. What is the point of code scanning if it just sends crap code into production?

@nutjob4life
Copy link
Member

@al-niessner could you toss in some handy links to the old and new issues that's getting the undue attention?

@al-niessner
Copy link
Contributor Author

al-niessner commented Jan 2, 2025

@nutjob4life

Sure. First thing I noticed was that the code scan resulted in a red X in checks area of this PR. So I clicked on it. It says near the top of the page in big bold letters: "New alerts in code changed by this pull request". In finer print, the file service/ProductService.java:107. Now you can follow that link, view the file, then do a blame to see line 107 is 3 years old. Or, you can follow the links in the problem setup (what I did) for the resource leak, select "code scanning" top left of page, then notice 85 is the log injection. That page says it found it 5 days ago too and file is service/ProductService.java:107. I guess I should have said 3 years ago not 3 weeks ago from the blame. Code scanning was telling me 5 days. Must have mixed them up in my head.

@jordanpadams
Copy link
Member

@al-niessner I just removed removed the legacy branch protections and instead created a new branch ruleset that should no longer cause this confusion.

@jordanpadams jordanpadams changed the title 118: fix resource leaks Fix resource leaks Jan 6, 2025
@jordanpadams jordanpadams merged commit bedc001 into main Jan 6, 2025
6 of 7 checks passed
@jordanpadams jordanpadams deleted the issue_118 branch January 6, 2025 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cleanup potential resource leaks
3 participants