Add insecure option, deprecate allowSelfSigned #101
Conversation
agateau-gg
force-pushed
the
agateau/insecure
branch
from
675a73d to
703f104
Compare
There was a problem hiding this comment.
LGTM, just a couple comments
Also, adding @GabrielCousin as reviewer as he's a typescript expert
| ): Promise<GGShieldConfiguration> { | ||
| const config = workspace.getConfiguration("gitguardian"); | ||
|
|
||
| const ggshieldPath: string | undefined = config.get("GGShieldPath"); |
There was a problem hiding this comment.
I think that's worth keeping for local debugging
| "type": "boolean", | ||
| "default": false, | ||
| "markdownDescription": "Allow Self Signed Certificates" | ||
| "markdownDescription": "Allow Self Signed Certificates", |
There was a problem hiding this comment.
is the option still showing on the extension settings? Can you share a screenshot?
There was a problem hiding this comment.
Since this is not a title, I'd suggest Allow self-signed certificates without capitalization.
| const insecure: boolean = config.get( | ||
| "insecure", | ||
| // Read allowSelfSigned for backward compatibility | ||
| config.get("allowSelfSigned", false), |
There was a problem hiding this comment.
Just wondering if it could make sense to use a notification if the deprecated setting is still present? https://code.visualstudio.com/api/ux-guidelines/notifications
On one hand, it's seemless for the user but on the other hand, you can't know about the deprecation very easily, right?
| }); | ||
|
|
||
| const testCasesAllowSelfSigned = [ | ||
| const testCasesInsecure = [ |
There was a problem hiding this comment.
Maybe consider temporarily keeping a test that asserts the --insecure flag is forced if providing allowSelfSigned?
| "type": "string", | ||
| "default": "", | ||
| "markdownDescription": "You can override the value here for On Premise installations" | ||
| "markdownDescription": "You can override the value here for On Premise installations", |
| "type": "boolean", | ||
| "default": false, | ||
| "markdownDescription": "Allow Self Signed Certificates" | ||
| "markdownDescription": "Allow Self Signed Certificates", |
There was a problem hiding this comment.
Since this is not a title, I'd suggest Allow self-signed certificates without capitalization.
Context
ggshield 1.44 introduced the
--insecureoption to replace the ambiguous--allow-self-signed. This PR makes similar changes to the VSCode extension.What has been done
insecure.allowSelfSignedsetting as deprecated.--insecureinstead of--allow-self-signed.PR check list