Define approach for adding last changed by and last reviewed by to documents

[mmr1909]

This discussion contains possible approaches how to add approver/reviewer information to documents.
See issue: #205

[mmr1909]

2025-02-28

Participants

@AlexanderLanin
@hoe-jo
@mmr1909

Minutes

@AlexanderLanin:
Proposed usage of Github GraphQL API instead of REST API https://docs.github.com/en/graphql
Advantage: Reduces number of requests due to possibility of combined complex requests

@mmr1909:
Will investigate how to alter the merge commit log (Github bot/app, action)

Possible approaches to determine review/approver information:

1 GitHub API with PyGithub

Pro:

• Extracts all information from Github
• No modification of merge commit messages necessary
• Works for the current PR

Con:

• Additional Python library is need
• Github API access is needed during document generation
• Additional caching of API calls is needed

2 Store Reviewer information in commit messages

Pro:

• Uses only git functionality
• no github API access necessary

Con:

• Needs automatic modification of the commit message on merge
  • e.g. Github bot/app
  • e.g. Github action
• Commit message is only available for previous PRs << ??

3 Combined approach

Get all modified files with review-header directive from Sphinx
Extract merge commit hashes from last merge commit
Request reviewer/approver information via Github GraphQL API
Pro:

• Reduced number of Github calls
• No modification of merge commit messages necessary

Con:

• Github API access is needed during document generation
• Additional caching of API calls is needed

Untested query: https://chatgpt.com/share/67c86c41-b0c8-800a-b263-c7f700099969

[mmr1909]

Analysis results:

Github bot/app

Idea: bot/app modifies the merge commit message

• high implementation effort
• server application needs hosting
• unclear of the text of the generated commit message can be altered before the merge commit
• could use a commit amend call to modify commit message
• Pre-commit hook?

Github action

• Execution on event pull_request type closed
  (see https://docs.github.com/de/actions/writing-workflows/choosing-when-your-workflow-runs/events-that-trigger-workflows#running-your-pull_request-workflow-when-a-pull-request-merges)
• Extraction of reviewer/approver information via GraphQL API
• Alter merge commit message with commit --amend call

Disadvantage: Only works for merged PRs

[AlexanderLanin]

I've looked into that pull_request.closed event. It is triggered after main has been modified. So it's already too late for our use case.

[AlexanderLanin]

I wanna try a new summary. Let's see if that helps or not.

Basically the approach is always the same: we use GitHub API and query the reviewer etc.

When to query the API:

• Before merge
• Right after merge
• Daily via CI
• Locally on demand

Cache can be:

• in commit msg (only with "before merge" option)
• in git branch (this includes git notes)
• in separate repository
• in a file in git
• only locally
• in bazel cache

This makes roughly 4*6=24 possible approaches.

---

Before merge

Option 1) GitHub webhook -> no proper hook available, see above
Option 2) GitHub Actions -> no proper trigger available, see above
Option 3) Do not use GitHub UI/cli for merging -> veto by @AlexanderLanin

❌ Therefore before merge is off the table

Right after merge + Daily via CI

Trivially implementable with GitHub Actions. The more interesting question is where to cache the data.

Caching

If we cache the data somewhere in git:

• we want to avoid any kind of merge conflicts
• we want a local cache (just mentioning, therefore e.g. storing as pure text in GitHub issues or gist is not an option)

Open Points

• Just how expensive is it to query the reviewers based on 50-100 files + hashes retrieved from git log? That will decide how rigorous our caching needs to be.

[mmr1909]

2025-03-07

Participants

@AlexanderLanin
@mmr1909
@MaximilianSoerenPollak

Minutes

• Open Points:
  • "before merge" might be possible after all. It's really unclear based on available documentation. We'll need to simply try it ourselfes.
  • Just how expensive is it to query the reviewers based on 50-100 files + hashes retrieved from git log? That will decide how rigorous our caching needs to be. Build GraphQL query and try it.
• Most promising options:
  • "before merge" if it really triggers before merge
  • cache locally in some yaml file (for docs:docs)
  • just ignore the problem, since we do not have that many documents at the moment. just query every time.

[mmr1909]

The following query has a total cost of 1.

