OCPERT-368: Replace GitHub token with Github apps#1060
Conversation
rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
openshift-ci-robot
added
the
jira/valid-reference
|
@tomasdavidorg: This pull request references OCPERT-368 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the sub-task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (8)
WalkthroughThis PR replaces personal access token authentication with GitHub App writer/reader credentials across OAR, Prow, tools, tests, service configuration, and documentation. It adds installation-scoped client helpers and removes token parameters from affected APIs. ChangesGitHub App authentication migration
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
prow/job/job.py (1)
183-195:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd explicit timeouts to GitHub Contents API requests to avoid indefinite hangs.
prow/job/job.pyhas barerequests.get/putcalls withouttimeoutinget_sha()(183),push_action()(194-195),push_versions()(209), andget_recored_version()(244). Other GitHub calls in this file already passtimeout=3, so these should too.Suggested change
- res = requests.get(url=url, headers=self.get_release_tests_github_headers()) + res = requests.get( + url=url, + headers=self.get_release_tests_github_headers(), + timeout=self.REQUEST_TIMEOUT, + ) @@ - res = requests.put(url=url, json=data, - headers=self.get_release_tests_github_headers()) + res = requests.put( + url=url, + json=data, + headers=self.get_release_tests_github_headers(), + timeout=self.REQUEST_TIMEOUT, + ) @@ - res = requests.get(url=url, headers=self.get_release_tests_github_headers()) + res = requests.get( + url=url, + headers=self.get_release_tests_github_headers(), + timeout=self.REQUEST_TIMEOUT, + ) @@ - res = requests.get(url=url, headers=self.get_release_tests_github_headers()) + res = requests.get( + url=url, + headers=self.get_release_tests_github_headers(), + timeout=self.REQUEST_TIMEOUT, + )🤖 Prompt for 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. In `@prow/job/job.py` around lines 183 - 195, The requests to GitHub in get_sha(), push_action(), push_versions(), and get_recored_version() lack timeouts and can hang; update each requests.get and requests.put call in those functions to include a timeout parameter (use the same timeout=3 used elsewhere) by passing timeout=3 to requests.get(...) and requests.put(...), ensuring you keep existing headers/json arguments (e.g., in get_sha() where requests.get(url=..., headers=...), in push_action() where requests.put(url=..., json=..., headers=...), and similarly in push_versions() and get_recored_version()) so all GitHub API calls use an explicit timeout.
🧹 Nitpick comments (2)
tools/auto_release_test_dashboard.py (1)
22-27: ⚡ Quick winLog the missing-credentials path before stopping.
This branch only surfaces the failure in the UI, so a headless/service deployment has no backend log explaining why startup stopped. Add a
logger.warning(...)here beforest.stop().As per coding guidelines
**/*.py: All automated agents and services should use Python's standard logging module with configurable log levels for monitoring key events such as new releases detected, new builds found, test jobs triggered, test results aggregated, builds accepted/rejected, and notifications sent.🤖 Prompt for 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. In `@tools/auto_release_test_dashboard.py` around lines 22 - 27, Add a standard-library logging call to record the missing-credentials branch before halting the app: when github_app_id or github_app_private_key are falsy (the branch that currently calls st.warning(...) and st.stop()), call logger.warning(...) with a clear message that includes the ENV_VAR_GITHUB_APP_WRITER_ID and ENV_VAR_GITHUB_APP_WRITER_PRIVATE_KEY (or the variables github_app_id/github_app_private_key) so headless/service deployments emit a backend log entry prior to the existing st.stop().tools/auto_release_test_result_checker.py (1)
16-16: ⚡ Quick winFinish the logger migration in this service.
The file now has a module logger, but fetch failures, JSON decode failures, and Slack notification outcomes still go through
print(...), which bypasses configured log levels and service log routing. Please move those paths tologger.As per coding guidelines
**/*.py: All automated agents and services should use Python's standard logging module with configurable log levels for monitoring key events such as new releases detected, new builds found, test jobs triggered, test results aggregated, builds accepted/rejected, and notifications sent.🤖 Prompt for 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. In `@tools/auto_release_test_result_checker.py` at line 16, The service added a module logger (logger = logging.getLogger(__name__)) but still uses print(...) for error and outcome reporting; replace all print(...) calls related to fetch failures, JSON decode failures, and Slack notification results with appropriate logger calls (e.g., logger.error for fetch/JSON failures, logger.warning for unexpected but nonfatal conditions, logger.info or logger.debug for successful notification outcomes) by updating the functions that perform HTTP fetches, JSON parsing, and Slack notifications (search for print usages in fetch_* / parse_* / notify_* code paths) to call logger.<level>(...) and preserve the original message text and any exception/context details. Ensure exceptions passed to logger (exc_info=True or include exception text) where available so stack traces are preserved.
🤖 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 `@AGENTS.md`:
- Around line 316-317: Update the "Jira Notificator" section in AGENTS.md to
match the top-level env requirements by adding the GITHUB_APP_READER_ID and
GITHUB_APP_READER_PRIVATE_KEY variables (and include "Kerberos ticket" if the
section elsewhere lists it), so the Jira Notificator's environment requirements
list contains JIRA_TOKEN plus GITHUB_APP_READER_ID and
GITHUB_APP_READER_PRIVATE_KEY (and Kerberos ticket when applicable); edit the
"Jira Notificator" heading content to mirror the same bullet format used where
GITHUB_APP_READER_* and Kerberos ticket are declared.
In `@oar/core/github_app.py`:
- Around line 11-28: The constructor of the GitHub App helper
(GitHubApp.__init__) currently rejects inline PEM content and requires a
filesystem path; change it to accept either a path or inline PEM content: detect
if private_key_path contains PEM markers (e.g. "-----BEGIN") or newlines and
treat it as the key string directly, otherwise expand and read the file as
before; keep existing error handling for missing file but only when treating the
input as a path, and use the resolved key when calling Auth.AppAuth and creating
GithubIntegration.
In `@oar/notificator/jira_notificator.py`:
- Around line 92-135: The helpers currently swallow real GitHub errors by
returning None; change _init_github_app and _github_client_for_repo so they only
return None when auth is truly absent (env vars missing or _github_app is None)
but raise or propagate exceptions when initialization or client_for_repo fails
(invalid key, missing installation, transient GitHub errors). Concretely: in
_init_github_app, if ENV_VAR_GITHUB_APP_READER_ID or PRIVATE_KEY is missing
return None, but if GitHubApp(app_id, private_key_path) raises, re-raise or wrap
the exception (include type(e).__name__ and message) instead of returning None;
in _github_client_for_repo, if self._github_app is None return None, but if
self._github_app.client_for_repo(org, repo) raises (e.g., auth/installation
errors or network issues) propagate that exception (or raise a specific
GitHubAuthError) rather than logging and returning None so callers like
is_pre_merge_verified can surface operational failures vs genuine “not verified”
results.
In `@prow/job/github_auth.py`:
- Around line 23-49: Replace the direct print/sys.exit pattern in _github_app,
_installation_token, and _github_client with proper logging and exceptions: add
a module-level logger (logging.getLogger(__name__)), log errors via logger.error
including the caught exception details (type and message), and re-raise an
appropriate exception instead of calling sys.exit; in _github_app raise a
ValueError or custom exception when env vars missing and raise the caught
exception (or wrap it) on GitHubApp(...) failure, and in _installation_token and
_github_client log failures and raise the underlying exception (or a wrapped
exception) instead of exiting after calling GitHubApp.installation_token and
GitHubApp.client_for_repo.
In `@prow/job/selector.py`:
- Around line 67-72: The constructor currently uses
tempfile.NamedTemporaryFile(...).name which yields a file path not a directory;
change the clone target to a real temp directory by replacing the
NamedTemporaryFile usage in __init__ with tempfile.mkdtemp(dir="/tmp",
prefix="release-tests-") (or use tempfile.TemporaryDirectory() and store its
.name if you want managed cleanup) so that self._repo_local_dir is a directory
before calling Repo.clone_from(repo_url, self._repo_local_dir); update any
cleanup logic accordingly.
In `@tests/test_statebox.py`:
- Line 8: The tests reference ConfigStore but it isn’t imported, causing
NameError; add the missing import for ConfigStore at the top of the test file
(e.g., add "from oar.core.config import ConfigStore") so usages like
self.configstore = ConfigStore(...), dummy_configstore = ConfigStore(...),
cm_configstore = ConfigStore(...), and cs1/cs2 = ConfigStore(...) resolve
correctly.
In `@tools/auto_release_test_dashboard.py`:
- Around line 8-21: The file is using writer credentials
(ENV_VAR_GITHUB_APP_WRITER_ID and ENV_VAR_GITHUB_APP_WRITER_PRIVATE_KEY) to
create github_app access even though the dashboard only needs read access;
change the environment variables used to the reader app variants (e.g.
ENV_VAR_GITHUB_APP_READER_ID and ENV_VAR_GITHUB_APP_READER_PRIVATE_KEY) so
github_app is initialized with reader credentials—update the variables
github_app_id and github_app_private_key to read from the reader constants and
ensure any downstream references still use those variables.
In `@tools/auto_release_test_result_checker.py`:
- Around line 105-124: The _github_repo function is currently loading writer
credentials (ENV_VAR_GITHUB_APP_WRITER_ID /
ENV_VAR_GITHUB_APP_WRITER_PRIVATE_KEY) which grants unnecessary write access;
switch it to use the reader app environment variables
(ENV_VAR_GITHUB_APP_READER_ID and ENV_VAR_GITHUB_APP_READER_PRIVATE_KEY)
instead, update the initial env lookup and the ClickException message to
reference those reader vars, and otherwise keep the GitHubApp instantiation and
client_for_repo(...) usage the same so TestResultChecker continues to only read
releases.
---
Outside diff comments:
In `@prow/job/job.py`:
- Around line 183-195: The requests to GitHub in get_sha(), push_action(),
push_versions(), and get_recored_version() lack timeouts and can hang; update
each requests.get and requests.put call in those functions to include a timeout
parameter (use the same timeout=3 used elsewhere) by passing timeout=3 to
requests.get(...) and requests.put(...), ensuring you keep existing headers/json
arguments (e.g., in get_sha() where requests.get(url=..., headers=...), in
push_action() where requests.put(url=..., json=..., headers=...), and similarly
in push_versions() and get_recored_version()) so all GitHub API calls use an
explicit timeout.
---
Nitpick comments:
In `@tools/auto_release_test_dashboard.py`:
- Around line 22-27: Add a standard-library logging call to record the
missing-credentials branch before halting the app: when github_app_id or
github_app_private_key are falsy (the branch that currently calls
st.warning(...) and st.stop()), call logger.warning(...) with a clear message
that includes the ENV_VAR_GITHUB_APP_WRITER_ID and
ENV_VAR_GITHUB_APP_WRITER_PRIVATE_KEY (or the variables
github_app_id/github_app_private_key) so headless/service deployments emit a
backend log entry prior to the existing st.stop().
In `@tools/auto_release_test_result_checker.py`:
- Line 16: The service added a module logger (logger =
logging.getLogger(__name__)) but still uses print(...) for error and outcome
reporting; replace all print(...) calls related to fetch failures, JSON decode
failures, and Slack notification results with appropriate logger calls (e.g.,
logger.error for fetch/JSON failures, logger.warning for unexpected but nonfatal
conditions, logger.info or logger.debug for successful notification outcomes) by
updating the functions that perform HTTP fetches, JSON parsing, and Slack
notifications (search for print usages in fetch_* / parse_* / notify_* code
paths) to call logger.<level>(...) and preserve the original message text and
any exception/context details. Ensure exceptions passed to logger (exc_info=True
or include exception text) where available so stack traces are preserved.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5657b3d9-ab63-491f-9009-555019ea1ed8
📒 Files selected for processing (20)
AGENTS.mdCLAUDE.mddeployment/systemd/release-progress-dashboard.servicemcp_server/check_env.shoar/core/configstore.pyoar/core/const.pyoar/core/github_app.pyoar/core/release_discovery.pyoar/core/statebox.pyoar/notificator/jira_notificator.pyprow/README.mdprow/job/controller.pyprow/job/github_auth.pyprow/job/job.pyprow/job/selector.pytests/test_jira_notificator.pytests/test_release_discovery.pytests/test_statebox.pytools/auto_release_test_dashboard.pytools/auto_release_test_result_checker.py
rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/auto_release_test_result_checker.py (1)
113-118:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject partial
owner/repovalues before initializing GitHub.
split("/", 1)still accepts"/repo"and"owner/", so malformed CLI input falls through to the generic GitHub App error path instead of the specific--repo-namevalidation error. Validate both segments explicitly here.Suggested fix
- try: - owner, repo = repo_name.split("/", 1) - except ValueError as e: + try: + owner, repo = repo_name.split("/", 1) + if not owner or not repo or "/" in repo: + raise ValueError + except ValueError as e: raise click.ClickException( f"--repo-name must be in owner/repo format, got {repo_name!r}" ) from eAs per coding guidelines, "Validate at trust boundaries with allow-lists, not deny-lists."
🤖 Prompt for 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. In `@tools/auto_release_test_result_checker.py` around lines 113 - 118, The repo parsing currently uses owner, repo = repo_name.split("/", 1) which still accepts malformed inputs like "/repo" or "owner/"; after splitting explicitly validate that both owner and repo are non-empty (and optionally strip whitespace) and if either is empty raise the same click.ClickException (use the existing message f"--repo-name must be in owner/repo format, got {repo_name!r}") before any GitHub initialization; update the block around repo_name, owner, repo and the except handler to perform this explicit check.
🤖 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.
Outside diff comments:
In `@tools/auto_release_test_result_checker.py`:
- Around line 113-118: The repo parsing currently uses owner, repo =
repo_name.split("/", 1) which still accepts malformed inputs like "/repo" or
"owner/"; after splitting explicitly validate that both owner and repo are
non-empty (and optionally strip whitespace) and if either is empty raise the
same click.ClickException (use the existing message f"--repo-name must be in
owner/repo format, got {repo_name!r}") before any GitHub initialization; update
the block around repo_name, owner, repo and the except handler to perform this
explicit check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: be34510b-6612-47b8-845f-c0a910da4c0c
📒 Files selected for processing (4)
AGENTS.mdoar/notificator/jira_notificator.pytools/auto_release_test_dashboard.pytools/auto_release_test_result_checker.py
✅ Files skipped from review due to trivial changes (1)
- AGENTS.md
🚧 Files skipped from review as they are similar to previous changes (2)
- tools/auto_release_test_dashboard.py
- oar/notificator/jira_notificator.py
rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
LuboTerifaj
left a comment
LuboTerifaj
left a comment
There was a problem hiding this comment.
Hello @tomasdavidorg ,
GITHUB_APP_READER could be used for release discovery instead of GITHUB_APP_WRITER.
See my inline comments.
Just a note for improvement discussion for future:
- We might want creating a shared utility package for github_app, so there is no duplication in oar and prow.
- Eventually, we could move there some common utilities like versioning (get_y_stream_version).
- In the other hand, this may add unnecessary complexity, so for now, I would keep it as it is.
Thanks!
rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
|
@LuboTerifaj
We have two GitHub Apps because we cannot grant write access to all openshift/* repos. The split is: Writer — installed on openshift/release-tests only (read + write). Used for StateBox, job controller, Release Discovery, etc. Writer already has read permissions on release-tests; using it for read-only operations there is safe because it’s scoped to that single repo.
This PR only migrates off personal GitHub tokens; we’re not planning further structural changes since the project will be handed over to ART soon. I’d keep it as-is for now. |
|
@tomasdavidorg: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
rh-pre-commit.version: 2.4.0
rh-pre-commit.check-secrets: ENABLED
https://redhat.atlassian.net/browse/OCPERT-368
Summary by CodeRabbit
Documentation
raw.githubusercontent.comaccess without a GitHub token.Chores