Set up CI and AI pull request review via Forgejo actions #300
No reviewers
kparal
Labels
No labels
Closed As
Duplicate
Closed As
Fixed
Closed As
Invalid
discussions
easyfix
enhancement
task
ai-review-please
Backlog Status
Needs Review
Backlog Status
Ready
chore
documentation
points
01
points
02
points
03
points
05
points
08
points
13
pr2jira
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Sprint Status
Blocked
Sprint Status
Done
Sprint Status
In Progress
Sprint Status
Review
Sprint Status
To Do
Technical Debt
Work Item
Bug
Work Item
Epic
Work Item
Spike
Work Item
Task
Work Item
User Story
No milestone
No project
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
quality/blockerbugs!300
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "tox-ci-ai"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Set up CI and AI pull request review via Forgejo actions
CI is a bit tricky because the requirement files are designed
to match the rather odd environment we deploy blockerbugs in in
production: Python 3.11 on RHEL 9 with almost all dependencies
installed via pip. This includes compiling gssapi and krb5. So
instead of my usual "just throw tox at it" approach, this
replicates that same environment: we use an Alma 9 container,
install the Python bits and the necessary compiler and devel
dependencies, then use pip to install all the deps. Then we run
the linters and the tests (not using
runbecause this ancientpip on this ancient Python on this ancient RHEL doesn't install
the binaries, you have to use
python -m).The linters fail at present because there are lots of real
problems; I assume nobody ran these checks for a while. I included
a fix for the biggest flood of mypy issues, but otherwise I
figured to leave the problems alone because I don't want to
cause conflicts with the forgejo port PR. I figure once we get
that PR merged we can then fix up the linting issues. But at least
we'll have tests and AI review run on that PR while we work on
it.
Signed-off-by: Adam Williamson awilliam@redhat.com
77db88f3fa7ade9a1ff57ade9a1ff5882c142343882c142343bb18c238dbbb18c238dba4473cd687a4473cd687fbc61baa96fbc61baa9626942d0d7126942d0d7108454ce92908454ce9292456a23eaa2456a23eaa4e5bd648b04e5bd648b0a53e4daeb9a53e4daeb95ca085bc315ca085bc314bc6a86a784bc6a86a78c58d3fdec0c58d3fdec033343000103bf3fdef21bcb9845e97Sooo, this is great, but the testing process is likely to get considerably changed in #296 (PR #299) - it's getting run in a container instead, similarly to testdays-web. I think we should leave this open until #296 is done, and then we can look at what changes are required here. Please note that updating the baseline OS/Python/libraries is definitely the better way forward. It's all archaic, because at some point we matched it to our production OpenShift environment and then nobody touched it for a long time 🙂
Please note that we have a similar ticket in quality/testdays-web#96 , which should already look similar to how BBA might end up looking. So if you want to look there and prepare a PR for it, you're very welcome 🙂
From a quick look at that PR, I'm not sure it would change how the tests are invoked at all. It uses
testcontainers, which does all the container stuff 'internally'. The container environment is not used as the build environment, AFAICS, so it doesn't affect the need to install development packages so gssapi and krb5 can be built, I don't think - I think that still happens the same way. The containers are used as an execution environment for doing E2E testing (which was not present before, only unit tests). In fact, AFAICS, blockerbugs itself isn't run in a container; the containers are used for deploying Pagure and Postgres, for blockerbugs to run against.AFAICS you still trigger the tests by just running
pytest, which is what this PR does. And it barely touchesrequirements.txtat all, at least at present. So, unless I'm missing something, I think these are actually probably perfectly compatible.Ah, hmm, but some experimenting with testdays-web does expose a problem: our infra-hosted Forgejo actions runners are container-based, and nested containers are difficult. We'd probably need to set up VM runners, somehow, to get it to work.
AI review and running the linters might still be useful, though?
bcb9845e97873995d622Update: @jgroman figured out how to finesse the testcontainers situation in quality/testdays-web#99 , hopefully we can do the same here. There's a question whether we should merge this first then have #299 make the adjustments so the new container-based tests work in CI, or merge #299 first and make adjustments to the testcontainers stuff in this PR. I kinda like the idea of merging this first, but WDYT?
@ -0,0 +1,152 @@## Project OverviewWhat is the purpose of this document? I originally assumed it's some leftover that was included in this PR by accident. At least quality/testdays-web!99 didn't need one.
If this is intentional, I'd like to understand the process behind it (and ideally have some hyperlink to the documentation, so that I can add it here). What is the envisioned life cycle of this doc? Obviously it will get outdated quickly. Is this supposed to be regenerated regularly? Since AI is used to generate it, why is it better to have it stored in a repo, rather than let AI generate it on the fly during the PR review, in a freshly current form, and then throw it away?
I think the main reason for keeping this file around is that while originally it is generated, it can be manually updated with details which were not picked up by context-generator and you think should be mentioned anyway. Also I suspect that having previous version might even help the context-generator build updated version of this file.
Here are some notes from ai-reviewer docs: https://gitlab.com/redhat/edge/ci-cd/ai-code-review/-/blob/main/docs/user-guide.md#-project-context-configuration
The purpose is to provide context about the project to the review tool. ai-code-review is written to always read this file and include it in the review context if it's present.
ai-code-review (and other non-agentic review tools, generally) doesn't put the entire project in the context when querying the model. They just include the PR diff itself (and the commit message, and existing comments on the PR). So the review sometimes has issues or misses problems because the model isn't aware of significant context in untouched files, or outside the diff context in modified files.
You can get better results by using an agentic workflow and letting it check out the entire project and look through the commit history and stuff, but that's expensive.
The idea here is to instead have an agentic thing generate this static "here's how the project works overall" document periodically, and have the non-agentic review tool take that context into account so it can hopefully produce more comprehensive and accurate reviews.
The generator is https://github.com/juanje/context-generator . I did get a PR merged recently that makes it add a README.md explaining what the file is for and how it's generated - https://github.com/juanje/context-generator/pull/2 . I probably generated the file for this PR before that was merged. We can re-generate it, or just add an appropriate README.md manually, I guess.
So far the idea is just to re-generate or update the file manually, ad hoc, whenever it seems necessary. You could implement some kind of timed workflow to use opencode to update it every month, or something, but I don't know if it's worth the effort.
Edit: if I didn't put one in the testdays-web PR, it was an oversight or I did that just before I started making these files. I've been intending to include one of these files in every repo I'm enabling ai-code-review on, I think it's a good concept.
I added the README.md.
Your reply was very helpful, thanks. I had no idea that during a PR review the AI only sees the diff and not the whole project. I don't think that's really a difference between agent and non-agent AI (as I understand it), rather it's all just about cost and speed?
Anyway, I played with context-generator, generated the context myself just to see how it works. IIUIC, the manual section in it is pre-populated, but the idea is that we maintain it ourselves, to limit the errors/mistakes of AI reviews. Ok, sounds fine. I'm not going to spend time to fine-tuning it at this moment, but we can expand it according to future AI reviews.
@ -0,0 +22,4 @@python3.11 -m venv bb-envsource bb-env/bin/activatepython -m pip install -r requirements.txt -r requirements-development.txt- name: Lint and testCan we have two separate steps, one for linting and one for running unit tests? Currently in Forge I see:
That's not helpful to me, because the there are lots of linter errors, but those are really not important. The test suite is important, and that's currently hidden behind the linter warnings, so that I would need to click Details and inspect it manually every time. In order to make this useful, I believe we need two separate steps and two separate results.
Well...sort of no, unfortunately :(
AFAICS, Actions (both GitHub and Forgejo) has no easy way within a single job to implement 'run A, then run B, report the results separately, and make the overall state failed if A or B failed'. Usually, if A fails, it short-circuits there and never runs B. There is an option to "ignore failure", but this really ignores it - it will go ahead and run B if A fails...and if B passes, the overall result is a pass.
You can make them separate jobs, but then you have to repeat all the setup steps, which seems kinda crazy. Or you can hack it up by having each step dump its exit code to a file, using the 'ignore failure' thing on all of them, then having a final step which figures out whether to report 'pass' or 'fail', I think. But it's all a bit messy. I guess I can take another look and see if one of those is viable, or if I can figure anything else out (there may be some way to have two separate jobs both use an environment set up by a third job, or something).
I guess another approach is to write a little wrapper script for running the checks and gathering the status and reporting it. That's sort of like the "signal file" approach, but implemented more in the repo than in the workflow.
Do we need this aggregated result (A and B)? That would provide the exact same feedback we already have today.
I think it is good enough to just have a glance at PR status and see a line of checkmarks or Xs in there and dig deeper only if necessary. In my opinion it is not that crazy to have separate jobs - spinning up new container and installing dependencies in this project currently takes 2 minutes. It is not instant but I'd say it is acceptable.
Yeah, true. Splitting the jobs just feels like a waste of resources, but it's true, it's not going to melt any glaciers. I can maybe do it that way for now so we can merge this while I'm on PTO, then I can always look at trying to make it more efficient later.
oh, yeah, now I look at it, I especially disliked it because the dep install step requires actual compilation. But, oh well.
I've managed to run tests and linting together, but report them separately 😼 See the updated PR. I only forwarded the success/failure status of the step to a different job, not the full output, so when seeing linting output, you need to look to the test job. But I think this is good enough, and it runs only once.
Oh, I forgot to say - if we actually decide to block PRs on linter errors, then we don't need to have separated jobs. But currently we're not there yet (all the current errors would need to be fixed first), and I'm not even sure it's a good idea. The linters warn about too many little nitpicks like indentation and similar, and present it as an error. So we would first probably need to do some tweaking, what we want to consider an error and what we don't. I'm generally interested in programming warnings (unused variable, etc), but not style warnings (I refuse to call them errors 🙂).
So currently, having the jobs separated is the better option, in my view. (Or running just the test suite and skipping linters would also be OK for me).
FWIW, my personal preference for style linting is to simply enforce "whatever black says". It keeps things clean and simple and avoids philosophical debates. So on all my projects the CI is set to fail if black would change anything. You usually have to do one big 'black-ify' commit to get it into a 'correct' state to start with, though.
Yes, I want to try it, but not as part of the currently pending large PRs.
873995d6222265dc0fe72265dc0fe7ec1e3191dc@ -0,0 +6,4 @@jobs:test:runs-on: fedorapermissions:I can't find any documentation for this key. But I've tested that if I omit it, everything still runs just fine. I see that the ai-code-review includes this in its example. But I guess we can drop it here in the CI workflow.
I must have copied the file from an old base or something.
permissionsare significant on GitHub but not currently implemented on Forgejo.Ok, good to know, thanks.
(Btw I like adding new commits instead of force-pushing during a review, otherwise it's quite hard to inspect the diff in your git client (I see that Forge does a decent job, though). It's easy to squash it all after the review is done.)
7db5783c060e2dc77e73The problem I see with your approach, @kparal , is that we won't do the linter checks at all if the tests fail.
FWIW, there seems to be one more option suggested by CC:
The critical setting is fail-fast: false - this tells the runner to continue all matrix jobs even if one fails. Each matrix entry shows as a separate check.
@adamwill wrote in #300 (comment):
Ok, fixed by running linters first.
@jgroman wrote in #300 (comment):
According to documentation each of those also run as a separate job (full setup in each), so I believe this wouldn't help us (it would essentially be the same as having the static two jobs, like before).
@kparal You are right, I missed this bit in the docs.
Thanks a lot for this, I know it took me a long time to process it (sorry)! I can merge this if everyone is happy with it.
c8a3a07109112db31ca1Yup, good job everyone, we can fix any issues that show up as we go along.