Add support for fixing style as a task in another process #58
Conversation
scottwittenburg
force-pushed
the
fix-style-in-another-process
branch
3 times, most recently
from
7a3891c to
1633c72
Compare
scottwittenburg
force-pushed
the
fix-style-in-another-process
branch
11 times, most recently
from
4b9a7b0 to
16ff8e8
Compare
- [x] Install dependencies *before* bundling up all the code, so that
we don't have to reinstall all dependencies unless we modify
requirements.txt.
- [x] Add fingerprint validation for ssh-keyscan so that we don't get
MITM'd if github's host keys change.
- [x] Do the ssh-keyscan at image creation time instead of every time
the app runs. We'll want to rebuild the image if github's host
keys change.
scottwittenburg
force-pushed
the
fix-style-in-another-process
branch
from
16ff8e8 to
23b59ce
Compare
scottwittenburg
force-pushed
the
fix-style-in-another-process
branch
2 times, most recently
from
17da8d6 to
fee2c2b
Compare
|
@tgamblin who would you like to review this before it can be merged? Since spack-infrastructure is pointing at some specific old image by hash for the spackbot webservice, merging this isn't likely to have any adverse effects on the deployed spackbot, at least not until we merge the associated spack infrastructure PR. |
|
@tgamblin Reminder this is pending your review 😀 |
tgamblin
left a comment
tgamblin
left a comment
There was a problem hiding this comment.
some minor change requests but mostly LGTM!
scottwittenburg
force-pushed
the
fix-style-in-another-process
branch
3 times, most recently
from
9a51bf1 to
64ed9e7
Compare
scottwittenburg
force-pushed
the
fix-style-in-another-process
branch
from
64ed9e7 to
b0ebe44
Compare
|
@zackgalbreath Do you mind looking over this once? Thanks! |
- Move body of style fix into new worker module - Add Dockerfile/entrypoint defining worker container - Allow log level to be specified in environment (SPACKBOTLOGLEVEL) - Update gh workflow to build images for webservice and worker - Fix branch name used to run pipelines (remove the 'github/' prefix)
scottwittenburg
force-pushed
the
fix-style-in-another-process
branch
from
b0ebe44 to
1dc382c
Compare
| "https://official-joke-api.appspot.com/jokes/programming/random" | ||
| ) | ||
| except Exception: | ||
| return "To be honest, I haven't heard any good jokes lately." |
There was a problem hiding this comment.
I like the idea too but every time I try the site I get a Server Error.
There was a problem hiding this comment.
The jokes were already there (thanks to Vanessa). I just meant the exception response.
There was a problem hiding this comment.
I think both are great. I was commenting on the overall feature and the exception.
4 participants
To get spackbot main deployable again, try to fix the current bug by performing the long-running style fix in another process using the
rq(Redis Queue) module. This brings some of the functionality from #45, so we'll need to revisit that if this gets merged first.This will need to be coupled with a change to
spack-infrastructureas well, in order to provision a deployment for the worker and a connection to redis.Arguments sent to worker functions have to be stored in redis, and as such, I think it's best to avoiding sending complex objects (pickling is the default argument serializer I believe). I could be wrong, so some testing is in order to be sure.
For this reason, I'm currently not sending theghobject to the worker, so once we ship the task, we can no longer post comments back to the users PR after that. This may not be acceptable, and so we may have to come up with a workaround of some kind, or maybe we could look into how we can shipgidgethub.aiohttpobjects to the worker.Update: I refactored to pass the authentication token to the worker task to allow creating a new github session there, and we can now post comments back to the PR from the worker.
I tested this work using the temporary/testing deployment
spackbotdev-spack-ioimplemented here. See this spack PR (ignore all but most recent comments) for examples of 1) pushing automatic style fix, and 2) finding that no further fixes could be made.This change also adds the ability to specify a custom log level, ala #50 (taking into account the review suggestions on that PR).