Skip to content

Conversation

@papafe
Copy link
Contributor

@papafe papafe commented Aug 15, 2025

No description provided.

@papafe papafe added the chore Non–user-facing code changes (tests, build scripts, etc.). label Aug 15, 2025
@BorisDog BorisDog requested a review from sanych-sun August 15, 2025 17:53
@papafe papafe marked this pull request as ready for review August 18, 2025 13:26
@papafe papafe requested a review from a team as a code owner August 18, 2025 13:26
sanych-sun
sanych-sun previously approved these changes Aug 18, 2025
Copy link
Member

@sanych-sun sanych-sun left a comment

Choose a reason for hiding this comment

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

LGTM + minor comments

{
RequireServer.Check().Tls(required: true);

string path, password;
Copy link
Member

Choose a reason for hiding this comment

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

Minor: It's unusual to have multiple variables declaration in a single line.

switch (certificateType)
{
case CertificateType.MONGO_X509:
RequireEnvironment.Check()
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest to reorganize the code a little: set the names of env variables inside the switch, but read the values out side. Something like this:

string cert_path_variable;
switch (certificateType)
{
    case CertificateType.MONGO_X509:
      cert_path_variable= MONGODB_X509_CLIENT_CERTIFICATE_PATH;
      break;
    ...
}

RequireEnvironment.Check().EnvironmentVariable(cert_path_variable);
var path = Environment.GetEnvironmentVariable(cert_path_variable);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense!

Copy link
Member

@sanych-sun sanych-sun left a comment

Choose a reason for hiding this comment

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

LGTM

@papafe papafe merged commit 3669ba7 into mongodb:main Aug 21, 2025
35 checks passed
papafe added a commit to papafe/mongo-csharp-driver that referenced this pull request Aug 21, 2025
papafe added a commit to papafe/mongo-csharp-driver that referenced this pull request Aug 21, 2025
@papafe papafe deleted the csharp5581 branch September 2, 2025 07:40
papafe added a commit to papafe/mongo-csharp-driver that referenced this pull request Sep 4, 2025
BorisDog pushed a commit to BorisDog/mongo-csharp-driver that referenced this pull request Sep 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chore Non–user-facing code changes (tests, build scripts, etc.).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants