Ability to upload to Generic Packages Repository #524

gep13 · 2023-09-11T19:37:10Z

No description provided.

gep13 · 2023-09-11T19:38:44Z

@meziantou I have opened this as draft, based on the comments that you have made here:

#511 (reply in thread)

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:

https://docs.gitlab.com/ee/user/packages/generic_packages/index.html#do-not-allow-duplicate-generic-packages

Thanks!

NGitLab/Models/PackagePublish.cs

NGitLab/IPackageClient.cs

NGitLab/Impl/PackageClient.cs

NGitLab/Models/PackageFile.cs

NGitLab/Models/Package.cs

NGitLab.Tests/PackageTests.cs

meziantou · 2023-09-11T21:04:38Z

I would love to get some feedback on what I have done here, and what changes/suggestions you could make.

That's a good start! I've added some comments, but the shape is ok

gep13 · 2023-09-12T10:17:54Z

NGitLab.Mock/Clients/PackageClient.cs

+            throw new System.NotImplementedException();
+        }
+
+        public IEnumerable<PackageSearchResult> Get(int projectId, PackageQuery packageQuery)


@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.

Other implementations use GitLabCollectionResponse<T>. This supports sync and async enumeration!

Ooo! Missed that one! Will get that fixed up just now!

gep13 · 2023-09-12T10:19:42Z

@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 Get and GetByIdAsync. Let me know if you have any further questions/suggestions.

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?

meziantou · 2023-09-12T14:29:34Z

NGitLab/Models/Package.cs

+    public class Package
+    {
+        [JsonPropertyName("id")]
+        public int Id { get; set; }


New classes should use long for ids

Suggested change

public int Id { get; set; }

public long Id { get; set; }

meziantou · 2023-09-12T14:29:43Z

NGitLab/Models/Package.cs

+        public int Id { get; set; }
+
+        [JsonPropertyName("package_id")]
+        public int PackageId { get; set; }


Suggested change

public int PackageId { get; set; }

public long PackageId { get; set; }

meziantou · 2023-09-12T14:30:16Z

NGitLab/Models/Package.cs

+        public DateTime? UpdatedAt { get; set; }
+
+        [JsonPropertyName("size")]
+        public int Size { get; set; }


File size is a long. I'm not sure GitLab would allow such big files, but you never know

Suggested change

public int Size { get; set; }

public long Size { get; set; }

meziantou · 2023-09-12T14:30:45Z

NGitLab/Models/Package.cs

+        public int FileStore { get; set; }
+
+        [JsonPropertyName("file_md5")]
+        public string FileMD5 { get; set; }


Suggested change

public string FileMD5 { get; set; }

public string FileMd5 { get; set; }

meziantou · 2023-09-12T14:30:53Z

NGitLab/Models/Package.cs

+        public string FileMD5 { get; set; }
+
+        [JsonPropertyName("file_sha1")]
+        public string FileSHA1 { get; set; }


Suggested change

public string FileSHA1 { get; set; }

public string FileSha1 { get; set; }

meziantou · 2023-09-12T14:33:47Z

NGitLab/Models/PackageStatus.cs

@@ -0,0 +1,11 @@
+namespace NGitLab.Models
+{
+    public enum PackageStatus


Use C# name (e.g. Default) and the EnumMember(Value = "")] attribute to map the actual name

Happy to make this change, but I am curious what your suggestion is when it comes to things like this:

https://github.com/ubisoft/NGitLab/pull/524/files#diff-27c124206cca88b65b78a9a2fe44d27dfcc3fab2b510dc692c6507c5cc7d8833R50

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?

@meziantou just wanted to follow up on this one. Any ideas on the best course of action here? Thanks!

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()); }

meziantou · 2023-09-12T14:33:56Z

NGitLab/Models/PackageType.cs

