-
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
refactor(web): bring back zFCP support (new HTTP / JSON API, queries, and TypeScript) #1570
Conversation
web/src/queries/zfcp.ts
Outdated
* Returns a query for retrieving the zFCP controllers | ||
*/ | ||
const zfcpControllersQuery = () => ({ | ||
const zfcpControllersQuery = { |
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.
BTW, maybe I miss something at some point but the only suggestion I see is about the naming: #1570 (review)
But not about the full structure. I've no strong opinion about writing them as function or object. But I though we were writing them always as function for consistence since they could receive args for building the query. As far as I can see, @imobachgs has wrote some as function and some as object in the storage part too. We can go ahead with such approach if you guys think it is better, but we should be consistent in all places.
PLEASE, do not take this comment as a suggestion for continue sending changes against this PR. Whatever we decide to choose should be addressed in a change unifying all the queries across the code.
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.
This change was based on this another suggestion #1570 (comment), in that case objects were used, so, I also does not have an strong opinion and agree on unifying.
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.
Ok, I see.
As said, let's go ahead and talk about it later, once we finish the queries implementation.
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.
This change was based on this another suggestion #1570 (comment), in that case objects were used, so, I also does not have an strong opinion and agree on unifying.
Well, the point of my comment was not the queryKey
thingie, but the overall approach of the whole function. I added another note about it (see #1570 (comment)).
01deb71
to
faa5e8b
Compare
let mut devices: Vec<(OwnedObjectPath, ZFCPController)> = vec![]; | ||
for (path, ifaces) in managed_objects { | ||
if let Some(properties) = ifaces.get("org.opensuse.Agama.Storage1.ZFCP.Controller") { | ||
// TODO: maybe move it to model? but it needs also additional calls to dbus |
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.
// TODO: maybe move it to model? but it needs also additional calls to dbus | |
// TODO: maybe move it to model? but it needs also additional calls to D-Bus |
/// edge case with malformed path that will panic in Owned path already | ||
/// ```should_panic | ||
/// let path = zbus::zvariant::OwnedObjectPath::try_from("mangled").unwrap(); | ||
/// let id = agama_lib::storage::client::zfcp::ZFCPClient::controller_id_from_path(&path); |
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.
According to this comment, the call to OwnedPath::try_from
will fail, right? So what is the point of this code?
/// let id = agama_lib::storage::client::zfcp::ZFCPClient::controller_id_from_path(&path); | ||
/// assert_eq!(id, "mangled".to_string()); | ||
/// ``` | ||
pub fn controller_id_from_path(path: &OwnedObjectPath) -> String { |
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 already had a helper to get the ID from a path. See https://github.com/openSUSE/agama/blob/bc057f474f9783717f355af970440e7215b2b705/rust/agama-lib/src/dbus.rs#L73.
Ok(dbus) | ||
} | ||
|
||
// TODO: does all those method with controller id as parameter deserve to be moved to controller model? |
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: Do you plan to do something with this comment? Otherwise, I would remove the TODO
prefix.
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.
@jreidinger I guess it is a question you wanted to raise and discuss, so, will remove the comment but maybe this thread could be used for having that discussion... by now it looks good to me to leave the definition where it is, @imobachgs, what do you think?
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. I do not like that we use two different approaches, but let's move on.
11b28f5
to
7af24f8
Compare
b3228bb
to
8b1af6b
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.
LGTM. Congrats!
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
https://build.opensuse.org/request/show/1202590 by user IGonzalezSosa + anag+factory - Version 10 - Change the license to GPL-2.0-or-later (gh#agama-project/agama#1621). - Bring back zFCP management support (gh#agama-project/agama#1570). - Consider TypeScript files for translation (gh#agama-project/agama#1604).
https://build.opensuse.org/request/show/1202589 by user IGonzalezSosa + anag+factory - Version 10 - Change the license to GPL-2.0-or-later (gh#agama-project/agama#1621). - Expose the zFCP D-Bus API through HTTP (gh#agama-project/agama#1570). - For CLI, use HTTP clients instead of D-Bus clients, final piece: Storage (gh#agama-project/agama#1600) - added StorageHTTPClient - Add additional wireless settings (gh#agama-project/agama#1602).
Trello: https://trello.com/c/KMtaVltF (internal link)
This PR could be seen as a follow up of https://trello.com/c/2LC8JEZR (internal link) but in this case bringing back the support for managing zFCP devices on s390x architectures.
It includes:
HTTP / JSON API Examples
This section contains a few examples you can use to explore the API. For the ones using curl, it is required to have a headers.txt file containing the credentials. The file can be generated with:
zFCP operations
Probing
Check if zFCP is supported
Check whether allow_lun_scan is globally enabled or not
Get Controllers with complete information like WWPNs and LUNs
Get Active Disks
Disk activation
Web UI Screenshots
The web has been adapted according to the recommended components to be used for Pages and content as well the Disk Activation Form has been moved from a Popup to its own route and Page.
Additional notes
The PR also includes minor internal changes related to Storage DASD components, some of them related to page actions.