-
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
Replace D-Bus with HTTP-based clients #1438
Conversation
c49b025
to
5d0396e
Compare
One odd thing, OpenAPI seems to omit the |
@jreidinger do you want to have a quick look at this stage? |
Argh, my flawless tests fail CI with
it looks like I mock the call changed in this PR but the auth call still goes to the live service? |
to enable - connecting to alternative servers, either production or test mocks - unauthenticated calls, either for the initial auth or test mocks This is needed for both #1438 and #1495 In the end, the API changes only by adding a `Default` trait implementation which does no authentication. Connecting to a mock server is achieved by assigning to the public `base_url` field.
enable debug prints in BaseHTTPClient
Problem: When there is an issue ("trying to use reserved username") it is reported as > Anyhow(Backend call failed with status 422 and text '["There is a conflict between the entered\nusername and an existing username.\nTry another one."]') (which is a wrapped `ServiceError::BackendError`) even though the CLI is ready to handle a `ServiceError::WrongUser` Solution: let BaseHTTPClient::put return a deserialized response but this commit still does not parse the JSON and does not convert the Err
put_void (a PUT that ignores the reponse body)
- remove debug prints - document `request_response` and use it - document `put_void` (which ignores response body, whereas `put` deserializes it)
so that the CLI client can use it too
as suggested by Josef
failure expected, no mocking yet
httpmock - It is what I found with DDG: rust mock http request No idea about alternatives, this one fits well my simple use case, but does bring a load of dependencies (how much?)
if !success { | ||
return Err(ServiceError::WrongUser(issues)); | ||
} | ||
self.users_client.set_first_user(&first_user).await?; |
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.
why is suddenly possible to ignore failed 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.
The D-Bus client set_first_user
returns a tuple (success,issues)
in band, as zbus::Result<(bool, Vec<String>)>
https://github.com/openSUSE/agama/blob/b9a73b57fc71f6eb47b3f98154ec61a31f4d57d9/rust/agama-lib/src/users/client.rs#L84
But the HTTP client set_first_user
returns a Result<(), ...>
where all the errors are moved to the Err
alternative, and the WrongUser
is handled on that lower level:
https://github.com/openSUSE/agama/blob/3dca6b5ed6a30389b0023aea9a1ea031934a5859/rust/agama-lib/src/users/http_client.rs#L26
(and it is the ?
in this line that propagates the Err
)
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.
Good job! In general, the PR looks good, although there are a few comments.
Checking the BaseHTTPClient
client again and how it is used, I am still not convinced about those "raw" methods (get_response
, post_response
, etc.). I am not even sure if we will need them. Additionally, they leak the Response
from reqwest
and I am not sure whether I like that.
Perhaps it is out of the scope of this PR. Let's see if we need them at some point.
/// Set the configuration for the first user | ||
pub async fn set_first_user(&self, first_user: &FirstUser) -> Result<(), ServiceError> { | ||
let result = self.client.put_void("/users/first", first_user).await; | ||
if let Err(ServiceError::BackendError(422, ref issues_s)) = result { |
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.
np: perhaps we could use the http::status::StatusCode from the HTTP crate.
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.
Yup, but then I can't use if let
like this... that's fine, a match
will look good too
@imobachgs thanks for the review, yes I left the
|
Unused by default but small enough to be included anyway. Useful to trace what the httpmock server actually receives in tests: When a test misbehaves, turn on logging for it: ```rs env_logger::init(); ``` and also set the log level to see the requests and responses: ```console $ (cd rust; RUST_LOG=httpmock=debug cargo test) ```
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 better now, although I am not convinced on the use case of those _void
functions.
@@ -142,9 +143,11 @@ mod test { | |||
}; | |||
let result = store.store(&settings).await; | |||
|
|||
// main assertion |
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.
If we are not using result
for anything else, what about just adding the question mark at the end of the previous line?
store.store(&settings).await?;
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.
The resulting behavior is of course identical.
Writing it in this way means to draw your attention to the main assertion of the test.
Actually I was bothered by the test code being obscure that I even looked up Rust RSpec in the hope to write tests that better express the intent. Well, BDD.
Apart from the constructor, only these are left in the public API: ``` get delete_void post post_void put put_void patch patch_void ``` The fns with `_void` ignore the body of a successful response and return `Ok(())`. The other fns expect a JSON body and deserialize it, returning `Ok(T)`. Unsuccessful responses return `Err(ServiceError::BackendError(status_code, body_as_string))` Other fns were deleted or made private. `*_response`: replaced by a private helper `method_response` FIXME: adjust the doc comments once we agree this API makes sense
Previously I was running ```rs let http_mock = ...; let result = object.method_under_test(); http_mock.assert(); assert!(result.is_ok()); ``` But when something unexpectedly fails, the `result` will be `Err` and we want to see it. ```rs let http_mock = ...; let result = object.method_under_test(); // main assertion FIRST, and if it fails we get to see the `Err` result?; // only then make sure that the mock server got calls as expected http_mock.assert(); ```
cargo clippy --no-deps
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 good to me but, please, add a changes entry.
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.
LGTM. Thanks!
About the *_void
methods, we agreed to review our HTTP API to be more consistent and, eventually, we could get rid of them. But it is out of the scope of this PR.
## Problem Problem description is in issue #1539 ## Solution Implement sd_notify for agama-web-service to notify systemd when web service is ready to serve requests. Improve systemd dependencies to better reflect dependencies. For agama-auto adapt dependencies to reflect that CLI switch from direct systemd communication to HTTP API. related PR is #1438 ## Testing - *Tested manually* ## Screenshots snippet from log before change ( truncated, but to demonstrate that agama-auto runs too soon): ``` Aug 14 13:10:47 dhcp118.suse.cz systemd[1]: Started Agama Installer Service. Aug 14 13:10:47 dhcp118.suse.cz systemd[1]: Started Agama Web Server. Aug 14 13:10:47 dhcp118.suse.cz systemd[1]: Started Agama automatic profile runner. Aug 14 13:10:47 dhcp118.suse.cz systemd[1]: Starting Postpone login prompt after the SSL fingerprint issue is generated... Aug 14 13:10:47 dhcp118.suse.cz agama-auto[2689]: + export YAST_SKIP_XML_VALIDATION=1 Aug 14 13:10:47 dhcp118.suse.cz agama-auto[2689]: + YAST_SKIP_XML_VALIDATION=1 Aug 14 13:10:47 dhcp118.suse.cz agama-auto[2689]: + '[' -z '' ']' Aug 14 13:10:47 dhcp118.suse.cz agama-auto[2691]: ++ awk -F agama.auto= '{sub(/ .*$/, "", $2); print $2}' Aug 14 13:10:47 dhcp118.suse.cz agama-auto[2689]: + url= Aug 14 13:10:47 dhcp118.suse.cz agama-auto[2689]: + '[' -z '' ']' Aug 14 13:10:47 dhcp118.suse.cz agama-auto[2689]: + echo 'no autoinstallation profile' Aug 14 13:10:47 dhcp118.suse.cz agama-auto[2689]: no autoinstallation profile Aug 14 13:10:47 dhcp118.suse.cz agama-auto[2689]: + exit 0 Aug 14 13:10:47 dhcp118.suse.cz systemd[1]: agama-auto.service: Deactivated successfully. Aug 14 13:10:47 dhcp118.suse.cz sshd-gen-keys-start[2683]: ssh-keygen: generating new host keys: RSA ECDSA ED25519 Aug 14 13:10:47 dhcp118.suse.cz sshd[2702]: Server listening on 0.0.0.0 port 22. Aug 14 13:10:47 dhcp118.suse.cz sshd[2702]: Server listening on :: port 22. Aug 14 13:10:47 dhcp118.suse.cz systemd[1]: Started OpenSSH Daemon. Aug 14 13:10:47 dhcp118.suse.cz systemd[1]: Starting Generate issue file for SSH host keys... Aug 14 13:10:47 dhcp118.suse.cz systemd[1]: agama-ssh-issue.service: Deactivated successfully. Aug 14 13:10:47 dhcp118.suse.cz systemd[1]: Finished Generate issue file for SSH host keys. Aug 14 13:10:47 dhcp118.suse.cz agama-web-server[2688]: registering event source with poller: token=Token(94923250365696), interests=READABLE | WRITABLE Aug 14 13:10:47 dhcp118.suse.cz agama-web-server[2688]: Initializing Aug 14 13:10:47 dhcp118.suse.cz agama-web-server[2688]: Waiting for DATA or OK from server Aug 14 13:10:47 dhcp118.suse.cz agama-web-server[2688]: Received OK from server Aug 14 13:10:47 dhcp118.suse.cz agama-web-server[2688]: Waiting for Unix FD passing agreement from server Aug 14 13:10:47 dhcp118.suse.cz agama-web-server[2688]: Unix FD passing agreed by server ``` and now after change: ``` Aug 15 10:19:15 dhcp118.suse.cz systemd[1]: Started Agama Web Server. Aug 15 10:19:15 dhcp118.suse.cz agama-web-server[12467]: Starting Agama web server at :::80 Aug 15 10:19:15 dhcp118.suse.cz agama-web-server[12467]: Starting Agama web server at :::443 ... Aug 15 10:19:15 dhcp118.suse.cz agama-web-server[12467]: Waiting for message on the socket.. ... Aug 15 10:19:15 dhcp118.suse.cz agama-web-server[12467]: Listening for property changes on org.freedesktop.NetworkManager.Device... Aug 15 10:19:15 dhcp118.suse.cz systemd[1]: Started Agama automatic profile runner. Aug 15 10:19:15 dhcp118.suse.cz agama-web-server[12467]: Message received on the socket: Msg { type: Signal, sender: UniqueName(Str(Borrowed(":1.2"))), path: ObjectPath("/org/freedesktop/systemd1/unit/agama_2dauto_2eservice"), iface: InterfaceName(Str(Borrowed("org.freedesktop.DBus.Properties"))), member: MemberName(Str(Borrowed("PropertiesChanged"))), body: Signature("sa{sv}as") } Aug 15 10:19:15 dhcp118.suse.cz agama-auto[12479]: + export YAST_SKIP_XML_VALIDATION=1 Aug 15 10:19:15 dhcp118.suse.cz agama-auto[12479]: + YAST_SKIP_XML_VALIDATION=1 Aug 15 10:19:15 dhcp118.suse.cz agama-auto[12479]: + '[' -z '' ']' ... Aug 15 10:19:15 dhcp118.suse.cz agama-web-server[12467]: Waiting for message on the socket.. Aug 15 10:19:15 dhcp118.suse.cz agama-auto[12482]: ++ awk -F agama.auto= '{sub(/ .*$/, "", $2); print $2}' Aug 15 10:19:15 dhcp118.suse.cz agama-auto[12479]: + url= Aug 15 10:19:15 dhcp118.suse.cz agama-auto[12479]: + '[' -z '' ']' Aug 15 10:19:15 dhcp118.suse.cz agama-auto[12479]: + echo 'no autoinstallation profile' Aug 15 10:19:15 dhcp118.suse.cz agama-auto[12479]: no autoinstallation profile Aug 15 10:19:15 dhcp118.suse.cz agama-auto[12479]: + exit 0 Aug 15 10:19:15 dhcp118.suse.cz systemd[1]: agama-auto.service: Deactivated successfully. ```
Prepare for releasing Agama 10· * #1263 * #1330 * #1407 * #1408 * #1410 * #1411 * #1412 * #1416 * #1417 * #1419 * #1420 * #1421 * #1422 * #1423 * #1424 * #1425 * #1428 * #1429 * #1430 * #1431 * #1432 * #1433 * #1436 * #1437 * #1438 * #1439 * #1440 * #1441 * #1443 * #1444 * #1445 * #1449 * #1450 * #1451 * #1452 * #1453 * #1454 * #1455 * #1456 * #1457 * #1459 * #1460 * #1462 * #1464 * #1465 * #1466 * #1467 * #1468 * #1469 * #1470 * #1471 * #1472 * #1473 * #1475 * #1476 * #1477 * #1478 * #1479 * #1480 * #1481 * #1482 * #1483 * #1484 * #1485 * #1486 * #1487 * #1488 * #1489 * #1491 * #1492 * #1493 * #1494 * #1496 * #1497 * #1498 * #1499 * #1500 * #1501 * #1502 * #1503 * #1504 * #1505 * #1506 * #1507 * #1508 * #1510 * #1511 * #1512 * #1513 * #1514 * #1515 * #1516 * #1517 * #1518 * #1519 * #1520 * #1522 * #1523 * #1524 * #1525 * #1526 * #1527 * #1528 * #1529 * #1530 * #1531 * #1532 * #1533 * #1534 * #1535 * #1536 * #1537 * #1540 * #1541 * #1543 * #1544 * #1545 * #1546 * #1547 * #1548 * #1549 * #1550 * #1552 * #1553 * #1554 * #1555 * #1556 * #1557 * #1558 * #1559 * #1560 * #1562 * #1563 * #1565 * #1566 * #1567 * #1568 * #1569 * #1570 * #1571 * #1572 * #1573 * #1574 * #1575 * #1576 * #1577 * #1578 * #1579 * #1580 * #1581 * #1583 * #1584 * #1585 * #1586 * #1587 * #1588 * #1589 * #1590 * #1591 * #1592 * #1593 * #1596 * #1597 * #1598 * #1600 * #1602 * #1605 * #1606 * #1607 * #1608 * #1610 * #1611 * #1612 * #1613 * #1614 * #1619 * #1620 * #1621
Problem
When moving to the new HTTP-based architecture, we took some shortcuts. One of them was not using HTTP clients in the command-line interface. We need to adapt the following clients:
Solution
UsersClient
- still needed, because the HTTP service uses it to talk to the Ruby backend.UsersHTTPClient
added.LocalizationClient
replaced withLocalizationHTTPClient
The rest will be done in subsequent PRs.
Testing
agama config show
andagama config load
. This is ripe for automation, even in this PR.Screenshots
If the fix affects the UI attach some screenshots here.