Repository navigation
NO-ISSUE: Assign osac-ci-bot, not a maintainer, to maintainers reviews - #288
Conversation
Turn review request delegation off for the maintainers team: any member may approve, and GitHub must not pick one of them. The resource keeps no review_request_delegation block, so every apply keeps it off. Notifications are the team's notification setting, which the provider cannot manage; the README says how it is set. Signed-off-by: Elior Erez <eerez@redhat.com>
|
Caution Review failedThe pull request is closed.
 |
| Layer / File(s) | Summary |
|---|---|
Maintainer team configuration teams.tf |
Adds osac-ci-bot as a direct member of maintainers. Replaces the prior settings resource with a removed block configured not to destroy it. |
Post-apply assignment configuration .github/workflows/apply.yaml, README.md |
After Terraform apply is not skipped and the workflow is not cancelled, the workflow queries the team and its direct members. It errors if the team is missing and warns without changing settings if the bot is not a direct member. Otherwise, it sets load-balanced assignment to one member, disables team-wide notifications and child-team inclusion, and checks the returned assignment settings. The README describes the assignment behavior and notes that folder-owning teams remain requested and notified. |
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix
Sequence Diagram(s)
sequenceDiagram
participant ApplyWorkflow
participant GitHubGraphQL
ApplyWorkflow->>GitHubGraphQL: Query maintainers team and direct members
GitHubGraphQL-->>ApplyWorkflow: Return team and membership data
ApplyWorkflow->>GitHubGraphQL: Set assignment fields if osac-ci-bot is a direct member
GitHubGraphQL-->>ApplyWorkflow: Return mutation result
Suggested labels: risk:ask
Suggested reviewers: sk-ilya, larsks
Merge Risk: 
8f141
-
Bot-only assignment is not guaranteed, and malformed API data can silently skip configuration; resolve both before relying on this workflow.
Pre-merge checks: 
-
Check name Status Explanation Resolution Ai-Attribution The PR uses an AI tool identified as CodeRabbit. All four commits in the reviewed range contain only a Signed-off-by trailer. None contains an Assisted-by: or Generated-by: trailer, and no Co-Authored… Add a Red Hat Assisted-by: or Generated-by: trailer naming CodeRabbit to each applicable commit. Do not add Co-Authored-By for the AI tool. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly describes the main change: assigning osac-ci-bottomaintainersreviews instead of a maintainer.Docstring Coverage No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. No-Hardcoded-Secrets No hardcoded secret is introduced. The changed workflow uses the generated GitHub App token via ${{ steps.generate-token.outputs.token }}, and the new literals are team names, GraphQL settings, and …No-Weak-Crypto The pull request adds no MD5, SHA1, DES, 3DES, RC4, Blowfish, or ECB usage, and no custom cryptography. The only token-related addition is passing GH_TOKEN to gh for GraphQL authentication; it is not … No-Injection-Vectors No configured injection vector is introduced. The new workflow uses fixed GraphQL queries, passes ORGandteam_idas quotedgharguments, and passesSINKtojqthrough--arg. No SQL concat…Container-Privileges The pull request changes only README.md, teams.tf, and a GitHub Actions workflow. The workflow uses runs-on: ubuntu-latestand adds no container or Kubernetes privilege settings. No changed code int…No-Sensitive-Data-In-Logs PASS: The pull request adds no sensitive-data logging. The GraphQL response containing the team ID and member logins is captured in answerand only queried locally. The only emitted values are fixed…Full details: Docstring Coverage
-
Explanation
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 0 files. (1 skipped: 1 unsupported.)
Full details: No-Hardcoded-Secrets
-
Explanation
No hardcoded secret is introduced. The changed workflow uses the generated GitHub App token via
${{ steps.generate-token.outputs.token }}, and the new literals are team names, GraphQL settings, and theosac-ci-botusername. No API-key/token/password assignment, private-key material, embedded credentials, or qualifying long base64 string appears in the added lines.
Full details: No-Weak-Crypto
-
Explanation
The pull request adds no MD5, SHA1, DES, 3DES, RC4, Blowfish, or ECB usage, and no custom cryptography. The only token-related addition is passing GH_TOKEN to gh for GraphQL authentication; it is not compared or processed cryptographically. The jq index checks the public team-member login against osac-ci-bot, not a secret or token.
Full details: No-Injection-Vectors
-
Explanation
No configured injection vector is introduced. The new workflow uses fixed GraphQL queries, passes
ORGandteam_idas quotedgharguments, and passesSINKtojqthrough--arg. No SQL concatenation,shell=True, eval/exec, unsafe YAML loading, pickle loading,os.system, ordangerouslySetInnerHTMLappears in the changed code.
Full details: Container-Privileges
-
Explanation
The pull request changes only README.md, teams.tf, and a GitHub Actions workflow. The workflow uses
runs-on: ubuntu-latestand adds no container or Kubernetes privilege settings. No changed code introducesprivileged: true, host PID/network/IPC,SYS_ADMIN,allowPrivilegeEscalation: true, or root container execution.
Full details: No-Sensitive-Data-In-Logs
-
Explanation
PASS: The pull request adds no sensitive-data logging. The GraphQL response containing the team ID and member logins is captured in
answerand only queried locally. The only emitted values are fixed warning/error text and the mutation's selected public team settings (slug, booleans, and member count). No token, password, API key, email, session ID, hostname, or customer data is printed.
-
Full details: Ai-Attribution
-
Explanation
The PR uses an AI tool identified as CodeRabbit. All four commits in the reviewed range contain only a Signed-off-by trailer. None contains an Assisted-by: or Generated-by: trailer, and no Co-Authored-By trailer is present.
Resolution
Add a Red Hat Assisted-by: or Generated-by: trailer naming CodeRabbit to each applicable commit. Do not add Co-Authored-By for the AI tool.
-
The provider sets notification_setting on every apply and defaults it to notifications_enabled, so a setting changed by hand is undone at the next apply. State it as notifications_disabled in teams.tf, and correct the README, which said the provider cannot manage it. Signed-off-by: Elior Erez <eerez@redhat.com>
The notification setting of a team does not cover review requests, so drop it. Make osac-ci-bot the only direct member of maintainers and set review assignment after every apply (the provider cannot set includeChildTeamMembers): it picks the bot, notifies only the bot, and leaves the members of the working groups out. Any of them may still approve. github_team_settings is forgotten, not destroyed. Signed-off-by: Elior Erez <eerez@redhat.com>
eliorerz
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
A broken resource elsewhere must not hold the maintainers review assignment back, so the step runs after a failed apply too. It first asks for the direct members of the team and does nothing unless osac-ci-bot is one of them: with nobody eligible the assignment would have no one to pick. Signed-off-by: Elior Erez <eerez@redhat.com>
Read back what GitHub says it stored after the mutation (enabled, one member, team not notified) and fail the run when it is not what was asked, so a setting that did not take is not silent. Signed-off-by: Elior Erez <eerez@redhat.com>
Only direct members of maintainers can be assigned, so any other direct member would be picked for pull requests. The step now fails when it finds one besides osac-ci-bot, and still leaves the assignment alone when the bot is not a member (yet). The README says so. Signed-off-by: Elior Erez <eerez@redhat.com>
Changes how the
maintainersteam is assigned review requests, so that nobody is picked at random and nobody is broadcast to. This replaces the earlier content of this pull request (turning delegation off and the team notification setting do not do it: GitHub's docs say the notification setting "does not affect notifications for reviews requested from the team", as @sk-ilya pointed out).What
CODEOWNERS sends every path no working group owns to
@osac-project/maintainers, and any member of a working group may approve (members of the child teams count as members ofmaintainers). GitHub notifies every member of a team that is requested for review; the only switch is review assignment, which assigns members and notifies only them. So:osac-ci-botbecomes the only direct member ofmaintainers(github_team_membership.maintainers_review_sink). It is already an organization member and inwg-infra. It approves nothing, and nothing waits for it.maintainers:LOAD_BALANCE, one member, team not notified, andincludeChildTeamMembers: false, so only direct members are eligible: it always picks the bot. A pull request listsmaintainersandosac-ci-botas reviewers; the bot's request stays pending.includeChildTeamMembers, soapply.yamlsets all of it with the GraphQL mutationupdateTeamReviewAssignmentafter every apply (scheduled ones included). It is idempotent and puts the setting back if someone changes it by hand.github_team_settings.maintainers(from NO-ISSUE: Assign one load-balanced maintainers reviewer #286) is forgotten with aremovedblock anddestroy = false, so nothing is destroyed or switched off, and the apply step owns the setting.wg-infra,wg-vmaas, ...) are requested and notified as usual. The README explains the design.Checked / not checked
includeChildTeamMembers(GitHub's schema), the enum and field types match the query, and the apply token already ran the same mutation throughgithub_team_settings.maintainersandosac-ci-bot, and no person should be notified.tofu fmtruns in CI only (notofuwhere I wrote it).Please review, @sk-ilya.
Summary
github_team_membership.maintainers_review_sink, makingosac-ci-bota direct member ofmaintainers. Removes the oldgithub_team_settings.maintainersresource from active management without destroying it.TF Applyis not skipped,.github/workflows/apply.yamlchecks for themaintainersteam and the bot’s direct membership. If the team is missing, the step fails. If the bot is not a direct member, it warns and leaves the assignment unchanged. Otherwise, it runsupdateTeamReviewAssignmentwith load balancing, one member, child-team members excluded, team notifications disabled, and the team request retained. It verifies the returned enabled, member-count, and notification settings. The step also runs after a failed apply unless the workflow is cancelled.Compatibility and verification
The change alters GitHub team review-assignment behavior and stops managing the previous provider resource without destroying its setting. The workflow cannot read back
includeChildTeamMembers, so that value is not covered by its read-back check. The intended GitHub behavior remains unverified. No test results are available.Risk classification
risk:ask — Applied provisionally because the intended assignment and notification behavior has not been verified. The risk-label criteria were not supplied, so this classification cannot be matched to documented criteria. risk:show may be a nearby alternative, but whether it qualifies cannot be determined without those criteria.