-
Notifications
You must be signed in to change notification settings - Fork 449
Removing Octokit #611
Removing Octokit #611
Conversation
9a312c9 to
76aaaef
Compare
# Conflicts: # src/GitHub.Api/Authentication/LoginManager.cs # src/UnityExtension/Assets/Editor/GitHub.Unity/GitHub.Unity.csproj
| { | ||
| command.Append(" -d "); | ||
| command.Append(newRepository.Description); | ||
| command.Append(description); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs quotes around it for the command line to parse properly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in #599
| { | ||
| var executingAssembly = typeof(ApplicationConfiguration).Assembly; | ||
| AssemblyName = executingAssembly.GetName(); | ||
| ProductHeader = new ProductHeaderValue(ApplicationInfo.ApplicationSafeName, AssemblyName.Version.ToString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not related directly to this PR, but you should make sure this information is sent along with the requests in the node app, so you might need to set it in the environment as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tracked in #617
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be fixed in #599
| return new LoginResultData(LoginResultCodes.Failed, Localization.LoginFailed, host); | ||
| } | ||
| await keychain.Clear(host, false); | ||
| return new LoginResultData(LoginResultCodes.Failed, Localization.LoginFailed, host); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is missing the other error conditions like locked out and the workaround for enterprise, isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Conflicts: # script # src/GitHub.Api/Application/ApiClient.cs # src/GitHub.Api/Authentication/LoginManager.cs
This targets #599 and does the work of removing Octokit from the application.
This includes libraries and a special solution used for debugging Octokit.
The majority of code changes to the application are removing
IGitHubClientand credentials objects.