Testing

[adamjstewart]

Unit testing? Mypy?

Also, the README has some instructions for running locally (still need to test these myself), but it might be good to have an official testing procedure before changes are pushed to the live bot.

Do we want to have official releases, or simply automatically update the live running bot every time a commit is pushed to master?

[vsoch]

I started to address part of this (docs and testing of developer setup in README) in #9. After some discussion there I can do the final steps to create a connected repo and test some of the integration (and add that to the README). After that I think we would want to do:

• a logo with branding
• a proper pretty/cute/lovely GitHub pages site to show most of the content in the README

And then the final issue for the issue is unit testing, which should be at least easier to think about with docker-compose.

I would say while the bot is pretty new, it's not terrible to not have releases (and just push to master and update from there). When that has evened out a bit and we are somewhat confident we don't need to add new features anytime soon, a release would make sense.

[vsoch]

@alecbcs what do you think - is spack bot a cousin of Smuggler and Autamus? 😆

[vsoch]

@adamjstewart what does spackbot actually do (and how do I trigger him?) I installed on my fork of spack and opened a PR, and I see the ping and install events but nothing more than that.

[adamjstewart]

spack-bot currently has two functionalities:

1. Add reviewers to PRs: https://github.com/spack/spack-bot/blob/main/spackbot/pr_add_reviewers.py
2. Add labels to PRs: https://github.com/spack/spack-bot/blob/main/spackbot/pr_add_labels.py

(1) happens only when you open a new PR (we don't want to comment on a PR every time a new commit is added). (2) happens every time a PR is opened or synchronized (a new commit is added, a force push happens, etc.).

We can add as many functionalities as we want.

[tgamblin]

@vsoch see here for most of it: spack/spack#24647

When it can’t add reviewers it comments and @‘s people. If it sees new maintainers in a package, it also invites them to the spack org and to the maintainers team, so they can be requested as reviewers in the future.

maintainers have triage privileges but can’t approve for merge (only writers can do that).

[tgamblin]

@vsoch also spackbot isn’t actually hooked up to @spackbot (at least not for @‘s). That just emails maintainers@spack.io (me) right now. I was thinking we’d make it listen for comments and notice @spackbot in the text.

[vsoch]

ah gotcha, sorry for the extra email!

[tgamblin]

No worries! Thanks for working on this!

also it’s fine — i was thinking we’d use the emails as kind of a log. Or I can just filter them.

[adamjstewart]

Do we want to merge @spackbot @spack-bot @spack-maintainer-bot?

[vsoch]

What do you mean merge?

[adamjstewart]

As in there is a spackbot account and both spack-bot and spack-maintainer-bot apps. I wonder if we really need the account or if we can just use a single app.

[vsoch]

I think the account isn't really important - if the bot is reading the comments it can parse any reference to @spackbot (or the others) and then respond. The one with the actual account looks better though because it is an actual account that gets bolded / you can click on. So I don't think we need it, but it makes it feel a little more like a real entity.

[tgamblin]

The app is just called spack-maintainer-bot — there’s also an account called spackbot.

[tgamblin]

@adamjstewart spack-bot was taken for the app name. Do you own that?

[vsoch]

yay my spackbot lives!

I didn't realize the key needed to be loaded - the impression of putting it in an envar is that it's a path (so I do a check for that and load it if the path exists).

[vsoch]

I made him cute too :)

[vsoch]

Do we want spackbot to respond to any kind of comments on issues?

[tgamblin]

Probably only ones from the author or people in the org. Also depends on the operation. Reformatting a PR should probably only be maintainers with write access and the author.

[tgamblin]

@vsoch: FYI reformatting may be tricky as we don’t want to run spack style code from the PR as the bot. I think the formatter should check out develop like the code for getting maintainers already does and run spack style from there. We would need a change to spack style so that it could run in a different location than it’s own spack.

[vsoch]

okay I'll hold off on the spack style commands for now then.

[tgamblin]

You could just get it working running from the PR and I’ll add the option.

the bot already knows how to check out develop and/or the PR. It’s a pretty simple fix.

[adamjstewart]

@adamjstewart spack-bot was taken for the app name. Do you own that?

I'm pretty sure I own that. I forget, where do apps live, somewhere in my settings? I'll see if I can transfer ownership or maybe just delete.

[vsoch]

