The Azure cloud shell connector #1808

PankajBhojwani · 2019-07-03T21:32:52Z

Implemented a new connection type, the Azure cloud shell connector

PR Checklist

Closes Feature Request: Azure cloud shell connector #1235
CLA signed. If not, go over here and sign the CLA
Tests added/passed
Requires documentation to be updated
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.

Implemented a new connection
Created a new default profile for that connection (includes icons)
Added very basic handling for the backspace VT sequence
Added a condition to check for \a (aka the BEL character) just so we don't try to print that character

…ealAzCon

miniksa

Also, we will want to find a solution that doesn't require us to commit the entire VCPKG nuget package into the repository since it's 50+MB.

src/cascadia/TerminalApp/App.cpp

miniksa · 2019-07-05T16:32:16Z

src/cascadia/TerminalApp/App.cpp

-        // Create a Conhost connection based on the values in our settings object.
-        TerminalConnection::ITerminalConnection controlConnection = TerminalConnection::ConhostConnection(controlSettings.Commandline(), controlSettings.StartingDirectory(), 30, 80, winrt::guid());
+        TerminalConnection::ITerminalConnection controlConnection{ nullptr };
+        if (controlSettings.Commandline() == L"Azure")


The magic string L"Azure" is used in multiple places. It should be defined once as a constant imported into all useful locations.

(This will still be applicable if you change it to an enum or guid)

Also, we will want to find a solution that doesn't require us to commit the entire VCPKG nuget package into the repository since it's 50+MB.

I recently discovered I had to use vcpkg to install cpprestsdk:x64-windows.

One solution would be to move away from submodules to git subtrees.
https://blog.developer.atlassian.com/the-power-of-git-subtree/

This would reduce the burden of a submodule which by default pulls in the entire repo (and its history).

I love subtrees, but.. we're not using submodules for cpprest or anything vcpkg-related.

src/cascadia/TerminalApp/App.cpp

src/cascadia/TerminalConnection/AzureConnection.cpp

src/cascadia/TerminalConnection/AzureConnection.h

src/cascadia/TerminalConnection/packages.config

src/cascadia/TerminalConnection/pch.h

src/cascadia/TerminalCore/TerminalDispatch.cpp

ras0219-msft · 2019-07-06T23:39:11Z

Creating a public Azure DevOps Artifacts Feed may be a better option than checking in the NuGet packages.

zadjii-msft

Michael has a bunch of other good comments already - I'll make a second pass when those are addressed

src/cascadia/TerminalConnection/AzureConnection.cpp

…ge smaller

miniksa · 2019-07-10T16:51:04Z

Creating a public Azure DevOps Artifacts Feed may be a better option than checking in the NuGet packages.

@ras0219-msft, as far as I could find... this is not a thing that exists. There was no such thing as a public feed. There's a 100% private feed, and a partially private feed for just your org. But there's no straight up public feed that anyone can pull from.

miniksa · 2019-07-10T17:17:33Z

Creating a public Azure DevOps Artifacts Feed may be a better option than checking in the NuGet packages.

@ras0219-msft Robert Schumacher FTE, as far as I could find... this is not a thing that exists. There was no such thing as a public feed. There's a 100% private feed, and a partially private feed for just your org. But there's no straight up public feed that anyone can pull from.

https://dev.azure.com/ms/Terminal has a banner on the top saying we can't publish artifacts after 7/15 without billing. @DHowett-MSFT, can you track down whomever owns the MS instance and have them fix the billing?

…. The connector now has the ability to store tokens for future logins

…rinting/comments changes

src/cascadia/TerminalConnection/AzureConnection.cpp

miniksa · 2019-07-17T20:19:42Z

src/cascadia/TerminalConnection/AzureConnection.cpp

+
+        //Initialize the request
+        http_request tenantRequest(L"GET");
+        tenantRequest.set_request_uri(L"tenants?api-version=2018-01-01");


So it's intentional that they're different? It just looks like an accident/typo.

src/cascadia/TerminalConnection/AzureConnectionStrings.h

…r to App to create connection, arm64 *should* build now

src/cascadia/TerminalApp/App.cpp

src/cascadia/TerminalApp/CascadiaSettings.h

src/cascadia/TerminalConnection/AzureConnection.cpp

zadjii-msft

I'd say none of my concerns really rise above the level of a nit. Nice work with this!

src/cascadia/TerminalConnection/AzureConnectionStrings.h

src/cascadia/TerminalConnection/AzureConnection.h

src/cascadia/TerminalApp/CascadiaSettings.cpp

zadjii-msft · 2019-07-23T11:58:34Z

src/cascadia/TerminalApp/App.cpp

