Prevent attempts to select PS7 on Functions v2 #407
Conversation
|
@Francisco-Gamino In order to make the error message more discoverable, I changed the implementation:
|
| @@ -307,4 +307,7 @@ | |||
| <data name="CommandNotFoundException_Exception" xml:space="preserve"> | |||
| <value>CommandNotFoundException detected (exception).</value> | |||
| </data> | |||
| <data name="InvalidFunctionsWorkerRuntimeVersion" xml:space="preserve"> | |||
| <value>Invalid PowerShell version specified in the {0} environment variable: {1}. This version is not supported on Azure Functions Runtime v2.</value> | |||
There was a problem hiding this comment.
@eamonoreilly Please review the message
|
|
||
| internal static string GetErrorMessage(string requestedVersion) | ||
| { | ||
| if (!string.IsNullOrWhiteSpace(requestedVersion) |
There was a problem hiding this comment.
I know that this change will only get merged to the v2/ps6 branch. However, would it make sense to also check FUNCTIONS_EXTENSION_VERSION to make sure we are running on V2? I am ok if we add this later in a separate PR given the time constrain.
There was a problem hiding this comment.
I'm not convinced we should do that here. One of the advantages of having separate branches for v2 and v3 is that we can make assumptions and reduce code complexity, eliminate some mental overhead, and reduce potential reasons for this code to break, like this:
- It would introduce a dependency on the
FUNCTIONS_EXTENSION_VERSIONformat, including all the possible variations, now and in the future. If anything changes, we'll need to remember about this dependency and have an upgrade plan. - This check would try to validate a build-time condition during runtime. This may lead to interesting results. Imagine someone actually made a mistake and included the worker intended for Runtime v2 into the v3 host build. If this check prevents function invocations, this mistake will cause a wide outage. But, if it does not prevent function invocations, who is going to notice it?
- In any case, this check would happen after the deployment, which is way too late. Furthermore, the users will be the first who notice the mistake, not the Functions engineers.
As long as we control the references to the PowerShell worker from the Functions host, I would treat the Functions Runtime version as a safe pre-condition, so we don't have to even think about issues like this.
I agree this check could notify us of the situation when someone makes a mistake on the Functions host and refers a wrong version. But, if we do need a check like this, it should be on the Functions host build/test process, so that this mistake is never deployed to begin with.
There was a problem hiding this comment.
I agree. Thanks you for the detailed explanation.
|
The code change looks good. One comment about checking for the host version 2 in addition to the runtime version. |
This is for the
v2.xbranch only.