query get_reviewer($owner: String!, $repo_name: String!, $commit_hash: String!){
    rateLimit {
        limit
        remaining
        used
        resetAt
        cost
    }
    repository(owner: $owner, name: $repo_name) {
        object(expression: $commit_hash) {
            ... on Commit {
                associatedPullRequests(first:1) {
                    nodes {
                        author {
                            login
                        }
                        approver: reviews(first:100, states: APPROVED) {
                            nodes {
                                state
                                author {
                                    login
                                }
                            }
                        }
                        reviewer: reviews(first:100, states: [COMMENTED, CHANGES_REQUESTED, DISMISSED]) {
                            nodes {
                                state
                                author {
                                    login
                                }
                            }
                        }
                    }
                }
            }
        }
    }
}

[mmr1909]

Accessing team information is blocked.

The following request

query {
  organization(login: "eclipse-score"){
    teams(first: 10) {
      nodes {
        name
      }
    }
  }

returns this error message:

"message": "Although you appear to have the correct authorization credentials, the eclipse-score organization has enabled OAuth App access restrictions, meaning that data access to third-parties is limited. For more information on these restrictions, including how to enable this app, visit https://docs.github.com/articles/restricting-access-to-your-organization-s-data/"

Checking whether an approver is part of an approver group is therefore not possible via GraphQL API.

[AlexanderLanin]

Works fine for me. Must be an issue with your token.

[{'name': 'automotive-score-contributors'},
 {'name': 'automotive-score-committers'},
 {'name': 'automotive-score-project-leads'},
 {'name': 'automotive-score-security'},
 {'name': 'eclipsefdn-security'},
 {'name': 'eclipsefdn-releng'},
 {'name': 'automotive-score-technical-leads'},
 {'name': 'community-operational'},
 {'name': 'community-testing'},
 {'name': 'community-architecture'},
 {'name': 'community-process'},
 {'name': 'cft-persistency'},
 {'name': 'cft-logging'},
 {'name': 'cft-communication'},
 {'name': 'cft-orchestration'},
 {'name': 'cft-feo'}]

[MaximilianSoerenPollak]

qCould be becuse you are a committer maybe?

[mmr1909]

I used the GrapQL Explorer.

[mmr1909]

With a newly generate token it works!

[AlexanderLanin]

GitHub Triggers.

Setup:

  o---o---o---o  (main)
       \
        o---o---o  (branchy)
                 |
                 PR
                 |
                 v
  o---o---o---o---o  (main, after merge)

Workflows are triggered by:

1. push to "branchy"
2. opened PR for branch -> main
3. push on main (after merge)
4. PR closed (even later)

=> PR closed is way after merge has completed.

[AlexanderLanin]

I've asked ChatGPT to construct a query to find the reviewers of all our rst files. It came up with this script based on the one by @mmr1909 above.

import subprocess

# GitHub repository details
ORG = "eclipse-score"
REPO = "score"

# Get the last commit for each .rst file
files = (
    subprocess.run(
        "git ls-files '*.rst' | while read file; do echo \"$(git log -1 --format='%H' -- $file) $file\"; done",
        shell=True,
        capture_output=True,
        text=True,
    )
    .stdout.strip()
    .split("\n")
)

# Generate GraphQL query
query = f"""query {{
    rateLimit {{
        limit
        remaining
        used
        resetAt
        cost
    }}
  repository(owner: "{ORG}", name: "{REPO}") {{
"""
for i, line in enumerate(files):
    commit_hash, file_path = line.split(" ", 1)
    query += f'    commit{i}: object(expression: "{commit_hash}") {{\n'
    query += """                  ... on Commit {
        associatedPullRequests(first:10) {
            nodes {
                author {
                    login
                }
                approver: reviews(first:100, states: APPROVED) {
                    nodes {
                        state
                        author {
                            login
                        }
                    }
                }
                reviewer: reviews(first:100, states: [COMMENTED, CHANGES_REQUESTED, DISMISSED]) {
                    nodes {
                        state
                        author {
                            login
                        }
                    }
                }
            }
        }
      }
    }\n"""
query += "  }\n}"

print(query)

This generates the following .graphql query:

query {
  rateLimit {
    limit
    remaining
    used
    resetAt
    cost
  }
  repository(owner: "eclipse-score", name: "score") {
    commit0: object(expression: "a7634dbccd021c3a059b6238391528d18a1b4272") {
      ... on Commit {
        associatedPullRequests(first: 10) {
          nodes {
            author {
              login
            }
            approver: reviews(first: 100, states: APPROVED) {
              nodes {
                state
                author {
                  login
                }
              }
            }
            reviewer: reviews(
              first: 100
              states: [COMMENTED, CHANGES_REQUESTED, DISMISSED]
            ) {
              nodes {
                state
                author {
                  login
                }
              }
            }
          }
        }
      }
    }
    commit1: object(expression: "a7634dbccd021c3a059b6238391528d18a1b4272") {
      ... on Commit {
        associatedPullRequests(first: 10) {
          nodes {
            author {
              login
            }
            approver: reviews(first: 100, states: APPROVED) {
              nodes {
                state
                author {
                  login
                }
              }
            }
            reviewer: reviews(
              first: 100
              states: [COMMENTED, CHANGES_REQUESTED, DISMISSED]
            ) {
              nodes {
                state
                author {
                  login
                }
              }
            }
          }
        }
      }
    }
    # ... (shortened for brevity)
    commit132: object(expression: "f03a8528be21e47eeff1e3d8590c4642215cec83") {
      ... on Commit {
        associatedPullRequests(first: 10) {
          nodes {
            author {
              login
            }
            approver: reviews(first: 100, states: APPROVED) {
              nodes {
                state
                author {
                  login
                }
              }
            }
            reviewer: reviews(
              first: 100
              states: [COMMENTED, CHANGES_REQUESTED, DISMISSED]
            ) {
              nodes {
                state
                author {
                  login
                }
              }
            }
          }
        }
      }
    }
  }
}

That runs in ~4 seconds and the cost is reported as 28.
So a rough rule might be, that combining multiple commits into one query reduces the query-costs by 5.

[AlexanderLanin]

With a limit of 3000 points per hour, that means that we can build the docu ~100 times per hour before hitting the limit.
It seems @MaximilianSoerenPollak proposal of "keep it simple & stupid" is the right one here.

Although it's painful from a design point of view.

[AlexanderLanin]

There is one more point we need to figure out, before putting this away as a low prio feature:

Can and should we run that from :docs?
We need to run git log and access the GitHub API when building the documentation.
As far as I understand the ideas behind bazel, the answer is "no, oh my god, no!" 😆
Any solution will violate basic guarantess of bazel build?!

Some ideas:

1. pass .git to bazel by adding build --sandbox_writable_path=$(git rev-parse --show-toplevel)/.git to .bazelrc.
2. add something like build --workspace_status_command=run_git_log.sh to .bazelrc to gather information from .git before building
3. run a script via ctx.execute as a repository_rule to collect information from .git. However that will only update on bazel sync.
4. run it manually before building docs, e.g. bazel run //docs:update_reviewer_information
   1. Option: wrap both scripts into bazel run //docs:update_reviewer_information_and_build_docs
5. aspects get the absolute paths to the original files in the workspace, so we can call git log from the aspect?!
6. rules get the absolute paths to the original files in the workspace, so we can call git log from the rule?!
7. drop support for docs:docs

@ltekieli we started that discussion in ~December. It's back! Would you comment on this? Or should we have a call instead?

Note: even if we abandon the idea of querying the info on demand. Instead query after merge (on push to main), and store the information somewhere. That would help reproducibility in case github is offline. But we still need to run git log, to get the correct info from our cache (e.g. branch, different repository, ...).

[mmr1909]

We could also use the GraphQL to determine the latest commit:

query {
  repository(owner: "eclipse-score", name: "score") {
    ref(qualifiedName: "main") {
      target {
        ... on Commit {
          history(first: 1, path: "docs/index.rst") {
            nodes {
              oid
            }
          }
        }
      }
    }
  }
}

[mmr1909]

2025-03-12

Participants

@AlexanderLanin
@mmr1909
@MaximilianSoerenPollak

Minutes

@AlexanderLanin:
Presentation of wrapper class for GitHub requests
Decision: This approach will not be used for the current implementation of the header-service for. The implementation might be adapted later.

Next steps: @mmr1909 will update the header-service to use GraphQL. Implementation for git log extraction will be removed.