Bicep deploy - fix bicep login and filename with space issue #6283
Conversation
bhsubra
commented
Mar 24, 2022
•
bhsubra
commented
- Fix login issue. This is a workaround for https://github.com/Azure/azure-sdk-for-net/issues/27263. Instead of relying on the sdk to get the credentials, we will now get the token from subscription and pass it on to language server layer and use it in creating ArmClient. We will also be masking the credentials information in trace logs.
- Fix filename with space issue
| @@ -74,7 +72,9 @@ export class DeployCommand implements Command { | |||
| } | |||
|
|
|||
| const documentPath = documentUri.fsPath; | |||
| const textDocument = TextDocumentIdentifier.create(documentUri.fsPath); | |||
| const textDocument = TextDocumentIdentifier.create( | |||
| encodeURIComponent(documentUri.path) | |||
There was a problem hiding this comment.
This handles scenario when there are spaces in folder name.
There was a problem hiding this comment.
Good idea to add comment (spaces or other special characters)
bhsubra
changed the title
bhsubra
changed the title
bhsubra
force-pushed
the
dev/bhsubra/FixLoginIssueDuringDeploy
branch
from
93ed93e to
1b5962d
Compare
| 'Params: {\n "textDocument": {\n "uri": "someUri"\n }, "token": "eyJ0eXAi",\n "expiresOnTimestamp": "1648143343698"\n}'; | ||
| const actual = removePropertiesWithPossibleUserInfoInDeployParams(value); | ||
| const expected = | ||
| 'Params: {\n "textDocument": {\n "uri": "someUri"\n }, "token": "<REDACTED: token>",\n "expiresOnTimestamp": "<REDACTED: expiresOnTimestamp>"\n}'; |
There was a problem hiding this comment.
nit: do we really need to redact timestamp?
|
|
||
| public appendLine(value: string): void { | ||
| const updatedValue = | ||
| removePropertiesWithPossibleUserInfoInDeployParams(value); |
There was a problem hiding this comment.
nit: Not thrilled about this implementation, but it's good enough for now. And at least it's clear what it's doing.
| { | ||
| public class CredentialFromTokenAndTimeStamp : TokenCredential | ||
| { | ||
| private AccessToken _accessToken; |
There was a problem hiding this comment.
[nit]: The convention for our repo is to not use the _ prefix in field names.
| export function removePropertiesWithPossibleUserInfoInDeployParams( | ||
| value: string | ||
| ): string { | ||
| const deployParamsPattern = new RegExp('.*"token":\\s*"(?<token>.*)"'); |
There was a problem hiding this comment.
Use /.*"token":\\s*"(?<token>.*)"/ instead of the RegExp constructor so V8 can cache it. An alternative way is to make the regex a file level constant.
There was a problem hiding this comment.
Yep, using a regex literal is better.
| import { AccessToken } from "@azure/identity"; | ||
| import { AzLoginTreeItem } from "../tree/AzLoginTreeItem"; | ||
| import { AzManagementGroupTreeItem } from "../tree/AzManagementGroupTreeItem"; | ||
| import { AzResourceGroupTreeItem } from "../tree/AzResourceGroupTreeItem"; |
There was a problem hiding this comment.
[nit]: Consider creating a barrel index.ts under the tree folder to simplify the imports (can be done in a seperate PR though).
There was a problem hiding this comment.
@shenglol Frankly I'm not convinced this is worth the effort. What advantage does it give?
There was a problem hiding this comment.
It's a way to simplify the imports, so instead of doing
import { X } from "foo/x";
import { Y } from "foo/y;
import { Z } from "foo/z;
we can just do
import { X, Y, Z } from "foo";
I don't have a strong opinion on this but thought this can save a few of lines.
