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

Use case insensitive comparison for determining which packages should be excluded from publishing #809

Merged
merged 2 commits into from
Feb 4, 2017

Conversation

dsplaisted
Copy link
Member

Fixes #376

@@ -37,7 +37,7 @@ public IEnumerable<LockFileTargetLibrary> GetRuntimeLibraries(IEnumerable<string
Dictionary<string, LockFileTargetLibrary> libraryLookup =
runtimeLibraries.ToDictionary(e => e.Name, StringComparer.OrdinalIgnoreCase);

HashSet<string> allExclusionList = new HashSet<string>();
HashSet<string> allExclusionList = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
Copy link
Member

Choose a reason for hiding this comment

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

Doing a quick code search, it appears

var exclusionList = new HashSet<string>();

has the same issue. What do you think about changing that as well? That method is only called from this method and unioned with this hashset, but in case someone else adds a call to it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, thanks. I've made the change you suggest.

@srivatsn
Copy link
Contributor

srivatsn commented Feb 4, 2017

@dsplaisted can you fill out the template and tag Matt?

@srivatsn
Copy link
Contributor

srivatsn commented Feb 4, 2017

Tagging @MattGertz

Customer scenario

Users who have a PackageReference that's marked as PrivateAssets will have those assets published if the case of the PackageReference doesn't exactly match the NuGet package's name.Bugs this fixes:

Fixes
#376

Workarounds, if any

Fix the casing.

Risk

Low.

Performance impact

Low.

Is this a regression from a previous update?

No.

Root cause analysis:

Used wrong string comparison.

How was the bug found?

Customer reported.

@srivatsn
Copy link
Contributor

srivatsn commented Feb 4, 2017

JoC approved this offline.

@srivatsn srivatsn merged commit fde4b29 into dotnet:master Feb 4, 2017
@dsplaisted
Copy link
Member Author

@srivatsn Thanks for filling out the escrow template and getting this merged!

@dsplaisted dsplaisted deleted the privateassets-publish-376 branch February 5, 2017 16:47
mmitche pushed a commit to mmitche/sdk that referenced this pull request Jun 5, 2020
…728.1 (dotnet#809)

- Microsoft.DotNet.Arcade.Sdk - 1.0.0-beta.19378.1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants