OCPEDGE-2695: Add decision notification#202
Merged
openshift-merge-bot[bot] merged 5 commits intoJun 26, 2026
Merged
Conversation
add an extra check on Fridays to the gh-notifier to gather information on pending ADR for edge-context Signed-off-by: ehila <ehila@redhat.com>
… html Signed-off-by: ehila <ehila@redhat.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eggfoobar The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Walkthrough
ChangesFriday pending-decisions digest
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 9 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@gh-notifier/gh-notifier.py`:
- Around line 246-328: The new decision parsing logic in _DECISION_FILE_RE,
parse_frontmatter, extract_decision_title, decision_number_from_filename, and
_decision_included_for_digest needs positive and negative coverage. Add tests
for valid decision filenames/frontmatter and for malformed frontmatter, missing
status, and non-matching filenames, plus both branches of
_decision_included_for_digest under DECISIONS_TEST and normal mode. Use focused
unit tests around these helpers so the parsing and status-filtering behavior is
locked down.
- Around line 833-845: The HTML table row builder in gh-notifier.py is not
rendering the actual decision makers value, so the Decision makers column only
shows a generic link. Update the decision row assembly in the pending loop to
display the escaped decision_makers content from each item instead of the
placeholder text, while keeping the existing link and escaping behavior
consistent with the other cells in decision_rows.
- Around line 357-363: The file-fetch loop in gh_notifier.py currently only
catches HTTPError and ValueError, so a URLError from gh_get_repo_file can escape
and abort the digest. Update the exception handling in this per-file try/except
block to also catch urllib.error.URLError, log it as a warning via
sys.stderr.write with the same per-file context, and continue processing the
remaining decision files.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 18bec8eb-f975-4a47-b5c5-ad5a6d366a06
📒 Files selected for processing (2)
gh-notifier/README.mdgh-notifier/gh-notifier.py
Signed-off-by: ehila <ehila@redhat.com>
coderabbitai
Bot
added
the
ready-for-human-review
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@gh-notifier/gh-notifier.py`:
- Around line 296-309: The PR file scan only processes the first response from
the pull-request files endpoint, so later pages can miss decision files in
larger PRs. Update the file-loading logic in gh-notifier.py where files are
fetched and iterated to paginate through all `/pulls/{number}/files` results,
using either `per_page=100` with a `page` loop or by following `Link` headers,
and then keep the existing filename filtering and yield behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 084060e5-5865-4359-be35-e106b40272c5
📒 Files selected for processing (3)
gh-notifier/README.mdgh-notifier/gh-notifier.pygh-notifier/tests/test_decisions.py
✅ Files skipped from review due to trivial changes (2)
- gh-notifier/README.md
- gh-notifier/tests/test_decisions.py
Signed-off-by: ehila <ehila@redhat.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@gh-notifier/gh-notifier.py`:
- Around line 360-362: The decision blob URL builder is only quoting the ref, so
filenames used in PendingDecision.url can still break Slack `<url|label>` links
and dashboard links. Update `_decision_blob_url` and the related URL
construction call sites (the main and PR URL paths referenced in
`PendingDecision`) to percent-encode the path as well, using a shared helper so
characters like `|`, `>`, spaces, and `#` are safely quoted before embedding the
URL in Slack.
In `@gh-notifier/tests/test_decisions.py`:
- Line 15: The test setup currently hardcodes a fallback GITHUB_TOKEN value,
which should be removed to avoid embedding credentials in tests. Update the test
initialization in the relevant test module so it relies on the
environment-provided GITHUB_TOKEN at runtime instead of calling setdefault with
a literal, and keep the rest of the test logic unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 0a6ca307-fef4-4b24-bdd3-719db8dea1df
📒 Files selected for processing (3)
gh-notifier/README.mdgh-notifier/gh-notifier.pygh-notifier/tests/test_decisions.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@gh-notifier/gh-notifier.py`:
- Line 243: The decision filename matcher in _DECISION_FILE_RE is too permissive
and currently accepts nested paths like 0001-folder/notes.md after
iter_open_decision_files() strips the decisions/ prefix. Tighten the regex in
gh-notifier.py so it only matches a flat decisions/NNNN-slug.md filename and
rejects any slash-containing suffixes, then update the tests for
iter_open_decision_files() or the filename validation path to include both a
positive case and a negative nested-path case as required by CONTRIBUTING.md.
In `@gh-notifier/tests/test_decisions.py`:
- Around line 46-57: The non-decision filename test in
test_rejects_non_decision_files should also cover nested paths so the
decisions/NNNN-slug.md contract is enforced. Add a negative subtest for a path
like decisions/0001-folder/notes.md and verify _DECISION_FILE_RE does not match
it, keeping the existing regex behavior and test structure intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: e7548aee-e443-414b-8c5b-c06b983aaa84
📒 Files selected for processing (3)
gh-notifier/README.mdgh-notifier/gh-notifier.pygh-notifier/tests/test_decisions.py
Signed-off-by: ehila <ehila@redhat.com>
eggfoobar
force-pushed
the
add-decision-notification
branch
from
214465f to
3556b37
Compare
Contributor
|
/lgtm |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit