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

A brave attempt to fix the most voted issue - support |DataDirectory| with .NET Core client #284

Merged
merged 11 commits into from Nov 14, 2019
Merged

A brave attempt to fix the most voted issue - support |DataDirectory| with .NET Core client #284

merged 11 commits into from Nov 14, 2019

Conversation

ErikEJ
Copy link
Contributor

@ErikEJ ErikEJ commented Oct 23, 2019

Probably WIP!

related to #16

@ErikEJ
Review feedback
61ac23e 
Add 2 missing tests
@ErikEJ
Copy link
Contributor Author

ErikEJ commented Oct 24, 2019

Nice, can now see test result on DevOps!

@ErikEJ
Copy link
Contributor Author

ErikEJ commented Oct 24, 2019

Hmm... Looks like the "positive" .netcoreapp2.1 tests should run on Windows only - how do I fix that?

@Wraith2
Copy link
Contributor

Wraith2 commented Oct 24, 2019 •
edited

[PlatformSpecific(TestPlatforms.Windows)] i think, or possibly [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsWindows)] if that doesn't work. Is there any specific reason this couldn't work on linux?

@ErikEJ
Copy link
Contributor Author

ErikEJ commented Oct 24, 2019

@Wraith2 Thanks, I figured it out: [PlatformSpecific(TestPlatforms.Windows)] did the job!

@Wraith2
Copy link
Contributor

Wraith2 commented Oct 24, 2019

Was it mostly a copy paste job from the netfx version or reference source?

@ErikEJ
Copy link
Contributor Author

ErikEJ commented Oct 25, 2019

@Wraith2 Yes, mostly copy paste forom netfx code, I will comment where I have made major changes

@ErikEJ
Copy link
Contributor Author

ErikEJ commented Oct 25, 2019

Failing test seems unrelated !?

@David-Engel
Copy link
Contributor

Failing test seems unrelated !?

I re-ran the builds and they passed. I've adjusted the pipelines so they don't spawn so many jobs simultaneously. I think the SQL endpoints can get overwhelmed if certain authentication-related tests all hit at the same time.

@ErikEJ
Copy link
Contributor Author

ErikEJ commented Oct 26, 2019

Again test errors that seem unrelated / flaky

@Wraith2 Wraith2 mentioned this pull request Nov 7, 2019
Closed
@ErikEJ
Fix spelling
9e4caf1
@ErikEJ
Move localized string
41ede35 
Add SqlConnectionStringBuilder tests
David-Engel
David-Engel requested changes Nov 13, 2019
View reviewed changes
Copy link
Contributor

@David-Engel David-Engel left a comment

Choose a reason for hiding this comment

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

Just the one line needs to be reverted. Otherwise, I approve.

src/Microsoft.Data.SqlClient/netcore/src/Resources/SR.Designer.cs Outdated Show resolved Hide resolved
@cheenamalhotra cheenamalhotra added this to the 1.1.0 milestone Nov 14, 2019
@cheenamalhotra cheenamalhotra added this to In progress in SqlClient v1.1.0 via automation Nov 14, 2019
@cheenamalhotra cheenamalhotra moved this from In progress to Reviewer approved in SqlClient v1.1.0 Nov 14, 2019
@cheenamalhotra cheenamalhotra merged commit 1a0748e into dotnet:master Nov 14, 2019
SqlClient v1.1.0 automation moved this from Reviewer approved to Done Nov 14, 2019
@ErikEJ ErikEJ mentioned this pull request Mar 26, 2020
Merged
@ErikEJ ErikEJ mentioned this pull request Jan 31, 2023
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Wraith2 Wraith2 Wraith2 left review comments

@cheenamalhotra cheenamalhotra cheenamalhotra approved these changes

@David-Engel David-Engel David-Engel approved these changes

@saurabh500 saurabh500 Awaiting requested review from saurabh500

Assignees
No one assigned
Labels
None yet
Projects
No open projects
SqlClient v1.1.0
  
Done
Milestone
1.1.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ErikEJ @Wraith2 @David-Engel @saurabh500 @cheenamalhotra