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

An auto-discovery implementation #5

Merged
merged 3 commits into from
Oct 30, 2023
Merged

An auto-discovery implementation #5

merged 3 commits into from
Oct 30, 2023

Conversation

algernon
Copy link
Collaborator

This builds on top of #2, and is a draft, as there are a number of ways to approach the implementation, and this is but one of them. The goal here is for /v1/:host to do an auto-discovery, if :host was not recognised as a forge by the other routes.

The implementation I chose is on the verbose side, because I wanted to delegate the discovery itself to api::v1::<forge>::is_<forge>, so that the auto discovery endpoint is a relatively thin wrapper around those. Where rime redirects to, however, I kept that in the auto discovery endpoint, but can easily delegate that to the forge endpoints themselves, too.

I chose to use a pattern like AutoDiscovery::new_for(&host).try_github().await.expect("...").try_flakehub().await.expect("...").url(&url), because... I dunno. I found this nice flat chain nicer than an if/else soup. The if/else soup would be less code, however, so if you think that's the nicer approach, I'll go and do that.

The implementation itself is in the third commit.

Copy link
Owner

@cafkafk cafkafk left a comment

Choose a reason for hiding this comment

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

I know this is in draft status, but can you base this on main? I'd like to take a look at it

@algernon
Copy link
Collaborator Author

Yep, I can! Done.

@algernon
Copy link
Collaborator Author

As an additional note: I left the github & flakehub auto-discovery as-is (they simply compare the host to github.com/flakehub.com, respectively), even though the main app could just mount the appropriate routes at those locations. The reason I left them as-is, is to (eventually) support GitHub EE and self-hosted flakehub (if there's such a thing?). However, that may be rather pointless, and a lot of unnecessary code, so I'm thinking about removing that, and just adding two extra routes instead.

@cafkafk
Copy link
Owner

cafkafk commented Oct 29, 2023

self-hosted flakehub (if there's such a thing?)

Not for now I think... but might be a thing later? They did talk about a private flake feature 🤷‍♀️

However, that may be rather pointless, and a lot of unnecessary code, so I'm thinking about removing that, and just adding two extra routes instead.

I'll take a look at it soon, I mean, having the code be easily extensible is a plus.

@algernon
Copy link
Collaborator Author

I'll take a look at it soon, I mean, having the code be easily extensible is a plus.

It'd be extensible even if I removed the github/flakehub auto-discovery for now: they use the same pattern as forgejo, so it's just a bit of copy pasta to add it back, if/when the need arises. But it's equally easy to just keep it. I'll defer that decision to you! =)

@cafkafk
Copy link
Owner

cafkafk commented Oct 30, 2023

I'll take a look at it soon, I mean, having the code be easily extensible is a plus.

It'd be extensible even if I removed the github/flakehub auto-discovery for now: they use the same pattern as forgejo, so it's just a bit of copy pasta to add it back, if/when the need arises. But it's equally easy to just keep it. I'll defer that decision to you! =)

I mean I suppose we should default to avoiding doing unnecessary computation 🤷‍♀️, after reviewing the code, I'm fine with removing them in favor of directly routing tbh

@cafkafk
Copy link
Owner

cafkafk commented Oct 30, 2023

Also in case I forget myself I did review the code and it looks good, I just haven't tested it yet (I will before merge).

Do you wanna add gitlab support before a merge? You don't have to of course (and it could always be done in a follow up PR if you prefer that).

@algernon
Copy link
Collaborator Author

I can add GitLab, yep (I've done so locally, just haven't pushed it yet, because I wanted to do some testing first). Will change it to do direct routing for github.com/flakehub.com, too.

@algernon
Copy link
Collaborator Author

Rebased on main, added GitLab auto-discovery, and changed github.com and flakehub.com to use direct routing instead.

The GitLab auto-discovery appears to work for gitlab.com itself, too. I could add a redirect like there is one for Codeberg, to avoid having to a discovery. But gitlab.com being behind a login wall... I don't feel like it, really. It still works, it's just a tiny bit slower, having to do the auto-detection.

@algernon algernon marked this pull request as ready for review October 30, 2023 09:33
algernon and others added 3 commits October 30, 2023 14:24
If the API is hit with a `/v1/:host`, where the host isn't recognized as
a forge, try to auto-detect the one in use.

Signed-off-by: Gergely Nagy <me@gergo.csillger.hu>
Add a direct mapping for `github.com` and `flakehub.com`, to shortcut
the auto-discovery mechanism by simply not going into it.

Signed-off-by: Gergely Nagy <me@gergo.csillger.hu>
Add basic autodiscovery integration tests to the justfile using `run_test`
Copy link
Owner

@cafkafk cafkafk left a comment

Choose a reason for hiding this comment

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

Looking at this now, this is actually a really nice feature, shaving off a whole path parameter is great UX. I added some tests to ensure we don't later introduce regressions (or to make us aware of changes to any source forge that break rime).

Good work, thanks for all the contributions so far, they really add some value to rime!

@cafkafk
Copy link
Owner

cafkafk commented Oct 30, 2023

My plan was to release on every thursday like I do for other software I maintain, but I kinda really want this for my personal use on rime.cx already, specially with all the forges and auto discovery... so maybe I'll cut a release after merging this and deploy it today...

@cafkafk cafkafk merged commit f181711 into cafkafk:main Oct 30, 2023
3 checks passed
@algernon algernon deleted the auto-discovery branch November 1, 2023 08:57
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.

None yet

2 participants