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

Remove SupportedOSPlatformAttribute("windows") from LdapSessionOption #61388

Merged
merged 5 commits into from
Nov 24, 2021

Conversation

macsux
Copy link
Contributor

@macsux macsux commented Nov 10, 2021

Remove SupportedOSPlatformAttribute("windows") from LdapSessionOptions due to LDAPS now being supported on Linux as of .NET 6. Relevant discussion in issue #60972

…s due to LDAPS now being supported on Linux as of .NET 6
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Nov 10, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.DirectoryServices new-api-needs-documentation and removed community-contribution Indicates that the PR has been added by a community member labels Nov 10, 2021
@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented Nov 10, 2021

Tagging subscribers to this area: @jay98014
See info in area-owners.md if you want to be subscribed.

Issue Details

Remove SupportedOSPlatformAttribute("windows") from LdapSessionOptions due to LDAPS now being supported on Linux as of .NET 6. Relevant discussion in issue #60972

Author: macsux
Assignees: -
Labels:

area-System.DirectoryServices, new-api-needs-documentation

Milestone: -

…nges previous commit that removed attribute entirely
@macsux
Copy link
Contributor Author

macsux commented Nov 10, 2021

@buyaa-n I just realized that I opened this PR against main branch. Should I reopen this against v6.0 release branch as this would be a bug fix?

@buyaa-n
Copy link
Member

buyaa-n commented Nov 10, 2021

@buyaa-n I just realized that I opened this PR against main branch. Should I reopen this against v6.0 release branch as this would be a bug fix?

  1. Every PR goes to the main first, it can be backported to other branches from there
  2. I am not sure if this could get approval for servicing in 6.0

@joperezr
Copy link
Member

Right, as @buyaa-n suggests let's get this in main first, and then if we think it meets the bar then we can open a backport PR for 6.0 branch and check with servicing if something like this can get approved.

Copy link
Member

@joperezr joperezr left a comment

Choose a reason for hiding this comment

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

Added a tiny comment but other than that this looks good, Thanks for fixing!

@macsux
Copy link
Contributor Author

macsux commented Nov 11, 2021

Since I'm in here, should we add a SupportedOSPlatformAttribute("windows") to VerifyServerCertificate? Seems like it's only implemented under Windows and throws on other implementations since OpenLDAP supports only a handful of preconfigured validation modes rather then offering a callback like Windows API does.

There should be a discussion may be on the linked issue as to whether it makes sense to add something like bool SkipSslCertificateValidation to LdapSessionOptions, but this is an API surface change which I think would require approval to be implemented.

@joperezr
Copy link
Member

Do you mind sending a separate PR for that one? I know that it is very related, but what I'm thinking is that we might want to take this specific change into release/6.0 branch (as we are only removing an attribute saying an API is not supported) so it is less of a breaking change than to adding an attribute to an API that claimed to be supported and now say it is not. I understand even without the attribute the API is actually not supported, but it just is a more impacting breaking change, so it will be easier to port just this change into release, than if we have both.

@joperezr joperezr merged commit 40dc863 into dotnet:main Nov 24, 2021
@joperezr
Copy link
Member

Sorry it took so long to merge this one, thanks a lot for your contribution @macsux!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants