Skip to content

Conversation

motm32
Copy link
Contributor

@motm32 motm32 commented Dec 20, 2023

Contributes to #3523

Currently the entry point is next to the current "Create New Project..." command in the workspace view:
image

@motm32 motm32 requested a review from a team as a code owner December 20, 2023 17:12
import { type IProjectWizardContext } from "../IProjectWizardContext";

export interface IDockerfileProjectContext extends IActionContext, ExecuteActivityContext, IProjectWizardContext {
projectLanguage?: string;
Copy link
Member

Choose a reason for hiding this comment

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

IProjectWizardContext already should have a language: ProjectLanguage property that you should be able to leverage.

import { ProjectLanguage } from "../../../constants";
import { type IDockerfileProjectContext } from "./IDockerfileProjectContext";

export class DockerfileProjectLanguageStep extends AzureWizardPromptStep<IDockerfileProjectContext> {
Copy link
Member

Choose a reason for hiding this comment

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

You could probably just use the NewProjectLanguageStep and add some filtering in the case for DockerfileProjectLanguageStep instead of creating a new file that does largely the same thing.

Copy link
Contributor Author

@motm32 motm32 Dec 29, 2023

Choose a reason for hiding this comment

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

I did think about that but the data property in the NewProjectLanguageStep does not match up with the data property I need. I can utilize language: ProjectLanguage property that you commented about above but will probably still need the new step since the data is different.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually update since I am not using the NewProjectLanguageStep I don't think I can use the language: ProjectLanguage since the data types don't match up.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah but you should be able to just change the type instead of using a string.

Copy link
Member

Choose a reason for hiding this comment

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

Looking at this more, I think we should just speak offline about this.

package.nls.json Outdated
@@ -18,6 +18,7 @@
"azureFunctions.createFunctionAppAdvanced": "Create Function App in Azure... (Advanced)",
"azureFunctions.createFunctionAppDetail": "For serverless, event driven apps and automation.",
"azureFunctions.createNewProject": "Create New Project...",
"azureFunctions.createNewProjectWithDockerfile": "Create New Project With Dockerfile...",
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should call it Create New Containerized Project...?

I don't have a strong preference, but the learn document does refer to it as a containerized function app.

https://learn.microsoft.com/en-us/azure/azure-functions/functions-deploy-container?tabs=acr%2Cbash%2Cazure-cli&pivots=programming-language-javascript

@@ -29,6 +29,8 @@ export interface IProjectWizardContext extends IActionContext {
openApiSpecificationFile?: Uri[];

targetFramework?: string | string[];

dockerfile?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

Property name is a bit odd for a boolean. If I see dockerfile, I think it's the dockerfile path or Uri. I would have called it addDockerfile or something like that.

But that being said, I don't think you need this property anymore. I'm assuming it used to be used to determine whether or not to add the CreateDockerfileProjectStep but we've covered that in another way.

public priority: number = 100;

public async execute(context: IFunctionWizardContext): Promise<void> {
const message: string = localize('installFuncTools', 'You must have the Azure Functions Core Tools installed to run this command.');
Copy link
Member

Choose a reason for hiding this comment

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

I mentioned this in another PR, but you should front-load this error. Nothing more annoying than going through a project wizard and then having it fail right at the end.

Copy link
Member

@nturinski nturinski left a comment

Choose a reason for hiding this comment

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

It's a just a nit, so if you disagree, feel free to just merge! Looks good!

@@ -54,6 +56,13 @@ export async function createNewProjectInternal(context: IActionContext, options:
const wizardContext: Partial<IFunctionWizardContext> & IActionContext = Object.assign(context, options, { language, version: tryParseFuncVersion(version), projectTemplateKey });
const optionalExecuteStep = options.executeStep;

if (optionalExecuteStep instanceof CreateDockerfileProjectStep) {
const message: string = localize('installFuncTools', 'You must have the Azure Functions Core Tools installed to run this command.');
Copy link
Member

Choose a reason for hiding this comment

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

nit: Since we know what command they are running, I think we should tell them specifically that they need to have the core tools installed to create a containerized function project or whatever.

@motm32 motm32 merged commit 040dbb9 into main Feb 16, 2024
@motm32 motm32 deleted the meganmott/dockerfileProject branch February 16, 2024 21:51
@microsoft microsoft locked and limited conversation to collaborators Apr 2, 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.

3 participants