Update version.go to fix running a server built from source #4170
Conversation
currently when you try to run a server using the binary built from
source and try to run any command it throws this error, this error can be fixed by stripping the metadata from version before parsing
bin/darwin/amd64/bacalhau serve --node-type compute --node-type requester
bin/darwin/amd64/bacalhau node list
```
Error: failed request: Unexpected response code: 403 ({
"Error": "Client version is outdated. Update your client",
"MinVersion": "1.4.0",
"ClientVersion": "1.4.0-6-g081eabfba",
"ServerVersion": "1.4.0-6-g081eabfba"
})
```
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
There was a problem hiding this comment.
The problem here is that versions with a pre-release are considered less than versions without a prerelease all other fields equal. i.e. v1.4.0 > v1.4.0-abc123. I believe the correct change here is to strip the pre-release tag from the client before doing the comparison. @wdbaruni wdyt?
|
|
||
| // Strip the pre-release tag | ||
| strippedClientVersion, err := stripPreRelease(clientVersion) | ||
| if err != nil { | ||
| notif.Message = fmt.Sprintf("error stripping pre-release tag from client version: %s", err) | ||
| return nil | ||
| } | ||
|
|
||
| // extract parsed client version for comparison | ||
| notif.ClientVersion = clientVersion.String() | ||
| notif.ClientVersion = strippedClientVersion.String() | ||
|
|
||
| diff := serverVersion.Compare(clientVersion) | ||
| diff := serverVersion.Compare(strippedClientVersion) |
There was a problem hiding this comment.
we don't want to strip the client version here, this method is for logging the version of the request received from the client. Please revert this change.
| func stripPreRelease(version *semver.Version) (*semver.Version, error) { | ||
| newVersion, _ := semver.NewVersion(version.String()) | ||
| _, err := newVersion.SetPrerelease("") | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return newVersion, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
| func stripPreRelease(version *semver.Version) (*semver.Version, error) { | |
| newVersion, _ := semver.NewVersion(version.String()) | |
| _, err := newVersion.SetPrerelease("") | |
| if err != nil { | |
| return nil, err | |
| } | |
| return newVersion, nil | |
| } | |
| func stripPreRelease(versionStr string) (*semver.Version, error) { | |
| v, err := semver.NewVersion(versionStr) | |
| if err != nil { | |
| return version.Unknown, fmt.Errorf("failed to parse client version: %w", err) | |
| } | |
| strippedVersion, err := v.SetPrerelease("") | |
| if err != nil { | |
| return version.Unknown, fmt.Errorf("failed to strip pre-release version from client: %w", err) | |
| } | |
| return &strippedVersion, nil | |
| } |
please move this utility method to the bottom of the file.
| @@ -109,12 +126,20 @@ func VersionCheckMiddleware(serverVersion, minVersion semver.Version) echo.Middl | |||
| }) | |||
| } | |||
|
|
|||
| if clientVersion.LessThan(&minVersion) { | |||
There was a problem hiding this comment.
please use the suggested implementation of stripPreRelease here to both parse and strip the client version, i.e.
// VersionCheckMiddleware returns a middleware that checks if the client version is at least minVersion.
func VersionCheckMiddleware(serverVersion, minVersion semver.Version) echo.MiddlewareFunc {
return func(next echo.HandlerFunc) echo.HandlerFunc {
return func(c echo.Context) error {
clientVersionStr := c.Request().Header.Get(apimodels.HTTPHeaderBacalhauGitVersion)
if clientVersionStr == "" ||
clientVersionStr == version.DevelopmentGitVersion ||
clientVersionStr == version.UnknownGitVersion {
// allow the request to pass through
return next(c)
}
clientVersion, err := stripPreRelease(clientVersionStr)
if err != nil {
return c.JSON(http.StatusBadRequest, map[string]string{
"Error": "Invalid client version " + clientVersionStr,
})
}
if clientVersion.LessThan(&minVersion) {
// Client version is less than the minimum required version
return c.JSON(http.StatusForbidden, VersionCheckError{
Error: "Client version is outdated. Update your client",
MinVersion: minVersion.String(),
ClientVersion: clientVersion.String(),
ServerVersion: serverVersion.String(),
})
}
// Client version is acceptable, proceed with the request
return next(c)
}
}
}
|
I don't think this is an issue anymore |
currently when you try to run a server using the binary built from source and try to run any command it throws this error, this error can be fixed by stripping the metadata from version before parsing
To run test jobs against the local cluster which uses the build from source binary without getting this error everytime a job is run this error needs to be fixed
bin/darwin/amd64/bacalhau serve --node-type compute --node-type requester
bin/darwin/amd64/bacalhau node list