Merge branch 'fixes/octorun-output-refactoring' into stanley/0.31-rc

StanleyGoldman · StanleyGoldman · commit d8f0c37b2773 · 2018-04-02T10:53:14.000-04:00
diff --git a/octorun/src/authentication.js b/octorun/src/authentication.js
@@ -2,9 +2,7 @@ var endOfLine = require('os').EOL;
 var config = require("./configuration");
 var octokitWrapper = require("./octokit");
 
-var lockedRegex = new RegExp("number of login attempts exceeded", "gi");
 var twoFactorRegex = new RegExp("must specify two-factor authentication otp code", "gi");
-var badCredentialsRegex = new RegExp("bad credentials", "gi");
 
 var scopes = ["user", "repo", "gist", "write:public_key"];
 
@@ -48,12 +46,6 @@ var handleAuthentication = function (username, password, onSuccess, onFailure, t
             else if (twoFactorRegex.test(err.message)) {
                 onSuccess(password, "2fa");
             }
-            else if (lockedRegex.test(err.message)) {
-                onFailure("locked")
-            }
-            else if (badCredentialsRegex.test(err.message)) {
-                onFailure("badcredentials")
-            }
             else {
                 onFailure(err)
             }
diff --git a/octorun/src/bin/app-login.js b/octorun/src/bin/app-login.js
@@ -13,15 +13,12 @@ var handleAuthentication = function (username, password, twoFactor) {
     authentication.handleAuthentication(username, password, function (token, status) {
         if (status) {
             output.custom(status, token);
-            process.exit();
         }
         else {
             output.success(token);
-            process.exit();
         }
     }, function (error) {
         output.error(error);
-        process.exit();
     }, twoFactor);
 }
 
@@ -41,7 +38,6 @@ if (commander.twoFactor) {
         }
         catch (error) {
             output.error(error);
-            process.exit();
         }
     }
     else {
@@ -108,7 +104,6 @@ else {
             }
             catch (error) {
                 output.error(error);
-                process.exit();
             }
         });
     }
diff --git a/octorun/src/bin/app-organizations.js b/octorun/src/bin/app-organizations.js
@@ -14,21 +14,18 @@ try {
     apiWrapper.getOrgs(function (error, result) {
         if (error) {
             output.error(error);
-            process.exit();
         }
         else {
-            results = [];
+            var results = [];
             for (var i = 0; i < result.length; i++) {
                 results.push(result[i].name);
                 results.push(result[i].login);
             }
 
             output.success(results);
-            process.exit();
         }
     });
 }
 catch (error) {
     output.error(error);
-    process.exit();
 }
diff --git a/octorun/src/bin/app-publish.js b/octorun/src/bin/app-publish.js
@@ -2,6 +2,7 @@ var commander = require("commander");
 var package = require('../../package.json')
 var ApiWrapper = require('../api')
 var endOfLine = require('os').EOL;
+var output = require('../output');
 
 commander
     .version(package.version)
@@ -13,11 +14,8 @@ commander
 
 if(!commander.repository)
 {
-    process.stdout.write("repository required");
-    process.stdout.write(endOfLine);
     commander.help();
     process.exit(-1);
-    return;
 }
 
 var private = false;
@@ -31,46 +29,13 @@ try {
     apiWrapper.publish(commander.repository, commander.description, private, commander.organization,
         function (error, result) {
             if (error) {
-                process.stdout.write("error");
-                process.stdout.write(endOfLine);
-
-                process.stdout.write("");
-                process.stdout.write(endOfLine);
-
-                process.stdout.write("");
-                process.stdout.write(endOfLine);
-
-                if (error) {
-                    process.stdout.write(error.toString());
-                    process.stdout.write(endOfLine);
-                }
-
-                process.exit();
+                output.error(error);
             }
             else {
-                process.stdout.write("success");
-                process.stdout.write(endOfLine);
-
-                process.stdout.write(commander.repository);
-                process.stdout.write(endOfLine);
-
-                process.stdout.write(result);
-                process.stdout.write(endOfLine);
-                process.exit();
+                output.success(commander.repository);
             }
         });
 }
 catch (error) {
-    process.stdout.write("error");
-    process.stdout.write(endOfLine);
-
-    process.stdout.write("");
-    process.stdout.write(endOfLine);
-
-    if (error) {
-        process.stdout.write(error.toString());
-        process.stdout.write(endOfLine);
-    }
-
-    process.exit();
+    output.error(error);
 }
