YPE-2486: harden and document the release procedure#268
Conversation
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| - name: Setup Node.js | ||
| uses: actions/setup-node@v6 | ||
| with: | ||
| node-version: 20 | ||
| cache: 'pnpm' |
There was a problem hiding this comment.
This workflow installs @commitlint/cli@21.1.0, whose locked package metadata requires Node >=22.12.0, but the job runs on Node 20. When a pull request runs this new check, dependency install or pnpm exec commitlint can fail before any commit message is linted, blocking every PR. The release workflow already uses Node 24, so this check should use a compatible Node version too.
Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/commitlint.yml
Line: 21-25
Comment:
**Use compatible Node**
This workflow installs `@commitlint/cli@21.1.0`, whose locked package metadata requires Node `>=22.12.0`, but the job runs on Node 20. When a pull request runs this new check, dependency install or `pnpm exec commitlint` can fail before any commit message is linted, blocking every PR. The release workflow already uses Node 24, so this check should use a compatible Node version too.
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Same treatments here around node versions.
| // Optional "YPE-1234: " or "YPE-1234 - " prefix, then the standard | ||
| // conventional header. Capture groups after the optional prefix must stay | ||
| // aligned with headerCorrespondence below. | ||
| headerPattern: /^(?:YPE-\d+(?:: | - ))?(\w+)(?:\(([^)]*)\))?(!)?: (.+)$/, |
There was a problem hiding this comment.
The new parser only preserves the ticket-prefix convention when the text after YPE-#### is already a conventional, lower-case subject. Recent merge subjects in this repo include forms like YPE-1146 - Prep React SDK for major bump, YPE-2923: feat(ui): Move ..., and plain titles such as Add Language detection ...; those now fail with type-empty, subject-empty, or subject-case. If this check is required on PRs, branches that follow those still-common repo title styles can be blocked even though the docs say the team's existing convention keeps passing.
Prompt To Fix With AI
This is a comment left during a code review.
Path: commitlint.config.js
Line: 28
Comment:
**Rejects common subjects**
The new parser only preserves the ticket-prefix convention when the text after `YPE-####` is already a conventional, lower-case subject. Recent merge subjects in this repo include forms like `YPE-1146 - Prep React SDK for major bump`, `YPE-2923: feat(ui): Move ...`, and plain titles such as `Add Language detection ...`; those now fail with `type-empty`, `subject-empty`, or `subject-case`. If this check is required on PRs, branches that follow those still-common repo title styles can be blocked even though the docs say the team's existing convention keeps passing.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Public contributions will not start with a ticket number, but the PR title may get updated with one after we discuss and accept the change. Let's add this to the plan feedback and "turn the crank again".
I understand that there are no changesets, but there should be one, or at least there should be a recommendation about the version impact. In a perfect world, we would at least see the next version calculated based on conventional commits and semantic release conventions. So in this case, there are two commits that contain the prefix fix. And for those, that should trigger a patch increase. Now obviously when the PR lands, it would be a single patch level increase, not multiple patch level increases. I also wonder what happens if we don't have a change set and instead we decide to roll together n number of changes in a specific build. In that case, I suppose the next time we went to go do a change set, it would calculate and find the highest version bump that was available. The problem with that is that now we have a batching problem. In lean software engineering, you'd want to be releasing more often. So the fact that there is a PR without a changeset to me feels like a smell or as possibly an anti pattern. I'm willing to have the conversation and there are no right answers here. There are just a bunch of potentially wrong ones. So please take this as more of a, let's think about this – this is feedback v "sosmething here is wrong." I think this is suboptimal right now, and I think that we can probably improve it. Those of us who are handling releases have the confidence that we're doing the right thing for each individual PR and that we're shipping improvements to the community as fast as humanly possible and as responsibly as our mission demands. For things like patch releases that may address small issues (and don't necessarily rise to the level of a point increase), most consumers will allow patch level increases for things like security fixes, or small bugs. But even we shouldn't wholesale be adopting point level increases, especially when we consider things like supply chain attacks. Small PRs that are patch fixes and bug fixes tend to get approved as long as the change set is relatively small. The size of a change and its "reviewability" are a signal about a given change's impact, and smaller changes should receive careful-enough scrutiny to avoid loosening security or introducing other subtle bugs, especially since so many of us allow these patch-level increases automatically (like Dependabot, e.g.) Things like point increases should trigger much more diligence. Our downstream dependencies and their transitive dependencies may have wider wider version ranges that they accept. But generally speaking, in this day and age, we should be pinning specific versions and maybe allowing for patch increases. And we should probably be telling our consumers and partners to do the same. |
jhampton
left a comment
jhampton
left a comment
There was a problem hiding this comment.
Please take the feedback and make sure it's captured in this repo as learning (agents, skills, etc). Some of these need @davidfedor consulted if not deciding. We can (and should) defer landing this until we have clear decisions, then pin those behaviors with both deterministic (code/config) guardrails and semi-deterministic (Greptile, AGENTS.md) rules.
| - name: Setup Node.js | ||
| uses: actions/setup-node@v6 | ||
| with: | ||
| node-version: 20 | ||
| cache: 'pnpm' |
There was a problem hiding this comment.
Same treatments here around node versions.
| // Optional "YPE-1234: " or "YPE-1234 - " prefix, then the standard | ||
| // conventional header. Capture groups after the optional prefix must stay | ||
| // aligned with headerCorrespondence below. | ||
| headerPattern: /^(?:YPE-\d+(?:: | - ))?(\w+)(?:\(([^)]*)\))?(!)?: (.+)$/, |
There was a problem hiding this comment.
Public contributions will not start with a ticket number, but the PR title may get updated with one after we discuss and accept the change. Let's add this to the plan feedback and "turn the crank again".
There was a problem hiding this comment.
@jhampton @davidfedor , three things need a decision before this lands; recommendations are in the doc. #1 (commit-lint) and #2 (package manager) need David's input.
jhampton
left a comment
jhampton
left a comment
There was a problem hiding this comment.
Resolved the two questions. Lookin' good.
|
|
||
| --- | ||
|
|
||
| ## Decision 1 — Commit-lint subject model 🔴 |
There was a problem hiding this comment.
In the Swift repo, we enforce conventional commits to ensure the Changelog generation is complete and clean, and that our versioning math reads for the most-breaking change (i.e. patch -> minor -> major). We work around this issue entirely because Jira actually parses branch names, not just commits.
https://lifechurch.atlassian.net/browse/YPE-3340?search_id=8355af1d-6356-4d26-8e72-8190a66d6052
The correct answer is to enforce conventional commits. Ticket references can be front-and-center in our PRs, but when we squash-merge, the commit title matters as the changelog is calculated IIUC. Please check my math if that doesn't align to the implementation.
| 2. **pnpm, no Corepack** — document it, rely on `engines`/`packageManager`; mismatch can still bite locally. | ||
| 3. **Pure node** — not viable; the repo uses `workspace:*` (pnpm-only). | ||
|
|
||
| **Recommend:** Option 1. **Decides:** @davidfedor (approve README rationale). |
There was a problem hiding this comment.
Option 1 - great explanation, no notes.
That said, I saw that corepack was deprecated a while back and checked. Yup...we can rely on it through Node 24, and then we'll need to make some decisions. By then (April 2028), we may have fully-autonomous systems to worry about this, so let's defer this knock-on decision until we see what happens with the corepack team, NodeJS/Bun/Deno/JavaScriptCore/SpiderMonkey and the rest. Bun just got acquired. We'll see.
2 participants
Ports the release-hardening work from YPE-2486 to the React SDK.
What's here
origin/maintip, huskycommit-msghook, and config (allows theYPE-####prefix, ignores the release commit). Ported from the Kotlin SDK.Greptile Summary
This PR ports the YPE-2486 release-hardening work to the React SDK: a 10-section release runbook (
RELEASE-RUNBOOK.md), a commitlint CI workflow and local hook, an ESMcommitlint.config.jswith an optionalYPE-####ticket prefix, and supporting documentation. Several issues raised in prior review rounds have been addressed — commitlint was downgraded from@21to@19.8.1(resolving the Node 20 compat issue), the runbook now uses per-package tags (<pkg>@$VERSION), and the release-ignore regex is properly anchored.docs/release-hardening-decisions.mdexplicitly marks PR YPE-2486: harden and document the release procedure #268 as blocked until Decisions 1 (commit-lint subject model) and 2 (package-manager lane) are resolved, but the commitlint workflow, hook, and config that implement one specific uncommitted-to design are all included in this same PR.release.ymlworkflow.Confidence Score: 4/5
The runbook, PUBLISHING.md updates, and documentation files are safe to merge; the commitlint enforcement files ship while the team’s own decisions doc records that the subject model and package-manager approach haven’t been approved yet.
The decisions document explicitly states the PR should not land until Decisions 1 and 2 (both unresolved) are made, yet those same decisions govern the commitlint.yml, .husky/commit-msg, and commitlint.config.js included here — merging activates a live CI gate enforcing a design the team flagged for redesign.
docs/release-hardening-decisions.md, .github/workflows/commitlint.yml, .husky/commit-msg, and commitlint.config.js — the decisions that govern these enforcement files are unresolved per the decisions document included in the same PR.
Important Files Changed
pnpm exec commitlint. Package-manager lane decision (Decision 2) is unresolved.Reviews (6): Last reviewed commit: "YPE-2486: docs: add open-decisions recor..." | Re-trigger Greptile