mcp: use build-time version instead of hardcoded "0.1.0" - #3832
mcp: use build-time version instead of hardcoded "0.1.0"#3832Ankitsinghsisodya wants to merge 2 commits into
Conversation
|
Hi @Ankitsinghsisodya. Thanks for your PR. I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Introduces a centralized version parsing API and updates MCP/CLI version reporting to use a normalized semver string for machine consumers while preserving the original injected value for human-readable output.
Changes:
- Added
pkg/version.Get()withDefaultVersfallback and semver parsing, plus unit tests. - Updated MCP server implementation and healthcheck tool to report
version.Get().String()instead of a hardcoded constant. - Updated CLI version wiring to use
version.Get().Original()and deduplicated the default version constant.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/version/version.go | Adds semver-backed version parsing with a shared fallback constant. |
| pkg/version/version_test.go | Adds tests for empty, injected, and invalid version behavior. |
| pkg/mcp/mcp.go | Replaces hardcoded MCP server version with parsed version string. |
| pkg/mcp/tools_healthcheck.go | Reports normalized version string in healthcheck output. |
| pkg/app/app.go | Uses the original injected version string for CLI output. |
| cmd/root.go | Delegates default version constant to version.DefaultVers. |
| go.mod | Adds a direct dependency on github.com/Masterminds/semver/v3. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3832 +/- ##
==========================================
- Coverage 57.03% 53.33% -3.70%
==========================================
Files 182 201 +19
Lines 21376 23456 +2080
==========================================
+ Hits 12191 12511 +320
- Misses 7953 9699 +1746
- Partials 1232 1246 +14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Let's keep the Finally, we'd need a patch the Makefile to ensure it doesn't overwrite the default with an empty value ( Here's the whole package version
const Default = "v0.0.0+source"
var (
Vers = Default // overwritten by ldflags when make actually has a describe
Kver string
Hash string
)No And here's a proper conditional for the Makefile: LDFLAGS :=
ifneq ($(VERS),)
LDFLAGS += -X knative.dev/func/pkg/version.Vers=$(VERS)
endif
ifneq ($(KVER),)
LDFLAGS += -X knative.dev/func/pkg/version.Kver=$(KVER)
endif
ifneq ($(HASH),)
LDFLAGS += -X knative.dev/func/pkg/version.Hash=$(HASH)
endifMCP server can load it in on start: s := &Server{
prefix: "func",
version: version.Vers,
// ...
}Healthcheck can just use the member directly: output = HealthcheckOutput{
Status: "ok",
Message: "The MCP server is running!",
Version: s.version,
}CLI can use the strings directly, no // pkg/app/app.go
Version: cmd.Version{
Vers: version.Vers,
Kver: version.Kver,
Hash: version.Hash,
}// cmd/root.go
const DefaultVersion = version.DefaultFurthermore, the check in
Lastly, we can keep the leading |
lkingland
left a comment
There was a problem hiding this comment.
Let's keep the version package a simple "string-stamp" library. Move the default from the CLI in there like you did. Set Vers to that default. Then it's up to the user if they want it as a Semver or not.
Finally, we'd need a patch the Makefile to ensure it doesn't overwrite the default with an empty value (-X pkg.Var= with an empty value clears the Go initializer).
Here's the whole version package:
package version
const Default = "v0.0.0+source"
var (
Vers = Default // overwritten by ldflags when make actually has a describe
Kver string
Hash string
)No Get(), no Masterminds. Semver stays an exercise for the caller (func version already parses if it needs to).
And here's a proper conditional for the Makefile:
LDFLAGS :=
ifneq ($(VERS),)
LDFLAGS += -X knative.dev/func/pkg/version.Vers=$(VERS)
endif
ifneq ($(KVER),)
LDFLAGS += -X knative.dev/func/pkg/version.Kver=$(KVER)
endif
ifneq ($(HASH),)
LDFLAGS += -X knative.dev/func/pkg/version.Hash=$(HASH)
endif(be sure to keep the existing LDFLAGS += -X ... SocatImage / TarImage / FuncUtilImage lines after it)
MCP server can load it in on new:
s := &Server{
prefix: "func",
version: version.Vers,
// ...
}
i := mcp.NewServer(&mcp.Implementation{
Name: name,
Title: title,
Version: s.version, // same stamp
}, ...)Healthcheck can just use the member directly:
output = HealthcheckOutput{
Status: "ok",
Message: "The MCP server is running!",
Version: s.version,
}
CLI can use the strings directly, no .Get().Original() confusion:
// pkg/app/app.go
Version: cmd.Version{
Vers: version.Vers,
Kver: version.Kver,
Hash: version.Hash,
}// cmd/root.go
const DefaultVersion = version.DefaultFurthermore, the check in cmd/version.go can be removed because of the Makefile hardening:
if v.Vers == "" { v.Vers = DefaultVersion }
Lastly, we can keep the leading v for now because a semver is a simple strings.TrimPrefix away in most cases, and this way we're coherent with Git/Go/Knative/Kubernetes.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Ankitsinghsisodya The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
f7c7f8d to
91c910d
Compare
The MCP server advertised a static version = "0.1.0" regardless of the actual binary version. Stamp the Server with pkg/version.Vers (already set via ldflags, defaulting to "v0.0.0+source" for source builds) and use it for both the MCP Implementation version and the healthcheck tool's reported version. Per review feedback, keep pkg/version a plain string-stamp package with no semver parsing (no Get(), no Masterminds dependency), and harden the Makefile's LDFLAGS so an empty VERS/KVER/HASH doesn't clear the package-level defaults via -X.
91c910d to
0d86f23
Compare
|
@Ankitsinghsisodya: The following test failed, say
DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@lkingland Can you re run the tests? |
Summary
version = "0.1.0"constant to every MCP client regardless of the actual binary version, causing stale version metadata in clients that display server info.version.Versfrompkg/version, which is already injected at build time via ldflags (-X knative.dev/func/pkg/version.Vers=$(VERS))."0.0.0+source"when the variable is empty (source builds that bypass the Makefile), consistent with howcmd/root.gohandles the same case.Changes
pkg/mcp/mcp.go: removeconst version = "0.1.0", importknative.dev/func/pkg/version, and useversion.Vers(with fallback) when constructing themcp.Implementationpassed tomcp.NewServer.Test plan
go test ./pkg/mcp/...passesfunc versionandfunc mcp startreport the same version stringfunc mcp start