-
-
Notifications
You must be signed in to change notification settings - Fork 28
Add support for Forgejo forge #107
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
Conversation
|
Hi, thanks for your PR! I'm pretty finicky about my projects (see this blog post for details), so I rarely merge a PR as-is. I can move forward on your PR in one of two ways:
Please let me know which approach you'd prefer. If I don't hear from you before I get around to working on this PR I'll go with option 1. Thanks again for your contribution! |
|
I spent some time trying to tweak this, but I finally realized that Forgejo does not actually have the same API as GitHub, at least not the same as the GH v4 REST API. Specifically, when you get releases from GitHub, you get something like this: {
"assets": [
{
"url": "https://api.github.com/repos/houseabsolute/precious/releases/assets/237809592",
"name": "precious-FreeBSD-x86_64.tar.gz",
],
}But Forgejo doesn't include a |
|
All of which is to say, this does need its own forge type. But don't work on that now. In working on this myself, I've done a bunch of refactoring that I'd like to finish up first. |
|
Hi @autarch 👋🏼 First of all, thanks for your transparency. I'm open to whatever method works best for you (I'm not that attached to the idea of having to preserve my name in the work).
This is something I missed completely. Is there anything I can do to help you understand this better? Thanks a lot for helping move this one forward. I started moving some of my repositories to Codeberg, and being able to use UBI through Mise to install tools would be very valuable. |
|
So I think I'm done with the refactoring I was planning on doing. If you want to take a stab at adding support for ForgeJo, that'd be great! |
|
@autarch I doubt I'll find time in the following weeks to complete this work. Feel free to close it and I'll open a new one if I find the time for it. |
|
That's fine. I may take a look at some point, but right now I'm mostly filling my free time up by playing Satisfactory ;) |
|
@autarch I found some time to work on it. I'd appreciate if you could review the work and let me know how to proceed :) |
autarch
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. Thanks! There's a few things missing:
- Let's add
CODEBERG_TOKENas an env var to check for a token. - The
builder.rscode docs should mention the token env vars. - The
ForgeType::from_urlcode needs to accounts for the codeberg domain. - It'd be great to add an integration test in
ubi-cli/tests/ubi.rs. After fixing the above issue, I was able to get https://codeberg.org/Cyborus/forgejo-cli/releases installed usingubi, so I think this is a good one to use for the integration test. Note that it doesn't have a macOS release, so the test code needs to be configured to not run on macOS. You can see a bunch of examples of per-OS conditions in that file already.
Thanks again!
ubi/src/forgejo.rs
Outdated
| env::remove_var("GITLAB_TOKEN"); | ||
| env::remove_var("CI_JOB_TOKEN"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be whatever env var is used for a forgejo token. I think it'd make sense to support both CODEBERG_TOKEN and FORGEJO_TOKEN.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd vote for FORGEJO_TOKEN, following my other comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll use FORGEJO_TOKEN. Let me know if you'd want CODEBERG_TOKEN to be added too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should add both. It doesn't hurt.
ubi/src/forge.rs
Outdated
|
|
||
| const GITHUB_DOMAIN: &str = "github.com"; | ||
| const GITLAB_DOMAIN: &str = "gitlab.com"; | ||
| const FORGEJO_DOMAIN: &str = "codegerg.org"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has a typo in the domain name. Also, I think we should just call it CODEBERG_DOMAIN for simplicity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the software is forgejo, the default instance chosen here codeberg.org, so (apart from the typo) I think thats okay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer the variable name to match the actual domain name, just for consistency.
Finkregh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the effort!
ubi/src/forge.rs
Outdated
|
|
||
| const GITHUB_DOMAIN: &str = "github.com"; | ||
| const GITLAB_DOMAIN: &str = "gitlab.com"; | ||
| const FORGEJO_DOMAIN: &str = "codegerg.org"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the software is forgejo, the default instance chosen here codeberg.org, so (apart from the typo) I think thats okay?
ubi/src/forge.rs
Outdated
|
|
||
| const GITHUB_API_BASE: &str = "https://api.github.com"; | ||
| const GITLAB_API_BASE: &str = "https://gitlab.com/api/v4"; | ||
| const CODEBERG_API_BASE: &str = "https://codeberg.org/api/v1"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That should be FORGEJO_API_BASE, following the above variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I want this the other way around, so it matches the domain name.
ubi/src/forge.rs
Outdated
| ForgeType::GitLab => Url::parse(GITLAB_API_BASE).unwrap(), | ||
| // The maintainers of Forgejo is Codeberg, hence why the URL | ||
| // doesn't align with the name of the Git forge. | ||
| ForgeType::Forgejo => Url::parse(CODEBERG_API_BASE).unwrap(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the api base changes depending on which forgejo instance you want to talk to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can provide your own --api-base-url via the CLI or API, so this is just the default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which is to say that this is correct as is.
ubi/src/forgejo.rs
Outdated
| env::remove_var("GITLAB_TOKEN"); | ||
| env::remove_var("CI_JOB_TOKEN"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd vote for FORGEJO_TOKEN, following my other comments
|
I realized when looking at this that the only difference between this and GitHub is the one field name in the assert data structure. And we could reuse the existing So given that, I think I want to refactor things a bit more to reduce code duplication. I think it's probably easiest for me to just take what you have and do the rest myself. Are you okay with that? |
Totally :). I can't think of a better person to drive the refactors needed for that. |
I realized that there was almost no difference in the forge code, even with Forgejo to come, so there's no need for the `dyn` stuff.
47a3ae5 to
244bfd1
Compare
e767346 to
0faa9a6
Compare
|
This is in v0.8.0, which I just released. Thanks again! |
Forgejo support is added in houseabsolute/ubi#107, but I don't know what to do so ignored it.
This reverts commit 28eb48e. (#6700) Forgejo support is added in houseabsolute/ubi#107, but this PR just ignores it. The regression #6699 was caused by houseabsolute/ubi@a7742e6, which wrapped errors with contexts. To work around this, we need to keep `anyhow::Error` as is, instead of converting to `eyre::Error`, until we check the status code. --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
I'm user of Codeberg, a hosted instance of the Git forge Forgejo, and I'd like to use UBI against repositories in those repos.
Since Forgejo mimics the API of GitHub, the implementation of Forgejo's forge symbol is very much a copy & paste of GitHub's. Please, let me know if you'd want me to extract some common functionality into another module for the purpose of reusability.