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(nuget)!: Split the (technology) namespace from the name #6046

Merged
merged 4 commits into from
Sep 13, 2023

Conversation

sschuberth
Copy link
Member

@sschuberth sschuberth commented Nov 7, 2022

Use up to two namespace nodes from the name that identify the company
and product / technology as defined at 1 as the namespace. This allows
to write curations with empty name and version that affect a whole product
/ technology, and to set e.g. a concluded license for all
"Microsoft.AspNetCore" packages.

Note
This is a breaking change as IDs that refer to NuGet packages, like in package curations, probably need to be adjusted to accommodate for the proper namespace.

@sschuberth sschuberth force-pushed the nuget-namespaces branch 2 times, most recently from af515d3 to 1c48e14 Compare November 7, 2022 21:03
@sschuberth
Copy link
Member Author

See this for more context. Even if this would be a quite massive breaking change for existing NuGet curations, I believe it's the right thing to do, and afterwards writing (generic) NuGet curations would be much easier / possible. What do you thing @oss-review-toolkit/core-devs?

private fun getIdentifier(name: String, version: String) =
Identifier(type = "NuGet", namespace = "", name = name, version = version)
internal fun getIdentifier(name: String, version: String): Identifier {
val namespace = name.split('.', limit = 3).toMutableList()
Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe the splitting here should be made more strict and also require PascalCase as described in the linked article.

@sschuberth sschuberth changed the title NuGetSupport: Properly split the (technology) namespace from the name feat(nuget)!: Split the (technology) namespace from the name Sep 4, 2023
@codecov
Copy link

codecov bot commented Sep 4, 2023

Codecov Report

Patch coverage: 78.53% and no project coverage change.

Comparison is base (3fac582) 68.01% compared to head (22dbf21) 68.02%.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #6046   +/-   ##
=========================================
  Coverage     68.01%   68.02%           
  Complexity     2023     2023           
=========================================
  Files           339      343    +4     
  Lines         16719    16723    +4     
  Branches       2371     2371           
=========================================
+ Hits          11371    11375    +4     
- Misses         4363     4364    +1     
+ Partials        985      984    -1     
Flag Coverage Δ
funTest-docker 69.40% <78.53%> (+0.06%) ⬆️
funTest-non-docker 36.46% <ø> (ø)
test 35.51% <4.76%> (-0.60%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
...gers/nuget/src/main/kotlin/utils/NuGetInspector.kt 68.00% <ø> (-8.13%) ⬇️
...rs/python/src/main/kotlin/utils/PythonInspector.kt 67.67% <ø> (-3.20%) ⬇️
...nagers/python/src/main/kotlin/utils/PythonUtils.kt 57.89% <57.89%> (ø)
...src/main/kotlin/utils/PythonInspectorExtensions.kt 77.27% <77.27%> (ø)
.../src/main/kotlin/utils/NuGetInspectorExtensions.kt 83.75% <83.75%> (ø)
...managers/nuget/src/main/kotlin/utils/NuGetUtils.kt 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -197,6 +197,13 @@ private fun List<NuGetInspector.PackageData>.toPackageReferences(): Set<PackageR
)
}

internal fun getIdentifierWithNamespace(name: String, version: String): Identifier {
Copy link
Member Author

@sschuberth sschuberth Sep 4, 2023

Choose a reason for hiding this comment

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

@mnonnenmacher am I correct to assume that public access to this would be required here in order for GenerateAspNetCoreCurationsTask et al to make use of the new logic? Or would you simply duplicate this small piece of code?

This comment was marked as outdated.

Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to make it public just in case there are changes in the future. Otherwise the copy might get out of sync.

Separate generic Python utilities from those specific to `PythonInspector`
by moving them to a dedicated file.

Signed-off-by: Sebastian Schuberth <[email protected]>
Move funtions that build the bridge between PythonInspector's and ORT's
models to a dedicated file in order for `PythonInspector` to be
independent of ORT's model for a cleaner separation.

Signed-off-by: Sebastian Schuberth <[email protected]>
Move funtions that build the bridge between NuGetInspector's and ORT's
models to a dedicated file in order for `NuGetInspector` to be
independent of ORT's model for a cleaner separation.

Signed-off-by: Sebastian Schuberth <[email protected]>
@sschuberth sschuberth marked this pull request as ready for review September 13, 2023 15:20
@sschuberth sschuberth requested a review from a team as a code owner September 13, 2023 15:20
Use up to two namespace nodes from the name that identify the company
and product / technology as defined at [1] as the namespace. This allows
to write curations with empty name and version that affect a whole product
/ technology, and to set e.g. a concluded license for all
"Microsoft.AspNetCore" packages.

[1]: https://learn.microsoft.com/en-us/dotnet/standard/design-guidelines/names-of-namespaces

Signed-off-by: Sebastian Schuberth <[email protected]>
@sschuberth sschuberth merged commit 229c0fb into main Sep 13, 2023
21 checks passed
@sschuberth sschuberth deleted the nuget-namespaces branch September 13, 2023 19:49
sschuberth added a commit to oss-review-toolkit/ort-config that referenced this pull request Sep 15, 2023
ORT core changed the way to refer to NuGet packages [1]. Adapt for that
when generating new NuGet curations. Existing NuGet curations will be
migrated separately.

[1]: oss-review-toolkit/ort#6046

Signed-off-by: Sebastian Schuberth <[email protected]>
sschuberth added a commit to oss-review-toolkit/ort-config that referenced this pull request Sep 15, 2023
ORT core changed the way to refer to NuGet packages [1]. Adapt for that
when generating new NuGet curations. Existing NuGet curations will be
migrated separately.

[1]: oss-review-toolkit/ort#6046

Signed-off-by: Sebastian Schuberth <[email protected]>
sschuberth added a commit to oss-review-toolkit/ort-config that referenced this pull request Sep 21, 2023
sschuberth added a commit to oss-review-toolkit/ort-config that referenced this pull request Sep 21, 2023
sschuberth added a commit to oss-review-toolkit/ort-config that referenced this pull request Sep 21, 2023
This adopts to [1] by having run

    ort migrate --nuget-ids ort-config

[1]: oss-review-toolkit/ort#6046

Signed-off-by: Sebastian Schuberth <[email protected]>
sschuberth added a commit to oss-review-toolkit/ort-config that referenced this pull request Sep 21, 2023
This adopts to [1] by having run

    ort migrate --nuget-ids ort-config

[1]: oss-review-toolkit/ort#6046

Signed-off-by: Sebastian Schuberth <[email protected]>
sschuberth added a commit to oss-review-toolkit/ort-config that referenced this pull request Sep 21, 2023
This adopts to [1] by having run

    ort migrate --nuget-ids ort-config

[1]: oss-review-toolkit/ort#6046

Signed-off-by: Sebastian Schuberth <[email protected]>
mnonnenmacher added a commit that referenced this pull request Sep 21, 2023
Update a NuGet identifier to use a namespace as the analyzer would find
it since [1].

[1]: #6046

Signed-off-by: Martin Nonnenmacher <[email protected]>
mnonnenmacher added a commit that referenced this pull request Sep 21, 2023
Update a NuGet identifier to use a namespace as the analyzer would find
it since [1].

[1]: #6046

Signed-off-by: Martin Nonnenmacher <[email protected]>
mnonnenmacher added a commit that referenced this pull request Sep 21, 2023
Update a NuGet identifier to use a namespace as the analyzer would find
it since [1].

[1]: #6046

Signed-off-by: Martin Nonnenmacher <[email protected]>
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