Skip to content

Publish warning message changes #4357

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

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

Conversation

umangsriv
Copy link
Contributor

Publish warning message for other (NodeJS, Python, Java, etc.) worker runtime

resolves #4241

Pull request checklist

  • My changes do not require documentation changes
    • Otherwise: Documentation issue linked to PR
  • My changes do not need to be backported to a previous version
    • Otherwise: Backport tracked by issue/PR #issue_or_pr
  • I have added all required tests (Unit tests, E2E tests)

@umangsriv umangsriv requested a review from a team as a code owner April 10, 2025 10:50
@umangsriv umangsriv requested a review from aishwaryabh April 10, 2025 10:50
@umangsriv
Copy link
Contributor Author

Hi @aishwaryabh ,

I’ve created a new pull request as the previous one had multiple conflicts with the main branch. Could you please review the new PR when you get a moment? I’ll go ahead and close the old one.

Thank you!

DateTime currentDate = DateTime.Now;
if (workerRuntime == WorkerRuntime.python)
{
var linuxRuntimeSettings = stacks.GetOtherRuntimeSettings(workerRuntimeStr, runtimeVersion, out bool isLinuxLTS, s => s.LinuxRuntimeSettings);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still a bit confused on why this has to be an output variable if we're not using it anywhere. Can you clarify this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, thanks for pointing this out, got missed from my end, now I have removed this unused code.

// Get runtime stacks
var stacks = await AzureHelper.GetFunctionsStacks(AccessToken, ManagementURL);
DateTime currentDate = DateTime.Now;
if (workerRuntime == WorkerRuntime.python)
Copy link
Contributor

Choose a reason for hiding this comment

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

this logic can be simplified a bit to be less redundant:

 var runtimeSettings = (workerRuntime == WorkerRuntime.python) ? stacks.GetOtherRuntimeSettings(workerRuntimeStr, runtimeVersion, out bool isLinuxLTS, s => s.LinuxRuntimeSettings) : stacks.GetOtherRuntimeSettings(workerRuntimeStr, runtimeVersion, out bool isWindowsLTS, s => s.WindowsRuntimeSettings));


                        DateTime eolDate = runtimeSettings.EndOfLifeDate.Value;
                        DateTime warningThresholdDate = eolDate.AddMonths(-6);
                        if (currentDate > eolDate || currentDate >= warningThresholdDate)
                        {
                            //Show EOL warning message
                            ShowEolMessageForOtherStack(stacks, runtimeSettings.EndOfLifeDate.Value, workerRuntimeStr, runtimeVersion);
                        }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored this code.

}

[Theory]
[InlineData("node", "14", "22")] // Node.js 14 should return next supported 22
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a few tests where the next available version is not available? (ie GetNextRuntimeVersion returns null)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added test cases.

Copy link
Member

@fabiocav fabiocav left a comment

Choose a reason for hiding this comment

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

Before reviewing/asking additional questions, I wanted to check if the logic here was influenced by the existing implementation in other tools (e.g., Azure CLI). Wast that the case? Does the logic there align with how the target stack version resolution is done?

One minor comment in the meantime; please update the formatting logic for anything that is being displayed to the user to use the appropriate culture. Things like date formatting, for example, should be done using that user's culture information.

@umangsriv
Copy link
Contributor Author

Before reviewing/asking additional questions, I wanted to check if the logic here was influenced by the existing implementation in other tools (e.g., Azure CLI). Wast that the case? Does the logic there align with how the target stack version resolution is done?

One minor comment in the meantime; please update the formatting logic for anything that is being displayed to the user to use the appropriate culture. Things like date formatting, for example, should be done using that user's culture information.

Hey Fabio,

Regarding the logic: It is based on the existing implementation already present for the .NET isolated publish flow in Azure Functions Core Tools. That flow uses the Azure Functions Stacks API to determine the major .NET version, retrieve corresponding runtime settings, and evaluate deprecation/EOL dates before publishing.

I followed the same pattern and extended it to other worker runtimes (e.g., Python, Node.js, PowerShell, Java) using the same stack API-based approach.

This was not influenced by the Azure CLI logic specifically.

For culture-specific formatting, I’ll update all user-facing date formatting to respect CultureInfo.CurrentCulture so it reflects the user's locale properly.

@umangsriv
Copy link
Contributor Author

Hey @fabiocav , @aishwaryabh , kindly review the PR at your convenience and let me know if any further modifications are needed.

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.

Show warning message for runtimes reaching end-of-life during publish
3 participants