-
Notifications
You must be signed in to change notification settings - Fork 526
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
Uploading packages as multiform content type #651
Conversation
Uploading packages as multiform content type
thanks |
let Push maxTrials url apiKey packageFileName = | ||
let rec push trial = | ||
tracefn "Pushing package %s to %s - trial %d" packageFileName url trial | ||
try | ||
let client = Utils.createWebClient(url, None) | ||
client.Headers.Add("X-NuGet-ApiKey", apiKey) | ||
client.UploadFile(Uri(url + "/api/v2/package"), "PUT", packageFileName) |
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.
Removing the explicit /api/v2/package
seems to be important in order to make it work for the proget feed.
I don't know how to test against nuget.
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.
we test nuget by dogfooding in paket. seems to work
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.
A pity, that should really be standard. Still, specifying it for nuget.org is a small price to pay for it working everywhere else...
ok it doesn't work for nuget.org. - the build doesn't complain, but the package is not listed https://www.nuget.org/packages/Paket/ |
@mavnn what hack do I have to commit to make it work? we already have special cases for the "download" part. I have no issues with special casing nuget.org |
If push url is nuget.org, add "/api/v2/package" to the end. Thing which makes me reluctant is that this should be the right endpoint for any server implementing the nuget rest interface. |
Also, we'll need to check why the push didn't fail when it didn't work. |
afd8e52 actually works. summoning our special nuget hero @maartenba |
MyGet needs the api/v2/package if it's not there too :) It should be standard ideally. |
so only proget doesn't want it? sounds strange |
Convention rather than standard... |
The reason for ProGet to be picky is that they have a weird way of creating their URLs: The short form is Force appending Now we could try to get ProGet to adhere to the conventions, but fact is that the nuget client have no problems with this. And as such, neither should Paket (following the principle of least surprise). As far as I can see, Nuget will add (from http://nuget.codeplex.com/SourceControl/latest#src/Core/Server/PackageServer.cs) private const string ServiceEndpoint = "/api/v2/package";
internal static Uri GetServiceEndpointUrl(Uri baseUrl, string path)
{
Uri requestUri;
if (String.IsNullOrEmpty(baseUrl.AbsolutePath.TrimStart('/')))
{
// If there's no host portion specified, append the url to the client.
requestUri = new Uri(baseUrl, ServiceEndpoint + '/' + path);
}
else
{
requestUri = new Uri(baseUrl, path);
}
return requestUri;
} And also the baseurl is redirect-expanded (is that even a word?): private Uri ResolveBaseUrl()
{
Uri uri;
try
{
var client = new RedirectedHttpClient(new Uri(Source));
uri = client.Uri;
}
catch (WebException ex)
{
var response = (HttpWebResponse)ex.Response;
if (response == null)
{
throw;
}
uri = response.ResponseUri;
}
return EnsureTrailingSlash(uri);
} I don't have time to keep shaving this particular yak, but this should be a good starting point should someone feel the urge.. |
I'll add api end point as an optional parameter later today (defaulting to the nuget convention). I'd rather not have too much "magic", but nuget.org should also just work™. I think the optional override makes the best compromise between the two. |
At some point we should also look at following redirects, but that's a bigger yak than I have time to shave today! |
My research yesterday indicated that |
@Vidarls that's good news. |
Tested against our ProGet feed, and it seems to fix #646
Implementation is rather crude, let me know if you see anything that needs to be polished.