Skip to content

Commit c2ed93b

Browse files
derrickstoleederrickstolee
authored
Merge pull request #381: Remove some leftover pain points from VFS for Git
Scalar was created by forking the VFS for Git codebase, deleting a bunch of code, then renaming a bunch of files. It has since diverged significantly through many refactors. One thing that has _never worked_ is the `scalar upgrade` verb when using GitHub as the source. The reason is that we package our releases differently than VFS for Git does, so the upgrader fails to find the Git installer and Scalar installer (instead, we have two `.zip` files, one for each platform). Since we are moving to another system for the typical upgrade pattern, we should just delete the GitHub upgrader. The `scalar upgrade` verb will remain for the NuGet feed, when configured. This resolves #351. An issue was found in VFS for Git where the service would restart constantly when checking for an upgrade. The root cause was a problematic `Environment.Exit()` call. See microsoft/VFSForGit#1668 for an equivalent change. Another annoying thing is the verbose output during `scalar clone` that repeats the given options. In the case of a non-GVFS protocol clone, some of these just don't make sense. Delete this output for clarity. Finally, `scalar clone` failed miserably when using a non-standard URL such as `file://`. Fix that by doing some URL scanning in advance. If the URL does not start with `http://` or `https://`, we will not even try the GVFS protocol and will move on to a native Git clone. We also avoid any protocol prefix (`X://`) that is not `http://`, `https://` or `ssh://`. For SSH, the protocol prefix is not required, so we do not require a protocol prefix. Resolves #378. Since this is a grab-bag of things, please review commit-by-commit. For the GitHub upgrader removal, consider ignoring whitespace changes.
2 parents 11d3a24 + 275fa89 commit c2ed93b

10 files changed

Lines changed: 70 additions & 1232 deletions

File tree

‎Scalar.Common/ProductUpgrader.cs‎

Lines changed: 44 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -92,68 +92,62 @@ public static bool TryCreateUpgrader(
9292
bool containsUpgradePackageName = entries.ContainsKey(ScalarConstants.LocalScalarConfig.UpgradeFeedPackageName);
9393
bool containsOrgInfoServerUrl = entries.ContainsKey(ScalarConstants.LocalScalarConfig.OrgInfoServerUrl);
9494

95-
if (containsUpgradeFeedUrl || containsUpgradePackageName)
95+
if (!containsUpgradeFeedUrl && !containsUpgradePackageName)
9696
{
97-
// We are configured for NuGet - determine if we are using OrgNuGetUpgrader or not
98-
if (containsOrgInfoServerUrl)
97+
error = "Custom upgrade feed is not configured";
98+
tracer.RelatedWarning(error);
99+
newUpgrader = null;
100+
return false;
101+
}
102+
103+
// We are configured for NuGet - determine if we are using OrgNuGetUpgrader or not
104+
if (containsOrgInfoServerUrl)
105+
{
106+
if (OrgNuGetUpgrader.TryCreate(
107+
tracer,
108+
fileSystem,
109+
scalarConfig,
110+
new HttpClient(),
111+
credentialStore,
112+
dryRun,
113+
noVerify,
114+
out OrgNuGetUpgrader orgNuGetUpgrader,
115+
out error))
99116
{
100-
if (OrgNuGetUpgrader.TryCreate(
101-
tracer,
102-
fileSystem,
103-
scalarConfig,
104-
new HttpClient(),
105-
credentialStore,
106-
dryRun,
107-
noVerify,
108-
out OrgNuGetUpgrader orgNuGetUpgrader,
109-
out error))
110-
{
111-
// We were successfully able to load a NuGetUpgrader - use that.
112-
newUpgrader = orgNuGetUpgrader;
113-
return true;
114-
}
115-
else
116-
{
117-
tracer.RelatedError($"{nameof(TryCreateUpgrader)}: Could not create organization based upgrader. {error}");
118-
newUpgrader = null;
119-
return false;
120-
}
117+
// We were successfully able to load a NuGetUpgrader - use that.
118+
newUpgrader = orgNuGetUpgrader;
119+
return true;
121120
}
122121
else
123122
{
124-
if (NuGetUpgrader.TryCreate(
125-
tracer,
126-
fileSystem,
127-
scalarConfig,
128-
credentialStore,
129-
dryRun,
130-
noVerify,
131-
out NuGetUpgrader nuGetUpgrader,
132-
out bool isConfigured,
133-
out error))
134-
{
135-
// We were successfully able to load a NuGetUpgrader - use that.
136-
newUpgrader = nuGetUpgrader;
137-
return true;
138-
}
139-
else
140-
{
141-
tracer.RelatedError($"{nameof(TryCreateUpgrader)}: Could not create NuGet based upgrader. {error}");
142-
newUpgrader = null;
143-
return false;
144-
}
123+
tracer.RelatedError($"{nameof(TryCreateUpgrader)}: Could not create organization based upgrader. {error}");
124+
newUpgrader = null;
125+
return false;
145126
}
146127
}
147128
else
148129
{
149-
newUpgrader = GitHubUpgrader.Create(tracer, fileSystem, scalarConfig, dryRun, noVerify, out error);
150-
if (newUpgrader == null)
130+
if (NuGetUpgrader.TryCreate(
131+
tracer,
132+
fileSystem,
133+
scalarConfig,
134+
credentialStore,
135+
dryRun,
136+
noVerify,
137+
out NuGetUpgrader nuGetUpgrader,
138+
out bool isConfigured,
139+
out error))
151140
{
152-
tracer.RelatedError($"{nameof(TryCreateUpgrader)}: Could not create GitHub based upgrader. {error}");
141+
// We were successfully able to load a NuGetUpgrader - use that.
142+
newUpgrader = nuGetUpgrader;
143+
return true;
144+
}
145+
else
146+
{
147+
tracer.RelatedError($"{nameof(TryCreateUpgrader)}: Could not create NuGet based upgrader. {error}");
148+
newUpgrader = null;
153149
return false;
154150
}
155-
156-
return true;
157151
}
158152
}
159153

