Azure: fall back to powershell when no preferred shell is set #8197

Don-Vito · 2020-11-08T15:53:29Z

Fallback to powershell if "preferred shell" is not specified in the user
settings of Azure CLI.

I am still not sure what is the full set of scenarios that the problem
might occur, but for me it occurred for an "old" cloud shell account,
and didn't reproduce since I have reconfigured it. These behavior might
be explained by the fact that "preferred shell type" did not exist in
the API originally and thus was not set. In such case, Terminal
succeeds to retrieve to the settings but then crashes when reading the
missing field. To fix it, I handle the case where the field is missing
and fallback to powershell

Validation Steps Performed

Tested manually, only once.

Closes #7056

…rred shell type

zadjii-msft

This seems like a fair solution to me, but I'm gonna defer to @DHowett and @PankajBhojwani with this one.

zadjii-msft · 2020-11-09T11:23:36Z

src/cascadia/TerminalConnection/AzureConnection.cpp

+        if (!shellType.has_value())
+        {
+            _WriteStringWithNewline(RS_(L"AzureInvalidUserSettings"));
+            _transitionToState(ConnectionState::Failed);


If they're missing a preferred shell type, is there a way to just fall back to something reasonable, instead of failing entirely? Maybe we could display the warning, but then use PowerShell, and if they want bash then at least they'll know why powershell was chosen
/cc @DHowett

@zadjii-msft - absolutely. Actually, I already implemented something similar for: #4987

In that ticket I allow the user to always select a shell, where the preferred one is marked as default.
But I guess that ticket will be stuck on product, because UX is not trivial (extra user input is introduced).

So I can probably adjust the logic from there to here. I.e., read the preferred shell.. if no preferred shell was found - allow the user to select the shell manually.

I will do the adjustment, but hope you help me push it forward 😊

zadjii-msft · 2020-11-09T11:25:00Z

src/cascadia/TerminalConnection/Resources/en-US/Resources.resw

@@ -148,6 +148,10 @@
    <value>You have not set up your cloud shell account yet. Please go to https://shell.azure.com to set it up.</value>
    <comment>{Locked="https://shell.azure.com"} This URL should not be localized. Everything else should.</comment>
  </data>
+  <data name="AzureInvalidUserSettings" xml:space="preserve">
+    <value>Your cloud shell account misses some mandatory settings. Please go to https://shell.azure.com to set it up.</value>


Suggested change

<value>Your cloud shell account misses some mandatory settings. Please go to https://shell.azure.com to set it up.</value>

<value>Your cloud shell account does not have a default shell configured. Please go to https://shell.azure.com to set it up.</value>

Perhaps? We'd need to workshop the text if we go with the graceful fallback solution

DHowett · 2020-11-09T18:47:05Z

src/cascadia/TerminalConnection/AzureConnection.cpp

+        if (!settingsResponse.has_object_field(L"properties"))
+        {
+            return std::nullopt;
+        }
+
+        const auto userSettings = settingsResponse.at(L"properties");
+        if (!userSettings.has_string_field(L"preferredShellType"))
+        {
+            return std::nullopt;
+        }
+
+        const auto preferredShellTypeValue = userSettings.at(L"preferredShellType");
+        return std::optional(preferredShellTypeValue.as_string());


If I might suggest a little more telescoping to make this easier to read:

Suggested change

if (!settingsResponse.has_object_field(L"properties"))

{

return std::nullopt;

}

const auto userSettings = settingsResponse.at(L"properties");

if (!userSettings.has_string_field(L"preferredShellType"))

{

return std::nullopt;

}

const auto preferredShellTypeValue = userSettings.at(L"preferredShellType");

return std::optional(preferredShellTypeValue.as_string());

if (settingsResponse.has_object_field(L"properties"))

{

const auto userSettings = settingsResponse.at(L"properties");

if (userSettings.has_string_field(L"preferredShellType"))

{

const auto preferredShellTypeValue = userSettings.at(L"preferredShellType");

return preferredShellTypeValue.as_string();

}

}

return std::nullopt;

You also shouldn't need to construct an optional directly and instead rely on the optional constructor to let you return a string directly.

If you do need to construct the optional directly, prefer return { blah.as_string() }; 😄 so you don't need to repeat the optional type.

sorry, my suggestion text apparently contains tabs (thanks GitHub_)

DHowett · 2020-11-09T18:49:07Z

src/cascadia/TerminalConnection/AzureConnection.cpp

        _WriteStringWithNewline(RS_(L"AzureRequestingTerminal"));
-        const auto socketUri = _GetTerminal(shellType);
+        const auto socketUri = _GetTerminal(*shellType);


I'd say let's just tear off the band-aid and call it:

Suggested change

const auto socketUri = _GetTerminal(*shellType);

const auto socketUri = _GetTerminal(shellType.value_or("powershell"));

We don't need to emit a warning and we don't need to add another resource string 😄

DHowett

As above!

Don-Vito · 2020-11-09T18:53:43Z

@DHowett - wait, which one we are working on? They are mutually exclusive.
This one or #8204?

DHowett · 2020-11-09T18:54:02Z

I'll get to them in turn 😉

Don-Vito · 2020-11-09T18:57:08Z

I'll get to them in turn 😉

Do you mean you want to push both of them? The second one delayed? Since I need to fix the same comments in both branches.

DHowett · 2020-11-09T19:01:28Z

So, I think that this is a fine fix while we figure out how best to approach #8204. I share your concern about introducing new UI.

I had originally thought that we could add support for shell selection by overloading the meaning of commandline for the Azure Cloud Shell connection type. That is -- we could give users the option of setting "commandline": "bash" or "commandline": "powershell", or even adding multiple copies of ACS that launch different things.

In the interest of full full disclosure: I really wanted to land a feature that would allow a connection type constructor to see the json blob (or see a representation of the json blob, because I really do not want to push json through WinRT, ESPECIALLY not as a string (ugh)) and in so doing allow the connection class to add support for its own profile options.

That would, long-term, allow for something like:

{
"connectionType": "{azure-cloud-shell-ctype-guid}",
"shell": "bash",
"tenant": "00001111-4444-5555-6666-777788889999",
"cloud": "US Sovereign Cloud" // or whatever
}

so that folks could preconfigure an entire ACS profile that just skips all of the UI/UX/

…hell

DHowett

DHowett · 2020-11-09T19:22:22Z

src/cascadia/TerminalConnection/AzureConnection.cpp

+        if (!settingsResponse.has_object_field(L"properties"))
+        {
+            return std::nullopt;
+        }
+
+        const auto userSettings = settingsResponse.at(L"properties");
+        if (!userSettings.has_string_field(L"preferredShellType"))
+        {
+            return std::nullopt;
+        }
+
+        const auto preferredShellTypeValue = userSettings.at(L"preferredShellType");
+        return std::optional(preferredShellTypeValue.as_string());


sorry, my suggestion text apparently contains tabs (thanks GitHub_)

Don-Vito · 2020-11-09T19:24:21Z

@DHowett - no worries, I am doing it locally.. I also need to test it 😄 . Which I can only do artificially now, since the issue does not reproduce anymore in my subscriptions, so I have to change some eax's in WinDBG

Which could be by the way avoided, if we would allow configuring proxy for our connection (then I could test everything with Fiddler and autoresponder).

DHowett · 2020-11-09T19:48:56Z

src/cascadia/TerminalConnection/AzureConnection.cpp

        _WriteStringWithNewline(RS_(L"AzureRequestingTerminal"));
-        const auto socketUri = _GetTerminal(shellType);
+        const auto socketUri = _GetTerminal(shellType.value_or(L"pwsh"));


neat! i didn't realize pwsh was the official value for that one 😄

DHowett

Love it. Thank you!

DHowett · 2020-11-09T22:48:53Z

@msftbot merge this in 1 minute

ghost · 2020-11-09T22:48:56Z

Hello @DHowett!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I won't merge this pull request until after the UTC date Mon, 09 Nov 2020 22:49:55 GMT, which is in 1 minute

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

DHowett · 2020-11-09T22:50:43Z

Sorry you had to manually recover the client ID ;)

DHowett · 2020-11-09T22:53:11Z

Shoulder sign-off from @PankajBhojwani:

Just took a look - looks great to me!

ghost · 2020-11-11T18:09:08Z

🎉Windows Terminal Preview v1.5.3142.0 has been released which incorporates this pull request.:tada:

Handy links:

dave-007 · 2024-05-13T20:31:20Z

Hi,
This 'Key Not Found' issue has returned for my student in the new Azure Cloud Shell experience. The workarounds about (switch bach to Bash, then PowerShell again) do not resolve, nor does switching to classic experience.

Why must this error message be so unspecific? What key is missing, why can't it use an intelligent default?

Really bad experience here for new users.

DHowett · 2024-05-13T22:21:55Z

@dave-007 I'm sorry you're still hitting this issue. Unfortunately, this pull request went in almost four years ago. Would you be able to share which version number of Terminal you have installed?

I ask only because we just (two weeks ago) rolled out a fix for an API version change the Cloud Shell team published... which if you don't have could certainly be why you're hitting this error again.

dave-007 · 2024-05-14T01:53:57Z

It's on a Skillable lab for AZ-104, Lab 03b, running Windows Terminal version: 1.12.10983.0

DHowett · 2024-05-14T02:50:12Z

Oh, wow! This complicates things. I suspect you’re on Terminal 1.12 because you are on a version of Windows that has gone out of support.

We’ll look at the cost of bringing the new Azure Cloud Shell API version back to 1.12.

For reference, the current version is 1.20. It’s about two years newer than what you have. :/

dave-007 · 2024-05-16T10:00:58Z

I'll submit a ticket with scalable and refer to this thread to see if they can update the terminal version.

…

On Mon, May 13, 2024, 22:50 Dustin L. Howett ***@***.***> wrote: Oh, wow! This complicates things. I suspect you’re on Terminal 1.12 because you are on a version of Windows that has gone out of support. We’ll look at the cost of bringing the new Azure Cloud Shell API version back to 1.12. For reference, the current version is 1.20. It’s about two years newer than what you have. :/ — Reply to this email directly, view it on GitHub <#8197 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAB2ZFN63A3PHPUGJ2GQMMLZCF3XVAVCNFSM4TOMZPNKU5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TEMJQHEYTONRZGQYQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

7056: provide informative error if Azure CLI user settings miss prefe…

2eb93ed

…rred shell type

ghost added Area-AzureShell Workitems pertaining to the Azure Cloud Shell connection. Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. labels Nov 8, 2020

zadjii-msft reviewed Nov 9, 2020

View reviewed changes

zadjii-msft assigned DHowett and PankajBhojwani Nov 9, 2020

Don-Vito mentioned this pull request Nov 9, 2020

7056: AzureConnection - if preferred cloud shell wasn't set let the user choose rather than crashing #8204

Closed

6 tasks

DHowett reviewed Nov 9, 2020

View reviewed changes

DHowett requested changes Nov 9, 2020

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Nov 9, 2020

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Nov 9, 2020

Merge remote-tracking branch 'upstream/main' into 7056-no-preferred-s…

f1fcc45

…hell

Don-Vito changed the title ~~7056: Azure Connection - provide informative error if Azure CLI user settings miss preferred shell type~~ 7056: Azure Connection - fallback to powershell if Azure CLI user settings miss preferred shell type Nov 9, 2020

DHowett reviewed Nov 9, 2020

View reviewed changes

Don-Vito added 2 commits November 9, 2020 21:39

7056: fallback to powershell if no preferred shell is set

9520f5b

7056: move parsing logic to minimize PR diff

94d71ce

DHowett reviewed Nov 9, 2020

View reviewed changes

Don-Vito requested review from DHowett and zadjii-msft November 9, 2020 19:52

DHowett approved these changes Nov 9, 2020

View reviewed changes

ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Nov 9, 2020

DHowett changed the title ~~7056: Azure Connection - fallback to powershell if Azure CLI user settings miss preferred shell type~~ Azure: fall back to powershell when no preferred shell is set Nov 9, 2020

DHowett merged commit 0c0830b into microsoft:main Nov 9, 2020

ghost mentioned this pull request Nov 11, 2020

Azure Cloud Shell keeps resulting in "Key not found" error #7056

Closed

Don-Vito deleted the 7056-no-preferred-shell branch December 3, 2020 17:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Azure: fall back to powershell when no preferred shell is set #8197

Azure: fall back to powershell when no preferred shell is set #8197

Don-Vito commented Nov 8, 2020 •

edited by DHowett

Loading

zadjii-msft left a comment

zadjii-msft Nov 9, 2020

Don-Vito Nov 9, 2020 •

edited

Loading

zadjii-msft Nov 9, 2020

DHowett Nov 9, 2020

DHowett Nov 9, 2020

DHowett Nov 9, 2020

DHowett left a comment

Don-Vito commented Nov 9, 2020

DHowett commented Nov 9, 2020

Don-Vito commented Nov 9, 2020

DHowett commented Nov 9, 2020

DHowett left a comment

DHowett Nov 9, 2020

Don-Vito commented Nov 9, 2020 •

edited

Loading

DHowett Nov 9, 2020

DHowett left a comment

DHowett commented Nov 9, 2020

ghost commented Nov 9, 2020

DHowett commented Nov 9, 2020

DHowett commented Nov 9, 2020

ghost commented Nov 11, 2020

dave-007 commented May 13, 2024 •

edited

Loading

DHowett commented May 13, 2024

dave-007 commented May 14, 2024

DHowett commented May 14, 2024 •

edited

Loading

dave-007 commented May 16, 2024 via email

	<value>Your cloud shell account misses some mandatory settings. Please go to https://shell.azure.com to set it up.</value>
	<value>Your cloud shell account does not have a default shell configured. Please go to https://shell.azure.com to set it up.</value>

	const auto socketUri = _GetTerminal(*shellType);
	const auto socketUri = _GetTerminal(shellType.value_or("powershell"));

Azure: fall back to powershell when no preferred shell is set #8197

Azure: fall back to powershell when no preferred shell is set #8197

Conversation

Don-Vito commented Nov 8, 2020 • edited by DHowett Loading

Validation Steps Performed

zadjii-msft left a comment

Choose a reason for hiding this comment

zadjii-msft Nov 9, 2020

Choose a reason for hiding this comment

Don-Vito Nov 9, 2020 • edited Loading

Choose a reason for hiding this comment

zadjii-msft Nov 9, 2020

Choose a reason for hiding this comment

DHowett Nov 9, 2020

Choose a reason for hiding this comment

DHowett Nov 9, 2020

Choose a reason for hiding this comment

DHowett Nov 9, 2020

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

Don-Vito commented Nov 9, 2020

DHowett commented Nov 9, 2020

Don-Vito commented Nov 9, 2020

DHowett commented Nov 9, 2020

DHowett left a comment

Choose a reason for hiding this comment

DHowett Nov 9, 2020

Choose a reason for hiding this comment

Don-Vito commented Nov 9, 2020 • edited Loading

DHowett Nov 9, 2020

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

DHowett commented Nov 9, 2020

ghost commented Nov 9, 2020

DHowett commented Nov 9, 2020

DHowett commented Nov 9, 2020

ghost commented Nov 11, 2020

dave-007 commented May 13, 2024 • edited Loading

DHowett commented May 13, 2024

dave-007 commented May 14, 2024

DHowett commented May 14, 2024 • edited Loading

dave-007 commented May 16, 2024 via email

Don-Vito commented Nov 8, 2020 •

edited by DHowett

Loading

Don-Vito Nov 9, 2020 •

edited

Loading

Don-Vito commented Nov 9, 2020 •

edited

Loading

dave-007 commented May 13, 2024 •

edited

Loading

DHowett commented May 14, 2024 •

edited

Loading