-
Notifications
You must be signed in to change notification settings - Fork 69
Add --api URL support to agama profile
#2103
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
Conversation
|
First commit, client code is missing, server implements only the validation part. # save auth token
agama auth login
# try what surely works
curl -k -v -H "Authorization: Bearer $(cat $HOME/.local/agama/token)" https://localhost/api/l10n/config
# try out new code
curl -k -H "Authorization: Bearer $(cat $HOME/.local/agama/token)" 'https://localhost/api/profile/validate?path=/etc/os-release' |
Pull Request Test Coverage Report for Build 13897313808Details
💛 - Coveralls |
574cb8d to
9effe1e
Compare
da98de6 to
d065983
Compare
3874138 to
d530a71
Compare
5f82310 to
df9a2d0
Compare
include instance_path in message, per crate README
> To print causes as well using anyhow’s default formatting of causes, use the alternate selector “{:#}”.
Before:
> Anyhow(Backend call failed with status 400 and text '{"error":"Error: Could not evaluate the profile"}')
After:
> Anyhow(Backend call failed with status 400 and text '{"error":"Error: Could not evaluate the profile: Failed to read system's hardware information: Failed to read agama.libsonnet: No such file or directory (os error 2)"}')
fetches XML or ERB... returns JSON
in preparation for unified passing of input via request body OR ?path= OR ?url= but the CLI client does not take advantage of it yet agama profile import will take advantage of it and it will stay client only, no /api/profile/import
agama profile import file://$(pwd)/rust/agama-lib/share/examples/profile_tw.json
Error reading from file {localization: ...}
-agama profile evaluate PATH
+agama profile evaluate URL_OR_PATH
-agama profile validate PATH +agama profile validate URL_OR_PATH this also makes `agama profile autoyast URL` work again ... all of this is so far tested without the `--api URL` option
restore script execution in agama profile import, via POST /api/profile/execute_script
the web service has a different working directory so the client makes the path absolute, but the user does not need to
…ackend so that filenames with spaces written as " " or "%20" all work
df9a2d0 to
82eb63c
Compare
partial cherry pick from master 3415a88
imobachgs
left a comment
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 have reviewed the server part now and we are almost there. IMHO the blocking issues are:
- As you mentioned, the
execute_scriptblocks the whole server. - The progress reporting will not work anymore when using the CLI from a remote system (as it relied on D-Bus).
If you want, we could work on the second problem on a separate PR because it might imply some work.
| Ok(router) | ||
| } | ||
|
|
||
| /// For flexibility, the profile operations take the input as either of: |
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 is fine, but I don't know whether we need this flexibility. In a POST call, I would go with the request body.
| match importer_res { | ||
| Ok(importer) => Ok(importer.content), | ||
| Err(error) => { | ||
| // anyhow can be only displayed, not so nice |
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.
And that's precisely why I tend to avoid anyhow. Do not get me wrong: anyhow is great. However, I would prefer plain errors for libraries and internal APIs where you might need to do some matching.
|
JFTR, it could be a good idea to offer a way to upload a local profile from the CLI: But, of course, that's totally out of scope for this PR. |
`cargo doc` warned about that, and it turns out we don't need it also explain why some of the related tests are disabled
instead of reexporting Url from agama-lib also remove leftover axum::debug_handler macro usage
/usr/bin/busctl is not our program, don't rewrite its path
…tion wait only 50ms for a quick failure test for both
also improve by responding that something is not user's fault, via 500 internal errors
ece48fb to
3148fca
Compare
Progress monitoring is done with D-Bus and when using a remote Agama HTTP API, connecting to local D-Bus would crash the monitoring thread, continuing to work but ugly. Now it prints an error message, still not nice but better. Need to use something else for the monitoring.
Before: > ❯ agama download http:whatever /var > Anyhow(Cannot write the file '/var' > > Caused by: > Is a directory (os error 21)) After: > ❯ agama download http:whatever /var > Error: Cannot write the file '/var': Is a directory (os error 21)
rust/agama-server/src/profile/web.rs
Outdated
| .spawn() | ||
| .context(format!("Spawning script {:?}", path))?; | ||
|
|
||
| // Do not child.wait() for the script to finish, |
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.
As discussed in IRC, I am not sure about this behavior. I would expect to have always the same answer, no matter whether the call takes 50ms or 60ms. I would go for:
- Waiting until it finishes but without blocking the server. That's why I mentioned using
tokio::process::Commandinstead (but I have not tried if it works as expected). We could even move the script to a separate Tokio task. - Or always returning a 200 (if the script was started) and move the script execution to a separate Tokio task.
We even discussed about having an API to run jobs. You would create a job by calling POST /jobs, which would return a job ID. So you can check the current status with GET /jobs/1. Of course, we could integrate this API with the events system too. But I would say it is out of scope.
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.
Yay, tokio::process::Command simply works: its Child::wait is async so other requests are served
…hers It turns out that tokio::process::Command is an easy replacement of std::process::Command, only async. Yay!
## Problem The `agama-auto` service does not work after introducing some changes to make it possible to use the `agama profile` command from an remote system (see #2103). The call to the `POST /api/profile/autoyast` endpoint freezes the whole server. ## Solution * Make the code that takes care of importing the AutoYaST profile async (`AutoyastImporter::read`). * Temporarily disable AutoYaST fetch errors because now they are not properly handled in the `OEMDRV` feature (it does not pass the `YAST_SKIP_PROFILE_FETCH_ERROR` env-var). Consider this a temporary workaround to have a working development image again.
Prepare to release Agama 14: * #1994 * #2041 * #2103 * #2178 * #2189 * #2200 * #2205 * #2209 * #2212 * #2213 * #2214 * #2215 * #2216 * #2217 * #2219 * #2220 * #2224 * #2225 * #2226 * #2227 * #2228 * #2230 * #2231 * #2232 * #2233 * #2235 * #2237 * #2239 * #2241 * #2242 * #2244 * #2245 * #2246 * #2247 * #2248 * #2249 * #2250 * #2251 * #2252 * #2253 * #2254 * #2255 * #2256 * #2257 * #2259 * #2260 * #2262 * #2265 * #2266 * #2268 * #2269 * #2271 * #2272 * #2273 * #2275 * #2276 * #2278 * #2281
Problem
agama profilestill executes its actions directly from the CLI client, not using the web service. Fix it to make it work likeagama --api $URL config ...already does.agama profilecommand summary... it is a diverse bunch! (related, unplanned stub: https://trello.com/c/3aUupc2F/461-unify-config-and-profile-commands-in-the-cli )Solution
Added these 3 new web API paths corresponding 1-1 to the CLI commands:
But
agama profile importis notable that it effectively includes anagama config loadwhich is implemented as multiple backend calls, soimportstays at the front end and the back end only gets what's needed to make it work,which is running a script:
(TODO explain the rest)
Backend errors now include their causes
Before:
After:
JSON validation errors are more readable now
We used to include a debug representation of the validation error but reading the manual(!) shows there is a way to just point to the offending piece of JSON
Before:
After:
Testing
Unit tests: no
Manual tests
...
Added integration tests expecting a running web service.
Run like (part of .github/workflows/ci-integration-tests.yml)
Screenshots
(textual screenshots coming up)
Documentation
Documented the extended argument handling:
agama profilereference updated for the--api URLrelated changes agama-project.github.io#70But we are actually still missing documentation for the
--apioption itself, except for a mention in the blog. IMHO we should add it as part of theconfig/profilecleanup.