Publish warning message changes - 4241 #4324
Conversation
src/Azure.Functions.Cli/Actions/AzureActions/PublishFunctionAppAction.cs
Outdated
Show resolved
Hide resolved
| @@ -199,6 +199,43 @@ public override async Task RunAsync() | |||
| // We do not change the default targetFramework if no .csproj file is found | |||
| } | |||
|
|
|||
| // Show warning message for other worker runtimes (Node, Pyrhon) | |||
| if (workerRuntime == WorkerRuntime.node || workerRuntime == WorkerRuntime.python) | |||
There was a problem hiding this comment.
do we want this logic specifically for node or python? should we have this logic for all function stacks?
There was a problem hiding this comment.
I have made it generic, can used for other stacks as well.
| } | ||
| } | ||
|
|
||
| private void ShowEolMessageForPython(FunctionsStacks stacks, LinuxRuntimeSettings currentRuntimeSettings, string workerRuntime, string runtimeVersion) |
There was a problem hiding this comment.
this logic is very similar for python and node. Can we combine these methods and pass in a parameter for determining if we call GetNextRuntimeVersionForPython, GetNextRuntimeVersionForNode, etc
There was a problem hiding this comment.
Combined and make generic method.
| return minorVersion?.StackSettings?.LinuxRuntimeSettings; | ||
| } | ||
|
|
||
| public static WindowsRuntimeSettings GetRuntimeSettingsForNode(this FunctionsStacks stacks, string workerRuntime, string runtimeVersion, out bool isLTS) |
There was a problem hiding this comment.
again this logic is very similar for python and node. can we combine the methods?
There was a problem hiding this comment.
Combined and make generic method.
|
Hi @aishwaryabh, I have implemented the changes you mentioned in your comment, please review them when you have a moment. |
| @@ -199,6 +199,41 @@ public override async Task RunAsync() | |||
| // We do not change the default targetFramework if no .csproj file is found | |||
| } | |||
|
|
|||
| // Show warning message for other worker runtimes (Node, Python, Powershell, Java) | |||
| if (workerRuntime == WorkerRuntime.node || workerRuntime == WorkerRuntime.python || workerRuntime == WorkerRuntime.powershell || workerRuntime == WorkerRuntime.java) | |||
There was a problem hiding this comment.
is this all runtimes except dotnet? if so we can say the following:
if (workerRuntime != WorkerRuntime.dotnet && workerRuntime != WorkerRuntime.dotnetIsolated)
There was a problem hiding this comment.
Yes, it implies to all runtimes except dotnet, I have fixed it.
| DateTime currentDate = DateTime.Now; | ||
| if (workerRuntime == WorkerRuntime.python) | ||
| { | ||
| var linuxRuntimeSettings = stacks.GetOtherRuntimeSettings(workerRuntimeStr, runtimeVersion, out bool isPythonLTS, s => s.LinuxRuntimeSettings); |
There was a problem hiding this comment.
what is the point of IsPythonLTS and isNodeLTS being output variables?
There was a problem hiding this comment.
I have changed the output variable name, yes, you are right, “IsPythonLTS” and “IsNodeLTS” do not accurately represent the type of setting they return, instead, the new name reflects the actual setting type- such as Linux for Python and Windows for Node.
|
Hi @aishwaryabh , I have incorporated the changes you suggested, please review it again at your convenience. |
Showing warning message for runtimes reaching end-of-life during publish for Node and Python
resolves #4241
Pull request checklist