Skip to content

Dedupe NPM workflow steps#2045

Open
lionello wants to merge 7 commits intomainfrom
lio/add-npm-workflow
Open

Dedupe NPM workflow steps#2045
lionello wants to merge 7 commits intomainfrom
lio/add-npm-workflow

Conversation

@lionello
Copy link
Copy Markdown
Member

@lionello lionello commented Apr 10, 2026

Description

I added a new NPM workflow, but much of it was duplicated from the existing Go (release) workflow. Made an action out of it.

Linked Issues

#2044

Checklist

  • I have performed a self-review of my code
  • I have added appropriate tests
  • I have updated the Defang CLI docs and/or README to reflect my changes, if necessary

Summary by CodeRabbit

  • Chores

    • Added a reusable GitHub Actions composite for building npm packages with configurable Node version, registry, optional version bump, and optional test run.
    • Updated documentation comment to note Node version must align across tooling.
  • Refactor

    • Consolidated npm versioning, install, build and test steps from workflows into the new composite and expanded workflow triggers to include action changes.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 10, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 705449ea-9267-4daa-8a92-48f89d062afd

📥 Commits

Reviewing files that changed from the base of the PR and between 7c57b3f and a93feed.

📒 Files selected for processing (1)
  • .github/actions/npm-build/action.yml
✅ Files skipped from review due to trivial changes (1)
  • .github/actions/npm-build/action.yml

📝 Walkthrough

Walkthrough

Adds a reusable GitHub composite action .github/actions/npm-build/action.yml to centralize Node setup, optional package versioning, dependency installation, build, and conditional tests; updates two workflows to invoke this action and adjusts a comment in flake.nix.

Changes

Cohort / File(s) Summary
New Composite Action
/.github/actions/npm-build/action.yml
Adds local composite action with inputs working-directory, node-version, registry-url, version, run-tests; performs actions/setup-node, optional npm version (strips leading v), npm ci --ignore-scripts, npm run build, and conditional npm test.
Workflow Updates
/.github/workflows/go.yml, /.github/workflows/npm.yml
Replace inline Node/setup and npm commands with a single step calling ./.github/actions/npm-build; go.yml forwards registry-url and version (tag), npm.yml calls with run-tests: "true" and expands trigger to include .github/actions/npm-build/**.
Docs/Comment
flake.nix
Updated comment: require Node version alignment with both package.json and the new action's node-version.

Sequence Diagram(s)

sequenceDiagram
    participant WF as GitHub Workflow
    participant CA as npm-build composite action
    participant SN as actions/setup-node
    participant NPM as npm (working-directory)
    participant REG as Registry (optional)

    WF->>CA: invoke with inputs (working-directory, node-version, version?, run-tests?, registry-url?)
    CA->>SN: setup Node (node-version)
    SN-->>CA: Node ready
    CA->>NPM: optionally run `npm version` (strip leading v)
    CA->>NPM: `npm ci --ignore-scripts`
    CA->>NPM: `npm run build`
    alt run-tests == "true"
        CA->>NPM: `npm test`
    end
    CA-->>WF: step complete
    WF->>REG: later `npm publish` uses NODE_AUTH_TOKEN
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

Suggested reviewers

  • jordanstephens
  • raphaeltm

Poem

🐰 I hopped to code with whiskers keen,

One action now where steps had been.
A bump, an install, build that sings,
Tests on demand and tidy things.
🥕📦

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Dedupe NPM workflow steps' accurately summarizes the main objective: extracting duplicated workflow steps into a reusable composite action to eliminate duplication across npm and go workflows.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch lio/add-npm-workflow

Comment @coderabbitai help to get the list of available commands and usage tips.

@lionello lionello changed the title Lio/add npm workflow Dedupe NPM workflow steps Apr 10, 2026
@lionello lionello marked this pull request as ready for review April 10, 2026 20:42
@lionello lionello enabled auto-merge (squash) April 10, 2026 20:46
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
flake.nix (1)

32-32: Clarify the version-constraint wording in the comment.

Line 32 says the version “must match values in package.json,” which can be misleading if package.json defines an engine range (not an exact pin). Prefer wording like “must be compatible with package.json engines and match the npm-build action default.”

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@flake.nix` at line 32, Update the comment next to the nodejs_24 entry so it
clarifies that the Node.js version should be compatible with the "engines" field
in package.json (which may be a range) and should align with the npm-build
action default, e.g., change the wording around nodejs_24 to say it "must be
compatible with package.json engines and match the npm-build action default"
instead of saying it "must match values in package.json".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.github/actions/npm-build/action.yml:
- Around line 30-35: In the "Set package version" step avoid direct shell
interpolation of inputs.version; instead export the input into an environment
variable (e.g. VERSION), strip a leading "v" from it (so normalized versions
like "v1.2.3" become "1.2.3"), and call npm version with that variable as a
single quoted positional argument (npm version "$VERSION" --no-git-tag-version)
so shell injection is prevented and tag normalization is ensured.

---

Nitpick comments:
In `@flake.nix`:
- Line 32: Update the comment next to the nodejs_24 entry so it clarifies that
the Node.js version should be compatible with the "engines" field in
package.json (which may be a range) and should align with the npm-build action
default, e.g., change the wording around nodejs_24 to say it "must be compatible
with package.json engines and match the npm-build action default" instead of
saying it "must match values in package.json".
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 3b63f02b-84d2-4426-92a1-21877a8da38c

📥 Commits

Reviewing files that changed from the base of the PR and between 7aeaff4 and c797faa.

📒 Files selected for processing (4)
  • .github/actions/npm-build/action.yml
  • .github/workflows/go.yml
  • .github/workflows/npm.yml
  • flake.nix

Disallow command substitution evaluation.

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
@lionello lionello force-pushed the lio/add-npm-workflow branch from 7c57b3f to a93feed Compare April 10, 2026 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant