Add AzureFunctionV2 #20933
Add AzureFunctionV2 #20933
Conversation
|
/azp run |
|
/azp run |
| "label": "Query parameters", | ||
| "defaultValue": "", | ||
| "required": false, | ||
| "helpMarkDown": "Query parameters string to append to the function URL. It should not start with with \"?\" nor \"&\"." |
There was a problem hiding this comment.
nit; There is double negation in this sentence. I believe it should be "It should start neither with ? nor &." or "It should not start either with ? or &.". Also note duplicate "with".
|
|
||
| // Add query parameters if present | ||
| if (this.inputs.queryParameters) { | ||
| url += this.inputs.queryParameters; |
There was a problem hiding this comment.
The description of query param input in task.json says that input should not start with &. However, if you just concatenate it here it will not produce valid url. E.g.:
https://function.azure.com?code=xxxxxxqueryparam1=value&queryparam2=value
I would not rely on the fact that users wont put & or & as first character in query parameter input. Lets trim it or maybe even parse it and use only valid parts.
| tl.debug(`Building request with ARM authentication using service connection: ${this.inputs.serviceConnection}`); | ||
|
|
||
| try { | ||
| const request = this.buildKeyAuthRequest(); |
There was a problem hiding this comment.
This will produce url with code query param which is not valid in this scenario as auth is done via ARM endpoint. Please extract code that is common for both buildKeyAuthRequest and buildArmAuthRequest into separate method.
| private getDefaultHeaders(): Record<string, string> { | ||
| return { | ||
| 'Content-Type': 'application/json', | ||
| PlanUrl: tl.getVariable('system.CollectionUri') || '', |
There was a problem hiding this comment.
Is this something that was even in previous version? Why are we including these values? PlanId, JobId etc. are internal ADO details. They should be irrelevant for request that is targeting Azure.
| // Safe evaluation with a restricted context | ||
| try { | ||
| // Use with statement to provide the context to the expression | ||
| const result = eval(`with (context) { ${expression} }`); |
There was a problem hiding this comment.
Could you please explain what is expected format of the success criteria input? As far as I can see eval does not really work with expressions described in input's description. Apart of that using eval should be a big no no.
| * Checks if the callback has been received | ||
| * @returns The callback result, or null if not received yet | ||
| */ | ||
| private async checkCallbackStatus(): Promise<CallbackResult | null> { |
There was a problem hiding this comment.
Why is this async? I do not see any await inside the function.
|
One general comment. The PR description claims "it's impossible to add additional logic, which ARM service connection requires". What exactly is not possible to achieve in agent less task? For example |
|
Task name: AzureFunctionV2
Description: New version of AzureFunction task with support of ARM Service Connection authentication.
The previous version of the task was running on server and it's impossible to add additional logic, which ARM service connection requires. So, a completelly new version of the task was created to support ARM service connection authentication.
Risk Assesment(Low/Medium/High): Low
Tests Performed: Added e2e tests
Documentation changes required: Y
Attached related issue: AB#2221642
Checklist: