cli/token: tolerate missing token credentials when generating them#30416
cli/token: tolerate missing token credentials when generating them#30416andig wants to merge 1 commit into
Conversation
configureVehicles fully instantiates and validates the vehicle config, which fails with "missing required `accessToken`" precisely because the tokens this command exists to generate are not set yet. Gather the vehicle configs directly instead and, when the instance render fails validation, stub empty required params so the instance type can still be resolved. The stubbed params are the empty credentials, which the token generators do not read.
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
renderTokenInstance, whenDecodeOtherorByNamefail you currently return the original validationerr, which hides the real failure; consider returning or wrappingderr/terrinstead so callers see the actual root cause. tokenVehiclesappends from bothconf.Vehiclesandconfig.ConfigurationsByClasswithout deduplication, so a vehicle defined in both places would be processed twice; if that’s undesirable, add a simple name-based dedupe when buildingres.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `renderTokenInstance`, when `DecodeOther` or `ByName` fail you currently return the original validation `err`, which hides the real failure; consider returning or wrapping `derr`/`terr` instead so callers see the actual root cause.
- `tokenVehicles` appends from both `conf.Vehicles` and `config.ConfigurationsByClass` without deduplication, so a vehicle defined in both places would be processed twice; if that’s undesirable, add a simple name-based dedupe when building `res`.
## Individual Comments
### Comment 1
<location path="cmd/token.go" line_range="148-109" />
<code_context>
+ Template string
+ Other map[string]any `mapstructure:",remain"`
+ }
+ if derr := util.DecodeOther(other, &cc); derr != nil {
+ return nil, err
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Return the decode error instead of the original render error to avoid hiding the real failure cause.
If `util.DecodeOther` fails, returning the earlier `RenderInstance` error hides the actual decode failure and makes troubleshooting harder. This should return `derr` instead (optionally wrapped with additional context) so callers see the real root cause.
</issue_to_address>
### Comment 2
<location path="cmd/token.go" line_range="152-109" />
<code_context>
+ return nil, err
+ }
+
+ tmpl, terr := templates.ByName(templates.Vehicle, cc.Template)
+ if terr != nil {
+ return nil, err
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Return the template lookup error instead of the original render error for clearer diagnostics.
If `templates.ByName` fails, returning the earlier `RenderInstance` error hides the actual cause of the failure. Instead, return (or wrap) `terr` so callers can see that the template lookup failed.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| configurable, err := config.ConfigurationsByClass(templates.Vehicle) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
issue (bug_risk): Return the decode error instead of the original render error to avoid hiding the real failure cause.
If util.DecodeOther fails, returning the earlier RenderInstance error hides the actual decode failure and makes troubleshooting harder. This should return derr instead (optionally wrapped with additional context) so callers see the real root cause.
|
|
||
| configurable, err := config.ConfigurationsByClass(templates.Vehicle) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
issue (bug_risk): Return the template lookup error instead of the original render error for clearer diagnostics.
If templates.ByName fails, returning the earlier RenderInstance error hides the actual cause of the failure. Instead, return (or wrap) terr so callers can see that the template lookup failed.
fixes #29864
evcc token <name>aborts withcannot create vehicle type 'template': missing required accessTokenbefore it can generate anything.cmd/token.gocallsconfigureVehicles, which fully instantiates and validates each vehicle inRenderModeInstance; the Peugeot/PSA templates markaccessToken/refreshTokenasrequired: true, so validation rejects the empty tokens. This is a chicken-and-egg: the command exists to produce those tokens, but config validation refuses to run without them. Regression from #24716, which switched the command toconfigureVehicles. (Not reproducible ingo testbecause required-field validation is skipped undertesting.Testing().)Per the direction in the previous attempt (#30133): rather than resolving things through YAML, analyse the error and continue when it is the expected missing-credentials case.
tokenVehiclesgathers the vehicle configs (static + database) directly, without instantiating them, so the command works even while the credentials are missingrenderTokenInstanceresolves the template type via the normalRenderInstance; only when that returns aConfigErrordoes it retry with empty required params stubbed, so the instance type resolves. The stubbed params are exactly the empty credentials, which the token generators (PSA/ford/tronity) do not read — they perform a fresh OAuth flowVerified against the real binary: on master
evcc token carfatals withmissing required+"accessToken"+`; with this change it proceeds into the PSA OAuth flow (country-code prompt) as expected.