Skip to content

Add an API entry-point to the deployWorkspaceProject command#578

Merged
MicroFish91 merged 52 commits intomainfrom
mwf/deploy-workspace-project-api
Jan 24, 2024
Merged

Add an API entry-point to the deployWorkspaceProject command#578
MicroFish91 merged 52 commits intomainfrom
mwf/deploy-workspace-project-api

Conversation

@MicroFish91
Copy link
Copy Markdown
Contributor

@MicroFish91 MicroFish91 commented Dec 12, 2023

Closes #571

Will be leveraged by the Azure Functions extension.

Comment thread src/commands/deployWorkspaceProject/DeployWorkspaceProjectContext.ts Outdated
@MicroFish91 MicroFish91 force-pushed the mwf/deploy-workspace-project-api branch from e4c4296 to 3617d8e Compare December 12, 2023 18:41
@MicroFish91 MicroFish91 force-pushed the mwf/deploy-workspace-project-api branch from 21b1fd8 to 5aaa76e Compare December 13, 2023 17:19
@MicroFish91 MicroFish91 marked this pull request as ready for review January 2, 2024 17:29
@MicroFish91 MicroFish91 requested a review from a team as a code owner January 2, 2024 17:29
@MicroFish91 MicroFish91 force-pushed the mwf/deploy-workspace-project-api branch from a4cf584 to 7d2d52f Compare January 8, 2024 21:58
@MicroFish91
Copy link
Copy Markdown
Contributor Author

MicroFish91 commented Jan 8, 2024

Hey all, this should be ready for review again!

@nturinski Thanks for all the great feedback, I was able to take a step back and change up the implementation quite a bit in the spirit of your review.

I took your recommendation to decouple invokedFromApi from the old deployWorkspaceProject and went a step further. I actually ended up just completely removing that boolean and logic coupling altogether. Now instead of the api calling directly into deployWorkspaceProject... deployWorkspaceProjectApi calls directly into deployWorkspaceProjectInternal. This way deployWorkspaceProject and deployWorkspaceProjectInternal now have no concept of the API, and now you only need to look at the api function to completely understand how its parameters influence the overall behavior of the internal implementation.

For removing api references from the internal implementation - I exposed any related surfaces via toggle-able internal UI options. I was even able to consolidate the default context gathering logic into the internal implementation which I think really cleaned everything up quite a bit by allowing the API function to not have to call into deployWorkspaceProject as an outer command.

I also consolidated all output logs into the internal implementation to ensure uniform behavior regardless of entry-point. I took your recommendation and removed the references to API as well.

PR Breakdown

Added precursor PR covering the decoupling for the new internal implementation to reduce the amount of code to review in one sitting:
#585

I also have these two open PRs which will reduce the length of this PR by a few lines once merged:
#584
#583

Here's the setup PR in Functions to consume this API:
microsoft/vscode-azurefunctions#3934

// Let each client decide how it wants to show its own activity log updates.
suppressActivity: true,
suppressConfirmation,
suppressContainerAppCreation,
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Let me know if you think it would be a good idea to default some of these values

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does this comment still make sense? A lot of this seems defaulted already.

@MicroFish91 MicroFish91 changed the base branch from main to mwf/dwp-internal January 9, 2024 00:02
@MicroFish91 MicroFish91 force-pushed the mwf/deploy-workspace-project-api branch from 53a8267 to 17dd935 Compare January 9, 2024 01:44
Base automatically changed from mwf/dwp-internal to main January 12, 2024 17:42
Comment thread src/commands/api/CHANGELOG.md Outdated
// Let each client decide how it wants to show its own activity log updates.
suppressActivity: true,
suppressConfirmation,
suppressContainerAppCreation,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does this comment still make sense? A lot of this seems defaulted already.

return createApiProvider([<api.AzureContainerAppsExtensionApi>{
apiVersion: '0.0.1',

deployWorkspaceProject: async (options: api.DeployWorkspaceProjectOptionsContract) => await callWithTelemetryAndErrorHandling('containerApps.api.deployWorkspaceProject', async (context: IActionContext) => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: I prefer that we keep the apiProvider looking clean like so:

deployWorkspaceProject: deployWorkspaceProjectApi

And then in deployWorkspaceProjectApi, wrap that whole command with callWithTelemetryAndErrorHandling

Comment thread src/utils/azdUtils.ts
Comment thread src/commands/api/deployWorkspaceProjectApi.ts Outdated
@MicroFish91 MicroFish91 force-pushed the mwf/deploy-workspace-project-api branch from 88d846d to e0d9e70 Compare January 19, 2024 18:32
@MicroFish91 MicroFish91 merged commit a58a82c into main Jan 24, 2024
@MicroFish91 MicroFish91 deleted the mwf/deploy-workspace-project-api branch January 24, 2024 19:33
@microsoft microsoft locked and limited conversation to collaborators Mar 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Create an API for the deployWorkspaceProject command

3 participants