Ability to upload to Generic Packages Repository#524
Uh oh!
There was an error while loading. Please reload this page.
Conversation
gep13 commented Sep 11, 2023
@meziantou I have opened this as draft, based on the comments that you have made here: I would love to get some feedback on what I have done here, and what changes/suggestions you could make. This is quite a naive implementation based on my own needs, and the information that I could find here: Thanks! |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
meziantou commented Sep 11, 2023
That's a good start! I've added some comments, but the shape is ok |
gep13force-pushed the upload-generic-packages branch from e356b33 to 90326d8Compare
| throw new System.NotImplementedException(); | ||
| } | ||
| public IEnumerable<PackageSearchResult> Get(int projectId, PackageQuery packageQuery) |
There was a problem hiding this comment.
@meziantou I couldn't see any prior art for returning an IEnumerable<T> in any of the existing clients. Happy to make a change here, just wanted to make sure that we are on the same page before making any changes.
There was a problem hiding this comment.
Other implementations use GitLabCollectionResponse<T>. This supports sync and async enumeration!
There was a problem hiding this comment.
Ooo! Missed that one! Will get that fixed up just now!
gep13 commented Sep 12, 2023
@meziantou I have taken a stab at making the changes that you suggested, and I have implemented a couple more methods on the new PackageClient for Side Question... Should I create an issue for this feature addition, or assuming that we move forward with this, are you ok to merge the PR directly, without an associated issue? |
gep13force-pushed the upload-generic-packages branch from 90326d8 to cf00481Compare| public class Package | ||
| { | ||
| [JsonPropertyName("id")] | ||
| public int Id{get; set} |
There was a problem hiding this comment.
New classes should use long for ids
| publicintId{get;set;} | |
| publiclongId{get;set;} |
| public int Id{get; set} | ||
| [JsonPropertyName("package_id")] | ||
| public int PackageId{get; set} |
There was a problem hiding this comment.
| publicintPackageId{get;set;} | |
| publiclongPackageId{get;set;} |
| public DateTime? UpdatedAt{get; set} | ||
| [JsonPropertyName("size")] | ||
| public int Size{get; set} |
There was a problem hiding this comment.
File size is a long. I'm not sure GitLab would allow such big files, but you never know
| publicintSize{get;set;} | |
| publiclongSize{get;set;} |
| public int FileStore{get; set} | ||
| [JsonPropertyName("file_md5")] | ||
| public string FileMD5{get; set} |
There was a problem hiding this comment.
| publicstringFileMD5{get;set;} | |
| publicstringFileMd5{get;set;} |
| public string FileMD5{get; set} | ||
| [JsonPropertyName("file_sha1")] | ||
| public string FileSHA1{get; set} |
There was a problem hiding this comment.
| publicstringFileSHA1{get;set;} | |
| publicstringFileSha1{get;set;} |
| @@ -0,0 +1,11 @@ | |||
| namespace NGitLab.Models | |||
| { | |||
| public enum PackageStatus | |||
There was a problem hiding this comment.
Use C# name (e.g. Default) and the EnumMember(Value = "")] attribute to map the actual name
There was a problem hiding this comment.
Happy to make this change, but I am curious what your suggestion is when it comes to things like this:
url = Utils.AddParameter(url, "status", query.Status); i.e. when using the Enum value to construct the URL that is requested. With the change that you have suggested in play, the constructed URL doesn't work, as the GitLab server returns an error...
Any suggestions?
There was a problem hiding this comment.
@meziantou just wanted to follow up on this one. Any ideas on the best course of action here? Thanks!
There was a problem hiding this comment.
Sorry for the delay. I think the best solution is to improve Utils.AddParameter to better support enums:
public static string AddParameter<T>(string url, string parameterName, T value){if (typeof(T).IsEnum){// TODO // Note: ReflectionExtensions.GetEnumMappings can be used as an example } return Equals(value, null) ? url : AddParameterInternal(url, parameterName, value.ToString())} | { | ||
| public enum PackageType | ||
| { | ||
| all, |
There was a problem hiding this comment.
Use C# name (e.g. Default) and the EnumMember(Value = "")] attribute to map the actual name
| { | ||
| public enum PackageSort | ||
| { | ||
| asc, |
There was a problem hiding this comment.
Use C# name (e.g. Default) and the EnumMember(Value = "")] attribute to map the actual name
| _api = api; | ||
| } | ||
| public Task<Package> PublishAsync(int projectId, PackagePublish packagePublish, CancellationToken cancellationToken = default) |
There was a problem hiding this comment.
Based on the url, I think the method name should be PublishGenericPackageAsync
| publicTask<Package>PublishAsync(intprojectId,PackagePublishpackagePublish,CancellationTokencancellationToken=default) | |
| publicTask<Package>PublishGenericPackageAsync(intprojectId,PackagePublishpackagePublish,CancellationTokencancellationToken=default) |
| PackageName = "Packages", | ||
| PackageVersion = "1.0.0", | ||
| Status = "default", | ||
| PackageStream = File.OpenRead("../../../../README.md"), |
There was a problem hiding this comment.
You'll need to dispose the stream. Also, don't rely on the file system, you can use a MemoryStream to create a dummy content.
meziantou commented Sep 12, 2023
No need to create an issue |
ap0llo commented May 3, 2024
Sorry for barging in, but I was actually looking into adding support for GitLab's package registry to NGitLab myself when I saw there is already an open PR. Is there anything I can do to complete this? I'd be happy to help out |
gep13 commented May 4, 2024
I keep trying to get back to the PR, but I just haven't had the cycles to do it yet From memory, all that was remaining was some re-work for the tests to not depend on file system directly. If you have some time, I have no objections to you creating a PR into my fork, and then I can update this PR. |
ap0llo commented May 4, 2024
Great, I'll see what I can do |
3 participants
No description provided.