diff --git a/octorun/src/bin/app-usage.js b/octorun/src/bin/app-usage.js
@@ -3,6 +3,7 @@ var package = require('../../package.json')
 var endOfLine = require('os').EOL;
 var fs = require('fs');
 var util = require('util');
+var output = require('../output');
 
 commander
     .version(package.version)
@@ -50,38 +51,20 @@ if (fileContents && host) {
 
         res.on('data', function (d) {
             if (success) {
-                process.stdout.write("success");
-                process.stdout.write(endOfLine);
-                process.stdout.write(d);
-                process.stdout.write(endOfLine);
+                output.custom("success", d, true);
             }
             else {
-                process.stdout.write("error");
-                process.stdout.write(endOfLine);
-                
-                process.stdout.write("");
-                process.stdout.write(endOfLine);
-                
-                process.stdout.write(d);
-                process.stdout.write(endOfLine);
+                output.custom("error", "", true);
             }
         });
 
         res.on('end', function (d) {
-            process.exit(success ? 0 : -1);
+            process.exit();
         });
     });
 
     req.on('error', function (error) {
-        process.stdout.write("Error");
-        process.stdout.write(endOfLine);
-        
-        if (error) {
-            process.stdout.write(error.toString());
-            process.stdout.write(endOfLine);
-        }
-
-        process.exit(-1);
+        output.error(error);
     });
 
     req.write(fileContents);
diff --git a/octorun/src/output.js b/octorun/src/output.js
@@ -1,6 +1,6 @@
 var endOfLine = require('os').EOL;
 
-var outputResult = function (status, results, errors) {
+var outputResult = function (status, results, errors, preventExit) {
     process.stdout.write(status);
     process.stdout.write(endOfLine);
 
@@ -44,19 +44,27 @@ var outputResult = function (status, results, errors) {
                 process.stdout.write(endOfLine);
             }
         }
+        else if (errors.toString) {
+            process.stdout.write(errors.toString());
+            process.stdout.write(endOfLine);
+        }        
         else {
             process.stdout.write(errors);
             process.stdout.write(endOfLine);
         }
     }
+
+    if(!preventExit) {
+        process.exit();
+    }
 }
 
 var outputSuccess = function (results) {
     outputResult("success", results);
 }
 
-var outputCustom = function (status, results) {
-    outputResult(status, results);
+var outputCustom = function (status, results, preventExit) {
+    outputResult(status, results, undefined, preventExit);
 }
 
 var outputError = function (errors) {
diff --git a/octorun/version b/octorun/version
@@ -1 +1 @@
-b91b7b60
+196e9867
diff --git a/src/GitHub.Api/Application/ApiClient.cs b/src/GitHub.Api/Application/ApiClient.cs
@@ -247,12 +247,7 @@ private async Task<GitHubRepository> CreateRepositoryInternal(string repositoryN
                     };
                 }
 
-                if (ret.Output.Any())
-                {
-                    throw new ApiClientException(string.Join(Environment.NewLine, ret.Output));
-                }
-
-                throw new ApiClientException("Publish failed");
+                throw new ApiClientException(ret.GetApiErrorMessage() ?? "Publish failed");
             }
             catch (Exception ex)
             {
@@ -294,12 +289,7 @@ private async Task GetOrganizationInternal(Action<Organization[]> onSuccess, Act
                     return;
                 }
 
-                if (ret.Output.Any())
-                {
-                    throw new ApiClientException(string.Join(Environment.NewLine, ret.Output));
-                }
-
-                throw new ApiClientException("Error getting organizations");
+                throw new ApiClientException(ret.GetApiErrorMessage() ?? "Error getting organizations");
             }
             catch (Exception ex)
             {
@@ -332,12 +322,7 @@ private async Task<GitHubUser> GetCurrentUserInternal()
                     };
                 }
 
