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

Support namespace-level package curations from ort-config #8204

Merged
merged 2 commits into from
Feb 1, 2024

Conversation

sschuberth
Copy link
Member

Please have a look at the individual commit messages for the details.

@sschuberth sschuberth requested a review from a team as a code owner January 31, 2024 21:52
@sschuberth sschuberth enabled auto-merge (rebase) January 31, 2024 21:53
@sschuberth sschuberth force-pushed the namespace-ort-curations branch from 5991983 to e772ef3 Compare February 1, 2024 05:48
@@ -88,7 +88,7 @@ class DirPackageConfigurationProvider(
"The path '$it' does not exist."
}

if (it.isDirectory) FileFormat.findFilesWithKnownExtensions(it) else listOf(it)
if (it.isDirectory) FileFormat.findFilesWithKnownExtensions(it).sorted() else listOf(it)
Copy link
Member

Choose a reason for hiding this comment

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

commit-msg: Should the last sentence indicate, that it is specific to the OrtConfig...Provider?

Copy link
Member Author

Choose a reason for hiding this comment

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

No. While _.yml are conventional with the ort-config provider, they could be used with a file provider as well. Phrased differently, when pointing a file provider to the local checkout of the ort-config provider, you should get the same curations in the same order.

@@ -103,7 +103,7 @@ class FilePackageCurationProvider(
"The path '$it' does not exist."
}

if (it.isDirectory) FileFormat.findFilesWithKnownExtensions(it) else listOf(it)
if (it.isDirectory) FileFormat.findFilesWithKnownExtensions(it).sorted() else listOf(it)
Copy link
Member

Choose a reason for hiding this comment

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

We had this topic of ordering vs. added complexity already before, but I do not recall the details.

My preference still is to not assume any ordering but instead require each curation to be correct on its own.
Because for correct curations, the order does not matter. Anyhow, it'd be interested to hear further opinions on this. Also how much of an edge case this is. IMO it'd would be that problematic to not use namespace curations
if the curation does not apply to all packages in the namespace.

Copy link
Member Author

Choose a reason for hiding this comment

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

If it helps to get this merged more quickly, I'm happy to drop the last commit.

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 happy to drop the last commit.

Done.

Signed-off-by: Sebastian Schuberth <[email protected]>
This is required to support curations like the one added in [1].

[1]: oss-review-toolkit/ort-config#165

Signed-off-by: Sebastian Schuberth <[email protected]>
@sschuberth sschuberth force-pushed the namespace-ort-curations branch 2 times, most recently from d0e2367 to 42cb229 Compare February 1, 2024 13:58
@sschuberth sschuberth requested review from fviernau and a team February 1, 2024 13:58
@sschuberth sschuberth disabled auto-merge February 1, 2024 14:29
@sschuberth sschuberth merged commit b55f91f into main Feb 1, 2024
18 of 19 checks passed
@sschuberth sschuberth deleted the namespace-ort-curations branch February 1, 2024 14:29
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.

2 participants