Skip to content
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

Adapt the web UI to use the Agama network service #1006

Merged
merged 42 commits into from
Jan 30, 2024
Merged

Conversation

imobachgs
Copy link
Contributor

@imobachgs imobachgs commented Jan 18, 2024

Problem

The web UI still talks directly to NetworkManager to set up network connections.

Solution

This is the first set of changes aimed to stop using the NetworkManager service from the web UI. Instead, it should talk to Agama's network service (which relies on NetworkManager itself).

This pull request adapts the web UI to use Agama's network service to write the changes (including connecting to wireless devices). However, the status information is still retrieved from NetworkManager because those bits are missing from our API.

During the process, we needed to introduce a few changes to Agama's network service:

  • Use UUIDs to identify connections instead of the Id (the name).
  • Write only changed network connections.
  • Rollback to the previous state if some connection cannot be updated.
  • Better error handling at D-Bus level.

Future

  • Improve (almost non-existent) error handling.
  • Retrieve the status information from Agama's network service.

Testing

  • Adapted unit tests
  • Tested manually

imobachgs and others added 28 commits December 29, 2023 13:59
* At this point, it just wraps a NetworkManagerAdapter.
* We need to make sure that all changes happens before calling Apply.
* We cannot wait for the usual proxy assignment (e.g., proxy.Property = 'Value').
@coveralls
Copy link

coveralls commented Jan 26, 2024

Coverage Status

coverage: 74.509% (-0.2%) from 74.667%
when pulling 878ca8f on use-agama-network
into 1968c26 on master.

@imobachgs imobachgs marked this pull request as ready for review January 29, 2024 14:41
Copy link
Contributor

@joseivanlopez joseivanlopez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Only minor comments.

* @param {Connection} connection - Connection to add
* @return {Promise<Connection>} the added connection
*/
async addConnection(connection) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NP: What do think about renaming this method to AddOrUpdateConnection ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about that, but it is weird having an addOrUpdateConnection and updateConnection. Perhaps we might need to merge them and have something like saveConnection only. I will think about that.

web/src/client/network/index.js Show resolved Hide resolved
web/src/client/network/model.js Show resolved Hide resolved
Copy link
Contributor

@joseivanlopez joseivanlopez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@imobachgs imobachgs merged commit c208a57 into master Jan 30, 2024
5 checks passed
@imobachgs imobachgs deleted the use-agama-network branch January 30, 2024 11:00
@imobachgs imobachgs mentioned this pull request Feb 12, 2024
@imobachgs imobachgs mentioned this pull request May 17, 2024
imobachgs added a commit that referenced this pull request May 17, 2024
Prepare for releasing Agama 8. It includes the following pull requests:

* #884
* #886
* #914
* #918
* #956
* #957
* #958
* #959
* #960
* #961
* #962
* #963
* #964
* #965
* #966
* #969
* #970
* #976
* #977
* #978
* #979
* #980
* #981
* #983
* #984
* #985
* #986
* #988
* #991
* #992
* #995
* #996
* #997
* #999
* #1003
* #1004
* #1006
* #1007
* #1008
* #1009
* #1010
* #1011
* #1012
* #1014
* #1015
* #1016
* #1017
* #1020
* #1022
* #1023
* #1024
* #1025
* #1027
* #1028
* #1029
* #1030
* #1031
* #1032
* #1033
* #1034
* #1035
* #1036
* #1038
* #1039
* #1041
* #1042
* #1043
* #1045
* #1046
* #1047
* #1048
* #1052
* #1054
* #1056
* #1057
* #1060
* #1061
* #1062
* #1063
* #1064
* #1066
* #1067
* #1068
* #1069
* #1071
* #1072
* #1073
* #1074
* #1075
* #1079
* #1080
* #1081
* #1082
* #1085
* #1086
* #1087
* #1088
* #1089
* #1090
* #1091
* #1092
* #1093
* #1094
* #1095
* #1096
* #1097
* #1098
* #1099
* #1100
* #1102
* #1103
* #1104
* #1105
* #1106
* #1109
* #1110
* #1111
* #1112
* #1114
* #1116
* #1117
* #1118
* #1119
* #1120
* #1121
* #1122
* #1123
* #1125
* #1126
* #1127
* #1128
* #1129
* #1130
* #1131
* #1132
* #1133
* #1134
* #1135
* #1136
* #1138
* #1139
* #1140
* #1141
* #1142
* #1143
* #1144
* #1145
* #1146
* #1147
* #1148
* #1149
* #1151
* #1152
* #1153
* #1154
* #1155
* #1156
* #1157
* #1158
* #1160
* #1161
* #1162
* #1163
* #1164
* #1165
* #1166
* #1167
* #1168
* #1169
* #1170
* #1171
* #1172
* #1173
* #1174
* #1175
* #1177
* #1178
* #1180
* #1181
* #1182
* #1183
* #1184
* #1185
* #1187
* #1188
* #1189
* #1190
* #1191
* #1192
* #1193
* #1194
* #1195
* #1196
* #1198
* #1199
* #1200
* #1201
* #1203
* #1204
* #1205
* #1206
* #1207
* #1208
* #1209
* #1210
* #1211
* #1212
* #1213
* #1214
* #1215
* #1216
* #1217
* #1219
* #1220
* #1221
* #1222
* #1223
* #1224
* #1225
* #1226
* #1227
* #1229
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants