go.mod: don't specify toolchain and fix cli module #195
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR removes explicit Go toolchain directives, standardizes Go version to the lowest patch .0, corrects the CLI submodule's module path, and updates the Dockerfile's Go version argument. Entity relationship diagram for updated Go moduleserDiagram
"model-runner" {
string module_name
string go_version
}
"cli" {
string module_name
string go_version
}
"model-runner" ||--o| "cli" : contains
"model-runner" : module_name = "github.com/docker/model-runner"
"model-runner" : go_version = "1.24.0"
"cli" : module_name = "github.com/docker/model-runner/cmd/cli"
"cli" : go_version = "1.24.0"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary of ChangesHello @crazy-max, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the Go module configuration and Dockerfile Go versioning to improve build consistency and module management. It removes explicit toolchain declarations, standardizes Go version specifications to their base patch level, and corrects the module path for the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Pull Request Overview
This PR standardizes Go module configuration and fixes module naming issues. It removes explicit toolchain specifications and corrects the CLI submodule's module path to follow proper Go module conventions.
- Remove explicit
toolchainspecifications from go.mod files - Standardize Go version format to use
.0patch version - Fix CLI submodule path from
github.com/docker/model-clitogithub.com/docker/model-runner/cmd/cli
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| go.mod | Remove toolchain specification and standardize Go version to 1.24.0 |
| cmd/cli/go.mod | Fix module path and remove toolchain specification |
| Dockerfile | Update Go version ARG to match module version format |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| go 1.24 | ||
|
|
||
| toolchain go1.24.4 | ||
| go 1.24.0 |
There was a problem hiding this comment.
Go version in go.mod should not include patch version. Use 'go 1.24' instead of 'go 1.24.0' as the patch version is not meaningful in go.mod files and Go toolchain ignores it.
| go 1.24.0 | |
| go 1.24 |
There was a problem hiding this comment.
That's wrong copilot, starting with Go 1.21, the Go team introduced a more precise versioning format that includes the patch version so we should set it to the lowest and let the local toolchain deal with compat
| go 1.24 | ||
|
|
||
| toolchain go1.24.4 | ||
| go 1.24.0 |
There was a problem hiding this comment.
Go version in go.mod should not include patch version. Use 'go 1.24' instead of 'go 1.24.0' as the patch version is not meaningful in go.mod files and Go toolchain ignores it.
| go 1.24.0 | |
| go 1.24 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Run go mod tidy on both the root and cmd/cli modules to clean up go.sum and reflect these changes.
- Search for and update any remaining import paths or documentation references from github.com/docker/model-cli to github.com/docker/model-runner/cmd/cli.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Run go mod tidy on both the root and cmd/cli modules to clean up go.sum and reflect these changes.
- Search for and update any remaining import paths or documentation references from github.com/docker/model-cli to github.com/docker/model-runner/cmd/cli.
## Individual Comments
### Comment 1
<location> `Dockerfile:3` </location>
<code_context>
# syntax=docker/dockerfile:1
-ARG GO_VERSION=1.24.2
+ARG GO_VERSION=1.24
ARG LLAMA_SERVER_VERSION=latest
ARG LLAMA_SERVER_VARIANT=cpu
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Using a major.minor version for GO_VERSION may lead to non-deterministic builds.
Pinning GO_VERSION to a specific patch release ensures consistent builds and avoids unexpected changes from future patch updates.
```suggestion
ARG GO_VERSION=1.24.2
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several improvements to the Go module configuration and Docker build process. The changes correctly update the module path for the cmd/cli submodule, which is a crucial fix for its usability. Removing the toolchain directive from the go.mod files is a good practice that provides more flexibility for developers and CI environments. Setting the Go version to 1.24.0 and updating the GO_VERSION argument in the Dockerfile are consistent and align with modern Go practices. Overall, these are solid changes that improve the project's maintainability and build configuration.
<!--Delete sections as needed --> ## Description <!-- Tell us what you did and why --> ## Related issues or tickets * docker/model-runner#190 * docker/model-runner#195 ## Reviews <!-- Notes for reviewers here --> <!-- List applicable reviews (optionally @tag reviewers) --> - [ ] Technical review - [ ] Editorial review - [ ] Product review Signed-off-by: CrazyMax <[email protected]>
|
Do we know why CI didn't fail on the original PR? Seems like the code wouldn't compile before this fix 🤔 Or does CI have to be modified to account for multiple-modules? They can be a bit of a pain, because a |
3 participants
go.modat the lowest and should always be.0../cmd/cli.Also found that tag that have been created are wrong if they are meant for cmd/cli submodule: https://github.com/docker/model-runner/tags
@doringeman It should be
cmd/cli/v0.1.43so then you can vendorgithub.com/docker/model-runner/cmd/cli v0.1.43for the cli reference on docs repo.cc @thaJeztah @vvoland for extra 👀 related to packaging.
Summary by Sourcery
Remove explicit Go toolchain specs and align Go version patches in go.mod files, fix the CLI submodule’s module path, and simplify the Dockerfile’s GO_VERSION argument
Bug Fixes:
Enhancements:
Build: