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.

Sign up for GitHub

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

Jump to bottom

Use managed ntlm on linux-bionic #95274

Merged
merged 8 commits into from
Nov 29, 2023
Merged

Use managed ntlm on linux-bionic #95274

merged 8 commits into from
Nov 29, 2023

Conversation

rzikm
Copy link
Member

@rzikm rzikm commented Nov 27, 2023 •
edited by akoeplinger
Loading

Fixes #93104
Fixes #87665

@ghost ghost assigned rzikm Nov 27, 2023
@ghost
Copy link

ghost commented Nov 27, 2023

Tagging subscribers to this area: @dotnet/ncl, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #93104.

Author: rzikm
Assignees: -
Labels:

area-System.Net.Security
Milestone: -

@rzikm
Copy link
Member Author

rzikm commented Nov 27, 2023

/azp run runtime-linuxbionic

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

wfurt
wfurt approved these changes Nov 27, 2023
View reviewed changes
Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

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

LGTM

This was referenced Nov 27, 2023
Closed
Open
Closed
Closed
@wfurt wfurt mentioned this pull request Nov 28, 2023
Closed
@rzikm
Copy link
Member Author

rzikm commented Nov 28, 2023

/azp run runtime-linuxbionic

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

This was referenced Nov 28, 2023
Open
Closed
@rzikm
Copy link
Member Author

rzikm commented Nov 28, 2023

https://helixre107v0xdeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-95274-merge-c4a195fd94144e029e/System.Net.Security.Unit.Tests/1/console.d9c12e57.log?helixlogtype=result

[05:14:46] info: Running command System.Net.Security.Unit.Tests.sh took 2.6011403 seconds
[05:14:46] dbug: Exit code: 0
                 Std out:
                   Discovering: System.Net.Security.Unit.Tests (method display = ClassAndMethod, method display options = None)
                   Discovered:  System.Net.Security.Unit.Tests (found 75 of 78 test cases)
                   Starting:    System.Net.Security.Unit.Tests (parallel test collections = on, max threads = 8)
                     System.Net.Security.Tests.NegotiateAuthenticationTests.Package_Unsupported_NTLM [SKIP]
                       Condition(s) not met: "IsNtlmUnavailable"
                   Finished:    System.Net.Security.Unit.Tests
                 === TEST EXECUTION SUMMARY ===
                    System.Net.Security.Unit.Tests  Total: 97, Errors: 0, Failed: 0, Skipped: 1, Time: 1.251s

CI looks good

bartonjs
bartonjs requested changes Nov 28, 2023
View reviewed changes
Copy link
Member

@bartonjs bartonjs left a 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.

@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Nov 28, 2023
filipnavara
filipnavara approved these changes Nov 28, 2023
View reviewed changes
Copy link
Member

@filipnavara filipnavara left a 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.

@build-analysis build-analysis bot mentioned this pull request Nov 29, 2023
Closed
@MichalStrehovsky
Copy link
Member

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

<ProjectExclusions Include="$(MSBuildThisFileDirectory)System.Net.Security\tests\FunctionalTests\System.Net.Security.Tests.csproj" />

@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Nov 29, 2023
@rzikm
Copy link
Member Author

rzikm commented Nov 29, 2023

Can this test exclusion be also removed? Looks like it has been there since Bionic was initially added.

I don't think this is related to this change. NTLM tests are in the UnitTests project, not FunctionalTests.

@rzikm
Copy link
Member Author

rzikm commented Nov 29, 2023

/azp run runtime-linuxbionic

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rzikm
Copy link
Member Author

rzikm commented Nov 29, 2023

/azp run runtime-linuxbionic

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

This was referenced Nov 29, 2023
Closed
Closed
@rzikm @am11
Update src/libraries/Common/tests/System/Net/Capability.Security.Unix.cs
1ede567 
Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>
@rzikm
Copy link
Member Author

rzikm commented Nov 29, 2023

/azp run runtime-linuxbionic

@rzikm rzikm requested a review from bartonjs November 29, 2023 13:03
@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rzikm
Copy link
Member Author

rzikm commented Nov 29, 2023

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

[13:52:45] dbug: Exit code: 0
                 Std out:
                   Discovering: System.Net.Security.Unit.Tests (method display = ClassAndMethod, method display options = None)
                   Discovered:  System.Net.Security.Unit.Tests (found 75 of 78 test cases)
                   Starting:    System.Net.Security.Unit.Tests (parallel test collections = on, max threads = 1)
                     System.Net.Security.Tests.NegotiateAuthenticationTests.Package_Unsupported_NTLM [SKIP]
                       Condition(s) not met: "IsNtlmUnavailable"
                   Finished:    System.Net.Security.Unit.Tests
                 === TEST EXECUTION SUMMARY ===
                    System.Net.Security.Unit.Tests  Total: 97, Errors: 0, Failed: 0, Skipped: 1, Time: 4.565s

@rzikm
Copy link
Member Author

rzikm commented Nov 29, 2023

All CI failures are known build errors.

@rzikm rzikm merged commit c0dfcfd into dotnet:main Nov 29, 2023
106 of 114 checks passed
@build-analysis build-analysis bot mentioned this pull request Nov 29, 2023
Closed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 30, 2023
@karelz karelz added this to the 9.0.0 milestone May 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@akoeplinger akoeplinger akoeplinger left review comments

@filipnavara filipnavara filipnavara approved these changes

@am11 am11 am11 left review comments

@jkotas jkotas jkotas left review comments

@wfurt wfurt wfurt approved these changes

@bartonjs bartonjs bartonjs approved these changes

Assignees

@rzikm rzikm

Labels
area-System.Net.Security
Projects
None yet
Milestone
9.0.0
9 participants
@rzikm @MichalStrehovsky @akoeplinger @filipnavara @am11 @jkotas @bartonjs @wfurt @karelz