feat: consume @dnd-mapp/changelog from the public registry - #57
Conversation
Replaces the in-tree scripts/changelog copy with the published @dnd-mapp/changelog devDependency, drops the now-empty scripts workspace entry, and switches release-prepare.yml/release-publish.yml to `pnpm exec changelog`. The GitHub Packages .npmrc scope is removed since the package now resolves from npm's default public registry.
Configures dnd-mapp-bot as the posting account with auto suggested-event.
…e split scripts/changelog was the only workspace package with its own typecheck script, so `pnpm -r run typecheck` errors now that it's gone. The `-w`/`-r` split for tests and lint-ts existed for the same reason and collapses to a plain `pnpm run` now that the workspace has just the one package.
Docker's dependencies stage copied .npmrc and mounted an npm_token secret purely to authenticate `pnpm install` against GitHub Packages for scoped @dnd-mapp/* packages. That scope no longer resolves there now that @dnd-mapp/changelog is public, so the .npmrc copy, secret mount, and the packages:read permissions/GitHub App token step feeding it (in both pull-request.yml and push-main.yml) are all dead. Docs updated to match.
dnd-mapp-bot
left a comment
dnd-mapp-bot
left a comment
There was a problem hiding this comment.
Code Review
Standards: 1 finding (hard violation). Spec: 1 finding (scope creep, non-blocking).
Standards
Hard violation: missing CHANGELOG.md entry. Per Changelog Conventions, consumer-facing scope explicitly includes "scaffolding changes in template repositories," and authors must manually add entries in the introducing PR. This PR deletes the entire in-tree scripts/changelog package and switches to the published @dnd-mapp/changelog devDependency, but CHANGELOG.md is untouched. Its existing ## [Unreleased] section still contains entries describing the package this PR deletes, such as "Added a scripts/changelog package (@dnd-mapp/changelog) with a bump/extract/check CLI..." and "Added pnpm workspaces (scripts/*)...". As merged, the changelog would misdescribe the shipped state. Please add an entry (likely Changed/Removed) reflecting the migration, and consider whether the now-stale Added entries need adjusting.
No other hard Standards violations were found. Commits are cleanly grouped Conventional Commits. All GitHub Actions stay SHA-pinned with version comments. Docker Hub credentials sourcing is untouched. No baseline code smells (duplication, feature envy, shotgun surgery, etc.) were introduced: the diff is almost entirely subtractive, and each removal is a direct consequence of dropping the in-tree package.
Spec
All four asks in #55 are fully and correctly implemented: @dnd-mapp/changelog added as a ~1.0.0-pinned devDependency (consistent with the repo's savePrefix: '~' convention), the in-tree scripts/changelog copy and scripts/* workspace entry removed, .npmrc deleted, and all release-prepare.yml/release-publish.yml call sites switched to pnpm exec changelog .... No missing or incorrect implementation found.
The NPM_TOKEN/GitHub-App-token removal and the dropped typecheck/-w/-r CI split go beyond #55's literal text. Both are necessary consequences of the four asks: dead auth scaffolding and dead recursive-workspace invocations once the second package is gone. Not flagged as an issue.
One small unrelated addition is called out in the inline comment below.
Summary: Standards found 1 finding (1 hard violation). Spec found 1 finding (scope creep, non-blocking). Worst issue per axis: Standards is the missing/stale CHANGELOG.md entry; Spec is the unrelated docs/agents/review-comments.md addition riding along in this PR.
…removal Trims stale `Added` bullets that described the deleted in-tree scripts/changelog package's CLI, README, and features. As merged, they would have misdescribed shipped state. Adds one `Changed` entry covering the migration to the published @dnd-mapp/changelog devDependency. Addresses a review finding on #57.
|
Fixed in d742213. Trimmed the stale |
dnd-mapp-bot
left a comment
dnd-mapp-bot
left a comment
There was a problem hiding this comment.
Follow-up on my previous review. The missing-CHANGELOG.md-entry finding is resolved by d742213. Not re-raising the docs/agents/review-comments.md scope-creep note: it's already been discussed, and you've decided to keep it in this PR.
One minor, non-blocking item outside this PR's diff: .github/actions/detect-relevant-changes/action.yml:38 still lists .npmrc in the docker filter's path list, even though this PR deletes that file. Harmless dead reference, worth dropping here or in a fast follow. Not left as an inline comment since that file isn't touched by this PR's diff.
One inline suggestion below.
Addresses PR review feedback that the Unreleased entry covered the package swap but not the Docker/CI auth-scaffolding removal.
dnd-mapp-bot
left a comment
dnd-mapp-bot
left a comment
There was a problem hiding this comment.
Two small follow-ups from reviewing the changelog-package migration. Neither blocks this PR.
- Dead catalog entry:
pnpm-workspace.yamlstill hascommander: ~15.0.0undercatalogs.default(line 45). It was only used by the now-deletedscripts/changelogpackage, andpnpm-lock.yaml's generated catalog block confirms nothing else references it. Worth deleting in a follow-up. - Leftover GitHub Packages vestige:
.github/actions/prepare-workspace/action.yml:16still setsscope: '@dnd-mapp'onactions/setup-node. It's inert without a pairedregistry-url, but it's a leftover from the GitHub-Packages-scoped setup this PR otherwise cleans up. It's arguably in scope for the.npmrc-removal cleanup here, but it isn't touched in this diff, so flagging it as a follow-up rather than blocking on it.
Everything issue #55 asked for is implemented: the devDependency was added, scripts/changelog was removed, .npmrc was removed, and the release-*.yml call sites now use pnpm exec changelog. The lockfile confirms it resolves from the public registry.
Removes three dead references left behind after switching from the in-tree scripts/changelog package to the published @dnd-mapp/changelog devDependency: the deleted .npmrc from the docker change filter, the now-unused commander catalog entry, and the inert GitHub Packages scope on actions/setup-node.
|
Fixed in 82854ec. Cleaned up the three leftover npm-registry vestiges flagged across the last two reviews:
Pushed and ready for another look. |
dnd-mapp-bot
left a comment
dnd-mapp-bot
left a comment
There was a problem hiding this comment.
Re-reviewed since the last pass. Standards and Spec both come back clean: the migration to @dnd-mapp/changelog matches issue #55's four requirements exactly. No new standards violations or spec deviations turned up, so there are no new findings to raise.
2 participants
Summary
Switches this repo from the in-tree
scripts/changelogcopy to the published@dnd-mapp/changelogpackage (now on the public npm registry).pnpm-workspace.yamlgains a@dnd-mapp/changelogcatalog entry pinned~1.0.0, the now-emptyscripts/*workspace entry is dropped, andrelease-prepare.yml/release-publish.ymlcallpnpm exec changeloginstead ofnode scripts/changelog/index.ts. The.npmrcGitHub Packages scope override is removed since@dnd-mapp/changelognow resolves from npm's default public registry and no other@dnd-mapppackage is currently consumed via GitHub Packages.Verified locally:
pnpm exec changelog check/bump-levelrun correctly against the installed package, andformat-check/lint-mdpass with the deleted directory gone.Related issues
Part of dnd-mapp/template-app-angular#55, itself part of the Extract scripts/changelog into a published npm package map.