+            // Remove the TabView from the page. We'll hang on to it, we need to
+            // put it in the titlebar.
+            uint32_t index = 0;
+            if (terminalPage->Root().Children().IndexOf(_tabRow,


Should we maybe (file a follow up task) to display an error dialog if they try using the azure connection type in arm64?

Might not be a bad idea, though I think its an issue that very few people will run into - there's no default profile for an azure connection in an arm64 build, so someone would have to build in non-arm64 first (therefore getting a profiles file with the azure connection as an option), and then build arm64 and use that same profiles file and then try to open up an azure connection to run into that error. At the same time its not hard to implement so I guess we could just do it

zadjii-msft · 2019-07-23T12:00:33Z

src/cascadia/TerminalConnection/AzureConnection.cpp

+    // - the handler
+    // Return value:
+    // - the event token for the handler
+    winrt::event_token AzureConnection::TerminalOutput(Microsoft::Terminal::TerminalConnection::TerminalOutputEventArgs const& handler)


I think we might have some DEFINE_EVENT_HANDLER macros lying around so you don't need to put these in here manually, but they're fine for now.

Yeah. They're in src\cascadia\inc\cppwinrt_utils.h

Ah I was just following the way ConhostConnection.cpp did it

src/cascadia/TerminalConnection/AzureConnection.cpp

zadjii-msft · 2019-07-23T12:07:05Z

src/cascadia/TerminalConnection/AzureConnection.cpp

+                // We are connected, continuously read from the websocket until its closed
+                case State::termConnected:
+                {
+                    while (true)


Why didn't this state get it's own helper method too?

It felt like the real meat of the output thread (since its the state we'll be in most of the time) so it really was just a stylistic choice to leave it there. Can change it if anyone feels strongly about it

src/cascadia/TerminalConnection/AzureConnection.cpp

carlos-zamora

Make sure we have the Copyright header at the top of these new files. I think there's some missing.

Also, GitHub gave me a super not-useful diff on App.cpp and WindowsTerminal.vcxproj. I'll try and get around to those sometime, but I think there aren't many changes there right?

src/cascadia/CascadiaPackage/CascadiaPackage.wapproj

src/cascadia/TerminalApp/CascadiaSettings.cpp

src/cascadia/TerminalConnection/AzureClientID.h

carlos-zamora · 2019-07-23T15:50:38Z

src/cascadia/TerminalConnection/AzureConnection.cpp

+    // - the handler
+    // Return value:
+    // - the event token for the handler
+    winrt::event_token AzureConnection::TerminalOutput(Microsoft::Terminal::TerminalConnection::TerminalOutputEventArgs const& handler)


Yeah. They're in src\cascadia\inc\cppwinrt_utils.h

src/cascadia/TerminalConnection/AzureConnection.cpp

miniksa · 2019-07-24T20:46:40Z

I still don't see notes anywhere about why ARM64 is compiled out.

I know why, we talked about it. I just want to see it commented in the code.

DHowett-MSFT

Couple quick questions, but I wouldn't block on them.

src/cascadia/CascadiaPackage/CascadiaPackage.wapproj

DHowett-MSFT · 2019-07-25T05:39:07Z

src/cascadia/TerminalConnection/AzureConnection.cpp

+        _HeaderHelper(shellRequest);
+        const auto innerBody = json::value::parse(U("{ \"osType\" : \"linux\" }"));
+        json::value body;
+        body[U("properties")] = innerBody;


curious why you parsed json to get the inner value, then manually created the outer value. you could manually create both, or parse json ({"properties":{"osType":"linux"}}) for both 😄

src/cascadia/TerminalConnection/AzureConnection.cpp

DHowett-MSFT · 2019-07-25T05:41:04Z

src/cascadia/TerminalConnection/AzureConnection.cpp

+        refreshRequest.set_request_uri(_tenantID + L"/oauth2/token");
+        const auto body = L"client_id=" + AzureClientID + L"&resource=" + _wantedResource + L"&grant_type=refresh_token" + L"&refresh_token=" + _refreshToken;
+        refreshRequest.set_body(body, L"application/x-www-form-urlencoded");
+        refreshRequest.headers().add(L"User-Agent", resource);


We may need a better user agent than Terminal; perhaps we can get it from the package version (like App.cpp does?)

miniksa · 2019-07-25T16:53:36Z

#2096 will remove the static analysis build here.

miniksa · 2019-07-25T17:45:04Z

/azp run

azure-pipelines · 2019-07-25T17:45:31Z

Azure Pipelines successfully started running 1 pipeline(s).

* We can now connect to the Azure cloud shell microsoft#1235

ghost · 2019-08-03T01:49:40Z

🎉Windows Terminal Preview v0.3.2142.0 has been released which incorporates this pull request.:tada:

Handy links:

Pankaj Bhojwani added 4 commits June 27, 2019 10:26

App now initializes the connection instead of term control.

e9ead80

formatting changes

091a4cb

Azure cloud shell connector microsoft#1235

4b4a984

Merge branch 'master' of https://github.com/microsoft/terminal into r…

3c05858

…ealAzCon

PankajBhojwani changed the title ~~The Azure cloud shell connector is here!~~ The Azure cloud shell connector Jul 3, 2019

split the dependency package into two, one for x86 one for x64

10b7b57

miniksa requested changes Jul 5, 2019

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Jul 5, 2019

zadjii-msft reviewed Jul 8, 2019

View reviewed changes

src/cascadia/TerminalConnection/AzureConnection.cpp Outdated Show resolved Hide resolved

Pankaj Bhojwani added 4 commits July 8, 2019 16:48

made changes based on reviewer comments and made the dependency packa…

8efcc87

…ge smaller

formatting changes

ddb857d

the package dependency is even smaller now

e41221f

package should work now

a810d69

Pankaj Bhojwani added 4 commits July 11, 2019 16:12

1. Significant restructuring - it now behaves like a state machine; 2…

5a031b9

…. The connector now has the ability to store tokens for future logins

formatting fixes

4885fb6

Merge remote-tracking branch 'origin' into realAzCon

08e787b

removed the package (we have a devops feed now) and made some minor p…

9a27a16

…rinting/comments changes

miniksa requested changes Jul 17, 2019

View reviewed changes

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 17, 2019

made requested changes, added a GUID for connection type, added helpe…

14e12e2

…r to App to create connection, arm64 *should* build now

miniksa reviewed Jul 22, 2019

View reviewed changes

zadjii-msft approved these changes Jul 23, 2019

View reviewed changes

carlos-zamora reviewed Jul 23, 2019

View reviewed changes

Pankaj Bhojwani added 2 commits July 23, 2019 09:59

made most of the requested changes

27f8eac

Merge remote-tracking branch 'origin/master' into realAzCon

a791993

PankajBhojwani force-pushed the realAzCon branch from 93ddc19 to a791993 Compare July 23, 2019 18:41

minor edits to address requested changes

3182557

Pankaj Bhojwani added 2 commits July 24, 2019 13:59

added comments for why we compile out ARM64

ff3033a

added copyright headers

213e442

miniksa approved these changes Jul 24, 2019

View reviewed changes

DHowett-MSFT approved these changes Jul 25, 2019

View reviewed changes

mcpiroman mentioned this pull request Jul 25, 2019

Handle connection's state #2091

Closed

5 tasks

Pankaj Bhojwani added 2 commits July 25, 2019 12:55

minor edits

cec13d4

Merge branch 'master' into realAzCon

7d2630f

PankajBhojwani merged commit 63347f4 into microsoft:master Jul 25, 2019

PankajBhojwani deleted the realAzCon branch July 25, 2019 20:33

mcpiroman pushed a commit to mcpiroman/terminal that referenced this pull request Jul 26, 2019

The Azure cloud shell connector (microsoft#1808)

4846d33

* We can now connect to the Azure cloud shell microsoft#1235

ghost mentioned this pull request Aug 3, 2019

Feature Request: Azure cloud shell connector #1235

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

The Azure cloud shell connector #1808

The Azure cloud shell connector #1808

PankajBhojwani commented Jul 3, 2019

miniksa left a comment

miniksa Jul 5, 2019

shayne Aug 8, 2019

DHowett-MSFT Aug 8, 2019

ras0219-msft commented Jul 6, 2019

zadjii-msft left a comment

miniksa commented Jul 10, 2019

miniksa commented Jul 10, 2019

miniksa Jul 17, 2019

zadjii-msft left a comment

zadjii-msft Jul 23, 2019

PankajBhojwani Jul 23, 2019

zadjii-msft Jul 23, 2019

carlos-zamora Jul 23, 2019

PankajBhojwani Jul 23, 2019

zadjii-msft Jul 23, 2019

PankajBhojwani Jul 23, 2019

carlos-zamora left a comment

carlos-zamora Jul 23, 2019

miniksa commented Jul 24, 2019

DHowett-MSFT left a comment

DHowett-MSFT Jul 25, 2019

DHowett-MSFT Jul 25, 2019

miniksa commented Jul 25, 2019

miniksa commented Jul 25, 2019

azure-pipelines bot commented Jul 25, 2019

ghost commented Aug 3, 2019

The Azure cloud shell connector #1808

The Azure cloud shell connector #1808

Conversation

PankajBhojwani commented Jul 3, 2019

PR Checklist

miniksa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ras0219-msft commented Jul 6, 2019

zadjii-msft left a comment

Choose a reason for hiding this comment

miniksa commented Jul 10, 2019

miniksa commented Jul 10, 2019

Choose a reason for hiding this comment

zadjii-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlos-zamora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miniksa commented Jul 24, 2019

DHowett-MSFT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miniksa commented Jul 25, 2019

miniksa commented Jul 25, 2019

azure-pipelines bot commented Jul 25, 2019

ghost commented Aug 3, 2019