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

Fix | Adding type convertor support for SqlConnectionEncryptOption #2057

Merged
merged 25 commits into from
Jul 6, 2023
Merged

Fix | Adding type convertor support for SqlConnectionEncryptOption #2057

merged 25 commits into from
Jul 6, 2023

Conversation

arellegue
Copy link
Contributor

@arellegue arellegue commented Jun 8, 2023 •
edited by JRahnama
Loading

fixes #1894
Added type converter for SqlConnectionEncryptOption.

@arellegue arellegue added the 🐛 Bug! Issues that are bugs in the drivers we maintain. label Jun 8, 2023
@arellegue arellegue added this to the 5.2.0-preview2 milestone Jun 8, 2023
@codecov
Copy link

codecov bot commented Jun 9, 2023 •
edited
Loading

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.80 ⚠️

Comparison is base (288a70c) 70.68% compared to head (7415ec1) 69.89%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2057      +/-   ##
==========================================
- Coverage   70.68%   69.89%   -0.80%     
==========================================
  Files         305      306       +1     
  Lines       61819    61956     +137     
==========================================
- Hits        43699    43302     -397     
- Misses      18120    18654     +534
Flag Coverage Δ
addons 92.88% <ø> (ø)
netcore 73.35% <100.00%> (-0.02%) ⬇️
netfx 68.17% <100.00%> (-1.20%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...osoft/Data/SqlClient/SqlConnectionEncryptOption.cs 93.33% <ø> (ø)
...a/SqlClient/SqlConnectionEncryptOptionConverter.cs 100.00% <100.00%> (ø)

... and 22 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@DavoudEshtehari DavoudEshtehari changed the title Sql connection encrypt option Fix | Adding type convertor support for SqlConnectionEncryptOption Jun 13, 2023
DavoudEshtehari
DavoudEshtehari reviewed Jun 15, 2023
View reviewed changes
...Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionEncryptOptionConverter.cs Outdated Show resolved Hide resolved
...Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionEncryptOptionConverter.cs Outdated Show resolved Hide resolved
src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionStringBuilderTest.cs Outdated Show resolved Hide resolved
src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionStringBuilderTest.cs Outdated Show resolved Hide resolved
src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionStringBuilderTest.cs Outdated Show resolved Hide resolved
src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionStringBuilderTest.cs Outdated Show resolved Hide resolved
...Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionEncryptOptionConverter.cs Show resolved Hide resolved
...Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionEncryptOptionConverter.cs Outdated Show resolved Hide resolved
src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionStringBuilderTest.cs Outdated Show resolved Hide resolved
tools/props/Versions.props Outdated Show resolved Hide resolved
DavoudEshtehari
DavoudEshtehari reviewed Jun 19, 2023
View reviewed changes
Copy link
Contributor

@DavoudEshtehari DavoudEshtehari left a comment

Choose a reason for hiding this comment

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

The xml files are only required for public APIs. In this case, if SqlConnectionEncryptOptionConverter type is going to be internal, remove the xml file and inline dependencies.

src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionStringBuilderTest.cs Outdated Show resolved Hide resolved
src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionStringBuilderTest.cs Outdated Show resolved Hide resolved
src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionStringBuilderTest.cs Outdated Show resolved Hide resolved
src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionStringBuilderTest.cs Outdated Show resolved Hide resolved
src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionStringBuilderTest.cs Show resolved Hide resolved
...Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionEncryptOptionConverter.cs Outdated Show resolved Hide resolved
...Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionEncryptOptionConverter.cs Outdated Show resolved Hide resolved
DavoudEshtehari and others added 2 commits June 19, 2023 17:06
@DavoudEshtehari
Apply suggestions from code review
7dee649
@arellegue
Removed unnecessary XML file for internal class SqlConnectionEncryptO…
c9d115c 
…ptionConverter.

Removed misplaced region.
Removed function that is not used.
@Kaur-Parminder Kaur-Parminder merged commit 1aa9af6 into dotnet:main Jul 6, 2023
DavoudEshtehari added a commit to DavoudEshtehari/SqlClient that referenced this pull request Oct 25, 2023
@DavoudEshtehari
Merged PR 4030: [5.1.2] Fix | Adding type convertor support for SqlCo…
0b51b63 
…nnectionEncryptOption (dotnet#2057)

Ports [dotnet#2057](dotnet#2057)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DavoudEshtehari DavoudEshtehari DavoudEshtehari approved these changes

@JRahnama JRahnama JRahnama approved these changes

@David-Engel David-Engel Awaiting requested review from David-Engel

@Kaur-Parminder Kaur-Parminder Awaiting requested review from Kaur-Parminder

Assignees
No one assigned
Labels
🐛 Bug! Issues that are bugs in the drivers we maintain.
Projects
None yet
Milestone
5.2.0-preview3
Development

Successfully merging this pull request may close these issues.

SQLConnectionStringBuilder 5.0 is not Backwards Compatible with appSettings.json
4 participants
@arellegue @JRahnama @DavoudEshtehari @Kaur-Parminder