-
Notifications
You must be signed in to change notification settings - Fork 2.7k
AzureCLI V3 task with Azure DevOps Service Connection support #21229
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: master
Are you sure you want to change the base?
Conversation
Tasks/AzureCLIV3/azureclitask.ts
Outdated
Utility.throwIfError(tl.execSync("az", `devops configure --defaults project="${project}"`), tl.loc("FailedToSetAzureDevOpsProject")); | ||
} | ||
|
||
} |
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.
Please fix the indentation here for closing curly bracet.
Also I recommend throwing an error here if authScheme.toLowerCase() != "workloadidentityfederation"
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.
Yes, I agree. I’ve added an error for that case
Tasks/AzureCLIV3/azureclitask.ts
Outdated
if (project) { | ||
Utility.throwIfError(tl.execSync("az", `devops configure --defaults project="${project}"`), tl.loc("FailedToSetAzureDevOpsProject")); | ||
} | ||
|
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.
Remove redundant empty line here
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.
Removed, thank you!
Tasks/AzureCLIV3/azureclitask.ts
Outdated
tl.setSecret(federatedToken); | ||
let args = `login --service-principal -u "${servicePrincipalId}" --tenant "${tenantId}" --allow-no-subscriptions --federated-token "${federatedToken}"`; | ||
|
||
if(!visibleAzLogin ){ |
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.
Please add white spaces for If (...) condition - like it's done in in this method below
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.
White spaces added, 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.
Thank you for your PR. Please take a look at my comments. I highly recommend introducing unit tests before we release this task.
const project = tl.getVariable('System.TeamProject'); | ||
|
||
if (organization) { | ||
Utility.throwIfError(tl.execSync("az", `devops configure --defaults organization="${organization}"`), tl.loc("FailedToSetAzureDevOpsOrganization")); |
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.
Should not we clean these settings once the task is completed?
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 it be okay to proceed like this?
az devops configure --defaults organization='' project=''
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.
Sounds good.
Tasks/AzureCLIV3/Tests/L0.ts
Outdated
}); | ||
|
||
it('Does a basic hello world test', function (done: MochaDone) { | ||
// TODO - add real tests |
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.
Don't forget to add some real tests. :)
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.
Thank you, tests have been added!
Tasks/AzureCLIV3/Tests/package.json
Outdated
{ | ||
"name": "azure-cli-tests", | ||
"version": "1.0.0", | ||
"description": "Azure Pipelines Azure CLI V2 Task Tests", |
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.
Here it still says V2. Maybe check the whole folder for unintentional mentions of V2 where V3 needs to be used instead.
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.
I agree, thank you!
Context
The new version of the task for Azure CLI supports Azure DevOps service connections. Regarding the ARM connection type, everything remains the same as in AzureCLI v2. However, when using Azure DevOps service connections, the following occurs:
Task Name
AzureCLIV3
Description
Added new
connectionType
input with options:azureRM
(default)azureDevOps
- New support for Azure DevOps service connectionsMethod
setupAzureDevOpsCLI()
:az extension add -n azure-devops -y
New error messages:
FailedToInstallAzureDevOpsCLI
- "Failed to install Azure DevOps CLI extension",FailedToSetAzureDevOpsOrganization
- "Failed to set Azure DevOps organization",FailedToSetAzureDevOpsProject
- "Failed to set Azure DevOps project"Risk Assessment (Low / Medium / High)
Assess the level of risk and provide reasoning (e.g., scope, impact, backward compatibility).
Change Behind Feature Flag (Yes/No)
There’s no need to add a feature flag. If needed, the user can switch to V2.
Tech Design / Approach
Documentation Changes Required (Yes/No)
Yes
Unit Tests Added or Updated (Yes/No)
No
Additional Testing Performed
The task has been tested within the organization on Ring-1 and in DevFabric
Logging Added/Updated (Yes/No)
Yes
Telemetry Added/Updated (Yes/No)
No
Rollback Scenario and Process (Yes/No)
If a rollback is needed, the user can switch to a different task version
Dependency Impact Assessed and Regression Tested (Yes/No)
No
Checklist