- Notifications
You must be signed in to change notification settings - Fork812
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
Config UI: keep mqtt and influx secrets on edit#19956
Conversation
@leon558 sieht auf dem ersten Blick gut aus. Die Mechanik mit dem Ich füge hier bei Gelegenheit noch einen E2E Test hinzu. |
Playwright Test ist hinzugefügt und läuft ✅ @andig hast du noch Anmerkungen? |
Hatte noch keine Zeit. Wenn Test läuft rein damit? |
@leon558 super PR! Bzgl.
|
@andig habe Tests hinzugefügt und die Merge Funktion verwendet jetzt structs.Map. |
| dec := json.NewDecoder(r.Body) | ||
| dec.DisallowUnknownFields() | ||
| if err := dec.Decode(&struc); err != nil { | ||
| jsonError(w, http.StatusBadRequest, err) | ||
| return | ||
| } | ||
| oldStruc := newStruc() | ||
| settings.Json(key, &oldStruc) |
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 err := ...| @@ -111,3 +108,34 @@ func settingsDeleteJsonHandler(key string, valueChan chan<- util.Param, struc an | |||
| jsonResult(w, true) | |||
| } | |||
| } | |||
| func mergeSettingsOld(struc any, old 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.
Das Naming ist unglücklich- from/to?
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.
Kann ich gerne machen. Hat mir selbst nicht so gefallen. Hatte versucht mich an der Codebase zu orientieren.
Finde from/to aber auch bisschen schwierig nachzuvollziehen. Also es ist nicht direkt klar was from und was to sein muss.
Wie wäre es mit old/new und als Name der Funktion einfach nur mergeSettings? Es geht ja im Endeffekt darum die alten und die neuen Einstellung zu vereinen.
| return &TestStruct{} | ||
| } | ||
| handler := settingsSetJsonHandler(key, valueChan, newStruc) |
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.
Es sollte reichen die Funktion zu testen, den Handler brauchen wir m.E. nicht.
Grossartiger PR, vielen Dank! |
Mir fällt gerade im Nachgang auf, dass das embeddete mqtt.Config jetzt leider verschwunden ist. Das ist ziemlich unglücklich, da es an anderer Stelle noch verwendet wird und daher die Gefahr besteht, dass Änderungen an der einen nicht mehr an der anderen Stelle sichtbar sind oder nachgezogen werden. Das sollten wir vor Release noch korrigieren. |
Wo wir das noch verwendet? Ich hatte eigentlich extra geschaut, dass es nicht noch irgendwo benutzt wird. |
Hier: |
Ich habe deine vorherige Aussage falsch verstanden. Ich schaue es mir mal an |
Habe es unter#20340 weitergeführt |
fixes#17084
Passwörter und Schlüssel wurden bei Json Settings wie mqtt und influx bei jedem Bearbeiten überschrieben.
Ich habe die Redacted Funktionen angepasst, sodass jetzt der Websocket und die API alle geheimen Werte mit *** übergeben. Ist aber nicht festgesetzt, kann auch für einzelne Werte angepasst werden.
Ich habe die Get Endpoints für die Settings wieder aktiviert und frage diese beim Bearbeiten ab um die aktuelle Config zu haben.
Beim aktualisieren der Config, vergleiche ich die neue Config mit der alten redacted Config. Wenn vom Frontend genau der selbe Werte zurückkommt wie hingeschickt wurden, wird der alte Wert aus der schon vorhandenen Config genommen. Dadurch kann die Redacted Funktion jederzeit angepasst werden und der Vergleich funktioniert immer noch.
Was haltet ihr von dieser Lösung? Wenn jemand eine bessere Idee hat immer her damit :)
TODO:
state@leon558