-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
EEBUS: configure by default #26944
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: master
Are you sure you want to change the base?
EEBUS: configure by default #26944
Conversation
Co-authored-by: Michael Geers <[email protected]>
Co-authored-by: Michael Geers <[email protected]>
Co-authored-by: Michael Geers <[email protected]>
|
I've added shipid/ski fields and explanation. Also adjusted the json save/delete workflow. The roundtrip should work now. @Maschga Can you do the e2e? Basic flow like verify values exist on start. Change shipid and restart. Verify SKI changes after reboot when user deletes config and restarts. Also noticed that private key is not redacted yet.
|
Since we're currently still using the hacky "list as texturea" ui I'd not do autocomplete. We should upgrade the ui in da dedicated ticket and make it a list of inputs. Doing a similar thing in #26698 for zones. Could be similar. |
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.
Hey - I've found 1 issue, and left some high level feedback:
- The new util.Masked helper takes an
anyparameter but compares it directly to a string literal (s != ""), which will not compile; consider changing the parameter type tostring(or converting viafmt.Sprint) to avoid type errors and make its intent explicit. - The frontend
EebusConfigTypeScript type still declares a requireduri: string, while the backend now treatsurias deprecated (URI_with no default set) and the UI does not expose it; consider either removing this field or making it optional to keep the TS model aligned with the actual JSON payload.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new util.Masked helper takes an `any` parameter but compares it directly to a string literal (`s != ""`), which will not compile; consider changing the parameter type to `string` (or converting via `fmt.Sprint`) to avoid type errors and make its intent explicit.
- The frontend `EebusConfig` TypeScript type still declares a required `uri: string`, while the backend now treats `uri` as deprecated (`URI_` with no default set) and the UI does not expose it; consider either removing this field or making it optional to keep the TS model aligned with the actual JSON payload.
## Individual Comments
### Comment 1
<location> `util/redact.go:3-4` </location>
<code_context>
+package util
+
+func Masked(s any) string {
+ if s != "" {
+ return "***"
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** The Masked helper uses `any` but compares directly to a string, which wonβt compile.
Because `s` is typed as `any`, `s != ""` is an invalid comparison and the function will not compile. Either change the signature to `func Masked(s string) string` (since all call sites pass strings), or convert to string first (e.g. `str := fmt.Sprint(s)` and compare `str`). The former is likely the safest and clearest fix.
</issue_to_address>Help me be more useful! Please click π or π on each comment and I'll use the feedback to improve your reviews.
api/globalconfig/types.go
Outdated
| FromYaml bool `json:"fromYaml,omitempty"` | ||
| } | ||
|
|
||
| func (i Info) Redacted() any { |
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.
Die redacted Methode gehΓΆrt nicht auf das Info
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.
Wie bin ich dann in der Lage darunterliegende Structs wie hier EEBus zu redacten?
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.
Genau wie jetzt auch oder vorher.
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.
Info ist ein Wrapper fΓΌr die Structs wie EEBus, das gabs vorher so noch nicht, soweit ich weiΓ.
Ich finde zumindest kein Muster im aktuellen Backend Code, wie ich EEBus ansonsten redacten kann?
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.
Siehe letzter Commit- einfach verschoben. Was mir noch auffΓ€llt: bei Sponsor redacten wir Status, sonst Config. Ist das Objekt bei Sponsor an der richtigen Stelle?
| ) | ||
|
|
||
| // Info for publishing config, status and source to UI and external systems | ||
| type Info struct { |
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.
Can we find a better name? Info is as arbitrary as it gets.
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.
\cc @naltatis



Fix #26596
Ref #26615
πββ‘οΈ run EEBus automatically at startup
π© add services section for OCPP, SHM and EEBus
π modal is readonly if configured in evcc.yaml
TODO:
autocompletion for interfaces (?)\cc @andig @naltatis