mirror of
https://github.com/hanzoai/datastore.git
synced 2026-09-26 13:55:07 +00:00
Alexey MilovidovandClaude Opus 4.8
a28d9ef4b0
`GH.get_changed_files` fetches the list of files a PR touches via
`gh api .../pulls/<n>/files`. The result is stored once into the job
KV data by the `store_data` pre-hook and read back by several other
workflow hooks (`pr_labels_and_category`, `team_notifications`,
`feature_docs`, ...).
The function had a `for attempt in range(3)` retry loop, but for the PR
path it was dead code: an `assert exit_code == 0` ran right after the
`gh api` call, so the very first transient failure raised before any
retry could happen. When the GitHub API hiccupped (rate limiting, a
5xx, or a GraphQL "Something went wrong" error), `store_data` aborted
without populating `changed_files`, and every downstream hook then read
`None` from the KV data and died far away with an opaque
`TypeError: 'NoneType' object is not iterable` / `can only join an
iterable`, with nothing in the report pointing at the real cause.
Changes:
- Make the retry actually retry: drop the premature `assert`, unify the
PR and commit code paths into a single command, and retry every
non-zero exit code (this is an idempotent read, so a spurious retry is
cheap and the exit code alone does not reliably distinguish transient
from permanent errors). Five attempts with a small linear backoff.
- On final failure, print and (when `strict`) raise a message that
includes the command, exit code, stderr and stdout, so the failure is
diagnosable directly from the report. The non-strict contract of
returning `None` is preserved for callers that tolerate it
(`Runner` and the master/release path of `store_data`).
- In the three consumer hooks, assert `changed_files is not None` with
an actionable message instead of letting a missing value surface as a
bare `TypeError`.
Observed on PR #74691 (commit 8e08fc02), which blocked the whole
pipeline because the failing pre-hooks were never able to dispatch the
real jobs:
https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=74691&sha=8e08fc02490688916fb3bffe4e897cbe50f2702a&name_0=PR&name_1=Config%20Workflow
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
77 lines
2.4 KiB
Python
77 lines
2.4 KiB
Python
import sys
|
|
|
|
from praktika.info import Info
|
|
|
|
from ci.jobs.scripts.workflow_hooks.pr_labels_and_category import Labels
|
|
|
|
# Files that contain embedded documentation as part of their definitions
|
|
embedded_doc_files = [
|
|
"src/Common/ProfileEvents.cpp",
|
|
"src/Common/CurrentMetrics.cpp",
|
|
]
|
|
|
|
# Directories where source files may contain inline documentation
|
|
# (e.g. FunctionDocumentation structs registered with function factories)
|
|
inline_doc_paths = [
|
|
"src/Functions/",
|
|
"src/AggregateFunctions/",
|
|
"src/TableFunctions/",
|
|
]
|
|
|
|
# File name suffixes that indicate embedded documentation
|
|
# (e.g. settings files with DECLARE macros containing description strings)
|
|
# Settings.cpp files declare the per-setting DECLARE(...) macros for Settings/ServerSettings,
|
|
# whereas the format settings put DECLARE(...) blocks in FormatFactorySettings.h.
|
|
embedded_doc_suffixes = [
|
|
"Settings.cpp",
|
|
"Settings.h",
|
|
]
|
|
|
|
|
|
def file_contains_marker(file_path, marker):
|
|
"""Check if a file contains the given marker string."""
|
|
try:
|
|
with open(file_path) as f:
|
|
content = f.read()
|
|
return marker in content
|
|
except OSError:
|
|
return False
|
|
|
|
|
|
def check_docs():
|
|
info = Info()
|
|
if Labels.PR_FEATURE in info.pr_labels:
|
|
changed_files = info.get_kv_data("changed_files")
|
|
assert changed_files is not None, (
|
|
"changed_files is not populated in JOB_KV_DATA: the store_data pre-hook "
|
|
"most likely failed to fetch the PR file list from the GitHub API. "
|
|
"See the Config Workflow logs for the underlying error."
|
|
)
|
|
has_doc_changes = any(
|
|
file.startswith("docs/")
|
|
or file in embedded_doc_files
|
|
or (
|
|
any(file.startswith(path) for path in inline_doc_paths)
|
|
and file_contains_marker(file, "FunctionDocumentation")
|
|
)
|
|
or (
|
|
any(file.endswith(suffix) for suffix in embedded_doc_suffixes)
|
|
and (
|
|
file_contains_marker(file, "DECLARE_SETTING")
|
|
or file_contains_marker(file, "DECLARE(")
|
|
)
|
|
)
|
|
for file in changed_files
|
|
)
|
|
if not has_doc_changes:
|
|
print(
|
|
"No documentation changes found, please update the documentation"
|
|
)
|
|
return False
|
|
return True
|
|
|
|
|
|
if __name__ == "__main__":
|
|
if not check_docs():
|
|
sys.exit(1)
|