Page MenuHomeLubuntu

Lugito should do diffs
Closed, ResolvedPublic

Description

Someone needs to dig into the webhooks and python-phabricator to implement Diffusion support for Lugito.

It's a real pain not to have this in our IRC channel.

Event Timeline

tsimonq2 created this task.Oct 4 2018, 7:26 AM
tsimonq2 triaged this task as High priority.
tsimonq2 removed tsimonq2 as the assignee of this task.Oct 30 2018, 2:11 PM

Hi There,

I am new to the project, answering the call out on Mastodon for help from Australia (I live in Sydney). If no one has started on this I'm happy to give it a crack. I have some experience in Python so hopefully I can help.

Cheers,
Ben

Sure, absolutely @doc-E-brown!

You can find the source here (mirrored to GitHub here if that's how you prefer to contribute).

We use the python-phabricator module. It doesn't have documentation in and of itself, but it's a direct copy of the universal Phabricator methods. We do a weekly update of our Phabricator instance following the stable branch, so it should be pretty on-par with that page.

Since you said you had experience with Python I'll leave you to it, but if you need help, don't hesitate to reach out. Mainly, I have the following features in mind:

  1. Notification when someone creates a diff.
  2. Notification when someone comments on the diff or the diff is updated.
  3. Notification when a diff is approved or changes are requested.

Please do get an IRC bouncer if you could; while I'm mostly around in the evening when it's your morning, it would be good to leave messages for you and vice versa. :)

Awesome! I have a little experience with Phabricator source. Let's see how we go! :)

doc-E-brown moved this task from New to In Progress on the Meta board.Nov 2 2018, 5:20 AM

@tsimonq2 does phabricator have a sandbox environment or something similar I can use for testing? Also is there a way I can create an API key for testing?

Thanks!!!
Ben

Awesome, I have a local Phabricator instance all set up.

Hey @tsimonq2

I have a new webhook (/diffhook) responding to new diff and diff comment creates and am just working on the comments edited at the moment. A couple of implementation questions:

  1. I assume do you a find / replace for API and HMAC Keys before deployment. Is this correct?. I need to add another HMAC key for the /diffhook endpoint. Would you have any preference / strong objections to placing these values in environment variables or a dotfile (which obviously wouldn't get commited)?
  2. Would you mind if I did some refactoring? As there are now quite a few common tasks between the new /diffhook and the /commithook.
  3. It seems as though when you go to run the api in production it is simply
./lugito

is this correct?

Thanks

Heya @doc-E-brown!

I assume do you a find / replace for API and HMAC Keys before deployment. Is this correct?. I need to add another HMAC key for the /diffhook endpoint. Would you have any preference / strong objections to placing these values in environment variables or a dotfile (which obviously wouldn't get commited)?

That's correct. If you want to move things to a dotfile, that would be fine by me.

Would you mind if I did some refactoring? As there are now quite a few common tasks between the new /diffhook and the /commithook.

Go right ahead! It's a bit of a quick hack, and "it works," but I agree that it isn't really clean. 😉

It seems as though when you go to run the api in production it is simply

./lugito

is this correct?

Yup.

Thanks again, @doc-E-brown!

During the stand up meeting a reference to:

https://phab.lubuntu.me/T154#3228

Triggered this response in IRC:

-lugito/#lubuntu-devel- Error: T154#3228is an invalid task reference.

@doc-E-brown to investigate.

During the stand up meeting a reference to:

https://phab.lubuntu.me/T154#3228

Triggered this response in IRC:

-lugito/#lubuntu-devel- Error: T154#3228is an invalid task reference.

@doc-E-brown to investigate.

This error is due to the anchor reference (#3228) being present in the link. I am updating lugito within T88 to manage the anchor reference as well. The anchor reference will be removed prior to looking up the task.

@tsimonq2 I am just finalising the changes now with some additional testing on launchpadlib. I just noticed that launchpadlib is incompatible with the current version of httplib2 (which is grabs by default in a pip install). I have specified the correct version in a setup.py and requirements.txt file. I have filed a corresponding bug https://bugs.launchpad.net/launchpadlib/+bug/1803558

  • The anchor reference issue has been corrected and updated.
  • I have made quite a lot of changes which might be too big for one single diff. I have put lugito into its own class, the irc communicating component into base class for communicating with other services. I am working on putting launchpad into a similar class. The idea here being that if we want to plug in other services e.g. slack we just build out a class with the appropriate methods. Before finishing and submitting via arcane if you are interested in reviewing what I've done I've posted the current changes to a forked version on github https://github.com/doc-E-brown/lugito/tree/diffhook . I will not submit the patch via Github.

Still to do items include:

  • Improving test coverage
  • Documenting the package and classes
  • Uploading to pypi if desired and publishing documentation to read the docs

I'm off to bed now, see you in the stand-up.

@tsimonq2 are we good to close this?

@tsimonq2 can you have a look at the running instance of Lugito, it wasn't responding to links in the standup this morning. If there is a bug I would like to fix it.

Thanks!

tsimonq2 closed this task as Resolved.Thu, Dec 6, 9:33 AM

It occasionally errors out on an SSL error. I'll open another task in a bit, but this one is solved.

Thanks!