-
Notifications
You must be signed in to change notification settings - Fork 458
Refactor func pack
#4600
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
base: main
Are you sure you want to change the base?
Refactor func pack
#4600
Conversation
src/Cli/func/Actions/LocalActions/PackAction/DotnetPackSubcommandAction.cs
Outdated
Show resolved
Hide resolved
src/Cli/func/Actions/LocalActions/PackAction/DotnetPackSubcommandAction.cs
Outdated
Show resolved
Hide resolved
src/Cli/func/Actions/LocalActions/PackAction/DotnetPackSubcommandAction.cs
Outdated
Show resolved
Hide resolved
src/Cli/func/Actions/LocalActions/PackAction/DotnetPackSubcommandAction.cs
Outdated
Show resolved
Hide resolved
src/Cli/func/Actions/LocalActions/PackAction/DotnetPackSubcommandAction.cs
Outdated
Show resolved
Hide resolved
src/Cli/func/Actions/LocalActions/PackAction/DotnetPackSubcommandAction.cs
Outdated
Show resolved
Hide resolved
7aa5ae6
to
0a7f737
Compare
src/Cli/func/Actions/LocalActions/PackAction/DotnetPackSubcommandAction.cs
Show resolved
Hide resolved
src/Cli/func/Actions/LocalActions/PackAction/DotnetPackSubcommandAction.cs
Outdated
Show resolved
Hide resolved
src/Cli/func/Actions/LocalActions/PackAction/DotnetPackSubcommandAction.cs
Show resolved
Hide resolved
src/Cli/func/Actions/LocalActions/PackAction/NodePackSubcommandAction.cs
Outdated
Show resolved
Hide resolved
src/Cli/func/Actions/LocalActions/PackAction/NodePackSubcommandAction.cs
Show resolved
Hide resolved
src/Cli/func/Actions/LocalActions/PackAction/NodePackSubcommandAction.cs
Show resolved
Hide resolved
src/Cli/func/Actions/LocalActions/PackAction/PythonPackSubcommandAction.cs
Show resolved
Hide resolved
src/Cli/func/Actions/LocalActions/PackAction/PythonPackSubcommandAction.cs
Show resolved
Hide resolved
f0aba02
to
6b7ed31
Compare
0761d80
to
377294c
Compare
@@ -1,10 +1,9 @@ | |||
# Azure Functions CLI 4.2.2 | |||
# Azure Functions CLI 4.2.3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think new feature = minor
{ | ||
if (refreshSecrets) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does refresh secrets have to do with func pack? Does this need to be in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So in the secret manager, the appsettings are lazy loaded. This works for most scenarios, but it doesn't work for func pack
if the user inputs a directory that they are trying to pack, since the worker runtime is determined by this Init action which is called in ConsoleApp. We could switch directories directly before running this action, but the problem is that we still need to store what the value of the current directory is (in case the user doesn't have an ouput directory where they want the zip file to be located). I also don't think it made sense to have a hardcoded scenario for only if it's func pack, switch directories. Does that make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that we eventually want to allow all commands to have func <command> <folder path>
but I thought this was a bit out of the scope of the PR to modify the Init action to account for this for all of the commands (since we only want this to happen for func pack
), so I decided to have this as a workaround.
"When enabled, Core Tools starts a Docker container, builds the app inside that container," + | ||
" and creates a ZIP file with all dependencies restored in .python_packages.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: indent desc strings
"When enabled, Core Tools starts a Docker container, builds the app inside that container," + | |
" and creates a ZIP file with all dependencies restored in .python_packages.") | |
"When enabled, Core Tools starts a Docker container, builds the app inside that container," + | |
" and creates a ZIP file with all dependencies restored in .python_packages.") |
|
||
namespace Azure.Functions.Cli.Actions.LocalActions.PackAction | ||
{ | ||
[Action(Name = "pack powershell", ParentCommandName = "pack", ShowInHelp = false, HelpText = "Arguments specific to PowerShell apps when running func pack")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this false because there are no custom pwsh args right now? Can we add a comment that mentions that
Parser | ||
.Setup<bool>("no-build") | ||
.WithDescription("Do not build the project before packaging. Optionally provide a directory when func pack as the first argument that has the build contents." + | ||
"Otherwise, default is the current directory.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a line is connected to the one above, you should indent to make that clear
"Otherwise, default is the current directory.") | |
"Otherwise, default is the current directory.") |
NoBuild = NoBuild | ||
}; | ||
|
||
var oldCurrentDirectory = Environment.CurrentDirectory; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
originalCurrentDirectory
?
EnsureNpmExists(); | ||
|
||
// Change to the function app directory for npm operations | ||
var previousDirectory = Environment.CurrentDirectory; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's be consistent across all the commands on what we're calling this
|
||
if (string.IsNullOrEmpty(options.FolderPath)) | ||
{ | ||
ColoredConsole.WriteLine(WarningColor("No folder path specified. Using current directory as build output directory.")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this, can we do more than just defaulting to current? This is probably a good place to check for .props etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oo yes that's valid. I can add this scenario
Issue describing the changes in this PR
resolves#4593
This issue allows users to run
func pack
when they want to create a zip file that they can use the azure CLI to deploy. The general syntax of the command is as follows:func pack [path of folder to pack] [language specific options] [--no-build] [--output]
When the user runs

func --help
the following is displayed:Note that
func pack
will NOT be displayed when the user doesfunc --help
until we clean up the CLI and add all the validations for each command when runningfunc pack
. This will be done in a future PRThis PR adds the applicable E2E tests for the different scenarios and for each language worker runtime as well.
Pull request checklist
release_notes.md