Conversation
…om:CISCODE-MA/NotificationKit into develop
- Replace GitHub Actions version tags with full commit SHA hashes (v6 -> fd88b7d, v1 -> d304d05) - Fix SONAR_PROJECT_KEY from LoggingKit to NotificationKit - Add 'main' branch to PR trigger list for release-check workflow - Resolves 2 security hotspots (githubactions:S7637) for supply chain security
- Add explicit NotificationDocument type annotations to map callbacks - Fix mapToRecord signature to accept any type (handles both Map and Record) - Install mongoose as dev dependency for type checking - Fixes pre-push typecheck failures
- Add comprehensive instruction files in .github/instructions/ - Includes copilot, testing, bugfix, features, general guidelines - Standardize documentation across all repositories
- Remove deprecated instruction files from .github/ root - Consolidate all docs in .github/instructions/ directory - Improve documentation organization
- Mongoose required for type compilation (infra/repositories/mongoose) - ts-node required by Jest configuration - Resolves typecheck and test errors
|
There was a problem hiding this comment.
Pull request overview
This PR updates the package version and tightens the NPM publishing GitHub Actions workflow by adding tag/version validation to ensure releases are tied to a semver tag and match package.json.
Changes:
- Bumped package version from
0.0.0→0.0.1(and updatedpackage-lock.jsonaccordingly). - Enhanced the publish workflow to validate semver tag format and ensure tag version matches
package.json. - Updated
package-lock.jsondependency resolutions as part of the lockfile regeneration.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| package.json | Bumps package version to 0.0.1. |
| package-lock.json | Updates lockfile version fields and refreshes some dependency resolutions. |
| .github/workflows/publish.yml | Adds tag discovery + semver/tag-to-package.json version validation before publishing. |
| # Try to find a version tag on HEAD or recent commits | ||
| TAG=$(git describe --exact-match --tags HEAD 2>/dev/null || git tag --list --sort=-version:refname --merged HEAD 'v*.*.*' | head -1 || echo "") | ||
|
|
There was a problem hiding this comment.
This workflow no longer guarantees the HEAD commit is tagged: it falls back to the most recent merged v*.*.* tag, so a push to master without a new tag can still pass validation and attempt to publish again (likely failing with “version already exists” or, worse, publishing from an untagged commit). Consider requiring an exact tag on HEAD and/or triggering the workflow on push to tags: ['v*.*.*'] so the published commit is always the tagged one.
|
|
||
| echo "✅ Valid tag found: $TAG" | ||
| echo "✅ Version matches package.json: $PKG_VERSION" | ||
| echo "TAG_VERSION=$TAG" >> $GITHUB_ENV |
There was a problem hiding this comment.
TAG_VERSION is computed without the v prefix, but the value exported to $GITHUB_ENV is $TAG (still includes the v prefix). This makes the env var name misleading and can break later steps that expect a pure semver (e.g., 0.0.1). Export the actual TAG_VERSION value (or rename the variable to TAG).
| echo "TAG_VERSION=$TAG" >> $GITHUB_ENV | |
| echo "TAG_VERSION=$TAG_VERSION" >> $GITHUB_ENV |
|
|
||
| # Extract version from tag | ||
| TAG_VERSION="${TAG#v}" # Remove 'v' prefix | ||
| PKG_VERSION=$(grep '"version"' package.json | head -1 | sed 's/.*"version": "\([^"]*\)".*/\1/') |
There was a problem hiding this comment.
Parsing package.json with grep | sed is brittle (formatting changes, comments, or additional "version" fields can break it). Prefer a structured read (e.g., node -e with JSON.parse, npm pkg get version, or jq -r .version) to make the version check reliable.
| PKG_VERSION=$(grep '"version"' package.json | head -1 | sed 's/.*"version": "\([^"]*\)".*/\1/') | |
| PKG_VERSION=$(node -p "require('./package.json').version") |
| echo "This typically happens when:" | ||
| echo " 1. You forgot to run 'npm version patch|minor|major' on develop" | ||
| echo " 2. You didn't push tags when merging develop→master" | ||
| echo " 3. PR merge didn't include the tag from develop" | ||
| echo "" | ||
| echo "📋 Correct workflow:" | ||
| echo " 1. On develop: npm version patch (or minor/major)" | ||
| echo " 2. On develop: git push origin develop --tags" | ||
| echo " 3. Create PR develop→master and merge" | ||
| echo " 4. Workflow automatically triggers on master with the tag" |
There was a problem hiding this comment.
The failure message recommends npm version patch|minor|major, but this repo is already set up for Changesets (.changeset/ directory and changeset/version-packages scripts in package.json). This guidance is likely to confuse the intended release flow—either update the message to the actual Changesets-based commands, or remove/disable Changesets if it’s not the chosen approach.
| { | ||
| "name": "@ciscode/notification-kit", | ||
| "version": "0.0.0", | ||
| "version": "0.0.1", |
There was a problem hiding this comment.
This PR directly bumps package.json version, but the repository is configured for Changesets-based versioning (see changeset/version-packages scripts and .changeset/ config). If Changesets is the intended release mechanism, the version should typically be updated via changeset version (driven by changeset files) rather than manual edits, to avoid divergence between changesets and published versions.
| { | ||
| "name": "@ciscode/notification-kit", | ||
| "version": "0.0.0", | ||
| "version": "0.0.1", |
There was a problem hiding this comment.
PR description is still the default template (no Summary/Why/Notes filled in), which makes it hard to validate intent and release impact (especially around versioning/publishing). Please add a short summary of what’s being released and why, and call out the intended versioning approach (Changesets vs npm version).
Summary
Why
Checklist
npm run lintpassesnpm run typecheckpassesnpm testpassesnpm run buildpassesnpx changeset) if this affects consumersNotes