-
-
Notifications
You must be signed in to change notification settings - Fork 511
Implement SourceHut forge backend #5834
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
base: main
Are you sure you want to change the base?
Conversation
| c.gitURL, from.Repository.Owner.CanonicalName, from.Repository.Name, | ||
| update.New.Id) | ||
|
|
||
| return &model.Pipeline{ |
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 could add changed files here, but would need to parse the unified diff to do so. Would it be desirable to add a dependency for that?
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.
How much would that change the binary size? If it's not much we can add it I think.
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.
Not much, probably. I think this one would do the trick:
|
|
||
| func (c *SourceHut) Repos(ctx context.Context, u *model.User, p *model.ListOptions) ([]*model.Repo, error) { | ||
| // TODO: Pagination on SourceHut does not work well with Woodpecker's | ||
| // higher-level internals (and doesn't work well at all, to be frank) |
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.
SourceHut's API pagination leaves something to be desired, but one quality that needs to be considered is that it uses opaque strings for cursors, rather than integer page numbers, which requires higher level refactoring in Woodpecker.
| entries = append(entries, &forge_types.FileMeta{ | ||
| Name: ent.Name, | ||
| Data: data, | ||
| }) |
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 interface design requires fetching the full contents of every file in a directory, including ones that may or may not be applicable to the workflow. Would be nice to separate these steps in the interface.
| return err | ||
| } | ||
|
|
||
| // XXX: Ideally we would store the webhook ID here |
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.
It would be nice if forge backends could store arbitrary metadata in the database for, for instance, the repo table. This would allow us to store the webhook ID and simplify the webhook deletion process later.
| } | ||
|
|
||
| func (c *SourceHut) PullRequests(ctx context.Context, u *model.User, r *model.Repo, p *model.ListOptions) ([]*model.PullRequest, error) { | ||
| return nil, nil // TODO |
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.
SourceHut does not have "pull requests". Instead, we use patches sent to a mailing list. By associating patches with a git repository, we can use features like status indicators, but right now there is no means of getting this information into the forge backend because it would require a separate UI for configuring which mailing list applies to a git repository.
This leads into another question, which is that the woodpecker frontend makes a lot of assumptions that the forge will use GitHub-style workflows like pull requests. The list of applicable events, and terminology like "pull request", would ideally reflect the capabilities/design of the underlying forge.
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.
While this is right, changing this would probably add much complexity to the codebase. Having separate event handling for every forge would no longer allow us to let the UI define these labels, icons, etc. Also quite relevant features like the pull request gating is getting harder. Sure, it's possible, but I'm not sure if it's the right way to go (but I also don't know how complex that would be exactly).
About your first paragraph, can you elaborate some more here? Cause I don't know if I get that correctly. I never used sourcehut. Would it be possible to use the patches just (or mostly) like pull requests? Yes, there probably wouldn't be events like "pr closed" or "pr metadata updated". So the question mainly is if sourcehut can send a webhook linked to a repo if a patch comes in and if this patch can be cloned directly. If yes, this should work I think, if not, that would be hard to implement at woodpeckers side. And what do you mean by "separate UI"? A separate UI in woodpecker?
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.
We can send a webhook indeed when a patch comes in, but it's not necessarily linked to a repository. A single mailing list can have several associated repositories within it, or vice-versa. We would need a separate UI to associate a repository (the resource known to woodpecker) with a specific mailing list to process patches from, which would basically involve a UI very similar to the one for picking a repository from a list of repositories present on the forge, only you would be picking out a mailing list instead.
Applying the patch for testing is not as simple as cloning it directly and checking out a magic reference like GitHub does, though. You need a separate step which basically amounts to curl <patch url> | git am -3.
As for the UI being able to accommodate different events and such for different forges, I agree that it will add complexity, though in this case we could probably just treat "patch" as a synonym for "pull request". But I think that it's a bit shortsighted to avoid having forge-specific UI tweaks on the basis that it adds complexity to the codebase; on the other hand it adds constraints to how well woodpecker can fit into each of the forges it supports. Currently all of the forges supported are GitHub-like, but they do still have differences which would probably be accounted for in Woodpecker, and it's pretty limiting to suppose that Woodpecker will only be able to support GitHub-like forges.
| IsUser: true, | ||
| Private: false, | ||
| }, nil | ||
| } |
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.
SourceHut does not (currently) support orgs. I did my best here to shim in the necessary code to account for that, but some extra attention here would be great.
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.
Looks fine so far. Bitbucket also doesn't I think.
8a182db to
5cf085d
Compare
5cf085d to
c12a200
Compare
|
uuuuh :) |
Yes? |
|
= nice will look into it once i have time :) |
|
Oh, great :) I remember meeting you at FOSDEM, glad to be working together on this now. |
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! Added some comments to some of your points
| } | ||
|
|
||
| func (c *SourceHut) PullRequests(ctx context.Context, u *model.User, r *model.Repo, p *model.ListOptions) ([]*model.PullRequest, error) { | ||
| return nil, nil // TODO |
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.
While this is right, changing this would probably add much complexity to the codebase. Having separate event handling for every forge would no longer allow us to let the UI define these labels, icons, etc. Also quite relevant features like the pull request gating is getting harder. Sure, it's possible, but I'm not sure if it's the right way to go (but I also don't know how complex that would be exactly).
About your first paragraph, can you elaborate some more here? Cause I don't know if I get that correctly. I never used sourcehut. Would it be possible to use the patches just (or mostly) like pull requests? Yes, there probably wouldn't be events like "pr closed" or "pr metadata updated". So the question mainly is if sourcehut can send a webhook linked to a repo if a patch comes in and if this patch can be cloned directly. If yes, this should work I think, if not, that would be hard to implement at woodpeckers side. And what do you mean by "separate UI"? A separate UI in woodpecker?
| IsUser: true, | ||
| Private: false, | ||
| }, nil | ||
| } |
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.
Looks fine so far. Bitbucket also doesn't I think.
| c.gitURL, from.Repository.Owner.CanonicalName, from.Repository.Name, | ||
| update.New.Id) | ||
|
|
||
| return &model.Pipeline{ |
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.
How much would that change the binary size? If it's not much we can add it I think.
|
I think the CI failures are not entirely related to my work? Is that the case? |
|
They are related, the CI doesn't run |
|
Thanks qwerty for step in, i currently did not have mouch time ... and the holydays are packed with dates ... 😓 |
This is an initial proof-of-concept. Will add remarks inline to highlight discussion points.