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

Allow project-local package-configurations #4386

Conversation

MarcelBochtler
Copy link
Member

Package-configurations must be unique per ID and provenance even
if one configuration is placed in the .ort.yml and the other one
is placed in an ORT-wide package-configurations directory.
See:

private fun Collection<PackageConfiguration>.checkAtMostOneConfigurationPerIdAndProvenance() {
data class Key(val id: Identifier, val sourceArtifactUrl: String?, val vcsMatcher: VcsMatcher?)
fun PackageConfiguration.key() = Key(id, sourceArtifactUrl, vcs)
val configurationsWithSameMatcher = groupBy { it.key() }.filter { it.value.size > 1 }
require(configurationsWithSameMatcher.isEmpty()) {
"There must be at most one package configuration per Id and provenance, but found multiple for:\n" +
"${configurationsWithSameMatcher.keys.joinToString(prefix = " ", separator = "\n ")}."
}
}

@MarcelBochtler MarcelBochtler requested review from a team as code owners August 24, 2021 13:38
Comment on lines 93 to 94
configurations:
package_configurations:
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not completely happy with the naming here, but wanted to reuse package_configurations as they are a known term within ORT.
Any suggestions for a better naming?

Copy link
Member

Choose a reason for hiding this comment

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

I'd personally either go for package_configurations as single top level entry, or

configurations:
  packages:

Tendency to the former.
What's the reason for nesting it in Configurations? Consistency?

Copy link
Member Author

Choose a reason for hiding this comment

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

What's the reason for nesting it in Configurations? Consistency?

Yes. That is the only reason. I'll change it to package_configurations as a single top level entry.

Copy link
Member

Choose a reason for hiding this comment

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

Let's get @oss-review-toolkit/core-devs input on that first.

Copy link
Member Author

Choose a reason for hiding this comment

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

Already changed it, as I didn't see your answer. However: I'd be fine with changing it back after some input from the other @oss-review-toolkit/core-devs.

Copy link
Member

Choose a reason for hiding this comment

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

I also prefer a single top level entry package_configurations. Similarly curation could be added as another top level entry later.

Copy link
Member

@fviernau fviernau Aug 25, 2021

Choose a reason for hiding this comment

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

Similarly curation could be added as another top level entry later.

@mnonnenmacher I didn't get this bit. Do you mean refactoring as follows ?

curations:
  license_findings:
  packages:

to

package_curations:
license_finding_curations:

Copy link
Member Author

Choose a reason for hiding this comment

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

I also prefer a single top level entry package_configurations. Similarly curation could be added as another top level entry later.

Curations were already added in this PR: #4381
in this form:

curations:
  packages:
  - id: "Maven:id..."
    curations:
    ....

@MarcelBochtler
Copy link
Member Author

Package-configurations must be unique per ID and provenance

Is this how it should work? For package curations it is different: ORT merges the curations from a single source and all curations are applied.

@MarcelBochtler MarcelBochtler changed the title Evaluator: Apply project-local package-configurations Allow project-local package-configurations Aug 24, 2021
Comment on lines 93 to 94
configurations:
package_configurations:
Copy link
Member

Choose a reason for hiding this comment

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

I'd personally either go for package_configurations as single top level entry, or

configurations:
  packages:

Tendency to the former.
What's the reason for nesting it in Configurations? Consistency?

docs/config-file-ort-yml.md Show resolved Hide resolved
@fviernau
Copy link
Member

Is this how it should work? For package curations it is different: ORT merges the curations from a single source and all curations are applied.

Unique per ID and provenance: yes, that's how it's intended.s
That IMO should at least be the assumption for all the consumers of package configurations (providers).

It may make sense to enforce uniqueness within global and local configuration separately, and at the same time
merge two package configurations a and b if they correspond to the same package and one of them comes from local, the other from the global configuration. What do you think @MarcelBochtler ?

Package-configurations must be unique per ID and provenance even
if one configuration is placed in the `.ort.yml` and the other one
is placed in an ORT-wide package-configurations directory.

Signed-off-by: Marcel Bochtler <[email protected]>
@MarcelBochtler MarcelBochtler force-pushed the project-specific-package-configurations branch from 2aa2759 to c736fcd Compare August 24, 2021 14:44
@MarcelBochtler
Copy link
Member Author

MarcelBochtler commented Aug 24, 2021

It may make sense to enforce uniqueness within global and local configuration separately, and at the same time
merge two package configurations a and b if they correspond to the same package and one of them comes from local, the other from the global configuration. What do you think @MarcelBochtler ?

What would be the reasoning to disallow merging of package-configurations from one source?
If merging from different sources is possible, it would also be possible for the same sources.

@fviernau Do you think that the decision / implementation of merging package-configurations is blocking this PR?

@codecov-commenter
Copy link

Codecov Report

Merging #4386 (f4c56d1) into master (f4c56d1) will not change coverage.
The diff coverage is n/a.

❗ Current head f4c56d1 differs from pull request most recent head c736fcd. Consider uploading reports for the commit c736fcd to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##             master    #4386   +/-   ##
=========================================
  Coverage     68.82%   68.82%           
  Complexity      519      519           
=========================================
  Files           176      176           
  Lines          6873     6873           
  Branches        893      893           
=========================================
  Hits           4730     4730           
  Misses         1715     1715           
  Partials        428      428           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f4c56d1...c736fcd. Read the comment docs.

@fviernau
Copy link
Member

What would be the reasoning to disallow merging of package-configurations from one source?

I think that's clearly bad practice as I cannot see any advantage of setting up multiple package configs for a single
tuple: package / scan result. It just adds complexity.

If merging from different sources is possible, it would also be possible for the same sources.

@fviernau Do you think that the decision / implementation of merging package-configurations is blocking this PR?

Possible yes. As written above I believe it should not be implemented. I'm find with the PR as-is: without the merging implemented.

@MarcelBochtler MarcelBochtler merged commit e2b6a34 into oss-review-toolkit:master Aug 26, 2021
@MarcelBochtler MarcelBochtler deleted the project-specific-package-configurations branch August 26, 2021 10:27
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