-                if (ret.Output.Any())
-                {
-                    throw new ApiClientException(string.Join(Environment.NewLine, ret.Output));
-                }
-
-                throw new ApiClientException("Error validating current user");
+                throw new ApiClientException(ret.GetApiErrorMessage() ?? "Error validating current user");
             }
             catch (KeychainEmptyException)
             {
diff --git a/src/GitHub.Api/Authentication/Keychain.cs b/src/GitHub.Api/Authentication/Keychain.cs
@@ -323,6 +323,6 @@ private void UpdateConnections(Connection[] conns)
         public Connection[] Connections => connections.Values.ToArray();
         public IList<UriString> Hosts => connections.Keys.ToArray();
         public bool HasKeys => connections.Any();
-        public bool NeedsLoad => HasKeys && !string.IsNullOrEmpty(FindOrCreateAdapter(connections.First().Value.Host).Credential.Token);
+        public bool NeedsLoad => HasKeys && !string.IsNullOrEmpty(FindOrCreateAdapter(connections.First().Value.Host).Credential?.Token);
     }
 }
diff --git a/src/GitHub.Api/Authentication/LoginManager.cs b/src/GitHub.Api/Authentication/LoginManager.cs
@@ -193,22 +193,7 @@ private async Task<LoginResultData> TryLogin(
                 return new LoginResultData(resultCodes, message, host, ret.Output[0]);
             }
 
-            if (ret.IsBadCredentials)
-            {
-                return new LoginResultData(LoginResultCodes.Failed, "Bad credentials.", host, ret.Output[0]);
-            }
-
-            if (ret.IsLocked)
-            {
-                return new LoginResultData(LoginResultCodes.LockedOut, "Account locked.", host, ret.Output[0]);
-            }
-
-            if (ret.Output.Any())
-            {
-                return new LoginResultData(LoginResultCodes.Failed, "Failed.", host, ret.Output[0]);
-            }
-
-            return new LoginResultData(LoginResultCodes.Failed, "Failed.", host);
+            return new LoginResultData(LoginResultCodes.Failed, ret.GetApiErrorMessage() ?? "Failed.", host);
         }
     }
 
diff --git a/src/GitHub.Api/Installer/OctorunInstaller.cs b/src/GitHub.Api/Installer/OctorunInstaller.cs
@@ -102,7 +102,7 @@ public class OctorunInstallDetails
             public const string DefaultZipMd5Url = "https://ghfvs-installer.github.com/unity/octorun/octorun.zip.md5";
             public const string DefaultZipUrl = "https://ghfvs-installer.github.com/unity/octorun/octorun.zip";
 
-            public const string PackageVersion = "b91b7b60";
+            public const string PackageVersion = "196e9867";
             private const string PackageName = "octorun";
             private const string zipFile = "octorun.zip";
 
diff --git a/src/GitHub.Api/Resources/octorun.zip b/src/GitHub.Api/Resources/octorun.zip
@@ -1,3 +1,3 @@
 version https://git-lfs.github.com/spec/v1
-oid sha256:dcdf06517450ccbad14e1bea863387bef84b24d3709eec50c371f918839606e6
-size 212554
+oid sha256:b88a832fae41b944798323b91a745909bbf7d083fdf2455e2ec71dd51030523a
+size 211853
diff --git a/src/GitHub.Api/Tasks/OctorunTask.cs b/src/GitHub.Api/Tasks/OctorunTask.cs
@@ -3,12 +3,13 @@
 using System.Diagnostics;
 using System.Linq;
 using System.Reflection;
