[pve-devel] [PATCH-SERIES proxmox-apt/proxmox-widget-toolkit] Spawn a window when adding a repository

[pve-devel] [PATCH-SERIES proxmox-apt/proxmox-widget-toolkit] Spawn a window when adding a repository

[Fabian Ebner]

and add a description.

I couldn't get the validation to work properly and adding a repo
gives an "Unknown Error" (but does add the repo).


Same as last time:
Requires the not-yet-applied patches for pve-rs and pve-manager from
here:

https://lists.proxmox.com/pipermail/pve-devel/2021-June/048963.html

It's necessary to re-build pve-rs to get the changes of course. For
pve-manger no changes should be required (for completeness, the
'description' property can be added to the return schema).


proxmox-apt:

Fabian Ebner (1):
  standard repos: allow conversion from handle and improve information

 src/repositories/mod.rs      | 42 ++++-------------------
 src/repositories/standard.rs | 64 +++++++++++++++++++++++++++++++-----
 tests/repositories.rs        | 42 ++++-------------------
 3 files changed, 69 insertions(+), 79 deletions(-)


proxmox-widget-toolkit:

Fabian Ebner (1):
  node: apt: spawn a window for adding repository

 src/node/APTRepositories.js | 161 ++++++++++++++++++++++++++----------
 1 file changed, 117 insertions(+), 44 deletions(-)
-- 
2.30.2

[pve-devel] [PATCH proxmox-apt 1/1] standard repos: allow conversion from handle and improve information

[Fabian Ebner]

Add a description for the handle, which can be useful to display
alongside the name. The descriptions are essentially the first
sentence from PVE's "Package Repositories" docs, but without the
product name.

Also drop the " Repository" suffix from the names, as it's not useful,
but can be ugly: e.g. for the UI when the label already is
'Repository:'.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 src/repositories/mod.rs      | 42 ++++-------------------
 src/repositories/standard.rs | 64 +++++++++++++++++++++++++++++++-----
 tests/repositories.rs        | 42 ++++-------------------
 3 files changed, 69 insertions(+), 79 deletions(-)

diff --git a/src/repositories/mod.rs b/src/repositories/mod.rs
index e2b0b6b..7bac333 100644
--- a/src/repositories/mod.rs
+++ b/src/repositories/mod.rs
@@ -80,45 +80,17 @@ pub fn standard_repositories(
     files: &[APTRepositoryFile],
 ) -> Vec<APTStandardRepository> {
     let mut result = vec![
-        APTStandardRepository {
-            handle: APTRepositoryHandle::Enterprise,
-            status: None,
-            name: APTRepositoryHandle::Enterprise.name(),
-        },
-        APTStandardRepository {
-            handle: APTRepositoryHandle::NoSubscription,
-            status: None,
-            name: APTRepositoryHandle::NoSubscription.name(),
-        },
-        APTStandardRepository {
-            handle: APTRepositoryHandle::Test,
-            status: None,
-            name: APTRepositoryHandle::Test.name(),
-        },
+        APTStandardRepository::from(APTRepositoryHandle::Enterprise),
+        APTStandardRepository::from(APTRepositoryHandle::NoSubscription),
+        APTStandardRepository::from(APTRepositoryHandle::Test),
     ];
 
     if product == "pve" {
         result.append(&mut vec![
-            APTStandardRepository {
-                handle: APTRepositoryHandle::CephPacific,
-                status: None,
-                name: APTRepositoryHandle::CephPacific.name(),
-            },
-            APTStandardRepository {
-                handle: APTRepositoryHandle::CephPacificTest,
-                status: None,
-                name: APTRepositoryHandle::CephPacificTest.name(),
-            },
-            APTStandardRepository {
-                handle: APTRepositoryHandle::CephOctopus,
-                status: None,
-                name: APTRepositoryHandle::CephOctopus.name(),
-            },
-            APTStandardRepository {
-                handle: APTRepositoryHandle::CephOctopusTest,
-                status: None,
-                name: APTRepositoryHandle::CephOctopusTest.name(),
-            },
+            APTStandardRepository::from(APTRepositoryHandle::CephPacific),
+            APTStandardRepository::from(APTRepositoryHandle::CephPacificTest),
+            APTStandardRepository::from(APTRepositoryHandle::CephOctopus),
+            APTStandardRepository::from(APTRepositoryHandle::CephOctopusTest),
         ]);
     }
 
diff --git a/src/repositories/standard.rs b/src/repositories/standard.rs
index dcfe85b..0d2cc14 100644
--- a/src/repositories/standard.rs
+++ b/src/repositories/standard.rs
@@ -29,8 +29,11 @@ pub struct APTStandardRepository {
     #[serde(skip_serializing_if = "Option::is_none")]
     pub status: Option<bool>,
 
-    /// Full name of the repository.
+    /// Display name of the repository.
     pub name: String,
+
+    /// Description of the repository.
+    pub description: String,
 }
 
 #[api]
@@ -54,6 +57,17 @@ pub enum APTRepositoryHandle {
     CephOctopusTest,
 }
 
+impl From<APTRepositoryHandle> for APTStandardRepository {
+    fn from(handle: APTRepositoryHandle) -> Self {
+        APTStandardRepository {
+            handle,
+            status: None,
+            name: handle.name(),
+            description: handle.description(),
+        }
+    }
+}
+
 impl TryFrom<&str> for APTRepositoryHandle {
     type Error = Error;
 
@@ -86,16 +100,48 @@ impl Display for APTRepositoryHandle {
 }
 
 impl APTRepositoryHandle {
-    /// Get the full name of the repository.
+    /// Get the description for the repository.
+    pub fn description(self) -> String {
+        match self {
+            APTRepositoryHandle::Enterprise => {
+                "This is the default, stable, and recommended repository, available for all \
+                Proxmox subscription users."
+            }
+            APTRepositoryHandle::NoSubscription => {
+                "This is the recommended repository for testing and non-production use."
+            }
+            APTRepositoryHandle::Test => {
+                "This repository contains the latest packages and is primarily used by developers \
+                to test new features."
+            }
+            APTRepositoryHandle::CephPacific => {
+                "This repository holds the main Proxmox Ceph Pacific packages."
+            }
+            APTRepositoryHandle::CephPacificTest => {
+                "This repository contains the Ceph Pacific packages before they are moved to the \
+                main repository."
+            }
+            APTRepositoryHandle::CephOctopus => {
+                "This repository holds the main Proxmox Ceph Octopus packages."
+            }
+            APTRepositoryHandle::CephOctopusTest => {
+                "This repository contains the Ceph Octopus packages before they are moved to the \
+                main repository."
+            }
+        }
+        .to_string()
+    }
+
+    /// Get the display name of the repository.
     pub fn name(self) -> String {
         match self {
-            APTRepositoryHandle::Enterprise => "Enterprise Repository",
-            APTRepositoryHandle::NoSubscription => "No-Subscription Repository",
-            APTRepositoryHandle::Test => "Test Repository",
-            APTRepositoryHandle::CephPacific => "Ceph Pacific Repository",
-            APTRepositoryHandle::CephPacificTest => "Ceph Pacific Test Repository",
-            APTRepositoryHandle::CephOctopus => "Ceph Octopus Repository",
-            APTRepositoryHandle::CephOctopusTest => "Ceph Octopus Test Repository",
+            APTRepositoryHandle::Enterprise => "Enterprise",
+            APTRepositoryHandle::NoSubscription => "No-Subscription",
+            APTRepositoryHandle::Test => "Test",
+            APTRepositoryHandle::CephPacific => "Ceph Pacific",
+            APTRepositoryHandle::CephPacificTest => "Ceph Pacific Test",
+            APTRepositoryHandle::CephOctopus => "Ceph Octopus",
+            APTRepositoryHandle::CephOctopusTest => "Ceph Octopus Test",
         }
         .to_string()
     }
diff --git a/tests/repositories.rs b/tests/repositories.rs
index fefc608..289bbe4 100644
--- a/tests/repositories.rs
+++ b/tests/repositories.rs
@@ -317,41 +317,13 @@ fn test_standard_repositories() -> Result<(), Error> {
     let read_dir = test_dir.join("sources.list.d");
 
     let mut expected = vec![
-        APTStandardRepository {
-            handle: APTRepositoryHandle::Enterprise,
-            status: None,
-            name: APTRepositoryHandle::Enterprise.name(),
-        },
-        APTStandardRepository {
-            handle: APTRepositoryHandle::NoSubscription,
-            status: None,
-            name: APTRepositoryHandle::NoSubscription.name(),
-        },
-        APTStandardRepository {
-            handle: APTRepositoryHandle::Test,
-            status: None,
-            name: APTRepositoryHandle::Test.name(),
-        },
-        APTStandardRepository {
-            handle: APTRepositoryHandle::CephPacific,
-            status: None,
-            name: APTRepositoryHandle::CephPacific.name(),
-        },
-        APTStandardRepository {
-            handle: APTRepositoryHandle::CephPacificTest,
-            status: None,
-            name: APTRepositoryHandle::CephPacificTest.name(),
-        },
-        APTStandardRepository {
-            handle: APTRepositoryHandle::CephOctopus,
-            status: None,
-            name: APTRepositoryHandle::CephOctopus.name(),
-        },
-        APTStandardRepository {
-            handle: APTRepositoryHandle::CephOctopusTest,
-            status: None,
-            name: APTRepositoryHandle::CephOctopusTest.name(),
-        },
+        APTStandardRepository::from(APTRepositoryHandle::Enterprise),
+        APTStandardRepository::from(APTRepositoryHandle::NoSubscription),
+        APTStandardRepository::from(APTRepositoryHandle::Test),
+        APTStandardRepository::from(APTRepositoryHandle::CephPacific),
+        APTStandardRepository::from(APTRepositoryHandle::CephPacificTest),
+        APTStandardRepository::from(APTRepositoryHandle::CephOctopus),
+        APTStandardRepository::from(APTRepositoryHandle::CephOctopusTest),
     ];
 
     let absolute_suite_list = read_dir.join("absolute_suite.list");
-- 
2.30.2

[pve-devel] [RFC proxmox-widget-toolkit 1/1] node: apt: spawn a window for adding repository

[Fabian Ebner]

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

I couldn't get the validation to work properly and adding a repo
gives an "Unknown Error" (but does add the repo).

 src/node/APTRepositories.js | 161 ++++++++++++++++++++++++++----------
 1 file changed, 117 insertions(+), 44 deletions(-)

diff --git a/src/node/APTRepositories.js b/src/node/APTRepositories.js
index 6812d46..43297c9 100644
--- a/src/node/APTRepositories.js
+++ b/src/node/APTRepositories.js
@@ -15,6 +15,92 @@ Ext.define('apt-repolist', {
     ],
 });
 
+Ext.define('Proxmox.window.APTRepositoryAdd', {
+    extend: 'Proxmox.window.Edit',
+    alias: 'widget.pmxAPTRepositoryAdd',
+
+    isCreate: true,
+    isAdd: true,
+
+    subject: gettext('Repository'),
+
+    initComponent: function() {
+	let me = this;
+
+	if (!me.repoInfo || me.repoInfo.length === 0) {
+	    throw "repository information not initialized";
+	}
+
+	let description = Ext.create('Ext.form.field.Display', {
+	    fieldLabel: gettext('Description'),
+	    name: 'description',
+	});
+
+	let status = Ext.create('Ext.form.field.Display', {
+	    fieldLabel: gettext('Status'),
+	    name: 'status',
+	    renderer: function(value) {
+		let statusText = gettext('Not yet configured');
+		if (value !== '') {
+		    statusText = Ext.String.format(
+			'{0}: {1}',
+			gettext('Configured'),
+			value ? gettext('enabled') : gettext('disabled'),
+		    );
+		}
+
+		return statusText;
+	    },
+	});
+
+	let repoSelector = Ext.create('Proxmox.form.KVComboBox', {
+	    fieldLabel: gettext('Repository'),
+	    xtype: 'proxmoxKVComboBox',
+	    name: 'handle',
+	    allowBlank: false,
+	    comboItems: me.repoInfo.map(info => [info.handle, info.name]),
+	    isValid: function() {
+		const handle = this.value;
+
+		if (!handle) {
+		    return false;
+		}
+
+		const info = me.repoInfo.find(elem => elem.handle === handle);
+
+		if (!info) {
+		    return false;
+		}
+
+		// not yet configured
+		return info.status === undefined || info.status === null;
+	    },
+	    listeners: {
+		change: function(f, value) {
+		    const info = me.repoInfo.find(elem => elem.handle === value);
+		    description.setValue(info.description);
+		    status.setValue(info.status);
+		},
+	    },
+	});
+
+	repoSelector.setValue(me.repoInfo[0].handle);
+
+	let items = [
+	    repoSelector,
+	    description,
+	    status,
+	];
+
+	Ext.apply(me, {
+	    items: items,
+	    repoSelector: repoSelector,
+	});
+
+	me.callParent();
+    },
+});
+
 Ext.define('Proxmox.node.APTRepositoriesErrors', {
     extend: 'Ext.grid.GridPanel',
 
@@ -63,10 +149,31 @@ Ext.define('Proxmox.node.APTRepositoriesGrid', {
 	},
 	{
 	    text: gettext('Add'),
-	    menu: {
-		plain: true,
-		itemId: "addMenu",
-		items: [],
+	    id: 'addButton',
+	    disabled: true,
+	    repoInfo: undefined,
+	    handler: function(button, event, record) {
+		Proxmox.Utils.checked_command(() => {
+		    let me = this;
+		    let panel = me.up('proxmoxNodeAPTRepositories');
+
+		    let extraParams = {};
+		    if (panel.digest !== undefined) {
+		       extraParams.digest = panel.digest;
+		    }
+
+		    Ext.create('Proxmox.window.APTRepositoryAdd', {
+			repoInfo: me.repoInfo,
+			url: `/api2/json/nodes/${panel.nodename}/apt/repositories`,
+			method: 'PUT',
+			extraRequestParams: extraParams,
+			listeners: {
+			    destroy: function() {
+				panel.reload();
+			    },
+			},
+		    }).show();
+		});
 	    },
 	},
 	'-',
@@ -403,8 +510,8 @@ Ext.define('Proxmox.node.APTRepositories', {
 	let me = this;
 	let vm = me.getViewModel();
 
-	let menu = me.down('#addMenu');
-	menu.removeAll();
+	let addButton = me.down('#addButton');
+	addButton.repoInfo = [];
 
 	for (const standardRepo of standardRepos) {
 	    const handle = standardRepo.handle;
@@ -416,45 +523,11 @@ Ext.define('Proxmox.node.APTRepositories', {
 		vm.set('noSubscriptionRepo', status);
 	    }
 
-	    let status_text = '';
-	    if (status !== undefined && status !== null) {
-		status_text = Ext.String.format(
-		    ' ({0}, {1})',
-		    gettext('configured'),
-		    status ? gettext('enabled') : gettext('disabled'),
-		);
-	    }
-
-	    menu.add({
-		text: standardRepo.name + status_text,
-		disabled: status !== undefined && status !== null,
-		repoHandle: handle,
-		handler: function(menuItem) {
-		    Proxmox.Utils.checked_command(() => {
-			let params = {
-			    handle: menuItem.repoHandle,
-			};
-
-			if (me.digest !== undefined) {
-			    params.digest = me.digest;
-			}
-
-			Proxmox.Utils.API2Request({
-			    url: `/nodes/${me.nodename}/apt/repositories`,
-			    method: 'PUT',
-			    params: params,
-			    failure: function(response, opts) {
-				Ext.Msg.alert(gettext('Error'), response.htmlStatus);
-				me.reload();
-			    },
-			    success: function(response, opts) {
-				me.reload();
-			    },
-			});
-		    });
-		},
-	    });
+	    addButton.repoInfo.push(standardRepo);
+	    addButton.digest = me.digest;
 	}
+
+	addButton.setDisabled(false);
     },
 
     reload: function() {
-- 
2.30.2

[pve-devel] applied: [PATCH proxmox-apt 1/1] standard repos: allow conversion from handle and improve information

[Thomas Lamprecht]

On 30.06.21 17:07, Fabian Ebner wrote:
> Add a description for the handle, which can be useful to display
> alongside the name. The descriptions are essentially the first
> sentence from PVE's "Package Repositories" docs, but without the
> product name.
> 
> Also drop the " Repository" suffix from the names, as it's not useful,
> but can be ugly: e.g. for the UI when the label already is
> 'Repository:'.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  src/repositories/mod.rs      | 42 ++++-------------------
>  src/repositories/standard.rs | 64 +++++++++++++++++++++++++++++++-----
>  tests/repositories.rs        | 42 ++++-------------------
>  3 files changed, 69 insertions(+), 79 deletions(-)
> 
>

applied, thanks!

[pve-devel] applied: [RFC proxmox-widget-toolkit 1/1] node: apt: spawn a window for adding repository

[Thomas Lamprecht]

On 30.06.21 17:07, Fabian Ebner wrote:
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> I couldn't get the validation to work properly and adding a repo
> gives an "Unknown Error" (but does add the repo).
> 
>  src/node/APTRepositories.js | 161 ++++++++++++++++++++++++++----------
>  1 file changed, 117 insertions(+), 44 deletions(-)
> 
>

applied, thanks! I'll make the window a bit bigger, or at least wider though.