Skip to content
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

Adds support for NuGet V2 upstream package sources #630

Merged
merged 4 commits into from
Mar 3, 2021
Merged

Adds support for NuGet V2 upstream package sources #630

merged 4 commits into from
Mar 3, 2021

Conversation

patriksvensson
Copy link
Contributor

This PR adds support for NuGet V2 upstream package sources.

I opted for not rewriting the NuGetClient to use the official API, and instead, I'm just wrapping the existing NuGetClient in an adapter that implements IMirrorNuGetClient.

To configure the mirror service to use the NuGet V2 client:

"Mirror": {
  "Enabled": true,
  "PackageSource": "https://www.nuget.org/api/v2/",
  "Legacy": true
},

@@ -17,6 +17,11 @@ public class MirrorOptions : IValidatableObject
/// </summary>
public Uri PackageSource { get; set; }
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey this is a tiny bit of feature creep, but I think you can leverage the options validation here by adding attributes on the PackageSource property:

/// <summary>
/// The v3 index that will be mirrored.
/// </summary>
[RequiredIf(nameof(Enabled))]
[RequiredIf(nameof(Legacy))]
public Uri PackageSource { get; set; }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent idea. However when trying this it complains that only one IfRequired attribute can be used.

As I workaround, we can do the following, that will work but will show the error message Invalid 'Mirror' configs: 'PackageSource' is required because 'EnabledOrLegacy' has a value of 'True'. which does not reflect the settings properly, but maybe this is OK?

/// <summary>
/// If true, packages that aren't found locally will be indexed
/// using the upstream source.
/// </summary>
public bool Enabled { get; set; }

/// <summary>
/// The v3 index that will be mirrored.
/// </summary>
[RequiredIf(nameof(EnabledOrLegacy), true)]
public Uri PackageSource { get; set; }

/// <summary>
/// Whether or not the package source is a v2 package source feed.
/// </summary>
public bool Legacy { get; set; }

[NotMapped]
public bool EnabledOrLegacy => Enabled || Legacy;

Copy link
Owner

@loic-sharma loic-sharma Mar 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah good catch! It looks like you can support multiple attribute usages if we:

  1. Set the attribute's AttributeUsageAttribute.AllowMultiple to true
  2. Override the TypeId property (see this)

For an example of a validation attribute that supports multiple uses, see CustomValidationAttribute.

I'll do this in a follow-up PR since this is minor and I've blocked your changes for a little too long already!


if (options.Value?.PackageSource?.AbsolutePath == null)
{
throw new ArgumentException("No mirror package source has been set.");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally this should be done at startup by ASP.NET Core's options validation. See my comment up above: https://github.com/loic-sharma/BaGet/pull/630/files#r582447197

If that idea works, you should be able to remove this check.

@loic-sharma
Copy link
Owner

loic-sharma commented Feb 25, 2021

This is amazing. I really like your IMirrorNuGetClient abstraction, good idea there! This change will make it much easier to add support for private upstreams in the future too.

I left some minor comments, I'll merge this in once you've addressed them. Thanks!

P.S. Did you have any pain points when contributing to BaGet? Let me know if you have any feedback, would love to improve the experience however I can :)

@patriksvensson
Copy link
Contributor Author

@loic-sharma I've fixed all points except the validation part (see my comment). I'm happy to implement the changes if you want to. Btw, do you want me to squash the commits?

P.S. Did you have any pain points when contributing to BaGet? Let me know if you have any feedback, would love to improve the experience however I can :)

No pain points! I found the code base intuitive and easy to follow. Of course, there are aspects of how NuGet works that I'm not familiar with, but I think it's unavoidable to have to learn that.

One thing that I was a bit confused about was a test (RegistrationBuilderTests.TheRegistrationIndexResponseIsSortedByVersion) that fails consistently for me but the CI checks seem to be green. Not sure if this is something culture-related since I'm using a computer with Swedish regional settings.

@loic-sharma loic-sharma merged commit 5fbcc5e into loic-sharma:master Mar 3, 2021
@loic-sharma
Copy link
Owner

Thanks for the contribution, this is awesome! I'll work on publishing a new version of BaGet.

I've fixed all points except the validation part (see my comment). I'm happy to implement the changes if you want to.

That's alright, I'm very happy with your implementation already. I'd rather release the feature now, we can save incremental improvements for later :)

Btw, do you want me to squash the commits?

Feel free to do whatever works best for you! I squash the commits at the very end when I merge.

One thing that I was a bit confused about was a test (RegistrationBuilderTests.TheRegistrationIndexResponseIsSortedByVersion) that fails consistently for me

Thanks for the heads up, I'll take a look at that!

@patriksvensson patriksvensson deleted the feature/nuget-v2-read-through-caching branch March 3, 2021 08:24
viceice pushed a commit to visualon/baget that referenced this pull request Mar 5, 2021
)

To configure the mirror service to use the NuGet V2 client:

```json
"Mirror": {
  "Enabled": true,
  "PackageSource": "https://www.nuget.org/api/v2/",
  "Legacy": true
},
```

Co-authored-by: Patrik Svensson <patriksvensson@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants