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

feat(java): add support of test scope for pom.xml files #7486

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

DmitriyLewen
Copy link
Contributor

@DmitriyLewen DmitriyLewen commented Sep 11, 2024

Description

We added support of test scope.
but we were forced to roll back these changes (see #7488).

This PR adds support of test scope.
Also this PR takes into account the problem with child dependencies of a test dependency(mark test deps and children of this dependency as Dev).

Blocker PR

Checklist

  • I've read the guidelines for contributing to this repository.
  • I've followed the conventions in the PR title.
  • I've added tests that prove my fix is effective or that my feature works.
  • I've updated the documentation with the relevant information (if needed).
  • I've added usage information (if the PR introduces new options)
  • I've included a "before" and "after" example to the description (if the PR is a user interface change).

@@ -303,7 +303,7 @@ func (d pomDependency) ToArtifact(opts analysisOptions) artifact {
Exclusions: exclusions,
Locations: locations,
Relationship: ftypes.RelationshipIndirect, // default
Test: d.Scope == "test",
Test: d.Scope == "test" || opts.testScope,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm wondering if it works as expected. For example, if two direct dependencies have the same transitive dependency, and one scope is "test" and the other is not, I guess it would not be considered devDependency. How does Maven work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hm... Nice catch.

I updated logic and added test for this case (comment before test shows how mvn works).
Can you take a look and check that this is what you meant?

@@ -303,7 +303,7 @@ func (d pomDependency) ToArtifact(opts analysisOptions) artifact {
Exclusions: exclusions,
Locations: locations,
Relationship: ftypes.RelationshipIndirect, // default
Test: d.Scope == "test",
Test: d.Scope == "test" || opts.testScope,
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't it mean it'll include the test even if testScope is false? Unlike the other PR, this PR still includes test dependencies for me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm... that's weird.
Do you see any vulnerabilities for test dependencies (or test packages in json format)?

debug logs for test dependencies are normal for this PR, because Trivy currently analyzes all dependencies and excludes Dev dependencies later.

Copy link
Contributor

Choose a reason for hiding this comment

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

It takes ages to scan and then fails with:

semaphore acquire: context deadline exceeded

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is timeout error.
Trivy's runtime has increased because Trivy first checks all dependencies (including test ones), and only then excludes them from the report

Copy link
Contributor

@coheigea coheigea left a comment

Choose a reason for hiding this comment

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

It's not fixed yet. Is the option wired through properly so that it doesn't run by default?

@DmitriyLewen DmitriyLewen changed the title fix(java): mark child deps of test dependency as Dev feat(java): add support of test scope for pom.xml files Sep 13, 2024
Copy link

This PR is stale because it has been labeled with inactivity.

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and will be auto-closed. label Nov 13, 2024
@DmitriyLewen DmitriyLewen removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and will be auto-closed. label Nov 13, 2024
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.

3 participants