Skip to content

Conversation

satvu
Copy link
Member

@satvu satvu commented Aug 6, 2025

Issue describing the changes in this PR

resolves #4539

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
  • My changes should not be added to the release notes for the next release
    • Otherwise: I've added my notes to release_notes.md
  • I have added all required tests (Unit tests, E2E tests)

@satvu satvu requested a review from a team as a code owner August 6, 2025 18:47
@satvu satvu marked this pull request as draft August 6, 2025 18:47
{
ColoredConsole
.WriteLine($"This action is not supported when using the core tools directly. Please use one of the supported environments to test {workerRuntimeSelected} apps locally as specified by" +
$"https://learn.microsoft.com/en-us/azure/azure-functions/functions-develop-local?pivots=programming-language-java");
Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -309,6 +309,13 @@ public async Task UpdateLanguageAndRuntime()
throw new CliException("Selected language doesn't match worker set in local.settings.json." +
$"Selected worker is: {_workerRuntime} and selected language is: {workerRuntimeSelected}");
}

if (workerRuntimeSelected == WorkerRuntime.Java)
Copy link
Member

Choose a reason for hiding this comment

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

This check should happen much sooner and for all init and new. (Maybe in the GlobalSettings init method).

Scenarios to validate:

  • func init
  • func init --java
  • func init --worker-runtime java
  • func new
  • func new --java
  • func new --language java

@@ -99,6 +99,13 @@ public static void Init(ISecretsManager secretsManager, string[] args)
{
_currentWorkerRuntime = WorkerRuntime.None;
}

if (_currentWorkerRuntime == WorkerRuntime.Java || args.Any(arg => string.Equals(arg, WorkerRuntime.Java.ToString(), StringComparison.OrdinalIgnoreCase)))
Copy link
Member Author

Choose a reason for hiding this comment

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

The second check is necessary. Without it, commands that have java but do not have --java in it (such as func new --language java will be sent down this path and fail:

                else
                {
                    _currentWorkerRuntime = WorkerRuntimeLanguageHelper.GetCurrentWorkerRuntimeLanguage(secretsManager);
                }

public class GlobalCoreToolsSettingsTests
{
[Theory]
[InlineData("init", "--java")]
Copy link
Member Author

Choose a reason for hiding this comment

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

This is not the right way to test this (one will pass, others will fail because of how console is set up) but I don't know if checking the console output is correct - any suggestions on how to best this this with the existing core tools testing framework?

I need to test that these commands log a warning message and that a non-java language will not log any error message.

Copy link
Member

Choose a reason for hiding this comment

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

Sub the console writer instead:

        public GlobalCoreToolsSettingsTests()
        {
            // Reset the static state before each test
            GlobalCoreToolsSettings.SetWorkerRuntime(WorkerRuntime.None);
        }

        [Theory]
        [InlineData("init", "--java")]
        [InlineData("init", "--worker-runtime", "java")]
        [InlineData("new", "--java")]
        [InlineData("new", "--language", "java")]
        public void Init_LogsWarning_ForJavaWorkerRuntime(params string[] args)
        {
            // Arrange
            var output = new StringBuilder();
            var secretsManager = new Mock<ISecretsManager>();
            var console = Substitute.For<IConsoleWriter>();
            console.WriteLine(Arg.Do<object>(o => output.AppendLine(o?.ToString()))).Returns(console);
            console.Write(Arg.Do<object>(o => output.Append(o.ToString()))).Returns(console);
            ColoredConsole.Out = console;
            ColoredConsole.Error = console;

            // Act
            GlobalCoreToolsSettings.Init(secretsManager.Object, args);

            // Assert
            Assert.Contains("This action is not supported when using the core tools directly", output.ToString());
            Assert.Contains("java", output.ToString(), StringComparison.OrdinalIgnoreCase);
        }

@mattchenderson mattchenderson self-requested a review August 13, 2025 20:45
{
ColoredConsole
.WriteLine(WarningColor($"This action is not supported when using the core tools directly. Please use one of the supported environments to test {_currentWorkerRuntime} apps locally as specified by" +
$"https://learn.microsoft.com/en-us/azure/azure-functions/functions-develop-local?pivots=programming-language-java"));
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be a short link, without locale information.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, just a placeholder for now while we iterate. Will leave this comment unresolved until I fix it.

if (_currentWorkerRuntime == WorkerRuntime.Java || args.Any(arg => string.Equals(arg, WorkerRuntime.Java.ToString(), StringComparison.OrdinalIgnoreCase)))
{
ColoredConsole
.WriteLine(WarningColor($"This action is not supported when using the core tools directly. Please use one of the supported environments to test {_currentWorkerRuntime} apps locally as specified by" +
Copy link
Contributor

Choose a reason for hiding this comment

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

"Core Tools" capitalized.

Also, is this really about "supported environments" or even "testing"? It's more about scaffolding templates, right? Or I guess it could be multiple scenarios? It might be easier to just treat it as "work with".

Suggested change
.WriteLine(WarningColor($"This action is not supported when using the core tools directly. Please use one of the supported environments to test {_currentWorkerRuntime} apps locally as specified by" +
.WriteLine(WarningColor($"This action is not supported when using the Core Tools directly. Please see {link} for instructions on working with {_currentWorkerRuntime} apps locally." +

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 also questioning if this should be warning level in all cases. It feels like this should be an error for some actions, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's definitely an error for some actions -- we could also conditionally log warnings/errors based on the type of action. This is just the file location we landed on during design since all of the impacted commands would come through here.

@@ -99,6 +99,13 @@ public static void Init(ISecretsManager secretsManager, string[] args)
{
_currentWorkerRuntime = WorkerRuntime.None;
}

if (_currentWorkerRuntime == WorkerRuntime.Java || args.Any(arg => string.Equals(arg, WorkerRuntime.Java.ToString(), StringComparison.OrdinalIgnoreCase)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the right place for this check? It seems like this impacts all actions, perhaps not when intended. Even func host start --java would get this warning, which seems a little overly aggressive. Perhaps we could scope it to impacted actions?

Copy link
Member Author

@satvu satvu Aug 19, 2025

Choose a reason for hiding this comment

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

If we don't want customers to use Core Tools with Java at all (which was under discussion, but we were leaning that way), then I think this should show up for all actions (even if it works). We want to point users to other documented, supported tools.

We can definitely scope this to just impacted ones -- I guess we do "partially support" Java and we can continue to do so.

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.

Func new not working for java runtime
4 participants