Yeah go to settings -> Applications should be in the first tab.

[adamjstewart]

Found it, it was under Settings -> Developer Settings -> GitHub Apps. @tgamblin do you want me to transfer to you, transfer to @spack, or just delete it so you can rename spack-maintainer-bot?

[vsoch]

Sorry, you're right that's where the main page is - the page I am referring to is when you have it installed in your account (which I do) and it links to that same developer page since I own it.

[vsoch]

I don't know if it's my development environment, but I'm seeing pretty inconsistent behavior. E.g., it's working fine, and then I open a PR and there is no response and I have to restart the docker-compose containers for spack bot to wake up again. Have either of you seen the bot fail to respond in it's current implementation (suggesting it's an issue)?

[adamjstewart]

I haven't tested the current implementation locally, but I've definitely noticed a few PRs that it ignored for some reason. Are the logs available somewhere where I can view them and see why it skipped a PR?

[vsoch]

I haven't tested the current implementation locally, but I've definitely noticed a few PRs that it ignored for some reason. Are the logs available somewhere where I can view them and see why it skipped a PR?

They are probably on AWS where it's deployed? I don't know where the actual one is deployed. What I'm seeing with mine (I'm staring at the logs on my screen) is that sometimes I don't even see any activity at all (when there should be). That could be either something about the app running in docker, or using the service smee (maybe it's not perfect). When it happens again I'll just try restarting smee to see if that resolves it (in which case we don't need to worry because smee is only a development thing).

[vsoch]

@adamjstewart with respect to the testing procedure I'm thinking it would be possible to do if we can disable requiring the payload secret - is that possible with this sansio library?

[adamjstewart]

The current implementation is pretty different from my original, I think @tgamblin would have to answer that one.

[vsoch]

It looks like it might just skip it if we don't have a signature header or secret https://github.com/brettcannon/gidgethub/blob/2a8f57f438a14cf3bb7b961d79b8562d37b0a6e8/gidgethub/sansio.py#L126

[vsoch]

Anyhoo I can try that out for the next round of work for a PR.

[vsoch]

I've been testing different ways to test today, and the most "real world mimicking" one would be to save requests as json, and then submit them to the server, and disable needing the token. While this would work, we also have to mock all of the responses from GitHub that happen within the function (e.g., the collaborators_url and members url, just as example). At best, we would be testing something that is a little different - our best effort at a mock, which can't capture if an API endpoint changes, or if there is a case we haven't seen yet. Personally speaking this would not give me confidence to go from that to production. If it's relatively easy to install spackbot for a repository - why not just have a development version of the app connected to a development repository that we can open a pull request and then just test the interactions? We could even automate that step of opening the PR. Thoughts?

[tgamblin]

@vsoch I added you as an owner on the @spack-test org. We need a development org as well as a development repository since the bot invites people to the org and its teams, but that's what I used to test.

If all this can be automated with the spack-test org that would be very cool.

[vsoch]

I'll try creating a "new" development spack bot there. Would it be possible to have an actual server for the development app to run? I was thinking if we have such a staging server we could have an automated deploy there when a spack-bot PR is merged, and that is followed up by some kind of testing with a workflow that does something simple like open an PR and comment on it (it would still require a human to look and see that things are working). We could also have it happen before a PR is merged - something like a workflow that is triggered by a maintainer labeling the PR "do testing" or something like that. The staging server would require a constistent URL to register with the test action, and it would also mean that it could be broken at any point in time. Are there docs anywhere about how the current bot is deployed/updated?

[tgamblin]

Sure -- you can create a deployment with a new domain -- just base it on this one: https://github.com/spack/spack-infrastructure/tree/main/k8s/spack/spackbot-spack-io.

I think you could just call it spackbot-dev.spack.io or something. If you just make YAML files like the ones there and kubectl apply -f them, the domain will be created automatically. One you get it working you can submit a PR with the YAMLs to https://github.com/spack/spack-infrastructure.

[vsoch]

Is there documentation for the steps to do that? I don't have experience with kubernetes or the particular setup that the lab has.

[vsoch]

And the other option is to try to sit on a consistent smee URL and then use that in the CI, and developers that want to do stuffs locally would have to try and use that setup too. The issue there is that running smee in CI requires the pem and other secrets. The same is true if someone wants to run the app locally.