Skip to content

Port #3126 to release/6.0 - #3127

Merged
benrr101 merged 2 commits into
release/6.0from
dev/russellben/port-3126
Jan 22, 2025
Merged

Port #3126 to release/6.0#3127
benrr101 merged 2 commits into
release/6.0from
dev/russellben/port-3126

Conversation

@benrr101

Copy link
Copy Markdown
Contributor

Ports #3126 to the release/6.0 branch. A couple merge conflicts were resolved. Please see the original PR for more information.

* Added test for downlevel connectivity warning
* Correctly test bit flags for legacy SSL protocol warning
* Corrected warning disablement/restore.
(cherry picked from commit 198b906)
@benrr101
benrr101 requested a review from a teamJanuary 21, 2025 20:03
@benrr101

Copy link
Copy Markdown
ContributorAuthor

@edwardneal no need to make your own PR for this :)

@benrr101

Copy link
Copy Markdown
ContributorAuthor

@edwardneal Ok, there's a good chance I bungled something up with the unit tests, since there were a couple conflicts. Would you be willing to take a peek at the unit test failures? I'm looking into the issue on my end, too.

@edwardneal

Copy link
Copy Markdown
Contributor

These failures look odd. I've pulled your branch locally, and these tests succeed. The diff looks reasonable to me.
The operative error is within SslStream: "The client and server cannot communicate, because they do not possess a common algorithm". It looks as if TLS 1.0 and 1.1 are disabled on the build agents - which makes sense, given their age. We see the same thing on the main CI too.

I don't think we can test non-TLS 1.2 connections in CI, so I'd revert the change to ConnectionTestParametersData.cs. If that passes, I'll replicate the change in main.

@benrr101

Copy link
Copy Markdown
ContributorAuthor

@edwardneal, ok, let's try with the changes to the test matrix rolled back. I'm having a hard time confirming locally that it might work, I get the similar (same?) test failures when running main from before #3126 .

I have no idea how this happened but the PR to main didn't have passing CI run. So even main is broken....

@edwardneal

Copy link
Copy Markdown
Contributor

Thanks - once this is passing, I'll submit a PR correcting the main build break.

@codecov

codecovBot commented Jan 22, 2025

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 0% with 5 lines in your changes missing coverage. Please review.

Project coverage is 72.82%. Comparing base (e358d68) to head (359d2d2).
Report is 3 commits behind head on release/6.0.

Files with missing linesPatch %Lines
...Microsoft/Data/SqlClient/TdsParserHelperClasses.cs0.00%5 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## release/6.0 #3127 +/- ##
===============================================
+ Coverage 72.66% 72.82% +0.16% 
===============================================
Files 285 285 Lines 59162 59162 ===============================================
+ Hits 42988 43085 +97 + Misses 16174 16077 -97 
FlagCoverage Δ
addons92.58% <ø> (ø)
netcore75.53% <0.00%> (+0.03%)⬆️
netfx71.22% <0.00%> (+0.20%)⬆️

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 cfb007d into release/6.0Jan 22, 2025
@benrr101
benrr101 deleted the dev/russellben/port-3126 branch January 22, 2025 23:31
@mdaiglemdaigle added this to the 6.0.0 milestone Mar 19, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@benrr101@edwardneal@VladimirReshetnikov@mdaigle