fix fleet settings not being saved to node config #9259
Conversation
Pull Request Checklist
|
Jenkins BuildsClick to see older builds (12)
|
|
Thanks, Core should be owning this issue 100% as part of delivering v1. @adambabik even created this issue #9216 and core was pinged about it. It'd be good to have clarity and confidence that this will be dealt with from the Core team. |
oskarth
requested review from
andremedeiros and
rasom
and removed request for
oskarth
| (multiaccounts.update/update-settings new-settings | ||
| {}) | ||
| (node/prepare-new-config {:on-success #(when (not= (node/get-log-level settings) (node/get-log-level new-settings)) | ||
| (multiaccounts.update/update-settings new-settings {}) | ||
| (node/prepare-new-config {:on-success #(when (not= (node/get-log-level settings) | ||
| (node/get-log-level new-settings)) |
There was a problem hiding this comment.
Are there any changes at all? Is it just code formatting?
There was a problem hiding this comment.
just formatted to be the same layout as in fleet.core and more readable
| (def default-fleets (slurp "resources/config/fleets.json")) | ||
|
|
||
| (defn fleets [{:keys [custom-fleets]}] | ||
| (as-> [(default-fleets)] $ | ||
| (mapv #(:fleets (types/json->clj %)) $) | ||
| (conj $ custom-fleets) | ||
| (reduce merge $))) | ||
|
|
||
| (defn current-fleet-key [db] | ||
| (keyword (get-in db [:multiaccount :settings :fleet] | ||
| config/fleet))) | ||
|
|
||
| (defn get-current-fleet | ||
| [db] | ||
| (get (fleets db) | ||
| (current-fleet-key db))) | ||
|
|
There was a problem hiding this comment.
Why do we need to move this code form fleet.core to node.core?
There was a problem hiding this comment.
fleet.core is mostly about saving a new fleet config and to do that you need to call node.core. So if get-current-fleet is in fleet.core as well you get a circular dependency. I suppose we could make a fleet.db ns otherwsie. Right now it's similar to log-level.core and get-log-level is in node.core too.
There was a problem hiding this comment.
I would go for that fleet.something ns, then we actually don't need to make changes in other namespaces except fleet.core and node.core.
There was a problem hiding this comment.
ok but shouldn't I change log-level namespace as well for consistency then? Also it will still change other namespaces, but just the ns imported.
There was a problem hiding this comment.
We def can do it without changes in other namespaces, the call can go through fleet.core, not a big deal though.
Regarding log-level, it's not objective of this PR so I wouldn't do this here. Actually if everyone is fine with putting that fleet related code into node.core no problem. But it's a bit puzzling then what are fleet.core and node.core responsible for.
|
|
||
| (defn fleets [{:keys [custom-fleets]}] | ||
| (as-> [(default-fleets)] $ | ||
| (mapv #(:fleets (types/json->clj %)) $) |
There was a problem hiding this comment.
Should we not rather do the conversion in the def above?
yenda
force-pushed
the
fix/save-fleet-to-node-config
branch
from
6bd5cb6 to
1e60102
Compare
|
@yenda is this good to go? |
yenda
force-pushed
the
fix/save-fleet-to-node-config
branch
from
1e60102 to
19f1374
Compare
Signed-off-by: yenda <[email protected]>
yenda
force-pushed
the
fix/save-fleet-to-node-config
branch
from
19f1374 to
3528dd8
Compare
Summary
Change of fleet in settings was not persisted to the node config. This should fix it though I am not sure how to test that, @cammellos and @oskarth you could try the PR and see if it solves your issue.
status: ready