Adapt network to the new config based HTTP API.#2840
Adapt network to the new config based HTTP API.#2840
Conversation
teclator
force-pushed
the
api-v2_network
branch
from
0c1e825 to
53ae796
Compare
teclator
force-pushed
the
api-v2_network
branch
from
5fc2b64 to
df72d6a
Compare
imobachgs
left a comment
imobachgs
left a comment
There was a problem hiding this comment.
Good job! But it will be a long review (due to the size of the changes). I have submitted a first round of comments.
rust/agama-manager/src/start.rs
Outdated
| #[error(transparent)] | ||
| Storage(#[from] storage::start::Error), | ||
| #[error(transparent)] | ||
| NetworkSystem(#[from] network::NetworkSystemError), |
There was a problem hiding this comment.
| NetworkSystem(#[from] network::NetworkSystemError), | |
| Network(#[from] network::NetworkSystemError), |
There was a problem hiding this comment.
Additionally, why are you using NetworkSystemError? Just mentioning because you already have a agama_network::error:Error.
There was a problem hiding this comment.
The idea was to move it to agama_network::error::Error instead of NetworkSystemError or NetworkAdapterError but probably defined it but didn't moved it yet.
| impl TryFrom<ConnectionCollection> for NetworkConnectionsCollection { | ||
| type Error = NetworkStateError; | ||
|
|
||
| fn try_from(collection: ConnectionCollection) -> Result<Self, Self::Error> { |
There was a problem hiding this comment.
Can this function really fail?
There was a problem hiding this comment.
The same mentioned above, it should be a From instead of TryFrom but inherited from what we had for the Connection.
| if let Some(bridge) = &net_conn.bridge { | ||
| ports = bridge.ports.clone(); | ||
| } | ||
| if let Some(bond) = &net_conn.bond { |
There was a problem hiding this comment.
I see logic like this one in several places. I wonder if we could have some higher level API for that).
rust/agama-network/src/model.rs
Outdated
| } | ||
|
|
||
| for (port, uuid) in controller_ports { | ||
| let default = Connection::new(port.clone(), DeviceType::Ethernet); |
There was a problem hiding this comment.
Do not get me wrong, this is fine, but you are creating a struct that you might not use at the end. What about something like:
let mut conn = conns
.iter()
.find(|c| c.id == port || c.interface.as_ref() == Some(&port))
.cloned()
.unwrap_or_else(|| Connection::new(port, DeviceType::Ethernet));
rust/agama-network/src/model.rs
Outdated
| } | ||
| } | ||
|
|
||
| impl TryFrom<NetworkState> for NetworkConnectionsCollection { |
There was a problem hiding this comment.
I have the impression that we are abusing TryFrom. This or the implementation of TryFrom<NetworkState> for SystemInfo are some examples.
rust/agama-network/src/model.rs
Outdated
| impl TryFrom<NetworkState> for NetworkConnectionsCollection { | ||
| type Error = NetworkStateError; | ||
|
|
||
| fn try_from(state: NetworkState) -> Result<Self, Self::Error> { |
There was a problem hiding this comment.
The same comments I did for TryFrom<ConnectionCollection> for NetworkConnectionsCollection still apply.
## Problem There is a new config based HTTP API and all the services needs to be adapted. The aim of this PR is to adapt the network backend service (replace #2840). - #2715 ## Solution In the new architecture all the Agama modules will live in its own package implementing the actors/service as described in (#2715), but as network was already using a actors model using an enums approach it was decided to use the current service in the first iteration of changes. ### Changes done - Added the actions for getting the system, proposal and current network configuration using the actual actions passing messages system based on enums. - All the structs or types that are related to the API (needs to be serialized) has been moved to agama-utils, it was decided to not depend on agama-lib anymore. The types will be re-exported to be used by agama-network when needed. - As the current merge is concatenating the connections array, the patch **(updateConfig)** will replace the connections with the given one when present, but is something we need to revisit and decide how to do it in a better way. - Removed the network web service. - As the proposal, system info and config contains similar collections and just change the absence or presence of some of them we have introduced the **NetworkCollection** and **NetworkConnectionCollections** in order to do the conversion between them. ### TODO - Move to the actors/service model using traits instead of enums and adding the issues and progress monitor to the network service. - Remove all the actions which are not needed anymore (old network web service). - Add unit tests covering the new approach.
teclator
force-pushed
the
api-v2_network
branch
from
599e9b2 to
f8abcc7
Compare
|
As commented this PR has been replaced by #2862 and a following PR for adapting the UI. |
3 participants
Problem
There is a new config based HTTP API and all the services needs to be adapted. The aim of this PR is to adapt network.
Solution