-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Added Azure Func Tools Installer task. #11348
Conversation
@@ -0,0 +1,79 @@ | |||
"use strict"; |
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.
Is there any other task using this common module? If not you can keep it within the task itself
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.
Keeping it in Common, as we support installation as a part of main task itself for other tasks (Kubernetes, Helm etc), and if the customers ask for it we will be doing the same for this as well.
cachedToolpath = await toolLib.cacheDir(unzippedFuncPath, funcToolName, version); | ||
console.log(tl.loc("SuccessfullyDownloaded", version, cachedToolpath)); | ||
} | ||
else { |
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.
Use a single styling format. else should go up
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.
done
@@ -0,0 +1,16 @@ | |||
{ | |||
"loc.friendlyName": "Install Azure Func Core Tools", | |||
"loc.helpMarkDown": "[Learn more about this task](https://docs.microsoft.com/azure/devops/pipelines/tasks/tool/func-tools-installer)", |
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.
Use a redirection URL. like aka.ms/func-tools-installer
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.
done
"azure-pipelines-tool-lib": "0.12.0", | ||
"func-tools-common": "file:../../_build/Tasks/Common/func-tools-common-1.0.0.tgz" | ||
} | ||
} |
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.
Add in other meta-data to the package.json?
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.
not doing as discussed.
@@ -0,0 +1,31 @@ | |||
"use strict"; | |||
|
|||
import tl = require('azure-pipelines-task-lib/task'); |
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.
Use a single style here as well.
import * as tl from 'azure-pipelines-task-lib'
You can refer to the task functions by referring to the module as well.
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.
done
const funcToolsPath = await utils.downloadFuncTools(version); | ||
|
||
// prepend the tools path. instructs the agent to prepend for future tasks | ||
if (!process.env['PATH'].startsWith(path.dirname(funcToolsPath))) { |
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.
Instead of startsWith
shouldn't it be contains
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.
No..in the task code we do which
, and we want it to find this version
of the tool. If a tool is present in multiple paths
which
resolves to the first entry. So contains
may not work.
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.
If there are multiple tools being installed in the same pipeline, the PATH
var probably wouldn't have the func tool in the beginning.
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.
Agreed. But this is the best we can do to ensure that the most recent installation (in the pipeline) is available for the tasks that follow. Hypothetical example:
Install func 1.0.0
Deploy function 1
Install func 2.0.0
Deploy function 2
Install func 1.0.0
Deploy function 3
In such a case, with contains
check, Deploy function 3
would actually get func tool
version 2.0.0
(since its prepended in the PATH
after 1.0.0
).
Agree that in above example, if third step was Install kubectl 1.0.0
, prepending PATH in the 5th step would be redundant. Although one point to note is that the user is contributing to this redundancy.
82e7382
to
f60afb5
Compare
|
||
// handle user input scenerios | ||
function sanitizeVersionString(inputVersion: string) : string{ | ||
const version = toolLib.cleanVersion(inputVersion); |
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.
Would you like to add support to versions like v1
or 1.*
or something like that later down the line?
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.
Not planned as of now. I don't see a way we can achieve that.
|
||
export async function getFuncToolsVersion(): Promise<string> { | ||
const version = tl.getInput("version"); | ||
if (version && version != "latest") { |
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.
!==
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.
Done
Tasks/FuncToolsInstallerV0/task.json
Outdated
"instanceNameFormat": "Install func tools - $(version)", | ||
"execution": { | ||
"Node": { | ||
"target": "src//functoolsinstaller.js" |
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.
/
would work
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.
Changed to /
"name": "version", | ||
"label": "Version", | ||
"type": "string", | ||
"helpMarkDown": "Specify the version of Azure func tools to install. Ex:<br><br>2.7.1575<br>v2.7.1575<br>latest", |
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.
Have you tried using \n
instead of <br>
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.
No. <br>
works fine.
|
||
export async function getLatestFuncToolsVersion(): Promise<string> { | ||
const funcToolsLatestReleaseUrl = 'https://api.github.com/repos/Azure/azure-functions-core-tools/releases/latest'; | ||
const stableFuncToolsVersion = '2.7.1585'; |
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.
NIT: this can be moved outside the func
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.
Done
* Added Azure Func Tools Installer task. * Use toolLib cache to cache the installation. * remove unnecessary dependencies * Addressing PR review comments * PR review changes
No description provided.