-
Notifications
You must be signed in to change notification settings - Fork 6.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ci: update elasticsearch index with merged PR data #67332
Conversation
5f2509b
to
34c36db
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool!
34c36db
to
f9cf665
Compare
f9cf665
to
7ff668e
Compare
- name: checkout | ||
uses: actions/checkout@v3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the first run of this workflow is going to fail because it will check out the base branch (main
) instead of the pull request head (github.event.pull_request.head.sha
) -- that is a good thing because it does not expose ELASTICSEARCH_KEY
wide open if someone decides to change types: [closed]
to something else in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be great to see a slightly better handling of the "2 business days" rule as it leads to not surfacing problematic merges -- the rest of my comments can be seen as non-blocking
scripts/ci/stats/merged_prs.py
Outdated
if pr.assignee: | ||
prj['assignee'] = pr.assignee.login | ||
prj['reviewed_by_assignee'] = "yes" if pr.assignee.login in reviewers else "no" | ||
if pr.assignee.login in reviewers or 'Hotfix' in pr.labels or pr.assignee.login == pr.user.login: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's actually more than "assignee has approved" as per the merge criteria, and there should be a +1 from at least one maintainer/collaborator of each affected area.
But in fairness starting to track this might be overkill until we first provide release engineers with some tooling/UI to easily check that a given PR tick these boxes... So feel free to dismiss this comment / keep for a later update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, and we also need the 4 eye rule thing, but that requires more data analysis and thought, can be done later.
scripts/ci/stats/merged_prs.py
Outdated
if pr.assignee and r.user: | ||
if r.user.login == pr.assignee.login: | ||
assignee_reviews = assignee_reviews + 1 | ||
prj = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be great to have a succinct documentation of the schema, for example in the main()
docstring? Just looking at the code it's not trivial to get a sense of what data is captured, what might be missing should one want to enhance the dataset, and how to make changes without risking "breaking" the existing schema.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added some comments
7ff668e
to
6eb8fdd
Compare
6eb8fdd
to
07f1a3d
Compare
07f1a3d
to
1a71e0c
Compare
A simple workflow that runs when a PR is merged and updates the elasticsearch index with merged PR info. The dashboard for displaying the information can be found here: https://kibana.zephyrproject.io/ Signed-off-by: Anas Nashif <[email protected]>
8f28bf2
1a71e0c
to
8f28bf2
Compare
addressed case where assignee is the merger and did not approve, merging counts as a +1, so make the stats happy, just noticed this with #66752 |
A simple workflow that runs when a PR is merged and updates the
elasticsearch index with merged PR info.
The dashboard for displaying the information can be found here:
https://kibana.zephyrproject.io/
Signed-off-by: Anas Nashif [email protected]