-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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 managed ntlm on linux-bionic #95274
Conversation
Tagging subscribers to this area: @dotnet/ncl, @bartonjs, @vcsjones Issue DetailsFixes #93104.
|
/azp run runtime-linuxbionic |
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.Unix.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.Unix.cs
Outdated
Show resolved
Hide resolved
/azp run runtime-linuxbionic |
Azure Pipelines successfully started running 1 pipeline(s). |
CI looks good |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should either be StartsWith("linux-bionic-") or, if it's sometimes Equals and sometimes has a hyphen, should change to regex. But StartsWith ending with an "unterminated" string isn't good in production code.
src/libraries/Common/src/Interop/Unix/System.Net.Security.Native/Interop.GssApiException.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM in general. I agree with the StartsWith("linux-bionic-")
comment.
src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.Unix.cs
Show resolved
Hide resolved
Can this test exclusion be also removed? Looks like it has been there since Bionic was initially added. runtime/src/libraries/tests.proj Line 175 in 7543b05
|
I don't think this is related to this change. NTLM tests are in the UnitTests project, not FunctionalTests. |
/azp run runtime-linuxbionic |
Azure Pipelines successfully started running 1 pipeline(s). |
src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.Unix.cs
Outdated
Show resolved
Hide resolved
This reverts commit 82c1136.
/azp run runtime-linuxbionic |
Azure Pipelines successfully started running 1 pipeline(s). |
src/libraries/Common/tests/System/Net/Capability.Security.Unix.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>
/azp run runtime-linuxbionic |
Azure Pipelines successfully started running 1 pipeline(s). |
CI looks good https://helixre107v0xdcypoyl9e7f.blob.core.windows.net/dotnet-runtime-refs-pull-95274-merge-01c93e7798af48e0a3/System.Net.Security.Unit.Tests/1/console.a6959422.log?helixlogtype=result
|
All CI failures are known build errors. |
Fixes #93104
Fixes #87665