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

Add support for Gitea #124

Merged
merged 2 commits into from
Apr 30, 2024
Merged

Conversation

MagicRB
Copy link
Contributor

@MagicRB MagicRB commented Apr 22, 2024

This pull request is just there to publicize the code I'm working on, before being ready it'll get probably more cleaned up and merged with the Gitea backend pull request.

@@ -239,7 +262,9 @@ def __init__(self, **kwargs: Any) -> None:
@defer.inlineCallbacks
def run(self) -> Generator[Any, object, Any]:
# run `nix build`
cmd: remotecommand.RemoteCommand = yield self.makeRemoteShellCommand()
Copy link
Member

Choose a reason for hiding this comment

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

What was the issue with the previous line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of your questions, types, typechecking seemed to be broken for me, soemhow through a yield it wouldnt type check, my guess is that a yield can return an error value. I wanted to remove all the type errors so its easier for me to write (and for the future). I can revert these bits no problem if so desired

Copy link
Member

@Mic92 Mic92 Apr 23, 2024

Choose a reason for hiding this comment

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

What typechecker are you using? It works with mypy here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pyright

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will try mypy then and clean up these workarounds 👍. Sorry for the mess

Copy link
Member

Choose a reason for hiding this comment

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

Alright. You can use nix fmt to get a local type check.

@@ -224,7 +245,9 @@ def run(self) -> Generator[Any, object, Any]:
error = self.getProperty("error")
attr = self.getProperty("attr")
# show eval error
error_log: Log = yield self.addLog("nix_error")
# TODO why doesn't type information pass through yield again?
error_log_: object = yield self.addLog("nix_error")
Copy link
Member

Choose a reason for hiding this comment

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

same question as above.

self.observer = logobserver.BufferLogObserver()
self.addLogObserver("stdio", self.observer)
self.supported_systems = supported_systems

@defer.inlineCallbacks
def run(self) -> Generator[Any, object, Any]:
# run nix-eval-jobs --flake .#checks to generate the dict of stages
cmd: remotecommand.RemoteCommand = yield self.makeRemoteShellCommand()
Copy link
Member

Choose a reason for hiding this comment

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

Same question as above.

self.stdio_log: Log = yield self.addLogForRemoteCommands("stdio")

# TODO why doesn't type information pass through yield again?
stdio_log_: object = yield self.addLogForRemoteCommands("stdio")
Copy link
Member

Choose a reason for hiding this comment

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

Same question as above.

@Mic92
Copy link
Member

Mic92 commented Apr 23, 2024

@mergify rebase

Copy link

mergify bot commented Apr 23, 2024

rebase

✅ Branch has been successfully rebased

@Mic92 Mic92 mentioned this pull request Apr 23, 2024
@MagicRB MagicRB marked this pull request as ready for review April 27, 2024 15:28
@MagicRB
Copy link
Contributor Author

MagicRB commented Apr 27, 2024

Ready for review! I verified all functionality to be working against a fork of this repository at https://github.com/magicrb/buildbot-nix and https://codeberg.org/magic_rb/buildbot-nix. No regressions have been introduced as far as I can tell. The following section includes notes that I took while working on this.

Initial Setup

  • confused about how the GH integration works. The docs aren't great.
  • https://github.com/settings/tokens/new took me ages to find
  • the gh app must be installed and oauth enabled
  • git is required
  • force eval manually is broken, properties do not get populated
  • will only consider PRs or default branch, no way to make it consider other branches

Removing GitHub Assumption

Adding Gitea

  • Gitea's API is very similar to GitHub's, especially the responses. Only thing different so far is that Gitea doesn't include topics in the repo listing, but that is easy to splice in.
  • It is very unclear whether Gitea includes repos of orgs a user belongs to in the /api/v1/user/repos endpoint, if not it's easy to splice in again
  • what happens if the same project owner/repo is on Gitea and GitHub
  • the reporters are stupid, they will trigger all for every repo, i dont know how to make them not trigger for the incorrect one, maybe subclassing and a filter?s

Cleanup and Finalizing

  • still don't know how to resolve this

    # TODO Gitea doesn't include this information
    return False # self.data["owner"]["type"] == "Organization"

    Gitea really doesn't seem to expose that information, the API docs agree.

Lastly:
mypy is a much weaker type checker in my experience, it would miss many cases of errors that pyright would report, but I did undo the weird type casting since we're using mypy.

@mannp
Copy link
Contributor

mannp commented Apr 27, 2024

Very happy for this, thank you. Out of interest, would this support private forgejo/gitea repos or public only?

@MagicRB
Copy link
Contributor Author

MagicRB commented Apr 27, 2024

Very happy for this, thank you. Out of interest, would this support private forgejo/gitea repos or public only?

If the buildbot instance can access the instance, https works, then it should be fine I think. It is untested though.

@MagicRB
Copy link
Contributor Author

MagicRB commented Apr 27, 2024

oh yeah, there are a few unresolved things:

  1. reporters will trigger for both gitea and github, both of them. The gitea one will skip the report gracefully, the github one will throw an exception. But it seems to be harmless
  2. The Gitea plugin doesnt support avatars
  3. Gitea doesn't seem to distinguish between organizations and users in its API, so belongs_to_org is a noop and always returns False
  4. buildbot doesn't support multiple authentication plugins at once, the default is github

@MagicRB MagicRB changed the title Remove assumption of GitHub being the only forge Add support for Gitea Apr 27, 2024
Signed-off-by: magic_rb <richard@brezak.sk>
Signed-off-by: magic_rb <richard@brezak.sk>
@Mic92 Mic92 merged commit 508ceb8 into nix-community:main Apr 30, 2024
10 checks passed
Comment on lines +70 to +71
# TODO unused
buildbot_user: str
Copy link
Contributor

Choose a reason for hiding this comment

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

Does TODO unused mean buildbot_user can be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the time I wasn't a 100% sure, but now I am, and yes it can

Copy link
Contributor

@zowoq zowoq Jun 14, 2024

Choose a reason for hiding this comment

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

Thanks, I'll send a PR.

#186

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.

4 participants