Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Merge | Managed SNI#3345

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
benrr101 merged 29 commits intomainfromdev/russellben/merge/netcore-sni
Jun 3, 2025
Merged

Merge | Managed SNI#3345

benrr101 merged 29 commits intomainfromdev/russellben/merge/netcore-sni
Jun 3, 2025

Conversation

benrr101
Copy link
Contributor

Description: This PR looks a lot bigger than it really is. The goal here is to move all the files for netcore's managed SNI implementation into the common project. I also took the liberty to make some other changes at the same time.

  • Move all SNI files to the common project
  • Suffix all managed SNI files with ".netcore"
  • Wrap all files in #if NET
  • Rename SNI namespace to ManagedSni - I feel there's a good chance we can (and should) separate native SNI implementation into its own namespace as well, so having this separation will be useful
  • Rename SNI to Sni to follow naming conventions

Testing: Project builds on windows and linux. There is no change to functionality, just moving code around.

@benrr101benrr101 added this to the6.1-preview2 milestoneMay 13, 2025
@benrr101benrr101 requested review froma team andCopilotMay 13, 2025 18:52
@benrr101benrr101 added the Common Project 🚮Things that relate to the common project project labelMay 13, 2025
Copy link
Contributor

@CopilotCopilotAI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR moves all .NET Core managed SNI implementation files into the common project while updating their file names, namespace, and type names to follow new naming conventions. Key changes include moving files to a shared location, renaming types (e.g. SNIPacket to SniPacket, SNIAsyncCallback to SniAsyncCallback), and wrapping the code with “#if NET” conditionals.

Reviewed Changes

Copilot reviewed 35 out of 35 changed files in this pull request and generated no comments.

FileDescription
SniPacket.netcore.csRenaming of SNIPacket to SniPacket and updating internal references consistently.
SniNpHandle.netcore.csUpdated namespace and type names from SNINpHandle to SniNpHandle with consistent logging calls.
SniMarsHandle.netcore.csRenamed classes and updated event/logging references to adhere to the new ManagedSni naming conventions.
(Other files)Similar renaming updates and #if NET wrapping applied across the managed SNI implementation files.
Comments suppressed due to low confidence (3)

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ManagedSni/SniPacket.netcore.cs:15

  • Ensure that the renaming from SNIPacket to SniPacket and similar types consistently uses the new naming convention (lowercase 'i') across the entire file.
namespace Microsoft.Data.SqlClient.ManagedSni

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ManagedSni/SniCommon.netcore.cs:63

  • Verify that all log calls and references have been updated to use 'SniCommon' consistently, matching the updated naming convention.
SqlClientEventSource.Log.TrySNITraceEvent(nameof(SniCommon), ...

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ManagedSni/SniMarsHandle.netcore.cs:293

  • Double-check that internal event scope usages reflect 'SniMarsHandle' accurately to avoid mis-references following the renaming.
using (TrySNIEventScope.Create(nameof(SniMarsHandle)))

Copy link
Contributor

@mdaiglemdaigle left a comment

Choose a reason for hiding this comment

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

All the changes look good to me. I'll take another look and approve when the CI is passing.

paulmedynski
paulmedynski previously approved these changesMay 16, 2025
paulmedynski
paulmedynski previously approved these changesMay 20, 2025
@codecovCodecov
Copy link

codecovbot commentedMay 21, 2025•
edited
Loading

Codecov Report

Attention: Patch coverage is70.71429% with123 lines in your changes missing coverage. Please review.

Project coverage is 65.27%. Comparing base(072f318) to head(23b0553).
Report is 2 commits behind head on main.

Files with missing linesPatch %Lines
.../Data/SqlClient/ManagedSni/SniTcpHandle.netcore.cs62.85%26 Missing⚠️
...soft/Data/SqlClient/ManagedSni/SniProxy.netcore.cs33.33%22 Missing⚠️
...t/Data/SqlClient/ManagedSni/SniNpHandle.netcore.cs60.00%18 Missing⚠️
...ta/SqlClient/ManagedSni/LocalDB.netcore.Windows.cs0.00%11 Missing⚠️
.../SqlClient/ManagedSni/SniMarsConnection.netcore.cs75.00%11 Missing⚠️
...Data/SqlClient/ManagedSni/SniMarsHandle.netcore.cs87.50%8 Missing⚠️
...oft/Data/SqlClient/ManagedSni/SniCommon.netcore.cs58.82%7 Missing⚠️
...ft/Data/SqlClient/ManagedSni/SsrpClient.netcore.cs50.00%7 Missing⚠️
...qlClient/ManagedSni/SslOverTdsStream.NetCoreApp.cs0.00%4 Missing⚠️
...a/SqlClient/ManagedSni/SniNetworkStream.netcore.cs85.71%3 Missing⚠️
... and3 more
Additional details and impacted files
@@           Coverage Diff           @@##             main    #3345   +/-   ##=======================================  Coverage   65.26%   65.27%           =======================================  Files         299      300    +1       Lines       65640    65640           =======================================+ Hits        42842    42846    +4+ Misses      22798    22794    -4
FlagCoverage Δ
addons92.58% <ø> (ø)
netcore68.48% <70.71%> (+0.02%)⬆️
netfx66.54% <ø> (+0.01%)⬆️

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

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

🚀 New features to boost your workflow:
  • ❄️Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

mdaigle
mdaigle previously approved these changesMay 21, 2025
# Conflicts:#src/Microsoft.Data.SqlClient/netcore/src/Microsoft.Data.SqlClient.csproj#src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/PacketHandle.Windows.cs
paulmedynski
paulmedynski previously approved these changesMay 29, 2025
mdaigle
mdaigle previously approved these changesMay 29, 2025
@benrr101benrr101 dismissed stale reviews frommdaigle andpaulmedynski via23b0553May 29, 2025 22:24
@benrr101benrr101 merged commitcc23e04 intomainJun 3, 2025
251 checks passed
@benrr101benrr101 deleted the dev/russellben/merge/netcore-sni branchJune 3, 2025 16:06
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@edwardnealedwardnealedwardneal left review comments

Copilot code reviewCopilotCopilot left review comments

@mdaiglemdaiglemdaigle approved these changes

@paulmedynskipaulmedynskipaulmedynski approved these changes

Assignees
No one assigned
Labels
Common Project 🚮Things that relate to the common project project
Projects
None yet
Milestone
6.1-preview2
Development

Successfully merging this pull request may close these issues.

4 participants
@benrr101@mdaigle@paulmedynski@edwardneal
[8]ページ先頭
©2009-2025 Movatter.jp