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

Add a new Java configuration option to recursively search parent poms… #2274

Merged
merged 10 commits into from
Nov 3, 2023

Conversation

coheigea
Copy link
Contributor

… for licenses

Part 2 of #2103

#2228 added the ability to read a license from a parent pom in a remote repository like Maven Central. However that PR did not find the license when the license was embedded in a parent pom greater than one level up.

This PR introduces a new Java config option "max-parent-recursive-depth" which defaults to 5. When "search-maven-for-licenses" is enabled, it will recursively go through the parent poms up to "max-parent-recursive-depth" to find a license.

With this PR Syft can now successfully find licenses for:

@coheigea coheigea force-pushed the coheigea/recursive-parent-pom branch from 6f32ee0 to cbddc73 Compare October 31, 2023 11:39
@coheigea coheigea force-pushed the coheigea/recursive-parent-pom branch from cbddc73 to c4c6a83 Compare October 31, 2023 11:40
@spiffcs
Copy link
Contributor

spiffcs commented Nov 1, 2023

Thanks for the update @coheigea! I'm going to add a unit test here exercising the online behavior with a httptest.NewServer that way we have coverage for the previous state of requesting licenses from maven, as well as coverage of recursing to higher poms.

@spiffcs spiffcs requested a review from a team November 2, 2023 14:31
@spiffcs spiffcs self-assigned this Nov 2, 2023
@spiffcs
Copy link
Contributor

spiffcs commented Nov 2, 2023

I'm going to make a small change to this PR - we're going to remove the more specific

java:
    search-maven-for-licenses: true

In favor of a master top level switch

use-network: true

Signed-off-by: Christopher Phillips <[email protected]>
Copy link
Contributor

@kzantow kzantow left a comment

Choose a reason for hiding this comment

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

LGTM -- nothing blocking, but a few suggestions

README.md Outdated Show resolved Hide resolved
cmd/syft/cli/options/catalog.go Outdated Show resolved Hide resolved
syft/pkg/cataloger/java/archive_parser.go Show resolved Hide resolved
Signed-off-by: Christopher Phillips <[email protected]>
@spiffcs spiffcs added the breaking-change Change is not backwards compatible label Nov 2, 2023
@spiffcs
Copy link
Contributor

spiffcs commented Nov 2, 2023

@kzantow @wagoodman

After getting some feedback about adding the top level option of use-network I've factored in a breaking change in this PR. Users who might be using the golang config will need to explicitly enable use-network for that option to still be usable.

It's unclear to me of the primacy of the options in this case:

Below is the breaking change where users would need to flip the switch on to preserve golang option:

global use-network: false, golang remote search: true => false
global use-network: unset(defaults to false), golang remote search: true => false

@coheigea
Copy link
Contributor Author

coheigea commented Nov 3, 2023

Thanks a lot @spiffcs for whipping my PRs into shape :-)

@github-actions github-actions bot removed the breaking-change Change is not backwards compatible label Nov 3, 2023
@spiffcs
Copy link
Contributor

spiffcs commented Nov 3, 2023

After more thought, We've decided that adding use-network at the top level here leads to too many small breaking changes going forward. The best course of action is to rename the nested config value from search-maven-licenses => use-network. This does not break any behavior of other catalogers. It also tees up the java options for the future when we start work on the external-sources epic of work. This field is unlikely to be renamed when we start building in the master on/off network switch for syft runs.

Signed-off-by: Christopher Phillips <[email protected]>
@spiffcs spiffcs merged commit 2d582f7 into anchore:main Nov 3, 2023
10 checks passed
@coheigea coheigea deleted the coheigea/recursive-parent-pom branch November 3, 2023 14:34
GijsCalis pushed a commit to GijsCalis/syft that referenced this pull request Feb 19, 2024
anchore#2274)

- Add a new Java configuration option to recursively search parent poms for licenses
---------
Signed-off-by: Colm O hEigeartaigh <[email protected]>
Signed-off-by: Christopher Phillips <[email protected]>
Co-authored-by: Christopher Phillips <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants