Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 4 commits into from
Nov 9, 2020
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2eb93ed
7056: provide informative error if Azure CLI user settings miss prefe…
Don-Vito Nov 8, 2020
f1fcc45
Merge remote-tracking branch 'upstream/main' into 7056-no-preferred-s…
Don-Vito Nov 9, 2020
9520f5b
7056: fallback to powershell if no preferred shell is set
Don-Vito Nov 9, 2020
94d71ce
7056: move parsing logic to minimize PR diff
Don-Vito Nov 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 30 additions & 2 deletions src/cascadia/TerminalConnection/AzureConnection.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -687,6 +687,27 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
_state = AzureState::TermConnecting;
}

// Method description:
// - Helper function to parse the preferred shell type from user settings returned by cloud console API.
// We need this function because the field might be missing in the settings
// created with old versions of cloud console API.
std::optional<utility::string_t> AzureConnection::_ParsePreferredShellType(const web::json::value& settingsResponse)
{
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());
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

}

// Method description:
// - helper function to connect the user to the Azure cloud shell
void AzureConnection::_RunConnectState()
Expand All @@ -700,15 +721,22 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
return;
}

const auto shellType = _ParsePreferredShellType(settingsResponse);
if (!shellType.has_value())
{
_WriteStringWithNewline(RS_(L"AzureInvalidUserSettings"));
_transitionToState(ConnectionState::Failed);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@Don-Vito Don-Vito Nov 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@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 😊

return;
}

// Request for a cloud shell
_WriteStringWithNewline(RS_(L"AzureRequestingCloud"));
_cloudShellUri = _GetCloudShell();
_WriteStringWithNewline(RS_(L"AzureSuccess"));

// Request for a terminal for said cloud shell
const auto shellType = settingsResponse.at(L"properties").at(L"preferredShellType").as_string();
_WriteStringWithNewline(RS_(L"AzureRequestingTerminal"));
const auto socketUri = _GetTerminal(shellType);
const auto socketUri = _GetTerminal(*shellType);
Copy link
Member

Choose a reason for hiding this comment

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

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 😄

_TerminalOutputHandlers(L"\r\n");

// Step 8: connecting to said terminal
Expand Down
2 changes: 2 additions & 0 deletions src/cascadia/TerminalConnection/AzureConnection.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,8 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
std::optional<std::wstring> _ReadUserInput(InputMode mode);

web::websockets::client::websocket_client _cloudShellSocket;

static std::optional<utility::string_t> _ParsePreferredShellType(const web::json::value& settingsResponse);
};
}

Expand Down
4 changes: 4 additions & 0 deletions src/cascadia/TerminalConnection/Resources/en-US/Resources.resw
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Copy link
Member

Choose a reason for hiding this comment

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

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

<comment>{Locked="https://shell.azure.com"} This URL should not be localized. Everything else should.</comment>
</data>
<data name="AzureStorePrompt" xml:space="preserve">
<value>Do you want to save these connection settings for future logins? [{0}/{1}]</value>
<comment>{0} and {1} will be replaced with AzureUserEntry_Yes and AzureUserEntry_No. They are single-character shorthands for "yes" and "no" in this language.</comment>
Expand Down