quality/tickets

Fork 0

Enable AI code review for all our repos, sync the config #907

New issue

Open

opened 2026-05-29 15:38:49 +00:00 by kparal · 6 comments

kparal commented

2026-05-29 15:38:49 +00:00

Owner

Let's make sure all our code repos can use our AI code review. Let's use the new AICR_API_KEY secret instead of the old GEMINI_API_KEY secret.

However, Adam says it's good to configure AI code review to disable the summary, because otherwise it makes the AI comment too long (I agree), which is annoying in a ticket with repeated AI reviews. Instead of providing a custom config with include_mr_summary: false in all our repos (and then having to update all of them again, if we want to change it or some other options in the future), let's see if we can change the default value itself in https://forge.fedoraproject.org/quality/workflows (and if that changed default value then again be overridden in a specific repo, if needed). For that, we want to test it in staging.

Steps:

Define AICR_API_KEY in prod and stg
Test in stg, whether we can use include_mr_summary: false by default, and override it to true in a particular repo
If everything works, push the changes to https://forge.fedoraproject.org/quality/workflows
Adjust all our code repos to have ai-review.yml workflow enabled by default, and use the same definition
Drop GEMINI_API_KEY as no longer needed

@adamwill will probably be needed to define AICR_API_KEY in prod and stg, because he seems to be the only with access to that key (what about moving it to team's bitwarden collection?)

I'm happy to take this, but anyone feel free to take it yourself, if you want.

Let's make sure all our code repos can use our [AI code review](https://forge.fedoraproject.org/quality/workflows#ai-review-yml). Let's use the new `AICR_API_KEY` secret instead of the old `GEMINI_API_KEY` secret. However, Adam says it's good to configure AI code review to disable the summary, because otherwise it makes the AI comment too long (I agree), which is annoying in a ticket with repeated AI reviews. Instead of providing a custom config with `include_mr_summary: false` in all our repos (and then having to update all of them again, if we want to change it or some other options in the future), let's see if we can change the default value itself in https://forge.fedoraproject.org/quality/workflows (and if that changed default value then again be overridden in a specific repo, if needed). For that, we want to test it in staging. Steps: - [X] Define `AICR_API_KEY` in prod and stg - [ ] Test in stg, whether we can use `include_mr_summary: false` by default, and override it to `true` in a particular repo - [ ] If everything works, push the changes to https://forge.fedoraproject.org/quality/workflows - [ ] Adjust all our code repos to have ai-review.yml workflow enabled by default, and use the same definition - [ ] Drop `GEMINI_API_KEY` as no longer needed @adamwill will probably be needed to define `AICR_API_KEY` in prod and stg, because he seems to be the only with access to that key (what about moving it to team's bitwarden collection?) I'm happy to take this, but anyone feel free to take it yourself, if you want.

kparal added the

labels

2026-05-29 15:38:49 +00:00

kparal referenced this issue from quality/issuebot

2026-05-29 15:40:18 +00:00

add a workflow for AI reviews #21

adamwill commented

2026-05-29 16:24:56 +00:00

Owner

I'm not sure I understand what you're talking about with AICR_API_KEY. The intent is that each repo provides a key as a repo secret. I added AICR_API_KEY as a generic name when I made the action capable of supporting backends other than Gemini (by letting you pass in a config string), but I don't really intend to ever stop it 'supporting' the old name GEMINI_API_KEY, it's easy to do.

Were you just suggesting we should rename the secret in every repo where we currently use the workflow? I mean, sure, we can do that, but it's not really necessary/urgent.

If you were suggesting we somehow set it org wide, a) I don't think we can - org-wide secrets aren't a thing, AFAICS - and b) I don't think it's a good idea. Right now tokens are cheap and it's fine to just share a single key I got with a fairly generic justification across a bunch of repos. We don't know if that's going to be the case forever. I think the "every repo brings its own key" approach is appropriate.

I'm not sure I understand what you're talking about with `AICR_API_KEY`. The intent is that each repo provides a key as a repo secret. I added `AICR_API_KEY` as a generic name when I made the action capable of supporting backends other than Gemini (by letting you pass in a config string), but I don't really intend to ever stop it 'supporting' the old name `GEMINI_API_KEY`, it's easy to do. Were you just suggesting we should rename the secret in every repo where we currently use the workflow? I mean, sure, we can do that, but it's not really necessary/urgent. If you were suggesting we somehow set it org wide, a) I don't think we can - org-wide secrets aren't a thing, AFAICS - and b) I don't think it's a good idea. *Right now* tokens are cheap and it's fine to just share a single key I got with a fairly generic justification across a bunch of repos. We don't know if that's going to be the case forever. I think the "every repo brings its own key" approach is appropriate.

kparal commented

2026-06-02 10:33:01 +00:00

Author

Owner

@adamwill wrote in #907 (comment):

I mean, sure, we can do that, but it's not really necessary/urgent.

Yes, since I expected to create a PR everywhere, I assumed I might update the key name as well, instead of using the old GEMINI_API_KEY. I agree it's not really important if you want to keep compatibility long-term. It would be just a little bit clearer for people (when comparing a repo workflow file with https://forge.fedoraproject.org/quality/workflows#ai-review-yml ).

@adamwill wrote in #907 (comment):

org-wide secrets aren't a thing, AFAICS

Yes they are, that's how we currently do it, and you were the one to configure it :-)
https://forge.fedoraproject.org/org/quality/settings/actions/secrets

Sure, when it's no longer appropriate, we can define it per-repo (or override the org global in a particular repo). But currently I think it's fine.

But I can't do any testing on staging, as long as the secret is not present there. That's why I suggested adding it to Bitwarden, or at least configuring it manually yourself (in https://forge.stg.fedoraproject.org/org/quality/settings/actions/secrets ). Otherwise I'll need to figure out how to request another one and have it approved, before I can try include_mr_summary: false as a default value.

@adamwill wrote in https://forge.fedoraproject.org/quality/tickets/issues/907#issuecomment-779735: > I mean, sure, we can do that, but it's not really necessary/urgent. Yes, since I expected to create a PR everywhere, I assumed I might update the key name as well, instead of using the old `GEMINI_API_KEY`. I agree it's not really important if you want to keep compatibility long-term. It would be just a little bit clearer for people (when comparing a repo workflow file with https://forge.fedoraproject.org/quality/workflows#ai-review-yml ). @adamwill wrote in https://forge.fedoraproject.org/quality/tickets/issues/907#issuecomment-779735: > org-wide secrets aren't a thing, AFAICS Yes they are, that's how we currently do it, and you were the one to configure it :-) https://forge.fedoraproject.org/org/quality/settings/actions/secrets Sure, when it's no longer appropriate, we can define it per-repo (or override the org global in a particular repo). But currently I think it's fine. But I can't do any testing on staging, as long as the secret is not present there. That's why I suggested adding it to Bitwarden, or at least configuring it manually yourself (in https://forge.stg.fedoraproject.org/org/quality/settings/actions/secrets ). Otherwise I'll need to figure out how to request another one and have it approved, before I can try `include_mr_summary: false` as a default value.

adamwill commented

2026-06-02 16:41:33 +00:00

Owner

@kparal wrote in #907 (comment):

Yes they are, that's how we currently do it, and you were the one to configure it :-)
https://forge.fedoraproject.org/org/quality/settings/actions/secrets

Ohhh. Welp. I missed that button. This ticket makes more sense, then! Sure, we can rename the org-wide secret.

@kparal wrote in https://forge.fedoraproject.org/quality/tickets/issues/907#issuecomment-794565: > Yes they are, that's how we currently do it, and you were the one to configure it :-) > https://forge.fedoraproject.org/org/quality/settings/actions/secrets Ohhh. Welp. I missed that button. This ticket makes more sense, then! Sure, we can rename the org-wide secret.

adamwill commented

2026-06-02 16:49:33 +00:00

Owner

OK, I renamed the secret to AICR_API_KEY on prod and added it on stg.

adamwill commented

2026-06-02 23:21:51 +00:00

Owner

D'oh, had to add GEMINI_API_KEY back because the wrapper workflows in the repos refer to it explicitly as GEMINI_API_KEY. We'll have to change all of those before removing it.

kparal commented

2026-06-03 11:34:37 +00:00

Author

Owner

Yes, exactly 🙂️ Thanks for adding the new secrets. I'll try to test changing the default config first, and then submit PRs, but if somebody beats me to it, I really won't mind.

kparal added this to the Nominated next tasks project

2026-07-08 12:57:56 +00:00