-
Notifications
You must be signed in to change notification settings - Fork 458
Log warning message for unsupported action with Java #4582
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,4 +1,4 @@ | ||||||
// Copyright (c) .NET Foundation. All rights reserved. | ||||||
// Copyright (c) .NET Foundation. All rights reserved. | ||||||
// Licensed under the MIT License. See LICENSE in the project root for license information. | ||||||
|
||||||
using Azure.Functions.Cli.Common; | ||||||
|
@@ -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))) | ||||||
{ | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||||||
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")); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||||||
} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This needs to be a short link, without locale information. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||||||
} | ||||||
|
||||||
// Test helper method to set _currentWorkerRuntime for testing purpose | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
// Copyright (c) .NET Foundation. All rights reserved. | ||
// Licensed under the MIT License. See LICENSE in the project root for license information. | ||
|
||
using Azure.Functions.Cli.Helpers; | ||
using Azure.Functions.Cli.Interfaces; | ||
using Moq; | ||
using Xunit; | ||
|
||
namespace Azure.Functions.Cli.UnitTests | ||
{ | ||
public class GlobalCoreToolsSettingsTests | ||
{ | ||
[Theory] | ||
[InlineData("init", "--java")] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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);
} |
||
[InlineData("init", "--worker-runtime", "java")] | ||
[InlineData("new", "--java")] | ||
[InlineData("new", "--language", "java")] | ||
public void Init_LogsWarning_ForJavaWorkerRuntime(params string[] args) | ||
{ | ||
// Arrange | ||
var secretsManager = new Mock<ISecretsManager>(); | ||
|
||
var stringWriter = new StringWriter(); | ||
var originalOut = Console.Out; | ||
Console.SetOut(stringWriter); | ||
|
||
try | ||
{ | ||
// Act | ||
GlobalCoreToolsSettings.Init(secretsManager.Object, args); | ||
|
||
// Assert | ||
var output = stringWriter.ToString(); | ||
Assert.Contains("This action is not supported when using the core tools directly", output); | ||
Assert.Contains("java", output, StringComparison.OrdinalIgnoreCase); | ||
} | ||
finally | ||
{ | ||
Console.SetOut(originalOut); | ||
|
||
// Reset static state for other tests | ||
GlobalCoreToolsSettings.SetWorkerRuntime(WorkerRuntime.None); | ||
} | ||
} | ||
} | ||
} |
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.
The second check is necessary. Without it, commands that have
java
but do not have--java
in it (such asfunc new --language java
will be sent down this path and fail: