- Notifications
You must be signed in to change notification settings - Fork3
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 ourterms of service andprivacy 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
CLAassistant commentedNov 6, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
|
7484347 to466245cCompare
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)
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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 theserverVersion intodaemonVersion all over the place
| expectedResult:"localhost:8800", | ||
| }, | ||
| { | ||
| name:"Custom host and port should return the default.", |
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.
| name:"Custom host and port should returnthe default.", | |
| name:"Custom host and port should returnprovided host:port.", |
| cmd:=&cobra.Command{ | ||
| Use:"version", | ||
| Short:"Print the versionnumber ofArduino App CLI", | ||
| Short:"Print theclient and serverversionnumbers for theArduino App CLI.", |
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.
| Short:"Print the client and serverversion numbers for the Arduino App CLI.", | |
| Short:"Print the client and serverversions for the Arduino App CLI.", |
| returnnet.JoinHostPort(h,p),nil | ||
| } | ||
| funcgetServerVersion(httpClient http.Client,urlstring)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 astring, 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.
| } | ||
| ifdaemonVersion=="" { | ||
| returnresult,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
| AppNamestring`json:"appName"` | ||
| Versionstring`json:"version"` | ||
| Namestring`json:"name"` | ||
| ClientVersionstring`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.
Thisclient_version could be a breaking change in the JSON response.
Current:
GET http://127.0.0.1:8800/v1/version{"version":"v0.2.3"}Proposal: leave theversion and add thename anddaemon_version
GET http://127.0.0.1:8800/v1/version{"name": "Arduino App CLI","version":"v0.2.3""daemon_version": "v0.2.3" }
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 "ServerVersion" and removes "AppName" from the output.
Additional Notes
Reviewer checklist
main.