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

Adding support for Logic Apps extension. #907

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

alain-zhiyanov
Copy link

@alain-zhiyanov alain-zhiyanov commented Jul 16, 2024

'Create Static Web App' command now supports being called from Logic Apps VSCode extension. The command will now initialize and deploy a Static Web App which is in the same directory and a linked backend for the user's Logic App.

Please see more details in my Offboarding document .

@mkarmark @annikel @joslinmicrosoft

@alain-zhiyanov alain-zhiyanov requested a review from a team as a code owner July 16, 2024 21:42
// logic app
logicApp?: {backendResourceId: string, region: string, name: string};


Copy link

Choose a reason for hiding this comment

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

nit: remove extra lines

@@ -46,4 +46,9 @@ export interface IStaticWebAppWizardContext extends IResourceGroupWizardContext,

// created when the wizard is done executing
staticWebApp?: StaticSiteARMResource;

// logic app
Copy link

Choose a reason for hiding this comment

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

nit: the comment does not add any value here

@@ -3,19 +3,53 @@
* Licensed under the MIT License. See License.txt in the project root for license information.
*--------------------------------------------------------------------------------------------*/

import { StaticSiteARMResource } from "@azure/arm-appservice";
import { WebSiteManagementClient, type StaticSiteARMResource } from "@azure/arm-appservice";
Copy link

Choose a reason for hiding this comment

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

validate that all these imports are still being used


export class StaticWebAppCreateStep extends AzureWizardExecuteStep<IStaticWebAppWizardContext> {
public priority: number = 250;
public priority = 250;
Copy link

Choose a reason for hiding this comment

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

nit: remove extra line


public async execute(context: IStaticWebAppWizardContext, progress: Progress<{ message?: string | undefined; increment?: number | undefined }>): Promise<void> {

if(context.logicApp) {
//There was an issue where some fields of context would be lost when SWA called with LA. This is a temporary fix to find the branch data again here because of that. Note this will only work with alain github.
Copy link

Choose a reason for hiding this comment

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

Does this have to be fixed still?

const octokitClient: Octokit = await createOctokitClient(context);


const owner = "alain-zhiyanov";
Copy link

Choose a reason for hiding this comment

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

can you extract this to the top of the file and call it template owner





Copy link

Choose a reason for hiding this comment

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

nit: remove lines

export async function createStaticWebApp(context: IActionContext & Partial<ICreateChildImplContext> & Partial<IStaticWebAppWizardContext>, node?: SubscriptionTreeItemBase, _nodes?: SubscriptionTreeItemBase[], ...args: unknown[]): Promise<AppResource> {

//logic app paramater was passed in args so flow which inits SWA with LA is called
if (args.length > 0) {
Copy link

Choose a reason for hiding this comment

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

instead of comment here extract evaluation into var and add proper naming

//logic app paramater was passed in args so flow which inits SWA with LA is called
if (args.length > 0) {
type Resource = {backendResourceId: string, region: string, name: string};
const logicApp = args[0] as unknown as Resource;
Copy link

Choose a reason for hiding this comment

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

is the as unknown needed here?

@@ -113,27 +125,22 @@ export async function createStaticWebApp(context: IActionContext & Partial<ICrea
...node.subscription,
...(await createActivityContext())
};
Copy link

Choose a reason for hiding this comment

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

you removed a bunch of files in this file. Please make sure to revert any changes that are not relevant to your work

executeSteps.push(new VerifyProvidersStep([webProvider]));
executeSteps.push(new StaticWebAppCreateStep());
const wizard: AzureWizard<IStaticWebAppWizardContext> = new AzureWizard(wizardContext, {
title,
Copy link

Choose a reason for hiding this comment

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

can we extract certain parts into functions to make the code more readable please

Copy link

Choose a reason for hiding this comment

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

Or section off the code into different blocks

const wizard: AzureWizard<IStaticWebAppWizardContext> = new AzureWizard(wizardContext, {
title,
promptSteps,
executeSteps,
Copy link

Choose a reason for hiding this comment

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

please remove all extra lines and not needed comments

@@ -51,7 +50,7 @@ export function registerCommands(): void {
registerCommandWithTreeNodeUnwrapping('staticWebApps.toggleAppSettingVisibility', async (context: IActionContext, node?: AppSettingTreeItem) => { await nonNullValue(node).toggleValueVisibility(context); }, 250);
registerCommand('staticWebApps.showDocumentation', async (_context: IActionContext) => { await openUrl('https://aka.ms/AA92xai'); });
registerCommand('staticWebApps.showFunctionsDocumentation', async (_context: IActionContext) => { await openUrl('https://aka.ms/AAacf3z'); });
registerCommandWithTreeNodeUnwrapping('staticWebApps.openYAMLConfigFile', openYAMLConfigFile);
//registerCommandWithTreeNodeUnwrapping('staticWebApps.openYAMLConfigFile', openYAMLConfigFile);
Copy link

Choose a reason for hiding this comment

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

revert?

@@ -494,8 +494,9 @@
"webpack-cli": "^4.6.0"
},
"dependencies": {
"@azure/arm-appservice": "^11.0.0",
"@azure/arm-appservice": "^15.0.0",
Copy link

Choose a reason for hiding this comment

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

Did you update this on purpose?

}

public shouldExecute(_wizardContext: IStaticWebAppWizardContext): boolean {
return true;
}

Copy link

Choose a reason for hiding this comment

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

nit: remove extra line

const client = new WebSiteManagementClient(credential, context.subscriptionId);

try{
const result = await client.staticSites.beginLinkBackendAndWait(
Copy link

Choose a reason for hiding this comment

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

nit: the formatting here is off

const credential = new DefaultAzureCredential();
const client = new WebSiteManagementClient(credential, context.subscriptionId);

try{
Copy link

Choose a reason for hiding this comment

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

nit: there should also be a space inbetween the try and the parentheses

const creatingSwa: string = localize('creatingSwa', 'Creating new static web app "{0}"...', newName);
progress.report({ message: creatingSwa });
ext.outputChannel.appendLog(creatingSwa);
context.staticWebApp = await context.client.staticSites.beginCreateOrUpdateStaticSiteAndWait(nonNullValueAndProp(context.resourceGroup, 'name'), newName, siteEnvelope);
context.activityResult = context.staticWebApp as AppResource;

if (context.logicApp) {
//link backends only if SWA called with LA
Copy link

Choose a reason for hiding this comment

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

instead of the comment can we extract the content.logicApp into a varthat is named properly so that thise comment is not needed anymore

@@ -3,19 +3,26 @@
* Licensed under the MIT License. See License.txt in the project root for license information.
*--------------------------------------------------------------------------------------------*/

import { StaticSiteARMResource, WebSiteManagementClient } from '@azure/arm-appservice';
// biome-ignore lint/style/useNodejsImportProtocol: <explanation>
Copy link

Choose a reason for hiding this comment

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

Are these comments needed?

import { Utils } from 'vscode-uri';
import { StaticWebAppResolver } from '../../StaticWebAppResolver';
import { DetectorResults, NodeDetector } from '../../detectors/node/NodeDetector';
import { NodeDetector, type DetectorResults } from '../../detectors/node/NodeDetector';
Copy link

Choose a reason for hiding this comment

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

Is these line change on purpose? Try to only make changes that are related to your projects

import { NodeConstants } from '../../detectors/node/nodeConstants';
import { VerifyingWorkspaceError } from '../../errors';
import { ext } from '../../extensionVariables';
import { createActivityContext } from '../../utils/activityUtils';
import { createWebSiteClient } from '../../utils/azureClients';
import { cpUtils } from '../../utils/cpUtils';
Copy link

Choose a reason for hiding this comment

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

What are we using this for?

import { showSwaCreated } from '../showSwaCreated';
import { ApiLocationStep } from './ApiLocationStep';
import { AppLocationStep } from './AppLocationStep';
import { BuildPresetListStep } from './BuildPresetListStep';
import { GitHubOrgListStep } from './GitHubOrgListStep';
import { IStaticWebAppWizardContext } from './IStaticWebAppWizardContext';
import type { IStaticWebAppWizardContext } from './IStaticWebAppWizardContext';
Copy link

Choose a reason for hiding this comment

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

why did we have to add this here?

import { OutputLocationStep } from './OutputLocationStep';
import { SkuListStep } from './SkuListStep';
import { StaticWebAppCreateStep } from './StaticWebAppCreateStep';
import { StaticWebAppNameStep } from './StaticWebAppNameStep';
import { setGitWorkspaceContexts } from './setWorkspaceContexts';
import { tryGetApiLocations } from './tryGetApiLocations';


Copy link

Choose a reason for hiding this comment

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

nit: remove extra line

await window.withProgress(progressOptions, async () => {
const folder = await tryGetWorkspaceFolder(context);
if (folder) {
await telemetryUtils.runWithDurationTelemetry(context, 'tryGetFrameworks', async () => {
const detector = new NodeDetector();

const detectorResult = await detector.detect(folder.uri);
// comma separated list of all frameworks detected in this project
context.telemetry.properties.detectedFrameworks = `(${detectorResult?.frameworks.map(fi => fi.framework).join('), (')})` ?? 'N/A';
context.telemetry.properties.rootHasSrcFolder = (await AzExtFsExtra.pathExists(Uri.joinPath(folder.uri, NodeConstants.srcFolderName))).toString();
Copy link

Choose a reason for hiding this comment

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

you can see here on the left that you had removed some lines. please add them back

const executeSteps: AzureWizardExecuteStep<IStaticWebAppWizardContext>[] = [];
if (!context.advancedCreation) {
//[1] gets standard and not free
wizardContext.sku = SkuListStep.getSkus()[1];
Copy link

Choose a reason for hiding this comment

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

instead of the comment can we extract this to a var called standardSku and then set it

wizardContext.uri = wizardContext.uri || getSingleRootFsPath();
await wizard.prompt();

//create github template
Copy link

Choose a reason for hiding this comment

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

If the comment is needed here should this code maybe be extracted into a separate method

const clonePathUri: Uri = Uri.file(clonePath);
const octokitClient: Octokit = await createOctokitClient(context);
const { data: response } = await octokitClient.rest.repos.createUsingTemplate({
private: true,
Copy link

Choose a reason for hiding this comment

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

Can we have all these template details as consts extracted

@@ -176,11 +347,12 @@ export async function createStaticWebApp(context: IActionContext & Partial<ICrea
wizardContext.newResourceGroupName = await wizardContext.relatedNameTask;
}


Copy link

Choose a reason for hiding this comment

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

nit: remove line





Copy link

Choose a reason for hiding this comment

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

nit: remove all lines

@@ -25,7 +25,7 @@ import { openGitHubLog } from './github/jobLogs/openGitHubLog';
import { openGitHubRepo } from './github/openGitHubRepo';
import { showActions } from './github/showActions';
import { openInPortal } from './openInPortal';
import { openYAMLConfigFile } from './openYAMLConfigFile';
import { openYAMLConfigFile } from "./openYAMLConfigFile";
Copy link

Choose a reason for hiding this comment

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

nit: revert this change

const baseClonePath = join(homeDir, folderName);
const clonePath = join(baseClonePath, 'static-web-app');
const clonePathUri: Uri = Uri.file(clonePath);
const octokitClient: Octokit = await createOctokitClient(context);
Copy link

Choose a reason for hiding this comment

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

This code might be hard to interpret for someone not having the full context. Do you think maybe breaking it into smaller pieces can help with readability.
Something like

    const folderName = getFolderName(wizardContext);
    const clonePath = getClonePath(folderName);
    const clonePathUri: Uri = Uri.file(clonePath);
    const octokitClient: Octokit = await createOctokitClient(context);
    const response = await createRepository(octokitClient, folderName);

Copy link

Choose a reason for hiding this comment

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

And same below

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.

2 participants