C#: Re-factor DependabotProxy class to allow unit-testing. - #22477
Conversation
131e450 to
d870a57
Compare
michaelnebel
added
the
no-change-note-required
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The refactor preserves platform and runtime behavior while adding appropriate coverage for the extracted logic.
Review tier: Balanced
Findings: None
What changed in this PR
Refactors Dependabot proxy configuration behind an injectable interface, enabling focused unit testing while preserving production behavior.
Changes:
- Adds environment-backed proxy configuration.
- Separates platform gating from testable proxy creation.
- Tests credentials, certificates, and registry URL parsing.
| File | Description |
|---|---|
csharp/extractor/Semmle.Extraction.Tests/DependabotProxy.cs |
Adds proxy unit tests and stubs. |
csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/IDependabotProxyConfiguration.cs |
Defines injectable configuration. |
csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/DependencyManager.cs |
Uses the refactored factory. |
csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/DependabotProxyConfiguration.cs |
Reads configuration from environment variables. |
csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/DependabotProxy.cs |
Extracts configuration and creation logic for testing. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
mbg
left a comment
mbg
left a comment
There was a problem hiding this comment.
This looks like a very useful change, thank you for continuing to work on and improve this!
I had a few minor comments while I was looking over this (out of curiosity). From a functional perspective, this LGTM otherwise.
| return MakeAux(new DependabotProxyConfiguration(), logger, diagnosticsWriter, tempWorkingDirectory); | ||
| } | ||
|
|
||
| internal static IDependabotProxy? MakeAux( |
There was a problem hiding this comment.
Minor: It might be good to give this a more descriptive name or a doc comment explaining why it is separated out of Make.
| { | ||
| if (string.IsNullOrWhiteSpace(proxyConfig.Host) || string.IsNullOrWhiteSpace(proxyConfig.Port)) | ||
| { | ||
| logger.LogInfo("No Dependabot proxy credentials are configured."); |
There was a problem hiding this comment.
Minor: I probably chose this in the original implementation, but this could probably be changed to a Debug-level message, since we log at Info-level when the proxy is configured, so the absence of that message would imply this one under normal circumstances.
| } | ||
|
|
||
| var result = new DependabotProxy(proxyConfig, logger, tempWorkingDirectory); | ||
| logger.LogInfo($"Dependabot proxy configured at {result.Address}"); |
There was a problem hiding this comment.
Minor: Again probably something I was responsible for in the original implementation, but here and elsewhere, it might make sense to change the wording of the log messages to not mention "Dependabot". While that's technically accurate, it is probably confusing for users to see in the log. E.g. "Authentication proxy" or "Registry proxy" might be better.
There was a problem hiding this comment.
Agree. Should we rename the class to RegistryProxy?
There was a problem hiding this comment.
You could do if you wanted, but since the class name is not user-facing it's less significant.
There was a problem hiding this comment.
True, but it may be confusing at a later point in time, if we are not consistent with the (internal) naming and the user facing messaging. Will make a follow up PR with renaming/updating of log messages.
| var config = new DependabotConfigurationStub | ||
| { | ||
| Port = "8080", | ||
| Host = "my.private.server", |
There was a problem hiding this comment.
Minor: As things stand, Host would always be localhost if set. It would not currently ever point to some other address. In theory, it is possible for a user to set the env var manually before invoking CodeQL, but that's not really a use case we support since authentication to private registries can be handled more easily in advanced workflows or direct CLI usage. So for positive tests, it might make sense to use localhost as the value.
|
@mbg : Thank you very much for the comments; I will address them! |
d870a57 to
2dc8ef9
Compare
mbg
left a comment
mbg
left a comment
There was a problem hiding this comment.
Thanks for addressing the feedback. Looks good to me and I'm happy to approve this from my end.
In this PR we re-factor the DependabotProxy class to enable unit-testing (and also add some tests).
This is in preparation for adding support for
replaces-base(the RegistryUrl JSON object will become more "complex").DCA looks good.