-
Notifications
You must be signed in to change notification settings - Fork 644
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
Allow tests to run against URLs not matching "nuget.org" pattern #3489
Conversation
pieces = new List<string> | ||
{ | ||
"8c11c16610b7a147d10bbcc6a65ce23d321c12c2", // *.nugettest.org | ||
"9d984f91f40d8b3a1fb29153179415523c4e64d1", // *.int.nugettest.org |
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.
Can you please add a comment about why this is here, and that it will brake if certs are renewed
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.
Fixed.
} | ||
else | ||
{ | ||
|
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.
extra line
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.
Fixed.
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.
l_g_t_m
@@ -156,7 +156,13 @@ public CommandlineHelper(ITestOutputHelper testOutputHelper) | |||
var nugetProcess = new Process(); | |||
var pathToNugetExe = Path.Combine(Environment.CurrentDirectory, NugetExePath); | |||
|
|||
foreach (var trustedCertificate in EnvironmentSettings.TrustedHttpsCertificates) | |||
{ | |||
arguments.AddRange(new[] { "-TrustedHttpsCertificate", trustedCertificate }); |
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 a big deal, but I'd expect to see a single semicolon-delimited list rather than a series of -TrustedHttpsCertificate
arguments...
-TrustedHttpsCertificate thumb1;thumb2;thumb3
rather than
-TrustedHttpsCertificate thumb1 -TrustedHttpsCertificate thumb2 -TrustedHttpsCertificate thumb3
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.
Yeah, that would be more terse, but per the feature request mentioned above, I will likely configure this setting in NuGet.config and maybe an environment variable that nuget.exe expects.
Copy-Item $sourceNuGetExePath $targetNugetExePath | ||
} else { | ||
Invoke-WebRequest $sourceNugetExeUrl -OutFile $targetNugetExePath |
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.
maybe should add -UseBasicParsing
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.
-UseBasicParsing
only applies to HTML responses. This is a binary.
{ | ||
"8c11c16610b7a147d10bbcc6a65ce23d321c12c2", // *.nugettest.org | ||
"9d984f91f40d8b3a1fb29153179415523c4e64d1", // *.int.nugettest.org | ||
"3751cb513b93ee67ec9f18a1f2aec1eac87af9bc" // *.nuget.org |
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 will need to be changed for prod deployment, it will likely fail for prod staging functional tests, because it will use the new cert.
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.
Yes, intentional. I would like to exercise this test failure case. Thanks for the heads up.
Addresses #3483.
When testing the staging instance before deploying the gallery, the URL being tested is some random string (dot) cloudapp (dot) .net. This URL does not match the SSL certificate so nuget.exe rejects pushing.
This change adds a custom build of nuget.exe based off of this branch:
https://github.com/NuGet/NuGet.Client/tree/dev-jver-trust-https
This nuget.exe has a feature to trust the fingerprint (a.k.a. thumbprint) of specific certificates.
In short, this PR:
As a side note, the feature request for nuget.exe supporting this certificate trust option is here:
NuGet/Home#4387