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

Expose the Progress interface through the HTTP/JSON API #1092

Merged
merged 7 commits into from
Mar 15, 2024

Conversation

imobachgs
Copy link
Contributor

@imobachgs imobachgs commented Mar 13, 2024

Trello: https://trello.com/c/2RvcZBFR/3564-5-expose-the-progress-api-over-http

This PR exposes the Progress API for the Manager and Software services through the HTTP/JSON interface. It is implemented as a pair of functions that can be reused in all services that want to implement such an interface.

A /SERVICE/progress endpoint

The progress_router function allows to add a /SERVICE/progress route that exposes the current progress:

{
  "current_step": 4,
  "max_steps": 4,
  "current_title": "Calculating the software proposal",
  "finished": false
}

The events stream

The progress_stream builds an events stream that emits a new event when the current_step changes:

{
  "type": "Progress",
  "service": "org.opensuse.Agama.Software1",
  "current_step": 4,
  "max_steps": 4,
  "current_title": "Calculating the software proposal",
  "finished": false
}

Enabling ServiceStatus and Progress for the Software service

Additionally, the PR enables the ServiceStatus (implemented #1089) and the Progress for the Software service too.

@imobachgs imobachgs changed the title Http progress api Expose the Progress interface through the HTTP/JSON API Mar 13, 2024
@coveralls
Copy link

coveralls commented Mar 13, 2024

Coverage Status

coverage: 74.351% (-0.05%) from 74.398%
when pulling 859617b on http-progress-api
into 3a6ce33 on architecture_2024.

Base automatically changed from http-manager-srv to architecture_2024 March 13, 2024 17:16
"org.opensuse.Agama.Software1",
"/org/opensuse/Agama/Software1",
)
.await,
Copy link
Contributor

Choose a reason for hiding this comment

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

well, tokio explicitelly mention to not do multiple merge. See documentation at https://dtantsur.github.io/rust-openstack/tokio/stream/trait.StreamExt.html#method.merge
They suggest to use StreamMap for such case https://docs.rs/tokio-stream/latest/tokio_stream/struct.StreamMap.html

Copy link
Contributor Author

@imobachgs imobachgs Mar 13, 2024

Choose a reason for hiding this comment

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

Yes, I want to search for an alternative. Thanks for the hint, I skipped that part of the docs 😉. The StreamMap looks good enough. I was also checking the StreamExt of the futures crate.


async fn progress(State(state): State<ProgressState<'_>>) -> Json<Progress> {
let proxy = state.proxy;
let progress = Progress::from_proxy(&proxy).await.unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

so it should crash when there is issue with dbus? I think that having some error status would be nicer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, sure, it is just a draft. There are a couple of unwraps here and there.

) -> Result<ProgressProxy<'a>, zbus::Error> {
let proxy = ProgressProxy::builder(&dbus)
.destination(destination.to_string())?
.path(path.to_string())?
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if there is advantage of using &str when you inside always needs to create new string. If you have String in method call already, then it can be passed or created outside and just passed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, using &str in the boundaries looks more flexible to me. And, after all, from outside you do not need to know what is going to happen inside. But yes, I could reconsider it.

ServiceStatusChanged {
service: String,
status: u32,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

I expect that they usually come together that Busy and ServiceStatus change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are different things: one is the list of busy services, which is only kept by the Manager. The ServiceStatusChanged belongs to each separate service. However, I am thinking that perhaps we are not interested in the BusyServicesChanged event from the web UI. I will check.

@imobachgs imobachgs force-pushed the http-progress-api branch 2 times, most recently from 9a7046e to e234b29 Compare March 14, 2024 16:53
@imobachgs imobachgs marked this pull request as ready for review March 14, 2024 16:54
Copy link
Contributor

@jreidinger jreidinger 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 64893e6 into architecture_2024 Mar 15, 2024
2 checks passed
@imobachgs imobachgs deleted the http-progress-api branch March 15, 2024 07:03
imobachgs added a commit that referenced this pull request Mar 19, 2024
Adapt the `WithProgress` to the HTTP-based implementation of the D-Bus
progress interface (#1092).
imobachgs added a commit that referenced this pull request May 6, 2024
After a few months of work, it is time to merge the `architecture_2024`
branch into `master`. It is still a work-in-progress, but all the
efforts should be go now against that branch.

## Pull requests

* #1061
* #1064
* #1073
* #1074
* #1080
* #1089
* #1091
* #1092
* #1094
* #1095
* #1099
* #1100
* #1102
* #1103
* #1112
* #1114
* #1116
* #1117
* #1119
* #1120
* #1123
* #1126
* #1129
* #1130
* #1131
* #1132
* #1133
* #1134
* #1136
* #1139
* #1140
* #1143
* #1146

## Other commits

* 8efa41f
* 9e2dec0
@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.

3 participants