split out skill build command from skill publish#279
Conversation
Signed-off-by: Peter Jausovec <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR extracts Docker image build functionality out of arctl skill publish into a new arctl skill build command, and updates skill publish to support publishing via GitHub or a pre-built Docker image reference.
Changes:
- Added
arctl skill buildto build (and optionally push) Docker images from local skill folders. - Updated
arctl skill publishto remove Docker build/push flags and introduce--docker-imagefor registering pre-built images. - Improved skill folder detection by validating SKILL.md YAML frontmatter and expanded unit/e2e coverage for new modes.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/cli/skill/skill.go | Registers the new skill build subcommand under skill. |
| internal/cli/skill/publish.go | Removes docker-build-from-publish flow; adds --docker-image publish mode; tightens folder detection behavior. |
| internal/cli/skill/publish_test.go | Updates publish tests for new detection behavior and new docker-image direct mode; removes old docker-url build tests. |
| internal/cli/skill/build.go | Implements arctl skill build command and Docker build/push behavior. |
| internal/cli/skill/build_test.go | Adds unit tests for build command validation and skill detection. |
| e2e/skill_publish_test.go | Adds e2e coverage for skill build and docker-image publish workflows and updates existing publish tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Peter Jausovec <[email protected]>
Signed-off-by: Peter Jausovec <[email protected]>
inFocus7
left a comment
inFocus7
left a comment
There was a problem hiding this comment.
just 2 requests & 2 questions 👍🏼
Signed-off-by: Peter Jausovec <[email protected]>
|
You already have 3 pull requests open. Please consider working on getting the existing ones merged before opening new ones. Thanks! |
inFocus7
left a comment
inFocus7
left a comment
There was a problem hiding this comment.
one last nit, but overall lgtm
…mmands Signed-off-by: Peter Jausovec <[email protected]>
|
You already have 3 pull requests open. Please consider working on getting the existing ones merged before opening new ones. Thanks! |
timflannagan
left a comment
timflannagan
left a comment
There was a problem hiding this comment.
A couple of things I missed during the initial review when looking at the changes locally.
|
|
||
| func init() { | ||
| BuildCmd.Flags().StringVar(&buildImage, "image", "", "Full image specification (e.g., docker.io/myorg/my-skill:v1.0.0)") | ||
| BuildCmd.Flags().BoolVar(&buildPush, "push", false, "Push the image to the container registry, specififed by --image") |
There was a problem hiding this comment.
specififed typo, but that seems to also be applicable to the other build commands too.
| dockerExec := docker.NewExecutor(verbose, "") | ||
| if err := dockerExec.CheckAvailability(); err != nil { | ||
| return fmt.Errorf("docker check failed: %w", err) | ||
| } | ||
|
|
||
| if err := buildSkillImage(absPath, dockerExec); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
I missed this during the first review. We initialize a docker executor twice. One here and again on line 109. We already DI the docker executor in buildSkillImage.
| ) | ||
|
|
||
| func init() { | ||
| BuildCmd.Flags().StringVar(&buildImage, "image", "", "Full image specification (e.g., docker.io/myorg/my-skill:v1.0.0)") |
There was a problem hiding this comment.
It looks like we have inconsistent image shorthands across this command, MCP, and agent. MCP uses -n.
Description
Extracts out the 'build' functionality from the
skill publishcommand into a separate command (just like we have it with agents and MCP servers).Docs: agentregistry-dev/website#13
Fixes #243
Change Type
Changelog