-
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?
Conversation
{ | ||
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"); |
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.
placeholder - will prob add a fw link that points to https://learn.microsoft.com/en-us/azure/azure-functions/functions-develop-local?pivots=programming-language-java#local-development-environments
@@ -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) |
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.
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))) |
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 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")] |
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.
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 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);
}
{ | ||
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 comment
The 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 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" + |
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.
"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".
.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." + |
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'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 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))) |
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 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?
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 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.
Issue describing the changes in this PR
resolves #4539
Pull request checklist
release_notes.md