Skip to content

Merge | Add IDBColumnSchemaGenerator interface to netfx SqlDataReader#2967

Merged
mdaigle merged 14 commits into
dotnet:mainfrom
MichelZ:merge-sqldatareader-IDbColumnSchemaGenerator
Jan 2, 2025
Merged

Merge | Add IDBColumnSchemaGenerator interface to netfx SqlDataReader#2967
mdaigle merged 14 commits into
dotnet:mainfrom
MichelZ:merge-sqldatareader-IDbColumnSchemaGenerator

Conversation

@MichelZ

@MichelZ MichelZ commented Nov 2, 2024

Copy link
Copy Markdown
Contributor

Bring IDBColumnSchemaGenerator to netfx for later code base merging

I made sure to enable the respective test for netfx

Part of #2965

@MichelZ

MichelZ commented Nov 2, 2024

Copy link
Copy Markdown
Contributor Author

/azp run

@azure-pipelines

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

@MichelZ

MichelZ commented Nov 2, 2024

Copy link
Copy Markdown
Contributor Author

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

@edwardneal

Copy link
Copy Markdown
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

MichelZ commented Nov 3, 2024

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Contributor

/azp run

@azure-pipelines

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

@MichelZ

MichelZ commented Nov 3, 2024

Copy link
Copy Markdown
Contributor Author

Thanks for trying :)

@ErikEJ

ErikEJ commented Nov 3, 2024

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

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

@benrr101

benrr101 commented Nov 4, 2024

Copy link
Copy Markdown
Contributor

@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

benrr101 commented Nov 5, 2024

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

Copy link
Copy Markdown
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

MichelZ commented Nov 6, 2024

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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
Copy Markdown
Contributor

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@benrr101 benrr101 left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread src/Microsoft.Data.SqlClient/src/Microsoft.Data.SqlClient.csproj
@MichelZ MichelZ force-pushed the merge-sqldatareader-IDbColumnSchemaGenerator branch from 283d72a to 7fc9351 Compare November 12, 2024 09:40

@cheenamalhotra cheenamalhotra left a comment

Copy link
Copy Markdown
Member

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.

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.

7 participants