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.

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

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

Closed

Conversation

Don-Vito
Copy link
Contributor

@Don-Vito Don-Vito commented Nov 9, 2020

Summary of the Pull Request

This is an alternative implementation for #8197.
In rare occurrences, the shell settings might exist, but have no preferred shell configured.
Usually this will happen if the setting were created with old version of the cloud shell API.
Currently, we crash in such scenarios while trying to read a non-existing value.
This PR allows handles such cases by letting the user choose between PowerShell and Bash.

image

References

See #8197 for alternative solution (that simply fails the connection with informative message).

PR Checklist

  • Closes Azure Cloud Shell keeps resulting in "Key not found" error #7056
  • CLA signed.
  • Tests added/passed
  • Documentation updated.
  • Schema updated. - Irrelevant
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

  • Manual testing
  • Most of the testing was done with windbg changing the flow as if the preferred shell does not exist, as the problem does not reproduce in my subscriptions anymore

@ghost 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 9, 2020
@Don-Vito Don-Vito changed the title 7056: AzureConnection: If no preferred cloud shell is setup let the user choose shell rather than crashing 7056: AzureConnection - if preferred cloud shell wasn't set let the user choose rather than crashing Nov 9, 2020
{
_WriteStringWithNewline(_formatShell(i, shells[i]));
_WriteStringWithNewline(_formatShell(i, gsl::at(shells, i)));
Copy link
Member

Choose a reason for hiding this comment

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

This is nuanced, but for places where we know we're in-bounds we can use til::at. gsl::at does a bounds check 😄 which we don't need here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! :) I was looking for it 😊 Fixing it.

@zadjii-msft
Copy link
Member

Hey this one's no longer relevant with #8197 having merged, right?

@zadjii-msft zadjii-msft added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Nov 17, 2020
@Don-Vito Don-Vito closed this Nov 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-AzureShell Workitems pertaining to the Azure Cloud Shell connection. Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants