[pve-devel] [PATCH-SERIES apt/widget-toolkit/manager] small refinements for APT repositories

[pve-devel] [PATCH-SERIES apt/widget-toolkit/manager] small refinements for APT repositories

[Fabian Ebner]

Limit some checks to Proxmox (and Debian) repositories where
appropriate, and some other tiny improvements thrown in.

The RFCs (apt patch #3 and widget-toolkit patch #2) form a logcial
unit. The RFCs are also the only things requiring dependency bumps:

1. widget-toolkit patch #2 expects the new warnings from proxmox-apt

2. To avoid an (although soft) breaks, pve-manager needs to depend on
the new proxmox-apt (via pve-rs) at the same time as it depends on the
new widget-toolkit, because old widget-toolkit can end up showing
warnings without message for the 'Suites' column (see widget-tookit
patch #1 for the details/fix).


proxmox-apt:

Fabian Ebner (3):
  code cleanup: use contains()
  repository check: limit 'stable' to Proxmox and Debian origin
  repository check: check components for Proxmox repositories

 src/repositories/file.rs               | 87 +++++++++++++++-----------
 src/repositories/mod.rs                | 12 ++--
 tests/repositories.rs                  | 23 ++++++-
 tests/sources.list.d.expected/pve.list |  2 +
 tests/sources.list.d/pve.list          |  1 +
 5 files changed, 79 insertions(+), 46 deletions(-)


proxmox-widget-toolkit:

Fabian Ebner (2):
  node: repos: properly ignore warnings for other properties
  node: repos: show components warnings from the backend

 src/node/APTRepositories.js | 46 +++++++++++++++++++++----------------
 1 file changed, 26 insertions(+), 20 deletions(-)


pve-manager:

Fabian Ebner (1):
  api: apt: repositories: add description to return schema

 PVE/API2/APT.pm | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

-- 
2.30.2

[pve-devel] [PATCH proxmox-apt 1/3] code cleanup: use contains()

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 src/repositories/file.rs | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/src/repositories/file.rs b/src/repositories/file.rs
index 447fd0a..49cc358 100644
--- a/src/repositories/file.rs
+++ b/src/repositories/file.rs
@@ -304,11 +304,7 @@ impl APTRepositoryFile {
         let mut infos = vec![];
 
         for (n, repo) in self.repositories.iter().enumerate() {
-            if !repo
-                .types
-                .iter()
-                .any(|package_type| *package_type == APTRepositoryPackageType::Deb)
-            {
+            if !repo.types.contains(&APTRepositoryPackageType::Deb) {
                 continue;
             }
 
-- 
2.30.2

[pve-devel] applied: [PATCH proxmox-apt 1/3] code cleanup: use contains()

[Thomas Lamprecht]

On 05.07.21 15:50, Fabian Ebner wrote:
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  src/repositories/file.rs | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
>

applied, thanks!

[pve-devel] [PATCH proxmox-apt 2/3] repository check: limit 'stable' to Proxmox and Debian origin

[Fabian Ebner]

For foreign repositories, it's a better heuristic to assume it's used
in a non-dangerous (i.e. no sudden major upgrade on release day) way.

Reported-by: Dominik Csapak <d.csapak@proxmox.com>
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 src/repositories/file.rs | 57 ++++++++++++++++++----------------------
 src/repositories/mod.rs  | 12 ++++-----
 2 files changed, 30 insertions(+), 39 deletions(-)

diff --git a/src/repositories/file.rs b/src/repositories/file.rs
index 49cc358..b5bdb77 100644
--- a/src/repositories/file.rs
+++ b/src/repositories/file.rs
@@ -298,12 +298,31 @@ impl APTRepositoryFile {
         Ok(())
     }
 
-    /// Checks if old or unstable suites are configured and also that the
-    /// `stable` keyword is not used.
-    pub fn check_suites(&self) -> Result<Vec<APTRepositoryInfo>, Error> {
+    /// Checks if old or unstable suites are configured, and also tries to
+    /// determine the origin of each repository.
+    pub fn check(&self) -> Result<Vec<APTRepositoryInfo>, Error> {
         let mut infos = vec![];
 
         for (n, repo) in self.repositories.iter().enumerate() {
+            let mut origin = match repo.get_cached_origin() {
+                Ok(option) => option,
+                Err(_) => None,
+            };
+
+            if origin.is_none() {
+                origin = repo.origin_from_uris();
+            }
+
+            if let Some(ref origin) = origin {
+                infos.push(APTRepositoryInfo {
+                    path: self.path.clone(),
+                    index: n,
+                    kind: "origin".to_string(),
+                    property: None,
+                    message: origin.to_string(),
+                });
+            }
+
             if !repo.types.contains(&APTRepositoryPackageType::Deb) {
                 continue;
             }
@@ -353,7 +372,9 @@ impl APTRepositoryFile {
                 }
             }
 
-            if repo.has_suite_variant("stable") {
+            if (origin == Some("Proxmox".to_string()) || origin == Some("Debian".to_string()))
+                && repo.has_suite_variant("stable")
+            {
                 add_info(
                     "warning".to_string(),
                     "use the name of the stable distribution instead of 'stable'!".to_string(),
@@ -363,32 +384,4 @@ impl APTRepositoryFile {
 
         Ok(infos)
     }
-
-    /// Checks for official URIs.
-    pub fn check_uris(&self) -> Vec<APTRepositoryInfo> {
-        let mut infos = vec![];
-
-        for (n, repo) in self.repositories.iter().enumerate() {
-            let mut origin = match repo.get_cached_origin() {
-                Ok(option) => option,
-                Err(_) => None,
-            };
-
-            if origin.is_none() {
-                origin = repo.origin_from_uris();
-            }
-
-            if let Some(origin) = origin {
-                infos.push(APTRepositoryInfo {
-                    path: self.path.clone(),
-                    index: n,
-                    kind: "origin".to_string(),
-                    property: None,
-                    message: origin,
-                });
-            }
-        }
-
-        infos
-    }
 }
diff --git a/src/repositories/mod.rs b/src/repositories/mod.rs
index 7bac333..6dd07f0 100644
--- a/src/repositories/mod.rs
+++ b/src/repositories/mod.rs
@@ -43,18 +43,16 @@ fn common_digest(files: &[APTRepositoryFile]) -> [u8; 32] {
     openssl::sha::sha256(&common_raw[..])
 }
 
-/// Provides additional information about the repositories.
+/// Currently checks if old or unstable suites are configured, and also tries to
+/// determine the origin of each repository.
 ///
-/// The kind of information can be:
-/// `warnings` for bad suites.
-/// `ignore-pre-upgrade-warning` when the next stable suite is configured.
-/// `badge` for official URIs.
+/// For problems, the kind of info will be `warning` for enabled repositories
+/// and `info` for disabled repositories. For the origin, the kind is `origin`.
 pub fn check_repositories(files: &[APTRepositoryFile]) -> Result<Vec<APTRepositoryInfo>, Error> {
     let mut infos = vec![];
 
     for file in files.iter() {
-        infos.append(&mut file.check_suites()?);
-        infos.append(&mut file.check_uris());
+        infos.append(&mut file.check()?);
     }
 
     Ok(infos)
-- 
2.30.2

[pve-devel] [RFC proxmox-apt 3/3] repository check: check components for Proxmox repositories

[Fabian Ebner]

for no-subscription and test, which is currently done directly in the
front-end.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 src/repositories/file.rs               | 34 +++++++++++++++++++++++---
 src/repositories/mod.rs                |  4 +--
 tests/repositories.rs                  | 23 ++++++++++++++---
 tests/sources.list.d.expected/pve.list |  2 ++
 tests/sources.list.d/pve.list          |  1 +
 5 files changed, 55 insertions(+), 9 deletions(-)

diff --git a/src/repositories/file.rs b/src/repositories/file.rs
index b5bdb77..83bccf1 100644
--- a/src/repositories/file.rs
+++ b/src/repositories/file.rs
@@ -298,8 +298,8 @@ impl APTRepositoryFile {
         Ok(())
     }
 
-    /// Checks if old or unstable suites are configured, and also tries to
-    /// determine the origin of each repository.
+    /// Checks if old or unstable suites or components are configured, and also
+    /// tries to determine the origin of each repository.
     pub fn check(&self) -> Result<Vec<APTRepositoryInfo>, Error> {
         let mut infos = vec![];
 
@@ -327,11 +327,11 @@ impl APTRepositoryFile {
                 continue;
             }
 
-            let mut add_info = |kind, message| {
+            let mut add_info = |property, kind, message| {
                 infos.push(APTRepositoryInfo {
                     path: self.path.clone(),
                     index: n,
-                    property: Some("Suites".to_string()),
+                    property,
                     kind,
                     message,
                 })
@@ -351,6 +351,7 @@ impl APTRepositoryFile {
                 if repo.has_suite_variant(suite) {
                     if n < current_index {
                         add_info(
+                            Some("Suites".to_string()),
                             "warning".to_string(),
                             format!("old suite '{}' configured!", suite),
                         );
@@ -358,6 +359,7 @@ impl APTRepositoryFile {
 
                     if n == current_index + 1 {
                         add_info(
+                            Some("Suites".to_string()),
                             "ignore-pre-upgrade-warning".to_string(),
                             format!("suite '{}' should not be used in production!", suite),
                         );
@@ -365,6 +367,7 @@ impl APTRepositoryFile {
 
                     if n > current_index + 1 {
                         add_info(
+                            Some("Suites".to_string()),
                             "warning".to_string(),
                             format!("suite '{}' should not be used in production!", suite),
                         );
@@ -376,10 +379,33 @@ impl APTRepositoryFile {
                 && repo.has_suite_variant("stable")
             {
                 add_info(
+                    Some("Suites".to_string()),
                     "warning".to_string(),
                     "use the name of the stable distribution instead of 'stable'!".to_string(),
                 );
             }
+
+            if origin != Some("Proxmox".to_string()) {
+                continue;
+            }
+
+            for component in repo.components.iter() {
+                if component.ends_with("no-subscription") {
+                    add_info(
+                        Some("Components".to_string()),
+                        "warning".to_string(),
+                        "The no-subscription repository is NOT production-ready".to_string(),
+                    );
+                }
+
+                if component.ends_with("test") {
+                    add_info(
+                        Some("Components".to_string()),
+                        "warning".to_string(),
+                        "The test repository may contain unstable updates".to_string(),
+                    );
+                }
+            }
         }
 
         Ok(infos)
diff --git a/src/repositories/mod.rs b/src/repositories/mod.rs
index 6dd07f0..e5dea03 100644
--- a/src/repositories/mod.rs
+++ b/src/repositories/mod.rs
@@ -43,8 +43,8 @@ fn common_digest(files: &[APTRepositoryFile]) -> [u8; 32] {
     openssl::sha::sha256(&common_raw[..])
 }
 
-/// Currently checks if old or unstable suites are configured, and also tries to
-/// determine the origin of each repository.
+/// Currently checks if old or unstable suites and components are configured,
+/// and also tries to determine the origin of each repository.
 ///
 /// For problems, the kind of info will be `warning` for enabled repositories
 /// and `info` for disabled repositories. For the origin, the kind is `origin`.
diff --git a/tests/repositories.rs b/tests/repositories.rs
index 67b0255..940cd06 100644
--- a/tests/repositories.rs
+++ b/tests/repositories.rs
@@ -197,11 +197,26 @@ fn test_check_repositories() -> Result<(), Error> {
     let path_string = pve_list.into_os_string().into_string().unwrap();
 
     let origins = [
-        "Debian", "Debian", "Proxmox", "Proxmox", "Proxmox", "Debian",
+        "Debian", "Debian", "Proxmox", "Proxmox", "Proxmox", "Proxmox", "Debian",
     ];
 
-    let mut expected_infos = vec![];
-    for n in 0..=5 {
+    let mut expected_infos = vec![
+        APTRepositoryInfo {
+            path: path_string.clone(),
+            index: 2,
+            property: Some("Components".to_string()),
+            kind: "warning".to_string(),
+            message: "The no-subscription repository is NOT production-ready".to_string(),
+        },
+        APTRepositoryInfo {
+            path: path_string.clone(),
+            index: 3,
+            property: Some("Components".to_string()),
+            kind: "warning".to_string(),
+            message: "The test repository may contain unstable updates".to_string(),
+        },
+    ];
+    for n in 0..=6 {
         expected_infos.push(APTRepositoryInfo {
             path: path_string.clone(),
             index: n,
@@ -304,6 +319,7 @@ fn test_get_cached_origin() -> Result<(), Error> {
         Some("Debian".to_string()),
         Some("Debian".to_string()),
         Some("Proxmox".to_string()),
+        Some("Proxmox".to_string()),
         None, // no cache file exists
         None, // no cache file exists
         Some("Debian".to_string()),
@@ -353,6 +369,7 @@ fn test_standard_repositories() -> Result<(), Error> {
 
     expected[0].status = Some(false);
     expected[1].status = Some(true);
+    expected[2].status = Some(false);
 
     let std_repos = standard_repositories("pve", &file_vec);
 
diff --git a/tests/sources.list.d.expected/pve.list b/tests/sources.list.d.expected/pve.list
index c801261..a30566b 100644
--- a/tests/sources.list.d.expected/pve.list
+++ b/tests/sources.list.d.expected/pve.list
@@ -6,6 +6,8 @@ deb http://ftp.debian.org/debian bullseye-updates main contrib
 # NOT recommended for production use
 deb http://download.proxmox.com/debian/pve bullseye pve-no-subscription
 
+# deb http://download.proxmox.com/debian/pve bullseye pvetest
+
 # deb https://enterprise.proxmox.com/debian/pve bullseye pve-enterprise
 
 deb-src https://enterprise.proxmox.com/debian/pve buster pve-enterprise
diff --git a/tests/sources.list.d/pve.list b/tests/sources.list.d/pve.list
index 4d36d3d..1dfc857 100644
--- a/tests/sources.list.d/pve.list
+++ b/tests/sources.list.d/pve.list
@@ -4,6 +4,7 @@ deb http://ftp.debian.org/debian bullseye-updates main contrib
 # PVE pve-no-subscription repository provided by proxmox.com,
 # NOT recommended for production use
 deb http://download.proxmox.com/debian/pve bullseye pve-no-subscription
+# deb http://download.proxmox.com/debian/pve bullseye pvetest
 # deb https://enterprise.proxmox.com/debian/pve bullseye pve-enterprise
 
 deb-src https://enterprise.proxmox.com/debian/pve buster pve-enterprise
-- 
2.30.2

[pve-devel] [PATCH proxmox-widget-toolkit 1/2] node: repos: properly ignore warnings for other properties

[Fabian Ebner]

Previously, if there were some warnings, but no warnings for the
'Suites' property, it would still display as a warning (without
additional text).

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 src/node/APTRepositories.js | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/src/node/APTRepositories.js b/src/node/APTRepositories.js
index e8f807d..e9e8d92 100644
--- a/src/node/APTRepositories.js
+++ b/src/node/APTRepositories.js
@@ -275,14 +275,18 @@ Ext.define('Proxmox.node.APTRepositoriesGrid', {
 	    header: gettext('Suites'),
 	    dataIndex: 'Suites',
 	    renderer: function(suites, metaData, record) {
+		if (!record.data.warnings) {
+		    return suites.join(' ');
+		}
+
+		const warningTexts = record.data.warnings.filter(
+		    warning => warning.property === 'Suites',
+		).map(warning => warning.message);
+
 		let err = '';
-		if (record.data.warnings && record.data.warnings.length > 0) {
-		    let txt = [gettext('Warning')];
-		    record.data.warnings.forEach((warning) => {
-			if (warning.property === 'Suites') {
-			    txt.push(warning.message);
-			}
-		    });
+		if (warningTexts.length > 0) {
+		    const txt = [gettext('Warning')].concat(warningTexts);
+
 		    metaData.tdAttr = `data-qtip="${Ext.htmlEncode(txt.join('<br>'))}"`;
 		    if (record.data.Enabled) {
 			metaData.tdCls = 'proxmox-invalid-row';
-- 
2.30.2

[pve-devel] [RFC proxmox-widget-toolkit 2/2] node: repos: show components warnings from the backend

[Fabian Ebner]

There's no need to show anything new in the top status, as the test
and no-subscription repository already trigger a message.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

Depends on the new backend behavior.

I felt like this was a good fit for using the existing info/warnings
mechanism again, so I didn't go for what the FIXME suggested, but I
can switch and use that approach if preferred.

 src/node/APTRepositories.js | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/src/node/APTRepositories.js b/src/node/APTRepositories.js
index e9e8d92..13e19a1 100644
--- a/src/node/APTRepositories.js
+++ b/src/node/APTRepositories.js
@@ -307,21 +307,23 @@ Ext.define('Proxmox.node.APTRepositoriesGrid', {
 		if (components === undefined) {
 		    return '';
 		}
-		let err = '';
-		if (components.length === 1) {
-		    // FIXME: this should be a flag set to the actual repsotiories, i.e., a tristate
-		    // like production-ready = <yes|no|other> (Option<bool>)
-		    if (components[0].match(/\w+(-no-subscription|test)\s*$/i)) {
-			metaData.tdCls = 'proxmox-warning-row';
-			err = '<i class="fa fa-fw warning fa-exclamation-circle"></i> ';
 
-			let qtip = components[0].match(/no-subscription/)
-			    ? gettext('The no-subscription repository is NOT production-ready')
-			    : gettext('The test repository may contain unstable updates')
-			    ;
-			    metaData.tdAttr = `data-qtip="${Ext.htmlEncode(qtip)}"`;
-		    }
+		if (!record.data.warnings) {
+		    return components.join(' ');
 		}
+
+		const warningTexts = record.data.warnings.filter(
+		    warning => warning.property === 'Components',
+		).map(warning => warning.message);
+
+		let err = '';
+		if (warningTexts.length > 0) {
+		    const qtip = warningTexts.join('<br>');
+		    metaData.tdCls = 'proxmox-warning-row';
+		    err = '<i class="fa fa-fw warning fa-exclamation-circle"></i> ';
+		    metaData.tdAttr = `data-qtip="${Ext.htmlEncode(qtip)}"`;
+		}
+
 		return components.join(' ') + err;
 	    },
 	    width: 170,
-- 
2.30.2

[pve-devel] [PATCH manager 1/1] api: apt: repositories: add description to return schema

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

for completeness' sake.

 PVE/API2/APT.pm | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/PVE/API2/APT.pm b/PVE/API2/APT.pm
index bd60db33..daa297b9 100644
--- a/PVE/API2/APT.pm
+++ b/PVE/API2/APT.pm
@@ -655,7 +655,11 @@ __PACKAGE__->register_method({
 			},
 			name => {
 			    type => "string",
-			    description => "Full name of the repository.",
+			    description => "Display name of the repository.",
+			},
+			description => {
+			    type => "string",
+			    description => "Description of the repository.",
 			},
 			status => {
 			    type => "boolean",
-- 
2.30.2