refactor: make shareable components abstract - #61
Conversation
b116065 to
c86b9cb
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors the Rockcraft pack action to extract reusable, tool-agnostic building blocks (a shared CraftBuilder base class and a shared runPackAction/input reader), in preparation for additional Node-based Craft Actions.
Changes:
- Introduced an abstract
CraftBuilder(common pack/test execution, artifact discovery, pro/verbosity handling) and updatedRockcraftBuilderto extend it. - Added a shared
pack-actionhelper (readBaseInputs,runPackAction) and migratedrockcraft-pack-actionto use it. - Generalized tool capability checks (
haveFlag,haveSubcommand) and updated/added unit tests accordingly; simplified bundling viascripts/pack.mjs.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/tools.test.ts | Updates tests to cover ensureCraftTool, haveFlag, and haveSubcommand. |
| tests/rockcraft-pack.test.ts | Refactors tests to align with the new CraftBuilder-based implementation and focuses on Rockcraft-specific behavior (--ignore). |
| tests/pack-action.test.ts | Adds unit tests for the new shared readBaseInputs and runPackAction. |
| tests/craft-builder.test.ts | Adds comprehensive tests for the new CraftBuilder behavior (pack/test flow, flags, artifact discovery). |
| src/tools.ts | Generalizes Rockcraft-specific helpers into haveFlag, haveSubcommand, and ensureCraftTool. |
| src/rockcraft-pack.ts | Converts RockcraftBuilder into a CraftBuilder subclass and moves shared logic to the base class. |
| src/rockcraft-pack-action.ts | Switches the action entrypoint to the new shared pack-action helpers (but currently instantiates the builder at module scope). |
| src/pack-action.ts | Adds shared input parsing and standardized pack+artifact output logic. |
| src/craft-builder.ts | Introduces the abstract shared builder implementation used by tool-specific builders. |
| scripts/pack.mjs | Adds a dedicated esbuild bundling script. |
| package.json | Updates pack script to use scripts/pack.mjs. |
| dist/rockcraft-pack-action/index.js | Updates the bundled output to reflect the refactor. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
alesancor1
left a comment
There was a problem hiding this comment.
Many comments, but honestly the refactor is really good.
My comments are mostly regarding the repository structure and file names. The logic itself looks good to me, fantastic work!
Afaict none of the suggestions below would alter the current behaviour of the action, since it's just moving stuff around and removing some unnecessary checks:
alesancor1
left a comment
There was a problem hiding this comment.
LGTM!
Looking forward to see more actions coming in once the new structure is in place!
This PR prepares for the creation of other Node-based Craft Actions by refactoring the pieces of Rockcraft's pack action that seemed shareable out into "common" modules. I tried my best to keep the task focused and break up the commits, but frankly this was just a very loud PR. Sorry.
I made a temporary repository with two CI runs to validate this: https://github.com/bepri/test-actions/actions
The first run was the simplest case, just
rockcraft initwith a standard, optionless call to this action. The second run used an unmaintained base and customized many of the options.