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(pom): skipping missing modules #210

Merged
merged 1 commit into from
May 8, 2023

Conversation

liorj-orca
Copy link
Contributor

@liorj-orca liorj-orca commented Apr 20, 2023

Following the issue described at aquasecurity/trivy#3747 where for the case where you run :
trivy fs pom.xml
with the following pom.xml:

<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd">
    <dependencies>
        <dependency>
            <groupId>org.apache.hive</groupId>
            <artifactId>hive-exec</artifactId>
            <version>2.3.3</version>
        </dependency>
    </dependencies>
</project>

you get zero results due to a parser error of one of the dependencies in the pom.
The following PR suggests skipping those missing modules.

@CLAassistant
Copy link

CLAassistant commented Apr 20, 2023

CLA assistant check
All committers have signed the CLA.

@liorj-orca
Copy link
Contributor Author

@knqyf263 could you review this please?

Copy link
Collaborator

@DmitriyLewen DmitriyLewen left a comment

Choose a reason for hiding this comment

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

Hello @liorj-orca
Thanks for your work!

I added 1 comment.

Can you also add test for this case?

Regards, Dmitriy

pkg/java/pom/parse.go Outdated Show resolved Hide resolved
@liorj-orca
Copy link
Contributor Author

Hi @DmitriyLewen, I changed the log level to debug.
regarding the test, I noticed there was already a test with the same "issue" that I had to change its expected output - let me know if you would like me to add another dedicated test specifically for that as I believe the test now covers both cases.

@DmitriyLewen
Copy link
Collaborator

Hello @liorj-orca
Thanks for you work!

let me know if you would like me to add another dedicated test specifically for that as I believe the test now covers both cases.

Good solution!
I think that's enough.

PR currently looks good to me.

@liorj-orca
Copy link
Contributor Author

@DmitriyLewen what else needs to be done so we could merge this and update trivy go.mod so the change would be populated?

@DmitriyLewen
Copy link
Collaborator

Hello @liorj-orca
We then need @knqyf263 to look at that PR and merge it.

@knqyf263 knqyf263 merged commit b0acf2e into aquasecurity:main May 8, 2023
1 check passed
@DmitriyLewen
Copy link
Collaborator

DmitriyLewen commented May 10, 2023

Hello @liorj-orca
We merged this PR.

Can you open new PR in Trivy and update go-dep-parser version? (please use b0acf2e for your PR)
Some recommendations about PRs here.

Sq34sy pushed a commit to Sq34sy/go-dep-parser that referenced this pull request Jul 28, 2023
Sq34sy pushed a commit to Sq34sy/go-dep-parser that referenced this pull request Jul 28, 2023
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.

None yet

4 participants