Add daemon to the user agent when the CLI is started in daemon mode
#2789
Conversation
MatteoPologruto
added
topic: code
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2789 +/- ##
==========================================
- Coverage 67.57% 67.55% -0.03%
==========================================
Files 238 238
Lines 22362 22372 +10
==========================================
+ Hits 15111 15113 +2
- Misses 6062 6068 +6
- Partials 1189 1191 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Left some comments/
It would be cool if we're able to test it.
Maybe we could use a proxy HTTP, and inspect the call that arrives are with the expected User agent. Something similar to: https://github.com/arduino/arduino-cli/blob/master/internal/cli/configuration/network_test.go#L29-L51
| _, err := srv.SettingsSetValue(cmd.Context(), &rpc.SettingsSetValueRequest{ | ||
| Key: "network.user_agent_ext", | ||
| ValueFormat: "cli", | ||
| EncodedValue: "daemon", | ||
| }) |
There was a problem hiding this comment.
What should we do in case this value is not empty?
- Should we always override with the
daemonvalue - should we append to the previous value
- should we honor the user setting?
/cc @cmaglie
There was a problem hiding this comment.
I think we should go with the third option and honor the user setting. If it's okay with you, I'm going to make this change.
There was a problem hiding this comment.
@MatteoPologruto I agree. Let's ping also @cmaglie and @Xayton as I don't know if they might have better insight
| @@ -58,14 +58,25 @@ func NewCommand(srv rpc.ArduinoCoreServiceServer, settings *rpc.Configuration) * | |||
| }) | |||
| if err != nil { | |||
| // Should never happen... | |||
| panic("Failed to set default value for directories.builtin.libraries: " + err.Error()) | |||
| feedback.Fatal(i18n.Tr("Cannot set the configuration key directories.builtin.libraries: %v", err), feedback.ErrGeneric) | |||
There was a problem hiding this comment.
Why this change? it looks unrelated and adds a string that should never happen to the translations.
| // The user agent should include "daemon" for analytics purposes | ||
| _, err := srv.SettingsSetValue(cmd.Context(), &rpc.SettingsSetValueRequest{ |
There was a problem hiding this comment.
The user agent should include "daemon" for analytics purposes
Actually no, it should infer the user agent from the gRPC client:
arduino-cli/commands/instances.go
Lines 67 to 70 in 6d9c930
I think the Arduino IDE is passing something like arduino-ide/x.y.z into this string.
We should understand why this string is not propagated.
Please check if the PR fulfills these requirements
See how to contribute
before creating one)
our contributing guidelines
UPGRADING.mdhas been updated with a migration guide (for breaking changes)configuration.schema.jsonupdated if new parameters are added.What kind of change does this PR introduce?
Bug fix
What is the current behavior?
The user agent does not specify if the CLI runs in daemon mode. It is always something like
arduino-cli/git-snapshot (amd64; windows; go1.23.2) Commit:84fc413a.What is the new behavior?
network.user_agent_extis set toodaemonwhen the CLI runs in daemon mode. The complete user agent isarduino-cli/git-snapshot daemon (amd64; windows; go1.23.2) Commit:84fc413a.Does this PR introduce a breaking change, and is titled accordingly?
Other information