- Notifications
You must be signed in to change notification settings - Fork1
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?
Uh oh!
There was an error while loading.Please reload this page.
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 to466245cCompareThere 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)
| url,err:=getValidOrDefaultUrl(host) | ||
| iferr!=nil { | ||
| feedback.Fatal(i18n.Tr("Error: invalid host:port format"),feedback.ErrBadArgument) | ||
| } |
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.
Here you can just useurl.Parse, but anyway I would parse the url right after getting the flag, e.g.,host, _ := cmd.Flags().GetString("host"), so you can directly handle the error.
Then here you can just add the path with
url = url.Join("/v1/version")| returnserverResponse.Version,nil | ||
| } | ||
| typeserverVersionResponsestruct { |
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.
Usually structs used only for parsing JSON are defined directly inside the function that uses them
| } | ||
| varserverResponseserverVersionResponse | ||
| iferr:=json.Unmarshal(body,&serverResponse);err!=nil { |
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.
Probably in this case, you aren't improving much, but usually it is better to make JSON handling the decoding buffer instead of reading the body into a buffer and then passing it to json library.
| iferr:=json.Unmarshal(body,&serverResponse);err!=nil { | |
| varserverResponsestruct { | |
| Versionstring`json:"version"` | |
| } | |
| iferr:=json.NewDecoder(resp.Body).Decode(&serverResponse);err!=nil { |
| func (rversionResult)String()string { | ||
| returnfmt.Sprintf("%s v%s",r.AppName,r.Version) | ||
| returnfmt.Sprintf("client: %s\nserver: %s", |
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 am wondering if we should make a more text-friendly line like
Arduino App CLI v0.6.6daemon server at "localhost:8800" v0.6.6| ClientVersionstring`json:"version"` | ||
| ServerVersionstring`json:"serverVersion"` |
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 probably return those info
| ClientVersionstring`json:"version"` | |
| ServerVersionstring`json:"serverVersion"` | |
| AppNamestring`json:"app_name"` | |
| Versionstring`json:"version"` | |
| DaemonVersionstring`json:"daemon_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.
We could print the name with the arduino-flasher-cli outputhttps://github.com/arduino/arduino-flasher-cli/blob/1899741a4e890a2ff0727669d23d25b6499e0522/main.go#L63
Arduino App CLIBUT I would avoid theapp_name that can be misleading (the term "app" is for the Arduino Apps).
maybe only "name" in the json field.
| funcversionHandler(clientVersionstring,hoststring) { | ||
| httpClient:= http.Client{ | ||
| Timeout:time.Second, | ||
| } | ||
| result:=doVersionHandler(httpClient,clientVersion,host) | ||
| feedback.PrintResult(result) | ||
| } |
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.
Maybe this extra function is too much. I think it is ok to just move this code directly inside the Run function of the cobra command.
| const ( | ||
| DefaultHostname="localhost" | ||
| DefaultPort="8800" | ||
| ) |
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.
if there isn't any reason, I would just define a single const
| const ( | |
| DefaultHostname="localhost" | |
| DefaultPort="8800" | |
| ) | |
| constdefaultDaemon="localhost:8800" |
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.