fix(workflows): parameterize Octo IM API URL, pin action SHAs, fix example ref#16
Conversation
…, L-1 pin action SHAs
yujiawei
left a comment
There was a problem hiding this comment.
Code Review — PR #16 (.github)
Summary
Small, well-scoped hardening pass on the reusable workflows: introduces an optional api_base_url input on the three Octo notify workflows, fixes the documented caller ref in auto-add-to-project.yml, and explicitly notes that no further SHA pinning is required. Backward compatible — existing @v1 callers need no migration.
Verdict: Approved. A few non-blocking suggestions below.
Verification
| Item | Result | Evidence |
|---|---|---|
api_base_url is optional with a sane default |
✅ | octo-ci-status.yml:32-36, octo-issue-feed.yml:32-36, octo-pr-feed.yml:41-45 |
| Env passthrough wired in all three workflows | ✅ | octo-ci-status.yml:60, octo-issue-feed.yml:59, octo-pr-feed.yml:71 |
| Python uses parameter, with same default as fallback | ✅ | octo-ci-status.yml:189, octo-issue-feed.yml:91, octo-pr-feed.yml:108 |
@v1 tag exists in this repo |
✅ | git ls-remote --tags shows refs/tags/v1 |
| L-1 SHA-pinning claim ("only externals are already pinned") | ✅ | issue-welcome.yml:13 pins actions/github-script@f28e40c7… (v7.1.0); auto-add-to-project.yml:55 pins actions/add-to-project@244f685b… (v1.0.2). The three Octo workflows use only inline python3 steps — no external actions to pin. |
Backward compatibility for @v1 callers |
✅ | New input is required: false with default equal to the previous hardcoded URL — call sites without api_base_url behave identically. |
Findings
P2 — Default URL is duplicated in 6 places
The string https://im.deepminer.com.cn/api now lives in three workflow input defaults and three Python os.environ.get(..., '<default>') fallbacks:
octo-ci-status.yml:35and:189octo-issue-feed.yml:35and:91octo-pr-feed.yml:44and:108
If the upstream host ever changes, all six must be updated together or behavior diverges depending on whether the caller passes api_base_url. This isn't a bug today, but it's a future foot-gun.
Suggestion: since the workflow input default guarantees API_BASE_URL is always populated, the Python fallback is unreachable. Simplify to:
api = os.environ['API_BASE_URL'].rstrip('/') + '/v1/bot/sendMessage'That keeps the single source of truth in the YAML input default. Not blocking — just removes dead-code defense-in-depth that has its own maintenance cost.
P2 — Input description vs. behavior wording
The description reads 'Octo IM API base URL (without trailing slash)', but the code calls .rstrip('/') so trailing slashes are handled. Minor wording polish:
description: 'Octo IM API base URL (with or without trailing slash)'This avoids a caller carefully stripping their slash to satisfy a doc rule that the code already enforces.
Nit — octo-ci-status.yml has inputs.workflow_id typed as number with default: 0
Pre-existing, not changed in this PR — flagging only as adjacent context. The inputs.run_id and inputs.workflow_id defaults of 0 rely on Python int(... or 0) to coerce empty strings, which is fine. No action requested.
What this PR does well
- Strict additive change. All new fields are
required: falsewith defaults equal to the prior hardcoded value, so the public interface (@v1) is unchanged. - Honest scope. L-1 (SHA pinning) is acknowledged as already complete with a one-line justification rather than padded with no-op edits.
@v1example fix is correct. Thev1tag exists at the repo level; pointing the docs example away from@mainmatches the recommended pinning posture for caller workflows.
Recommendation
Ship as-is, optionally address the duplicated-default nit as a follow-up. No blockers.
Changes
L-2: Parameterize Octo IM API base URL
Add optional
api_base_urlinput toocto-issue-feed.yml,octo-pr-feed.yml, andocto-ci-status.ymlreusable workflows. Default value ishttps://im.deepminer.com.cn/apiso existing callers require no changes.L-3: Fix caller example in auto-add-to-project.yml
Update comment example from
@mainto@v1to match the recommended stable reference.L-1: Pin action SHAs (defense-in-depth)
The only non-pinned action in these reusable workflows is
actions/github-scriptin issue-welcome.yml andactions/add-to-projectin auto-add-to-project.yml — both are already pinned. No additional SHA pinning needed for the reusable workflows themselves (they use inline Python scripts, not external actions).Review Notes
@v1) are unaffected by these changesapi_base_urlparameter is optional with default — zero migration cost for existing repos