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

Added manager route for downloading logs #1216

Merged
merged 8 commits into from
May 16, 2024
Merged

Added manager route for downloading logs #1216

merged 8 commits into from
May 16, 2024

Conversation

teclator
Copy link
Contributor

@teclator teclator commented May 15, 2024

Problem

In the Cockpit times, Agama allowed to download the logs using the web UI. However, after the switch to the HTTP/JSON API, that’s not the case anymore. Agama relied on Cockpit’s file API which, for obvious reasons, is not available anymore.

Therefore the option to download the logs using our agama-web-server should be implemented bringing the functionality back.

Solution

The agama-web-server has been adapted adding the /logs route in the manager HTTP/JSON API. The Web UI has been also adapted bringing back the fetchLogs() method.

Last but not least the options for showing the logs and the terminal have been removed as them were also relying on Cockpit and therefore are currently broken.

@coveralls
Copy link

coveralls commented May 15, 2024

Coverage Status

coverage: 69.846% (-0.04%) from 69.889%
when pulling 192a7a3 on download_logs
into 5e6c665 on master.

@teclator teclator force-pushed the download_logs branch 3 times, most recently from 58c6c98 to cad0a14 Compare May 16, 2024 10:13
@teclator teclator marked this pull request as ready for review May 16, 2024 10:22
@teclator teclator force-pushed the download_logs branch 2 times, most recently from aa0a4f1 to b164047 Compare May 16, 2024 10:30
Copy link
Member

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

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

It looks quite good. However, I have a few notes.

Command::new("agama")
.args(["logs", "store", "-d", path.as_str()])
.status()
.expect("Cannot generate logs");
Copy link
Member

Choose a reason for hiding this comment

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

I think that if the call fails the client will not get any response. Or am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, you are right, added during development and did not replaced by a map_err

rust/package/agama.changes Show resolved Hide resolved
web/package/agama-web-ui.changes Outdated Show resolved Hide resolved
</Button>

{ isLogDisplayed &&
<FileViewer
Copy link
Member

Choose a reason for hiding this comment

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

I did not meant to drop the buttons completely (just removing the from the menu). But I am not against dropping that code. In that case, you should remove FileViewer too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We were also not sure, that is why a I put it in a separate commit just to revert in case.. :) Will remove it only from the menu by now as it does not hurt and probably we will bring them back soon.

* @param {string} url - Endpoint URL (e.g., "/l10n/config").
* @return {Promise<Response>} Server response.
*/
async getRaw(url) {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of extending the API (which is expected to be used only here), you could just use fetch. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Or if you want to keep this method (I am not against), I would rename it as "getFile". It is only used in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact originally it was download, and we (@dgdavid and me) were evaluating all the options you mentioned even adding an option for specifying the content type as a parameter for the get method but didn't liked any of them at all.

By now it is only used in this case so, just using fetch should be enough.

@teclator teclator force-pushed the download_logs branch 2 times, most recently from ef1d4b1 to 796edb2 Compare May 16, 2024 14:46
@teclator teclator merged commit 0a10e9a into master May 16, 2024
4 checks passed
@teclator teclator deleted the download_logs branch May 16, 2024 15:00
@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.

None yet

3 participants