Skip to content

Merge | LocalDBAPI #3163

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

Merged
merged 13 commits into from
Feb 20, 2025
Merged

Merge | LocalDBAPI #3163

merged 13 commits into from
Feb 20, 2025

Conversation

benrr101
Copy link
Contributor

This is a replacement of #3047 that will give us the complete CI

Description: This is one more PR that targets merging another file between the netfx project and the netcore project. This time, it's the LocalDBAPI. This one was a bit complicated because the files were split differently between netfx and netcore. The way I approached it was to merge the operative code into the LocalDBAPI.Windows file and merge the inoperative code to the LocalDBAPI.Unix file. The Windows file will be pulled in on both netfx and netcore projects, with it being conditionally included on netcore if the build is for Windows. The Unix file will be pulled in on netcore only, being conditionally included if the build is for Unix. Otherwise, this is a fairly standard merge with netfx having extra stuff that is #if'd in.

please note: cleanup to remove monitor sections, etc, is coming in a later PR.

Testing: There miiiiight be some issues with unix build, I might not have fully vetted it on unix.

@benrr101 benrr101 added this to the 7.0-preview1 milestone Feb 19, 2025
@benrr101 benrr101 requested a review from a team February 19, 2025 19:26
@benrr101 benrr101 added the Common Project 🚮 Things that relate to the common project project label Feb 19, 2025
@benrr101 benrr101 changed the title Dev/russellben/merge/localdbapi Merge | LocalDBAPI Feb 19, 2025
@benrr101 benrr101 mentioned this pull request Feb 19, 2025
@@ -8,6 +8,9 @@ namespace Microsoft.Data
{
internal static partial class LocalDBAPI
{
internal static string GetLocalDbInstanceNameFromServerName(string serverName) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a change in behavior, but I agree with it. Right now, we try to get the local db instance name for unix, and may return a non-null value, but later steps can fail with a PlatformNotSupportedException.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On second look, yes I think you're right that it's a behavior change. I think the one thing I'm not sure about is if this will raise a platformnotsupported exception or if it will just fail to connect. I'm going to fire this up on linux to verify.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can confirm, on Linux, the behavior is the same - we get the "LocalDB is not supported on this platform" exception when trying to connect.

Copy link

codecov bot commented Feb 20, 2025

Codecov Report

Attention: Patch coverage is 24.71910% with 67 lines in your changes missing coverage. Please review.

Project coverage is 72.84%. Comparing base (17cb0b0) to head (bc3857d).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
...src/Microsoft/Data/SqlClient/LocalDBAPI.Windows.cs 23.86% 67 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3163      +/-   ##
==========================================
- Coverage   72.96%   72.84%   -0.12%     
==========================================
  Files         283      282       -1     
  Lines       58997    59175     +178     
==========================================
+ Hits        43048    43107      +59     
- Misses      15949    16068     +119     
Flag Coverage Δ
addons 92.58% <ø> (ø)
netcore 75.48% <15.51%> (-0.25%) ⬇️
netfx 71.23% <24.41%> (-0.13%) ⬇️

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.

@benrr101 benrr101 merged commit e25c24a into main Feb 20, 2025
252 checks passed
@benrr101 benrr101 deleted the dev/russellben/merge/localdbapi branch February 20, 2025 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Common Project 🚮 Things that relate to the common project project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants