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

fix(rust): better profile URL handling #1831

Merged
merged 11 commits into from
Dec 16, 2024
Merged

fix(rust): better profile URL handling #1831

merged 11 commits into from
Dec 16, 2024

Conversation

imobachgs
Copy link
Contributor

@imobachgs imobachgs commented Dec 12, 2024

Fix for bsc#1234362. It should unblock QA.

It fixes the behavior when importing an auto-installation profile:

  • Follow redirections.
  • Determine the file format from the content instead of the extension. It does not apply to AutoYaST profiles, where it still uses the extension in the URL for backward compatibility.

Follow-up

The agama profile import command was not updated after adding the --api switch. Therefore, if you try to import a profile from a remote system, the profile will be processed on your machine (not in the remote one). Among other potential problems, it will use hardware information from the wrong system.

We need to move the logic to preprocess the profile from agama-cli to agama-server.

@imobachgs imobachgs marked this pull request as ready for review December 13, 2024 06:54
///
/// It returns `true` if the content starts with a shebang.
fn is_script(content: &str) -> bool {
content.starts_with("#!")
Copy link
Contributor

Choose a reason for hiding this comment

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

be aware that shebang is not mandatory for shell scripts and also that after shebang can be something like python :)
In general as this means that shell scripts without shebang ends with "unknown file" it think it would be nice to combine both approaches...suffix and content. I would say ideally if there is suffix base decision on it as it is fast...and then check content if suffix is not there or unknown.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is optional, but you cannot trust the suffix (as the script comes from a URL, you might not even have a suffix). I think we can educate our users to include a shebang line :-)

Actually, I do not know if using something different from bash would work at all. Check how we build the command.

.context("Could not evaluate the profile".to_string())?;
}
FileFormat::Script => {
let err = Command::new("bash").args([&tmp_profile_path]).exec();
Copy link
Contributor

Choose a reason for hiding this comment

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

As Josef said, we are checking for any script, even Python, but then run it with bash.

  1. Either check for #!/bin/bash,
  2. or make it executable and Command::new(tmp_profile_path)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's why I pointed to how we run those scripts after reading @jreidinger's comment . For the scripting support, I followed the 2nd approach (make it executable).

However, when handling the agama.auto we have never considered anything different from bash.

So we can keep the current check and make it executable; or just check for bash.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to clarify, this line of code was already there. It was just moved in this PR.

/// It returns `true` if the content starts with a shebang.
/// It returns `true` if the content starts with a shebang. However, it excludes the
/// case of a "jsonnet" script because it needs special handling: injecting the hardware
/// information and processing its output.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@imobachgs imobachgs merged commit d00fb95 into master Dec 16, 2024
6 checks passed
@imobachgs imobachgs deleted the better-url-handling branch December 16, 2024 12:50
@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