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.

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

feat(rust): Redesigned agama logs command to have functionality in the backend and accessible via HTTP API #1720

Merged
merged 18 commits into from
Nov 6, 2024

Conversation

mchf
Copy link
Contributor

@mchf mchf commented Nov 1, 2024

Problem

Agama's CLI logs command used to be implemented with idea CLI running on same machine as real installation (the backend). As this is not true anymore and we have installer's backend and frontend separated with HTTP API, which makes remote access possible, the logs command had to be updated to support this idea.

Its trello card

Solution

  • the particular code which is in control of collecting logs was moved into agama-lib
  • agama's web server has contains http(s)://<server_ip>/api/logs piece in its public API
  • agama's CLI uses the above API for downloading the logs

Testing

  • Tested manually

@mchf mchf marked this pull request as draft November 1, 2024 08:55
@coveralls
Copy link

coveralls commented Nov 1, 2024

Pull Request Test Coverage Report for Build 11708505683

Details

  • 0 of 171 (0.0%) changed or added relevant lines in 6 files are covered.
  • 2 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.06%) to 71.429%

Changes Missing Coverage Covered Lines Changed/Added Lines %
rust/agama-cli/src/lib.rs 0 1 0.0%
rust/agama-lib/src/base_http_client.rs 0 5 0.0%
rust/agama-lib/src/manager/http_client.rs 0 16 0.0%
rust/agama-cli/src/logs.rs 0 23 0.0%
rust/agama-server/src/manager/web.rs 0 28 0.0%
rust/agama-lib/src/logs.rs 0 98 0.0%
Files with Coverage Reduction New Missed Lines %
rust/agama-cli/src/lib.rs 1 0.0%
rust/agama-cli/src/logs.rs 1 0.0%
Totals Coverage Status
Change from base Build 11707344472: -0.06%
Covered Lines: 16895
Relevant Lines: 23653

💛 - Coveralls

@mchf mchf force-pushed the logs_api branch 2 times, most recently from 9f30650 to 991bc61 Compare November 4, 2024 07:48
@mchf mchf requested a review from imobachgs November 4, 2024 07:48
@mchf mchf marked this pull request as ready for review November 4, 2024 07:48
@imobachgs imobachgs changed the title [CLI, LIB] Redesigned agama logs command to have functionality in the backend and accessible via HTTP API feat(rust): Redesigned agama logs command to have functionality in the backend and accessible via HTTP API Nov 4, 2024
Copy link
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

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

Thanks! I have a few comments because it looks like there is some duplication.

rust/agama-server/src/web/http.rs Outdated Show resolved Hide resolved
rust/agama-lib/src/base_http_client.rs Outdated Show resolved Hide resolved
rust/agama-lib/src/logs/http_client.rs Outdated Show resolved Hide resolved
rust/agama-cli/src/logs.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@imobachgs imobachgs left a 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, but I still have a few questions/comments.

rust/package/agama.changes Outdated Show resolved Hide resolved
rust/agama-server/src/manager/web.rs Outdated Show resolved Hide resolved
rust/agama-server/src/manager/web.rs Outdated Show resolved Hide resolved
rust/agama-server/src/manager/web.rs Outdated Show resolved Hide resolved
rust/agama-server/src/manager/web.rs Outdated Show resolved Hide resolved
rust/agama-lib/src/logs.rs Outdated Show resolved Hide resolved
@@ -49,6 +49,7 @@ pub mod error;
pub mod install_settings;
pub mod jobs;
pub mod localization;
pub mod logs;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know whether this module belongs to agama-cli or agama-server. We can leave it here by now, but I don't think it will be used anywhere else (except of the set_archive_permissions that might belong to agama-lib, but not as something specific to logs).

rust/agama-server/src/manager/web.rs Outdated Show resolved Hide resolved
rust/agama-server/src/manager/web.rs Outdated Show resolved Hide resolved
rust/agama-lib/src/logs.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

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

It looks better know, but there is still an issue with the web UI client. Please, check.

web/src/api/manager.ts Show resolved Hide resolved
Copy link
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

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

It looks almost good. Please, update the agama-web-ui.changes file and that's it. Thanks!

.map_err(|e| ServiceError::CannotGenerateLogs(e.to_string()))?;
// See RFC2046, RFC2616 and
// https://www.iana.org/assignments/media-types/media-types.xhtml
// or /etc/mime.types
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not now how "standard" is the mime.types (outside of Linux), but we can find it out later.

-------------------------------------------------------------------
Wed Nov 6 06:06:51 UTC 2024 - Michal Filka <mfilka@suse.com>

- url for downloading agama logs adapted to use new HTTP API
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- url for downloading agama logs adapted to use new HTTP API
- URL for downloading Agama logs adapted to use new HTTP API

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i tried but your eye is too sharp even in the morning ;-)

@mchf mchf merged commit a714076 into master Nov 6, 2024
8 checks passed
@mchf mchf deleted the logs_api branch November 6, 2024 17:22
@imobachgs imobachgs mentioned this pull request Jan 10, 2025
imobachgs added a commit that referenced this pull request Jan 13, 2025
Update to release version 11.

* #1495
* #1564
* #1617
* #1618
* #1625
* #1626
* #1627
* #1628
* #1630
* #1631
* #1632
* #1633
* #1634
* #1635
* #1636
* #1639
* #1640
* #1641
* #1642
* #1643
* #1644
* #1645
* #1646
* #1647
* #1648
* #1649
* #1650
* #1651
* #1652
* #1654
* #1655
* #1656
* #1657
* #1660
* #1663
* #1666
* #1667
* #1668
* #1670
* #1671
* #1673
* #1674
* #1675
* #1676
* #1677
* #1681
* #1682
* #1683
* #1684
* #1687
* #1688
* #1689
* #1690
* #1691
* #1692
* #1693
* #1694
* #1695
* #1696
* #1698
* #1699
* #1702
* #1703
* #1704
* #1705
* #1707
* #1708
* #1709
* #1710
* #1711
* #1712
* #1713
* #1714
* #1715
* #1716
* #1717
* #1718
* #1720
* #1721
* #1722
* #1723
* #1727
* #1728
* #1729
* #1731
* #1732
* #1733
* #1734
* #1735
* #1736
* #1737
* #1740
* #1741
* #1743
* #1744
* #1745
* #1746
* #1751
* #1753
* #1754
* #1755
* #1757
* #1762
* #1763
* #1764
* #1765
* #1766
* #1767
* #1769
* #1771
* #1772
* #1773
* #1774
* #1777
* #1778
* #1785
* #1786
* #1787
* #1788
* #1789
* #1790
* #1791
* #1792
* #1793
* #1794
* #1795
* #1796
* #1797
* #1798
* #1799
* #1800
* #1802
* #1803
* #1804
* #1805
* #1807
* #1808
* #1809
* #1810
* #1811
* #1812
* #1814
* #1815
* #1821
* #1822
* #1823
* #1824
* #1825
* #1826
* #1827
* #1828
* #1830
* #1831
* #1832
* #1833
* #1834
* #1835
* #1836
* #1837
* #1838
* #1839
* #1840
* #1841
* #1842
* #1843
* #1844
* #1845
* #1847
* #1848
* #1849
* #1850
* #1851
* #1854
* #1855
* #1856
* #1857
* #1860
* #1861
* #1863
* #1864
* #1865
* #1866
* #1867
* #1871
* #1872
* #1873
* #1875
* #1876
* #1877
* #1878
* #1880
* #1881
* #1882
* #1883
* #1884
* #1885
* #1886
* #1888
* #1889
* #1890
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants