-
Notifications
You must be signed in to change notification settings - Fork 43
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
Conversation
58c6c98
to
cad0a14
Compare
aa0a4f1
to
b164047
Compare
There was a problem hiding this 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.
rust/agama-server/src/manager/web.rs
Outdated
Command::new("agama") | ||
.args(["logs", "store", "-d", path.as_str()]) | ||
.status() | ||
.expect("Cannot generate logs"); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
</Button> | ||
|
||
{ isLogDisplayed && | ||
<FileViewer |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
web/src/client/http.js
Outdated
* @param {string} url - Endpoint URL (e.g., "/l10n/config"). | ||
* @return {Promise<Response>} Server response. | ||
*/ | ||
async getRaw(url) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
ef1d4b1
to
796edb2
Compare
This reverts commit 0bd0fa1.
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
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.