Repository navigation
Conversation
Signed-off-by: Rishi Jat <rishijat098@gmail.com>
There was a problem hiding this comment.
Code Review
This pull request introduces a composite GitHub Action to automate the installation of modctl and the building of model artifacts, accompanied by updated documentation in the README and getting started guide. The review feedback identifies critical security risks related to shell injection and secret exposure when using GitHub Action expressions directly within shell scripts, advising that inputs and secrets be mapped to environment variables. Furthermore, there is a recommendation to replace brittle JSON parsing logic with more robust tools like the GitHub CLI or jq.
There was a problem hiding this comment.
Pull request overview
This PR introduces a composite GitHub Action (at the repo root) to install modctl and run modctl build in workflows, with optional registry login support, and adds docs + a CI workflow to exercise the action.
Changes:
- Added a root-level composite action (
action.yml) that installsmodctl, optionally logs into a registry, and builds a model artifact. - Added a dedicated workflow to validate the action against “latest” and a pinned
modctlversion and to execute the registry-login path. - Updated README and getting-started docs with usage examples and input reference.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
action.yml |
New composite action that validates inputs, installs modctl from GitHub Releases, optionally logs in, and runs modctl build. |
docs/getting-started.md |
Added a “GitHub Action” section documenting usage, inputs, version pinning, and optional registry integration. |
README.md |
Added a minimal action usage snippet and link to the detailed docs section. |
.github/workflows/modctl-action.yml |
Added CI workflow to exercise local action usage with latest/pinned versions and the registry-login execution path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Rishi Jat <rishijat098@gmail.com>
|
/cc @sabre1041 |
sabre1041
left a comment
There was a problem hiding this comment.
Thanks for submitting this contribution as it is the starting point for simplifying how users leverage ModelPack within their GitHub Actions Workflows. A few comments from my side
- Recommend changing the
output_remotevariable topushto make it more explicit - There should be a way to determine the latest release version without the use of
ghand the need for aGITHUB_TOKEN. Since the repository is already public, we should be able to determining the appropriate version without requiring credentials - If publishing (
push=true), include an output parameter with the location of the remote artifact. It would be good to include the digest of the published artifact (not yet a feature inmodctlbut might be a good feature request
Actions are best placed in a separate repository so that they can have an independent lifecycle to the primary modctl artifact. If others think the same, we can look to create a separate repository
|
@sabre1041 I don't have much experience using a separate github action repository. I have one question: how can we update the action when the actual code needs to be updated at the same time? It may not be the case for this PR, but it seems we'd have to send two PRs separately in such case?
|
For non API based changes in the modctl repository, no changes will need to be made on the actions repository as each invocation will retrieve the latest released version. End users can also version lock themselves on a particular version of the action as well as the modctl tool |
IMO, we have two kinds of actions:
We can still have both of these actions in a separate repo. Just a little bit complicated because we need to figure out what triggered the action and then decide if we need to build the modctl binary. |
Signed-off-by: Rishi Jat <rishijat098@gmail.com>
There was a problem hiding this comment.
Review of the current head (f35ffec) against today's main. The CI workflow in this PR passes for both the latest and the 0.2.0 matrix entries and for the login path, so the action works end to end. Findings, most important first.
1. Merge conflict in README.md. main moved; the PR needs a rebase before it can merge.
2. Latest-version lookup uses the unauthenticated GitHub API (action.yml, "Install modctl" step). curl https://api.github.com/repos/modelpack/modctl/releases/latest is limited to 60 requests per hour per IP. Shared runners hit that limit, and the step then fails with "Unable to resolve latest modctl release tag". @sabre1041 asked for a token-free method; the redirect of the releases page needs no API quota:
release_tag="$(curl -fsSIL -o /dev/null -w '%{url_effective}' https://github.com/modelpack/modctl/releases/latest)"
release_tag="${release_tag##*/}" # -> v0.2.2I verified the redirect target today: https://github.com/modelpack/modctl/releases/tag/v0.2.2.
3. artifact-location output is derived by grepping build output. The regex [A-Za-z0-9._/-]+:[A-Za-z0-9._-]+ matches many things in the progress output (timestamps, digests, sha256:...), and tail -n1 picks whichever came last. When push=true the location is already known: it is artifact_name. Set the output from the input and drop the grep.
4. Asset layout matches the releases. I checked modctl-0.2.2-linux-arm64.tar.gz: it contains LICENSE, README.md, modctl at the top level, so the install step's ${tmp_dir}/modctl check is correct for current releases.
5. Minor. build_output="$("${build_cmd[@]}" 2>&1)" hides the live progress bar until the build finishes; for large models that looks like a hang in the job log. modctl build has no --disable-progress flag today, so the simplest fix is to let the command write to the log directly and only check its exit status (which point 3 makes possible).
The open question from @sabre1041 and @bergwolf about whether the action should live in this repository or in a separate one is a maintainer decision and gates this PR more than the points above.
Summary
This change introduces a composite GitHub Action to streamline the use of modctl in GitHub Actions workflows.
The action installs modctl (latest or pinned version), builds a model artifact from a Modelfile, and optionally performs registry authentication for remote artifact output.
Key points:
Validation:
Documentation:
Notes:
Fixes #507