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

Remove timeout from advanced security scan #444

Merged
merged 7 commits into from
Nov 29, 2023

Conversation

Or-Geva
Copy link
Contributor

@Or-Geva Or-Geva commented Nov 15, 2023

  • All tests passed. If this feature is not already covered by the tests, I added new tests.
  • I used npm run format for formatting the code before submitting the pull request.

@Or-Geva Or-Geva added the improvement Automatically generated release notes label Nov 15, 2023
@Or-Geva Or-Geva requested a review from yahavi November 15, 2023 11:22
@Or-Geva Or-Geva added the safe to test Approve running integration tests on a pull request label Nov 15, 2023
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Nov 15, 2023
@Or-Geva Or-Geva added the safe to test Approve running integration tests on a pull request label Nov 15, 2023
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Nov 15, 2023
src/main/utils/scanUtils.ts Outdated Show resolved Hide resolved
src/main/utils/runUtils.ts Show resolved Hide resolved
@Or-Geva Or-Geva added the safe to test Approve running integration tests on a pull request label Nov 27, 2023
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Nov 27, 2023
@Or-Geva Or-Geva requested a review from yahavi November 27, 2023 16:07
@Or-Geva Or-Geva added the safe to test Approve running integration tests on a pull request label Nov 28, 2023
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Nov 28, 2023
@Or-Geva Or-Geva added the safe to test Approve running integration tests on a pull request label Nov 28, 2023
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Nov 28, 2023
@Or-Geva Or-Geva added the safe to test Approve running integration tests on a pull request label Nov 28, 2023
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Nov 28, 2023
Copy link
Contributor

👍 Frogbot scanned this pull request and found that it did not add vulnerable dependencies.


src/main/scanLogic/scanRunners/jasRunner.ts Outdated Show resolved Hide resolved
src/main/utils/resource.ts Outdated Show resolved Hide resolved
src/main/utils/resource.ts Outdated Show resolved Hide resolved
src/main/utils/scanUtils.ts Outdated Show resolved Hide resolved
*/
private static cancelProcess(childProcess: exec.ChildProcess, checkCancel: () => void, reject: (reason?: any) => void): void {
try {
checkCancel();
Copy link
Member

Choose a reason for hiding this comment

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

You already kill the process. Why running checkCancel?
Moreover - I think that running checkCancel without killing the process may end with a Zombie process.

Copy link
Contributor Author

@Or-Geva Or-Geva Nov 29, 2023

Choose a reason for hiding this comment

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

It assesses whether a cancellation has been requested. If a cancellation is detected, it proceeds to send a termination signal to the child process.

src/test/tests/scanUtils.test.ts Outdated Show resolved Hide resolved
@Or-Geva Or-Geva added the safe to test Approve running integration tests on a pull request label Nov 29, 2023
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Nov 29, 2023
@Or-Geva Or-Geva merged commit 692032a into jfrog:master Nov 29, 2023
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Automatically generated release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants