Skip to content
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

Allow authentication using Azure service connections #905

Closed
wants to merge 22 commits into from

Conversation

winstliu
Copy link
Member

@winstliu winstliu commented May 28, 2024

Adds a new V5 version of all the tasks with the following changes:

  • Adds a new service connection option for azurerm that allows using Azure credentials to authenticate to the Marketplace rather than PATs
    • See Common.ts & PublishVSExtension.ts for the changes
  • Updates code generation to output ES2022 modules, giving us top-level await support which helps a lot with the introduction of azure-pipelines-tasks-azure-arm-rest. (Luckily, Node 16 fully supports ES2022.)

I think those are the main changes - there are some small pathing updates, but I tried not to touch many other things.

To use, there's a new AzureRM value for the connectTo option, which when set allows you to use the (also-new) connectedServiceNameAzureRM input.

At this point. I'm pretty sure publishing works, though final validation is blocked as I'm running into microsoft/tfs-cli#362 which I've fixed in microsoft/tfs-cli#458. (The current VSIX size is now ~110MB - quite large!)

Mostly getting this out for feedback at this point.

Closes #506

@winstliu winstliu requested a review from jessehouwing as a code owner May 28, 2024 16:52
tfx.argIf(setServiceUrl, ["--service-url", "https://marketplace.visualstudio.com"]);
tfx.arg(["--auth-type", "pat"]);
tfx.arg(["--token", token]);
tl.setSecret(token);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move up to directly under const token =...

"type": "radio",
"label": "Connect to",
"required": true,
"defaultValue": "AzureRM",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is hairy. Since the YAML will not contain the default value people upgrading will end up with broken tasks.

For backwards compat it's probably better to keep the current default.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm maybe not... Looks like because this is set to required:true current users will have vsteam as their default.

@@ -150,7 +150,7 @@
"ms.vss-distributed-task.tasks"
],
"properties": {
"name": "BuildTasks/PublishVSExtension"
"name": "BuildTasks/PublishVsExtension"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This a concious change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, oops

@jessehouwing
Copy link
Collaborator

I'm considering dropping V3. Technically older versions of TFS support the latest agent so, they should be able to leverage the V4 and v5 branch.

@geekzter do you have any metrics on how many people still rely on these @v3 tasks?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These .snyk files can be deleted, the current lib no longer has this vulnerability.

"type": "module",
"license": "MIT",
"dependencies": {
"azure-pipelines-task-lib": "^4.12.0",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably upgrade to 4.12.1 while we're at it

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be deleted.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though it would require some work, specifying out the files section in more detail may strip enough package-lock.jsons to drop the file size considerably. Especially if we decide to keep V3 for now.

Technically they, nor the package.json and the .ts files are all extra unneeded ballast that I never left out for convenience' sake.

@jessehouwing jessehouwing mentioned this pull request May 28, 2024
@jessehouwing
Copy link
Collaborator

Merged as part of #906.

@jessehouwing
Copy link
Collaborator

There are a few issues publishing at the moment. @MOlausson can you look into the pipeline issues?

@winstliu
Copy link
Member Author

winstliu commented May 28, 2024

I don't have visibility into the failures, but I betcha based on the exit code that you're also hitting "Extension package is malformed/corrupted", in which case...microsoft/tfs-cli#458 😊.

@winstliu winstliu deleted the v5 branch May 28, 2024 21:19
@jessehouwing
Copy link
Collaborator

Nah the PAT token has expired. So I need a MSFT employee to publish in the UI on my behalf. And I need to temporarily test on my own publisher which I haven't had to do in forever.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Now in preview] Add support for Azure AD service Principals
2 participants