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

first commit was pushed related to update the trivy version #52

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

calsoftfaizal813
Copy link

I ran trivy scanning for the test project "Trivy-ci-test" and updated the trivy version.

there are three file changes included in this commit:
extension.ts
trivy_wrapper.ts
package-lock.json

here are the screenshots of the scanning results:
image

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@simar7
Copy link
Member

simar7 commented Oct 29, 2024

hi @calsoftfaizal813 - thanks for the PR. A few comments:

  1. Please format commit messages as per this convention https://www.conventionalcommits.org/en/v1.0.0/#summary
  2. Please sign the CLA with your GitHub account, the instructions are in the comment here
  3. Looks like the tests are failing, we'd probably have to update the assertion as the newer version of Trivy detects more problems than before.
image

Comment on lines -74 to +101
context.subscriptions.push(vscode.commands.registerCommand(
"trivy-vulnerability-scanner.scan",
() => {
const trivyScanCmd = "trivy --quiet filesystem --security-checks config,vuln --exit-code=10";
var scanResult = runCommand(trivyScanCmd, projectRootPath.toString());
context.subscriptions.push(
vscode.commands.registerCommand("trivy-vulnerability-scanner.scan", () => {
const trivyScanCmd =
"trivy --quiet filesystem --security-checks config,vuln --exit-code=10";
let scanResult = runCommand(trivyScanCmd, projectRootPath.toString());
Copy link
Member

Choose a reason for hiding this comment

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

Please update the commands as per the log output from the extension

2024-10-28T23:17:33-06:00	WARN	'--security-checks' is deprecated. Use '--scanners' instead.

2024-10-28T23:17:33-06:00	WARN	'--scanners config' is deprecated. Use '--scanners misconfig' instead. See https://github.com/aquasecurity/trivy/discussions/5586 for the detail.

Copy link
Author

Choose a reason for hiding this comment

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

when I replaced the '--security-checks' with the '--scanners' and '--scanners config' with '--scanners misconfig' then I got this error

image

Copy link
Member

Choose a reason for hiding this comment

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

Did you check the logs to see what's causing the error?

Copy link
Author

Choose a reason for hiding this comment

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

Hi @simar7, Could you please help me with this one where I can see the logs for this error, I tried to find but was not able to find the logs in my local environment

Copy link
Member

Choose a reason for hiding this comment

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

A lot of these changes are style based, are you running any linter to do so? It makes it hard to review this PR so can we update the linter changes in a separate PR? They are not really necessary for this PR.

Copy link
Author

Choose a reason for hiding this comment

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

I have default installed eslint and prettier extensions to format the javascript and typescript code that's why we have removed the extra white spaces and variable declaration changes, please let me know if it is not really required for now so I can revert the changes for this file?

Copy link
Member

Choose a reason for hiding this comment

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

As we mentioned earlier, we're just looking to upgrade this extension to support the latest version of Trivy. Any other changes should be done in a separate PR as they aren't related to that.

Copy link
Member

Choose a reason for hiding this comment

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

@simar7 simar7 self-requested a review October 29, 2024 05:23
Copy link
Member

@simar7 simar7 left a comment

Choose a reason for hiding this comment

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

Left some comments.

@calsoftfaizal813
Copy link
Author

hi @calsoftfaizal813 - thanks for the PR. A few comments:

  1. Please format commit messages as per this convention https://www.conventionalcommits.org/en/v1.0.0/#summary
  2. Please sign the CLA with your GitHub account, the instructions are in the comment here
  3. Looks like the tests are failing, we'd probably have to update the assertion as the newer version of Trivy detects more problems than before.
image

Hi Simarpreet Singh.
like this above screenshot,
Could you please help me with this running test case locally on my vscode so I can test my recently made changes and update the PR

@simar7
Copy link
Member

simar7 commented Oct 30, 2024

Could you please help me with this running test case locally on my vscode so I can test my recently made changes and update the PR

The project already contains two test targets, one is for running the extension and the other for tests. https://github.com/aquasecurity/trivy-vscode-extension/blob/master/src/test/suite/extension.test.ts

You can read more on how to run the test target here: https://code.visualstudio.com/api/working-with-extensions/testing-extension

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.

4 participants