‎Scalar.Service/ProductUpgradeTimer.cs‎

Lines changed: 1 addition & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,10 @@
11
using Scalar.Common;
22
using Scalar.Common.FileSystem;
33
using Scalar.Common.NamedPipes;
4-
using Scalar.Common.NuGetUpgrade;
54
using Scalar.Common.Tracing;
65
using Scalar.Service.Handlers;
76
using Scalar.Upgrader;
87
using System;
9-
using System.IO;
108
using System.Threading;
119

1210
namespace Scalar.Service
@@ -113,57 +111,6 @@ private void TimerCallback(object unusedState)
113111
return;
114112
}
115113

116-
if (!productUpgrader.SupportsAnonymousVersionQuery)
117-
{
118-
// If this is a NuGetUpgrader that does not support anonymous version query,
119-
// fall back to using the GitHubUpgrader, to preserve existing behavior.
120-
// Once we have completely transitioned to using the anonymous endpoint,
121-
// we can remove this code.
122-
if (productUpgrader is NuGetUpgrader)
123-
{
124-
productUpgrader = GitHubUpgrader.Create(
125-
this.tracer,
126-
this.fileSystem,
127-
new LocalScalarConfig(),
128-
dryRun: false,
129-
noVerify: false,
130-
error: out errorMessage);
131-
132-
if (productUpgrader == null)
133-
{
134-
string gitHubUpgraderFailedMessage = string.Format(
135-
"{0}.{1}: GitHubUpgrader.Create failed to create upgrader: {2}",
136-
nameof(ProductUpgradeTimer),
137-
nameof(this.TimerCallback),
138-
errorMessage);
139-
140-
activity.RelatedWarning(
141-
metadata: new EventMetadata(),
142-
message: gitHubUpgraderFailedMessage,
143-
keywords: Keywords.Telemetry);
144-
145-
info.RecordHighestAvailableVersion(highestAvailableVersion: null);
146-
return;
147-
}
148-
}
149-
else
150-
{
151-
errorMessage = string.Format(
152-
"{0}.{1}: Configured Product Upgrader does not support anonymous version queries.",
153-
nameof(ProductUpgradeTimer),
154-
nameof(this.TimerCallback),
155-
errorMessage);
156-
157-
activity.RelatedWarning(
158-
metadata: new EventMetadata(),
159-
message: errorMessage,
160-
keywords: Keywords.Telemetry);
161-
162-
info.RecordHighestAvailableVersion(highestAvailableVersion: null);
163-
return;
164-
}
165-
}
166-
167114
InstallerPreRunChecker prerunChecker = new InstallerPreRunChecker(this.tracer, string.Empty);
168115
if (!prerunChecker.TryRunPreUpgradeChecks(out errorMessage))
169116
{
@@ -220,22 +167,12 @@ private void TimerCallback(object unusedState)
220167

221168
this.DisplayUpgradeAvailableToast(newerVersion.ToString());
222169
}
223-
catch (Exception ex) when (
224-
ex is IOException ||
225-
ex is UnauthorizedAccessException ||
226-
ex is NotSupportedException)
170+
catch (Exception ex)
227171
{
228172
this.tracer.RelatedWarning(
229173
CreateEventMetadata(ex),
230174
"Exception encountered recording highest available version");
231175
}
232-
catch (Exception ex)
233-
{
234-
this.tracer.RelatedError(
235-
CreateEventMetadata(ex),
236-
"Unhanlded exception encountered recording highest available version");
237-
Environment.Exit((int)ReturnCode.GenericError);
238-
}
239176
}
240177
}
241178

‎Scalar.UnitTests/Common/TryCreateProductUpgraderTests.cs‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -88,9 +88,9 @@ public void CreatesNuGetUpgraderWhenConfiguredWithNoRing()
8888
}
8989

9090
[TestCase]
91-
public void CreatesGitHubUpgraderWhenConfigured()
91+
public void FailsWithDefaultConfiguration()
9292
{
93-
MockLocalScalarConfig scalarConfig = this.ConstructDefaultGitHubConfigBuilder()
93+
MockLocalScalarConfig scalarConfig = this.ConstructDefaultConfigBuilder()
9494
.Build();
9595

9696
bool success = ProductUpgrader.TryCreateUpgrader(
@@ -103,10 +103,9 @@ public void CreatesGitHubUpgraderWhenConfigured()
103103
out ProductUpgrader productUpgrader,
104104
out string error);
105105

106-
success.ShouldBeTrue();
107-
productUpgrader.ShouldNotBeNull();
108-
productUpgrader.ShouldBeOfType<GitHubUpgrader>();
109-
error.ShouldBeNull();
106+
success.ShouldBeFalse();
107+
productUpgrader.ShouldBeNull();
108+
error.ShouldNotBeNull();
110109
}
111110

112111
[TestCase]
@@ -240,7 +239,7 @@ private MockLocalScalarConfigBuilder ConstructDefaultMockOrgNuGetConfigBuilder()
240239
return configBuilder;
241240
}
242241

243-
private MockLocalScalarConfigBuilder ConstructDefaultGitHubConfigBuilder()
242+
private MockLocalScalarConfigBuilder ConstructDefaultConfigBuilder()
244243
{
245244
MockLocalScalarConfigBuilder configBuilder = this.ConstructMockLocalScalarConfigBuilder()
246245
.WithUpgradeRing();

0 commit comments

Comments
 (0)