refactor(web): use queries to handle iSCSI configuration #1598
Conversation
| * Sets the startup status of the connection | ||
| * | ||
| * @param node - ISCSI node | ||
| * @param startup - startup status |
There was a problem hiding this comment.
hmm, startup is string, so it can accept any value? or is it kind of enum with limited statuses?
There was a problem hiding this comment.
It is a generic string. I do not like it that much, but I decided to keep it as it was, considering it was out of this PR's scope. But sure we should improve how we handle the staus.
| })); | ||
|
|
||
| jest.mock("~/api/storage/iscsi", () => ({ | ||
| ...jest.requireActual("~/queries/storage/iscsi"), |
There was a problem hiding this comment.
is there reason to load rest of queries? Cannot it do some backend communication? I am asking this in all those mock that include also requireActual
There was a problem hiding this comment.
For no particular reason. I like to keep the original mode and mock only those functions that the test will use.
There was a problem hiding this comment.
The part that worry me is that it can cause accidental communication with backend...if it is not there, it will immediatelly fail in test, not?
There was a problem hiding this comment.
Yes, the test will fail in either case.
| export default function TargetsSection() { | ||
| const [isDiscoverFormOpen, setIsDiscoverFormOpen] = useState<boolean>(false); | ||
| const nodes = useNodes(); | ||
| useNodesChanges(); |
There was a problem hiding this comment.
Is there some changes done in changes queries? Previously it returns query. So it can be written like const nodes = useNodesChanges(); . And it is still done this way e.g. in DASD, but in others where it was before I do not see it anymore. ( e.g. https://github.com/openSUSE/agama/blob/master/web/src/queries/dasd.ts#L121 )
There was a problem hiding this comment.
Well, that's an interesting topic. We usually have separate hooks for retrieving the query and subscribing to changes. DASD handling is the only place where is done differently. DASD hooks 1) retrieve the value and 2) subscribe to the changes. So, if you use the useDASDFormatJobsChanges in more than one place, you are subscribing twice. In this particular case, the JobAdded event will not be handled correctly (that's easy to fix).
Let's consider that our DASD formats progress is not blocking (we discussed that during the review), and you want to add a counter to the pending jobs. Then you might want to access that query from your counter, too. And then, you will find the problem mentioned above.
Having two different hooks (useIssues and useIssuesChanges) might be a problem too. You need to be sure that the changes hooks are used at least at the same level as the query hook. That's why most changes hooks are called from the App component.
Finally, something we need to fix, is adapting all those changes queries to not update the query if it is not defined. I did that correctly (I think) in this pull request, but we might need to fix others.
There was a problem hiding this comment.
Perhaps I should move the call to useNodesChanges to the App level.
Co-authored-by: David Díaz <1691872+dgdavid@users.noreply.github.com>
imobachgs
force-pushed
the
iscsi-queries
branch
from
6a2fada to
684d796
Compare
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
Trello: https://trello.com/c/MsGbqiwA/
This is a follow-up of #1585 which adapts the iSCSI-related code to use queries instead of the regular Agama client. Additionally, it converts a good share of the code to TypeScript and adds some iSCSI tests, that were not included in the first version.