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.

Sign up for GitHub

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

Jump to bottom

agama config load/store for "product" uses the HTTP API #1563

Merged
merged 14 commits into from
Sep 10, 2024
Merged

agama config load/store for "product" uses the HTTP API #1563

merged 14 commits into from
Sep 10, 2024

Conversation

mvidner
Copy link
Contributor

@mvidner mvidner commented Aug 30, 2024 •
edited
Loading

Problem

Product and registration part of the migration of the CLI from D-Bus API to HTTP API:

Solution

  • Added ProductHTTPClient
  • Kept (D-Bus) ProductClient because it serves as the backend for the above
  • Added ManagerHTTPClient, /api/manager/probe_sync is called after the product changes

Testing

  • Added a new unit test
  • Tested manually

Screenshots

No

@mvidner
Copy link
Contributor Author

mvidner commented Sep 4, 2024

#1272 switched /api/manager/probe to not wait for the D-Bus reply

@coveralls
Copy link

coveralls commented Sep 5, 2024 •
edited
Loading

Coverage Status

coverage: 71.833% (+0.05%) from 71.788%
when pulling 11c4c5e on http-client-product
into 9d30030 on master.

@mvidner mvidner marked this pull request as ready for review September 5, 2024 14:15
imobachgs
imobachgs requested changes Sep 10, 2024
View reviewed changes
rust/agama-lib/src/manager/http_client.rs Outdated
// BaseHTTPClient did not anticipate POST without request body
let dummy_body: Vec<u8> = vec![];
self.client
.post_void("/manager/probe_sync", &dummy_body)
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of a dummy Vec<u8>, you could use just the void type.

Suggested change
.post_void("/manager/probe_sync", &dummy_body)
.post_void("/manager/probe_sync", &())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting... I thought your solution would elegantly produce an empty body but instead the body has the 4 characters: null 🧐

Copy link
Contributor

Choose a reason for hiding this comment

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

I had not thought about that, but I guess it makes sense. Alternatively, we could make the argument an Option<T>. I do not know what is better.

rust/agama-server/src/manager/web.rs Outdated Show resolved Hide resolved
testing_using_container.sh Show resolved Hide resolved
rust/agama-lib/src/product/http_client.rs Outdated Show resolved Hide resolved
rust/agama-lib/src/product/http_client.rs Outdated Show resolved Hide resolved
rust/agama-lib/src/product/http_client.rs Outdated Show resolved Hide resolved
@mvidner
testing_using_container: set password and run shell even in case of f…
60ccd06 
…ailure

it is more useful that way
@mvidner
WITH_RUBY_DBUS=1, fix up for renamed gemspec
e604568
@mvidner
refactor(rust): crate::software::model::{RegistrationParams,Registrat…
9dfa734 
…ionInfo}

move them from web.rs for use by ProductHTTPClient
@mvidner
ProductHTTPClient
f7c683e
@mvidner
add and use ManagerHTTPClient
69fe745 
compared to the D-Bus ManagerClient,
it has only the probe method which is the only one used by CLI

BUT now config load with product and patterns SOMETIMES fails with no such
pattern... does the probe, which spawns a new thread in the web server, make a
race?
@mvidner
Fix openapi for /api/manager, it uses POST not GET
e8b7d9d
@mvidner
testing_using_container: link built binaries to /usr/bin
e02f52d
@mvidner
web server+client: (re)introduce /api/manager/probe_sync
6ba1873 
This fixes the following scenario:

```console
agama # cat rust/agama-lib/share/examples/profile_tw_gnome.json
{
  "software": {
    "patterns": [
      "gnome"
    ]
  }
}
agama # systemctl restart agama
agama # agama config load < rust/agama-lib/share/examples/profile_gnome.json
Anyhow(Backend call failed with status 400 and text '{"error":"Agama service error: Failed to find these patterns: [\"gnome\"]"}')
agama # systemctl restart agama
agama # PROBE_SYNC=1 agama config load < rust/agama-lib/share/examples/profile_gnome.json
agama #
```

The asynchronous probing, introduced in PR#1272 results in a race(?) and an
error when performing the second PUT:

```
Sep 03 11:20:06 2cf2b88a0524 agama-web-server[9357]: request: GET /api/software/config
Sep 03 11:20:06 2cf2b88a0524 agama-web-server[9357]: request: PUT /api/software/config
Sep 03 11:20:06 2cf2b88a0524 agama-web-server[9357]: request: POST /api/manager/probe
Sep 03 11:20:06 2cf2b88a0524 agama-web-server[9357]: request: PUT /api/software/config
```
@mvidner
test Productstore::load
d565036
@mvidner
test ProductStore::store
a20aed3
@mvidner
changelog
996b4ce
@mvidner
call /api/manager/probe_sync with null body
56335ba 
that is, "null".

The server will ignore it anyway and it is shorter in the client code compared
to the empty array.
@mvidner
call ProductHTTPClient::get_registration directly to save 1 HTTP call
0e5fcdb
@mvidner
ProductHTTPClient: better naming
11c4c5e
@mvidner mvidner merged commit ff3cf07 into master Sep 10, 2024
2 checks passed
@mvidner mvidner deleted the http-client-product branch September 10, 2024 14:59
@imobachgs imobachgs mentioned this pull request Sep 20, 2024
Merged
imobachgs added a commit that referenced this pull request Sep 20, 2024
@imobachgs
Update to release 10 (#1622)
574a1a5 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@imobachgs imobachgs imobachgs approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mvidner @coveralls @imobachgs