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

[NuGet BaGet]: Added new BaGet service (Self hosted NuGet server) #9805

Closed
wants to merge 2 commits into from

Conversation

lluiscab
Copy link

This PR adds support for version & download badges from a BaGet server.

I'm not sure how to add examples / tests for this new service due to it's self hosted nature and no public instance being available (that I'm aware of)

I've tested badge generation against our local (private) BaGet instance with the following commands:

npm run badge -- /baget/v/<PackageName?source=<Instance url>
npm run badge -- /baget/vpre/<PackageName>?source=<Instance url>
npm run badge -- /baget/dt/<PackageName?source=<Instance url>

I've extended the logic from nuget-v3-service-family to support the small differences in API in the BaGet server api implementation, mainly searching without the packageid parameter and the package version being available in the response, without need for lookup in the versions array (which is also sorted in DESC, not ASC)

Copy link
Contributor

Warnings
⚠️ This PR modified service code for baget but not its test code.
That's okay so long as it's refactoring existing code.
Messages
📖 ✨ Thanks for your contribution to Shields, @lluiscab!

Generated by 🚫 dangerJS against bb2682f

@chris48s
Copy link
Member

chris48s commented Jan 3, 2024

Sorry it has taken quite a while to respond to this.

Given you say that there is no public instance of BaGet, this seems like a badge that would only be relevant to users who self host a shields instance. Would that be a reasonable assessment?

In general, we don't add these.

Related reading:

shields/doc/releases.md

Lines 31 to 36 in b1f5aec

### Important information
This Shields server is the exact same codebase that powers the main Shields.io service; we do not have a separate self-hosted version of Shields. This means that the server codebase is geared towards the development, maintenance, and operation of the Shields.io service so there are a few things to note:
- We often have to reject or de-prioritize feature requests that are only applicable for self-hosting and/or which may be problematic for the core Shields.io offering (e.g. requiring authentication on the badge server)
- We do not accept new badges that cannot be utilized with Shields.io (e.g. those that would always require user-specific authorization)

and

#9666 (comment)

I did start an issue about a possible way to deal with this at #9787 but I'm not sure it will go anywhere. I don't think any of us are convinced on the tradeoffs.

All of that said, quite a common thing we do is support compatibility with a private or self-hosted version of a centralised service as long as your private instance is API compatible with the centralised version. So our GitLab badges will work with your self-hosted GitLab as long as it is running a version that is API-compatible with https://gitlab.com/ . Our Packagist badges will work with your private packagist install as long as it is running a version that is API-compatible with https://packagist.org/ and so on. Essentially, if we can test everything against a public instance and also tack on a ?registry_url= param (or whatever), that works for us.

So here's my question: There are some BaGet-specific customisations going on in this PR, but would it be possible to deliver a subset of what you're trying to do here by bolting an optional parm onto NuGet and documenting that it also works with BaGet, or is BaGet not quite API compatible enough for that to work? i.e: are the changes in this PR BaGet-related enhancements or are they fundamentally necessary to get the thing to work?

@lluiscab
Copy link
Author

lluiscab commented Jan 3, 2024

I understand the concerns about maintaining an integration with a provider that can't easily be tested.

Reading the issues that you linked, is this something that should be implemented using the endpoint or dynamic/json badges then?

@chris48s
Copy link
Member

chris48s commented Jan 4, 2024

The dynamic json badge has the advantage that you can make custom badges just by constructing a URL. So using a URL that returns JSON like https://pypi.org/pypi/pip-abandoned/json I can construct a URL like https://img.shields.io/badge/dynamic/json?query=info.requires_python&label=python&url=https%3A%2F%2Fpypi.org%2Fpypi%2Fpip-abandoned%2Fjson that extracts a value from it and puts it on a badge.

The downside of this is that it can only take you so far. If you want to do something more advanced than just extract a value and display it, for example:

  • change the colour of the badge conditionally based on the value
  • perform some kind of calculation like adding several numbers together
  • additional formatting (e.g: rounding a number, or displaying a number of bytes as Kb/Mb/Gb)

you can't do that with the dynamic badge.

With the endpoint badge, the sky's the limit. You can have all that and a bag of chips, but you do need to host the endpoint that can call your API, do whatever processing you need, and serve a JSON document conforming to the badge schema somewhere, and we recognise this does represent some additional overhead.

@chris48s chris48s added the service-badge New or updated service badge label Jan 4, 2024
@PyvesB
Copy link
Member

PyvesB commented Sep 22, 2024

I'm personally aligned on above points as well. I don't think we'll want to move forward with this badge for the time being, so will go ahead and close the PR. Let's keep an eye on #9787 as the next step.

Anyhow, thanks for your interest in the Shields.io project, @lluiscab!

@PyvesB PyvesB closed this Sep 22, 2024
@lluiscab lluiscab deleted the feat/service/baget branch September 22, 2024 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service-badge New or updated service badge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants