-
Notifications
You must be signed in to change notification settings - Fork 76
Remove Dev Proxy CA certificate when uninstalling for Win #1208
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
base: main
Are you sure you want to change the base?
Remove Dev Proxy CA certificate when uninstalling for Win #1208
Conversation
Cool! We'll check it out asap. Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of things that I noticed. I'll try to run it asap to see it in action. I like the idea!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds an automatic step to remove the Dev Proxy CA certificate when uninstalling on Windows and exposes the same functionality via the CLI.
- Introduces a
DevProxyExecutable
macro and an[UninstallRun]
entry in bothinstall.iss
andinstall-beta.iss
to invokedevproxy.exe cert remove
during uninstall. - Registers a new
remove
subcommand in the CLI (CreateCertRemoveCommand
) and implements its handler (CertRemoveCommandHandler.RemoveCert
).
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
File | Description |
---|---|
install.iss | Defines DevProxyExecutable and adds [UninstallRun] to remove the cert on uninstall |
install-beta.iss | Same changes as install.iss , targeting the beta executable |
dev-proxy/ProxyHost.cs | Adds CreateCertRemoveCommand to the list of certificate commands |
dev-proxy/CommandHandlers/CertRemoveCommandHandler.cs | Implements the logic to remove the trusted root certificate |
Comments suppressed due to low confidence (3)
install.iss:50
- [nitpick] The RunOnceId "RemoveCert" is quite generic and may collide with other actions; consider namespacing it (e.g. "MyApp_RemoveCert") to avoid potential conflicts.
Filename: "{app}\{#DevProxyExecutable}"; Parameters: "cert remove"; RunOnceId: "RemoveCert"; Flags: runhidden;
dev-proxy/CommandHandlers/CertRemoveCommandHandler.cs:9
- The new
RemoveCert
handler isn’t covered by any unit tests. Consider adding tests to assert successful removal and proper error logging in failure scenarios.
public static void RemoveCert(ILogger logger)
dev-proxy/CommandHandlers/CertRemoveCommandHandler.cs:5
- This file references ILogger but doesn’t include the corresponding
using
directive. Addusing Microsoft.Extensions.Logging;
(or the appropriate namespace) at the top to resolve the type.
namespace DevProxy.CommandHandlers;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works nicely! Let's do a few small fixes before we merge it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice and clean implementation for the prompt. Let's add the --force
switch to the installer invocation so that it executes silently and we'll get it in
@waldekmastykarz The single key is faster, but Key+Enter method sort of safer, gives a better user control and seems to provide adherence to common CLI conventions. So I'd stick with key+enter. What do you think? PS I am working on MacOS related part of the issue. |
Good point, thank you for bringing it up. Let's do y/n followed by ENTER. I'll create a separate issue for this. |
#847