+using System.Text.RegularExpressions;
 using System.Threading;
 using GitHub.Logging;
 
 namespace GitHub.Unity
 {
-    class OctorunTaskOutputProcessor : BaseOutputProcessor<OctorunResult>
+    class OctorunResultOutputProcessor : BaseOutputProcessor<OctorunResult>
     {
         private int lineCount;
         private string status;
@@ -68,7 +69,7 @@ public OctorunTask(CancellationToken token, NPath pathToNodeJs, NPath pathToOcto
             string user = null,
             string userToken = null,
             IOutputProcessor<OctorunResult> processor = null)
-            : base(token, processor ?? new OctorunTaskOutputProcessor())
+            : base(token, processor ?? new OctorunResultOutputProcessor())
         {
             this.clientId = clientId;
             this.clientSecret = clientSecret;
@@ -132,7 +133,20 @@ public OctorunResult(string status, string[] output)
         public bool IsSuccess => Status.Equals("success", StringComparison.InvariantCultureIgnoreCase);
         public bool IsError => Status.Equals("error", StringComparison.InvariantCultureIgnoreCase);
         public bool IsTwoFactorRequired => Status.Equals("2fa", StringComparison.InvariantCultureIgnoreCase);
-        public bool IsLocked => Output.First().Equals("locked", StringComparison.InvariantCultureIgnoreCase);
-        public bool IsBadCredentials => Output.First().Equals("badcredentials", StringComparison.InvariantCultureIgnoreCase);
+    }
+
+    static class OctorunResultExtensions {
+        private static Regex ApiErrorMessageRegex = new Regex(@"\""message\"":\""(.*?)\""", RegexOptions.Compiled);
+
+        internal static string GetApiErrorMessage(this OctorunResult octorunResult)
+        {
+            if (!octorunResult.IsError || !octorunResult.Output.Any())
+            {
+                return null;
+            }
+
+            var match = ApiErrorMessageRegex.Match(octorunResult.Output[0]);
+            return match.Success ? match.Groups[1].Value : octorunResult.Output[0];
+        }
     }
 }
diff --git a/src/UnityExtension/Assets/Editor/GitHub.Unity/UI/GitPathView.cs b/src/UnityExtension/Assets/Editor/GitHub.Unity/UI/GitPathView.cs
@@ -253,7 +253,7 @@ private void ValidateAndSetGitInstallPath(string value)
                     }
                     else
                     {
-                        Logger.Warning("Software versions meet minimums Git:{0} GitLfs:{1}",
+                        Logger.Trace("Software versions meet minimums Git:{0} GitLfs:{1}",
                             result.GitVersion,
                             result.GitLfsVersion);
 

Original file line number	Diff line number	Diff line change
`@@ -13,15 +13,12 @@ var handleAuthentication = function (username, password, twoFactor) {`
`13`	`13`	`authentication.handleAuthentication(username, password, function (token, status) {`
`14`	`14`	`if (status) {`
`15`	`15`	`output.custom(status, token);`
`16`		`- process.exit();`
`17`	`16`	`}`
`18`	`17`	`else {`
`19`	`18`	`output.success(token);`
`20`		`- process.exit();`
`21`	`19`	`}`
`22`	`20`	`}, function (error) {`
`23`	`21`	`output.error(error);`
`24`		`- process.exit();`
`25`	`22`	`}, twoFactor);`
`26`	`23`	`}`
`27`	`24`
`@@ -41,7 +38,6 @@ if (commander.twoFactor) {`
`41`	`38`	`}`
`42`	`39`	`catch (error) {`
`43`	`40`	`output.error(error);`
`44`		`- process.exit();`
`45`	`41`	`}`
`46`	`42`	`}`
`47`	`43`	`else {`
`@@ -108,7 +104,6 @@ else {`
`108`	`104`	`}`
`109`	`105`	`catch (error) {`
`110`	`106`	`output.error(error);`
`111`		`- process.exit();`
`112`	`107`	`}`
`113`	`108`	`});`
`114`	`109`	`}`
Original file line number	Diff line number	Diff line change
`@@ -14,21 +14,18 @@ try {`
`14`	`14`	`apiWrapper.getOrgs(function (error, result) {`
`15`	`15`	`if (error) {`
`16`	`16`	`output.error(error);`
`17`		`- process.exit();`
`18`	`17`	`}`
`19`	`18`	`else {`
`20`		`- results = [];`
	`19`	`+ var results = [];`
`21`	`20`	`for (var i = 0; i < result.length; i++) {`
`22`	`21`	`results.push(result[i].name);`
`23`	`22`	`results.push(result[i].login);`
`24`	`23`	`}`
`25`	`24`
`26`	`25`	`output.success(results);`
`27`		`- process.exit();`
`28`	`26`	`}`
`29`	`27`	`});`
`30`	`28`	`}`
`31`	`29`	`catch (error) {`
`32`	`30`	`output.error(error);`
`33`		`- process.exit();`
`34`	`31`	`}`
Original file line number	Diff line number	Diff line change
`@@ -1,6 +1,6 @@`
`1`	`1`	`var endOfLine = require('os').EOL;`
`2`	`2`
`3`		`-var outputResult = function (status, results, errors) {`
	`3`	`+var outputResult = function (status, results, errors, preventExit) {`
`4`	`4`	`process.stdout.write(status);`
`5`	`5`	`process.stdout.write(endOfLine);`
`6`	`6`
`@@ -44,19 +44,27 @@ var outputResult = function (status, results, errors) {`
`44`	`44`	`process.stdout.write(endOfLine);`
`45`	`45`	`}`
`46`	`46`	`}`
	`47`	`+ else if (errors.toString) {`
	`48`	`+ process.stdout.write(errors.toString());`
	`49`	`+ process.stdout.write(endOfLine);`
	`50`	`+ }`
`47`	`51`	`else {`
`48`	`52`	`process.stdout.write(errors);`
`49`	`53`	`process.stdout.write(endOfLine);`
`50`	`54`	`}`
`51`	`55`	`}`
	`56`	`+`
	`57`	`+ if(!preventExit) {`
	`58`	`+ process.exit();`
	`59`	`+ }`
`52`	`60`	`}`
`53`	`61`
`54`	`62`	`var outputSuccess = function (results) {`
`55`	`63`	`outputResult("success", results);`
`56`	`64`	`}`
`57`	`65`
`58`		`-var outputCustom = function (status, results) {`
`59`		`- outputResult(status, results);`
	`66`	`+var outputCustom = function (status, results, preventExit) {`
	`67`	`+ outputResult(status, results, undefined, preventExit);`
`60`	`68`	`}`
`61`	`69`
`62`	`70`	`var outputError = function (errors) {`
Original file line number	Diff line number	Diff line change
`@@ -247,12 +247,7 @@ private async Task<GitHubRepository> CreateRepositoryInternal(string repositoryN`
`247`	`247`	`};`
`248`	`248`	`}`
`249`	`249`
`250`		`- if (ret.Output.Any())`
`251`		`- {`
`252`		`- throw new ApiClientException(string.Join(Environment.NewLine, ret.Output));`
`253`		`- }`
`254`		`-`
`255`		`- throw new ApiClientException("Publish failed");`
	`250`	`+ throw new ApiClientException(ret.GetApiErrorMessage() ?? "Publish failed");`
`256`	`251`	`}`
`257`	`252`	`catch (Exception ex)`
`258`	`253`	`{`
`@@ -294,12 +289,7 @@ private async Task GetOrganizationInternal(Action<Organization[]> onSuccess, Act`
`294`	`289`	`return;`
`295`	`290`	`}`
`296`	`291`
`297`		`- if (ret.Output.Any())`
`298`		`- {`
`299`		`- throw new ApiClientException(string.Join(Environment.NewLine, ret.Output));`
`300`		`- }`
`301`		`-`
`302`		`- throw new ApiClientException("Error getting organizations");`
	`292`	`+ throw new ApiClientException(ret.GetApiErrorMessage() ?? "Error getting organizations");`
`303`	`293`	`}`
`304`	`294`	`catch (Exception ex)`
`305`	`295`	`{`
`@@ -332,12 +322,7 @@ private async Task<GitHubUser> GetCurrentUserInternal()`
`332`	`322`	`};`
`333`	`323`	`}`
`334`	`324`
`335`		`- if (ret.Output.Any())`
`336`		`- {`
`337`		`- throw new ApiClientException(string.Join(Environment.NewLine, ret.Output));`
`338`		`- }`
`339`		`-`
`340`		`- throw new ApiClientException("Error validating current user");`
	`325`	`+ throw new ApiClientException(ret.GetApiErrorMessage() ?? "Error validating current user");`
`341`	`326`	`}`
`342`	`327`	`catch (KeychainEmptyException)`
`343`	`328`	`{`
Original file line number	Diff line number	Diff line change
`@@ -323,6 +323,6 @@ private void UpdateConnections(Connection[] conns)`
`323`	`323`	`public Connection[] Connections => connections.Values.ToArray();`
`324`	`324`	`public IList<UriString> Hosts => connections.Keys.ToArray();`
`325`	`325`	`public bool HasKeys => connections.Any();`
`326`		`- public bool NeedsLoad => HasKeys && !string.IsNullOrEmpty(FindOrCreateAdapter(connections.First().Value.Host).Credential.Token);`
	`326`	`+ public bool NeedsLoad => HasKeys && !string.IsNullOrEmpty(FindOrCreateAdapter(connections.First().Value.Host).Credential?.Token);`
`327`	`327`	`}`
`328`	`328`	`}`