Skip to content

Add support for creating functions projects with dockerfiles #3929

Merged
motm32 merged 5 commits intomainfrom
meganmott/dockerfileProject
Feb 16, 2024
Merged

Add support for creating functions projects with dockerfiles #3929
motm32 merged 5 commits intomainfrom
meganmott/dockerfileProject

Conversation

@motm32
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

Comment thread src/commands/createNewProject/dockerfileSteps/DockefileProjectLanguageStep.ts Outdated
Comment thread package.nls.json Outdated
"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
Copy Markdown
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


targetFramework?: string | string[];

dockerfile?: boolean;
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.

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
Copy Markdown
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
Copy Markdown
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!

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
Copy Markdown
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