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

Add IDBColumnSchemaGenerator interface to netfx SqlDataReader #2967

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

MichelZ
Copy link
Contributor

@MichelZ MichelZ commented Nov 2, 2024

Bring IDBColumnSchemaGenerator to netfx for later code base merging

I made sure to enable the respective test for netfx

Part of #2965

@MichelZ
Copy link
Contributor Author

MichelZ commented Nov 2, 2024

/azp run

Copy link

Commenter does not have sufficient privileges for PR 2967 in repo dotnet/SqlClient

@MichelZ
Copy link
Contributor Author

MichelZ commented Nov 2, 2024

@edwardneal Would you mind running the pipeline for me on this one? :)

@edwardneal
Copy link
Contributor

Thanks for this MichelZ. The changes look good to me; would you mind feeding the extra package reference through to the nuspec file and the .NET Framework reference csproj please?

I don't have access to the pipelines, but hopefully the SqlClient team will be able to look at it in a few days. Something's definitely odd there - your PRs didn't run the CI builds, and my commit ran the CI build but encountered a lot more timeouts than normal in the tests.

@MichelZ
Copy link
Contributor Author

MichelZ commented Nov 3, 2024

Will do. I'm not a contributor, that's probably why the pipelines don't run for me (yet)
(Can you try to just do an /azp run and see what happens?)

@edwardneal
Copy link
Contributor

/azp run

Copy link

Commenter does not have sufficient privileges for PR 2967 in repo dotnet/SqlClient

@MichelZ
Copy link
Contributor Author

MichelZ commented Nov 3, 2024

Thanks for trying :)

@ErikEJ
Copy link
Contributor

ErikEJ commented Nov 3, 2024

/azp run

Copy link

Commenter does not have sufficient privileges for PR 2967 in repo dotnet/SqlClient

@benrr101
Copy link
Contributor

benrr101 commented Nov 4, 2024

@ErikEJ @edwardneal @MichelZ We have changed security rules recently such that only contributors can kick off pipeline runs. This is due to the potential for contributors to run code in PRs that could be hazardous to our build agents or cause a DoS.

@benrr101
Copy link
Contributor

benrr101 commented Nov 5, 2024

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@benrr101 benrr101 added the Common Project 🚮 Things that relate to the common project project label Nov 5, 2024
@MichelZ
Copy link
Contributor Author

MichelZ commented Nov 6, 2024

This package needs to be added to the sqlclientdriver nuget feed for this build to succeed:
System.Data.Common: 4.3.0

@benrr101
Copy link
Contributor

@MichelZ I might've mentioned it before but yep, we've got security on the internal nuget feed such that only contributors can pull upstream packages from the feed. I've gone ahead and added System.Data.Common 4.3.0 to the feed, so it should be good to go.

@benrr101
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link
Contributor

@benrr101 benrr101 left a comment

Choose a reason for hiding this comment

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

I'd need to see @cheenamalhotra 's opinion regarding the System.Data.Common inclusion before I explicitly approve.

@MichelZ MichelZ force-pushed the merge-sqldatareader-IDbColumnSchemaGenerator branch from 283d72a to 7fc9351 Compare November 12, 2024 09:40
Copy link
Member

@cheenamalhotra cheenamalhotra left a comment

Choose a reason for hiding this comment

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

LGTM overall, please resolve conflicts when possible.

@MichelZ
Copy link
Contributor Author

MichelZ commented Nov 19, 2024

Done. I'd need another /azp run I guess :)

@David-Engel
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link

codecov bot commented Nov 22, 2024

Codecov Report

Attention: Patch coverage is 77.41935% with 7 lines in your changes missing coverage. Please review.

Project coverage is 72.65%. Comparing base (1b9df10) to head (27a117e).

Files with missing lines Patch % Lines
...etfx/src/Microsoft/Data/SqlClient/SqlDataReader.cs 77.41% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2967      +/-   ##
==========================================
+ Coverage   72.64%   72.65%   +0.01%     
==========================================
  Files         285      285              
  Lines       59160    59191      +31     
==========================================
+ Hits        42979    43008      +29     
- Misses      16181    16183       +2     
Flag Coverage Δ
addons 92.58% <ø> (ø)
netcore 75.39% <ø> (+0.01%) ⬆️
netfx 71.09% <77.41%> (+<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.


🚨 Try these New Features:

@MichelZ
Copy link
Contributor Author

MichelZ commented Nov 22, 2024

I'm currently unsure what causes this issue....


"G:\MIZE\DEV\GitHub\SqlClient\build.proj" (BuildTestsNetFx target) (1) ->
"G:\MIZE\DEV\GitHub\SqlClient\src\Microsoft.Data.SqlClient\tests\ManualTests\Microsoft.Data.SqlClient.ManualTesting.Tests.csproj" (default target) (11:11) ->
"G:\MIZE\DEV\GitHub\SqlClient\src\Microsoft.Data.SqlClient\tests\ManualTests\Microsoft.Data.SqlClient.ManualTesting.Tests.csproj" (Build target) (11:12) ->
(CoreCompile target) ->
  G:\MIZE\DEV\GitHub\SqlClient\src\Microsoft.Data.SqlClient\tests\ManualTests\SQL\UdtTest\SqlServerTypesTest.cs(264,86): error CS1061: 'SqlDataReader' does not contain a definition for 'GetColumnSchema' and no accessib
le extension method 'GetColumnSchema' accepting a first argument of type 'SqlDataReader' could be found (are you missing a using directive or an assembly reference?) [G:\MIZE\DEV\GitHub\SqlClient\src\Microsoft.Data.Sql
Client\tests\ManualTests\Microsoft.Data.SqlClient.ManualTesting.Tests.csproj::TargetFramework=net462]

The method is clearly available

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.

6 participants