-
Notifications
You must be signed in to change notification settings - Fork 3
Improve the arduino-app-cli version command by adding the "server version" #31 #49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
7484347 to
466245c
Compare
lucarin91
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I added some minor suggestions to make it more readable (in my opinion)
dido18
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rename the serverVersion into daemonVersion all over the place
| return net.JoinHostPort(h, p), nil | ||
| } | ||
|
|
||
| func getServerVersion(httpClient http.Client, url string) string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be useful to log DEBUG/WARN message with the error of this check.
Proposal:
The function returns a string, error.
If there is an error, the caller logs it, assigns the "empty" daemon version and prints a generic "error on get daemon version" in the cli response.
| } | ||
|
|
||
| if daemonVersion == "" { | ||
| return result, fmt.Errorf("cannot connect to %s", hostAndPort) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This hardcoded error could be misleading if it's not a connection error.
See the proposal to return the error and log it in the comment below
| AppName string `json:"appName"` | ||
| Version string `json:"version"` | ||
| Name string `json:"name"` | ||
| ClientVersion string `json:"client_version"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This client_version is a breaking change in the JSON response.
Current:
GET http://127.0.0.1:8800/v1/version
{
"version":"v0.2.3"
}
Proposal: leave the version and add the name and daemon_version
GET http://127.0.0.1:8800/v1/version
{
"name": "Arduino App CLI",
"version":"v0.2.3"
"daemon_version": "v0.2.3"
}
| serverMessage := fmt.Sprintf("%s client version %s", | ||
| ProgramName, r.ClientVersion) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| serverMessage := fmt.Sprintf("%s client version %s", | |
| ProgramName, r.ClientVersion) | |
| serverMessage := fmt.Sprintf("%s client version %s", ProgramName, r.ClientVersion) |
| serverMessage = fmt.Sprintf("%s\ndaemon version: %s", | ||
| serverMessage, r.DaemonVersion) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| serverMessage = fmt.Sprintf("%s\ndaemon version: %s", | |
| serverMessage, r.DaemonVersion) | |
| serverMessage = fmt.Sprintf("%s\ndaemon version: %s", serverMessage, r.DaemonVersion) |
|
|
||
| // Leverage the http.Client's RoundTripper | ||
| // to return a canned response and bypass network calls. | ||
| type Tripper func(*http.Request) (*http.Response, error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I usually prefer using the httptest module instead of mocking the http client
| resultMessage = fmt.Sprintf("%s\ndaemon version: %s", | ||
| resultMessage, r.DaemonVersion) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit
| resultMessage = fmt.Sprintf("%s\ndaemon version: %s", | |
| resultMessage, r.DaemonVersion) | |
| resultMessage = fmt.Sprintf("%s\ndaemon version: %s", resultMessage, r.DaemonVersion) |
Co-authored-by: Luca Rinaldi <l.rinaldi@arduino.cc>
Co-authored-by: Luca Rinaldi <l.rinaldi@arduino.cc>
Co-authored-by: Luca Rinaldi <l.rinaldi@arduino.cc>
Motivation
When running arduino-app-cli version instead of only returning the version of the currently run executable (the CLI) we should also return the version of the server (the daemon/service running and waiting for HTTP API requests).
Change description
This change make a version request to the running daemon and adds the output to the
arduino-app-cli versioncommand.It also adds "daemon_version" and rename "appName" to "name" from the json output.
Additional Notes
Test done with a daemon with version 8.8.8-server-dev running on port 8080. The output is:
The http server part is unaffected:
Reviewer checklist
main.