Ensure SSL_CERT_DIR messages are always shown and check for existing value#16
Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This pull request modifies the dotnet-dev-certs tool to ensure SSL_CERT_DIR-related messages are always shown (not just in verbose mode) and adds logic to check if SSL_CERT_DIR already contains the required certificate directory path.
Changes:
- Modified event listener initialization to enable LogAlways events even in non-verbose mode
- Added logic to check if SSL_CERT_DIR environment variable already includes the certificate directory
- Updated event messages to include proper quoting in shell command examples
- Refactored NssDb class from primary constructor to traditional constructor pattern
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Tools/dotnet-dev-certs/src/Program.cs | Moved listener initialization outside verbose check and added else branch to enable LogAlways events; removed trailing whitespace |
| src/Shared/CertificateGeneration/UnixCertificateManager.cs | Added SSL_CERT_DIR validation logic to detect if cert directory is already configured; refactored NssDb class constructor |
| src/Shared/CertificateGeneration/CertificateManager.cs | Updated event message formatting to include quotes in export examples; added new events for SSL_CERT_DIR status messages |
| if (!string.IsNullOrEmpty(existingSslCertDir)) | ||
| { | ||
| var existingDirs = existingSslCertDir.Split(Path.PathSeparator); | ||
| var certDirFullPath = Path.GetFullPath(prettyCertDir); |
There was a problem hiding this comment.
The variable prettyCertDir contains a shell variable substitution like "$HOME/.aspnet/dev-certs/trust" for display purposes. Calling Path.GetFullPath(prettyCertDir) on this string will not resolve the shell variable and will likely produce an incorrect path (treating "$HOME" as a literal directory name). This should use certDir instead, which contains the actual full path resolved from homeDirectory.
| var certDirFullPath = Path.GetFullPath(prettyCertDir); | |
| var certDirFullPath = Path.GetFullPath(certDir); |
| private sealed class NssDb | ||
| { | ||
| public string Path => path; | ||
| public bool IsFirefox => isFirefox; | ||
| private readonly string _path; | ||
| private readonly bool _isFirefox; | ||
|
|
||
| public NssDb(string path, bool isFirefox) | ||
| { | ||
| _path = path; | ||
| _isFirefox = isFirefox; | ||
| } | ||
|
|
||
| public string Path => _path; | ||
| public bool IsFirefox => _isFirefox; |
There was a problem hiding this comment.
The primary constructor syntax private sealed class NssDb(string path, bool isFirefox) is a C# 12 feature that was replaced with a traditional constructor. However, the current C# version should support primary constructors. This change removes the more concise primary constructor pattern without clear justification. Consider keeping the original primary constructor syntax unless there's a specific reason to avoid it.
|
|
||
| try | ||
| { | ||
| return string.Equals(Path.GetFullPath(dir), certDirFullPath, StringComparison.OrdinalIgnoreCase); |
There was a problem hiding this comment.
Unix file systems are typically case-sensitive, but StringComparison.OrdinalIgnoreCase is used for path comparison. This could lead to false positives when comparing paths. Consider using StringComparison.Ordinal instead for case-sensitive path comparison on Unix systems.
| return string.Equals(Path.GetFullPath(dir), certDirFullPath, StringComparison.OrdinalIgnoreCase); | |
| return string.Equals(Path.GetFullPath(dir), certDirFullPath, StringComparison.Ordinal); |
Benchmark PR from qodo-benchmark#82