+{
+    public enum PackageType
+    {
+        all,


Use C# name (e.g. Default) and the EnumMember(Value = "")] attribute to map the actual name

meziantou · 2023-09-12T14:34:08Z

NGitLab/Models/PackageSort.cs

+{
+    public enum PackageSort
+    {
+        asc,


Use C# name (e.g. Default) and the EnumMember(Value = "")] attribute to map the actual name

meziantou · 2023-09-12T14:39:25Z

NGitLab/Impl/PackageClient.cs

+            _api = api;
+        }
+
+        public Task<Package> PublishAsync(int projectId, PackagePublish packagePublish, CancellationToken cancellationToken = default)


Based on the url, I think the method name should be PublishGenericPackageAsync

Suggested change

public Task<Package> PublishAsync(int projectId, PackagePublish packagePublish, CancellationToken cancellationToken = default)

public Task<Package> PublishGenericPackageAsync(int projectId, PackagePublish packagePublish, CancellationToken cancellationToken = default)

meziantou · 2023-09-12T14:41:14Z

NGitLab.Tests/PackageTests.cs

+                PackageName = "Packages",
+                PackageVersion = "1.0.0",
+                Status = "default",
+                PackageStream = File.OpenRead("../../../../README.md"),


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 · 2023-09-12T14:41:55Z

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?

No need to create an issue

ap0llo · 2024-05-03T19:48:57Z

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 · 2024-05-04T06:07:19Z

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 · 2024-05-04T14:46:31Z

If you have some time, I have no objections to you creating a PR into my fork, and then I can update this PR.

Great, I'll see what I can do

meziantou requested changes Sep 11, 2023

View reviewed changes

gep13 force-pushed the upload-generic-packages branch from e356b33 to 90326d8 Compare September 12, 2023 10:15

gep13 commented Sep 12, 2023

View reviewed changes

gep13 requested a review from meziantou September 12, 2023 10:18

Ability to upload to Generic Packages Repository

cf00481

gep13 force-pushed the upload-generic-packages branch from 90326d8 to cf00481 Compare September 12, 2023 10:20

meziantou reviewed Sep 12, 2023

View reviewed changes

ap0llo mentioned this pull request May 5, 2024

Address change requests for Generic Package upload from ubisoft/NGitLab#524 gep13/NGitLab#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ability to upload to Generic Packages Repository #524

Ability to upload to Generic Packages Repository #524

gep13 commented Sep 11, 2023

gep13 commented Sep 11, 2023

meziantou commented Sep 11, 2023

gep13 Sep 12, 2023

meziantou Sep 12, 2023

gep13 Sep 12, 2023

gep13 commented Sep 12, 2023

meziantou Sep 12, 2023

meziantou Sep 12, 2023

meziantou Sep 12, 2023

meziantou Sep 12, 2023

meziantou Sep 12, 2023

meziantou Sep 12, 2023

gep13 Sep 12, 2023

gep13 Sep 18, 2023

meziantou Sep 18, 2023

meziantou Sep 12, 2023

meziantou Sep 12, 2023

meziantou Sep 12, 2023

meziantou Sep 12, 2023

meziantou commented Sep 12, 2023

ap0llo commented May 3, 2024

gep13 commented May 4, 2024

ap0llo commented May 4, 2024

	public int PackageId { get; set; }
	public long PackageId { get; set; }

	public string FileMD5 { get; set; }
	public string FileMd5 { get; set; }

	public string FileSHA1 { get; set; }
	public string FileSha1 { get; set; }

	public Task<Package> PublishAsync(int projectId, PackagePublish packagePublish, CancellationToken cancellationToken = default)
	public Task<Package> PublishGenericPackageAsync(int projectId, PackagePublish packagePublish, CancellationToken cancellationToken = default)

Ability to upload to Generic Packages Repository #524

Are you sure you want to change the base?

Ability to upload to Generic Packages Repository #524

Conversation

gep13 commented Sep 11, 2023

gep13 commented Sep 11, 2023

meziantou commented Sep 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gep13 commented Sep 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meziantou commented Sep 12, 2023

ap0llo commented May 3, 2024

gep13 commented May 4, 2024

ap0llo commented May 4, 2024