mib1185 commented on a change in pull request #5110:
URL: https://github.com/apache/cloudstack/pull/5110#discussion_r651619054
##########
File path: packaging/centos8/cloud.spec
##########
@@ -96,21 +97,22 @@ The Apache CloudStack files shared between agent and
management server
%package agent
Summary: CloudStack Agent for KVM hypervisors
-Requires: openssh-clients
+Requires: openssh
Requires: java-11-openjdk
Requires: %{name}-common = %{_ver}
Requires: libvirt
Requires: ebtables
Requires: iptables
+Requires: selinux-tools
Requires: ethtool
Requires: net-tools
+Requires: net-tools-deprecated
Requires: iproute
Requires: ipset
Requires: perl
-Requires: python3-libvirt
-Requires: qemu-img
+Requires: python3-libvirt-python
Requires: qemu-kvm
-Requires: libgcrypt > 1.8.3
+Requires: qemu-tools
Review comment:
```suggestion
Requires: libgcrypt20
Requires: qemu-tools
```
`libgcrypt` is replaced by `libgcrypt20`
##########
File path: packaging/centos8/cloud.spec
##########
@@ -96,21 +97,22 @@ The Apache CloudStack files shared between agent and
management server
%package agent
Summary: CloudStack Agent for KVM hypervisors
-Requires: openssh-clients
+Requires: openssh
Requires: java-11-openjdk
Requires: %{name}-common = %{_ver}
Requires: libvirt
Requires: ebtables
Requires: iptables
+Requires: selinux-tools
Requires: ethtool
Requires: net-tools
+Requires: net-tools-deprecated
Review comment:
This package is needed for `python/lib/cloudutils/networkConfig.py` ,
since there are quite old tools `ifconfig` and `route` used, which were already
replaced by modern `ip` command since many years, those IMHO honestly would
recommend to adjust `networkConfig.py` to use `ip` command and not to install a
deprecated called package 🙂
##########
File path: packaging/centos8/cloud.spec
##########
@@ -64,23 +64,24 @@ Requires: bzip2
Requires: gzip
Requires: unzip
Requires: /sbin/mount.nfs
-Requires: openssh-clients
+Requires: openssh
Requires: nfs-utils
+Requires: nfs-client
Requires: iproute
Requires: wget
Requires: mysql
Requires: sudo
Requires: /sbin/service
Requires: /sbin/chkconfig
Requires: /usr/bin/ssh-keygen
-Requires: genisoimage
+# Requires: genisoimage -> renamed to mkisofs
Requires: ipmitool
Requires: %{name}-common = %{_ver}
-Requires: iptables-services
-Requires: qemu-img
+# Requires: iptables-services
+Requires: qemu-tools
Requires: python3-pip
Requires: python3-setuptools
-Requires: libgcrypt > 1.8.3
+# Requires: libgcrypt > 1.8.3
Review comment:
```suggestion
Requires: libgcrypt20
```
`libgcrypt` is replaced by `libgcrypt20`
##########
File path: packaging/centos8/cloud.spec
##########
@@ -64,23 +64,24 @@ Requires: bzip2
Requires: gzip
Requires: unzip
Requires: /sbin/mount.nfs
-Requires: openssh-clients
+Requires: openssh
Requires: nfs-utils
+Requires: nfs-client
Requires: iproute
Requires: wget
Requires: mysql
Requires: sudo
Requires: /sbin/service
Requires: /sbin/chkconfig
Requires: /usr/bin/ssh-keygen
-Requires: genisoimage
+# Requires: genisoimage -> renamed to mkisofs
Review comment:
```suggestion
# Requires: genisoimage -> renamed to mkisofs
Requires: mkisofs
```
Maybe needs still to be installed 🤔
##########
File path: scripts/vm/hypervisor/versions.sh
##########
@@ -37,8 +37,12 @@ elif [ -f /etc/lsb-release ] ; then
DIST=`cat /etc/lsb-release | grep DISTRIB_ID | tr "\n" ' '| sed s/.*=//`
REV=`cat /etc/lsb-release | grep DISTRIB_RELEASE | tr "\n" ' '| sed
s/.*=//`
CODENAME=`cat /etc/lsb-release | grep DISTRIB_CODENAME | tr "\n" ' '|
sed s/.*=//`
+elif [ -f /etc/os-release ] ; then
+ DIST=`cat /etc/os-release | grep NAME | head -n 1 | tr "\n" ' ' | tr -d
'"' | sed s/.*=//`
+ REV=`cat /etc/os-release | grep VERSION_ID | tr "\n" ' ' | tr -d '"' |
sed s/.*=//`
+ CODENAME=`cat /etc/os-release | grep PRETTY_NAME | tr "\n" ' ' | tr -d
'"' | sed s/.*=//`
Review comment:
```suggestion
DIST=`grep -e "^NAME=" /etc/os-release | awk -F\" '{print $2}'`
REV=`grep -e "^VERSION_ID=" /etc/os-release | awk -F\" '{print $2}'`
CODENAME=`grep -e "^PRETTY_NAME=" /etc/os-release | awk -F\" '{print
$2}'`
```
IMHO this looks more clean to me
##########
File path: python/lib/cloudutils/utilities.py
##########
@@ -134,17 +134,23 @@ def __init__(self):
self.distro = "Ubuntu"
else:
raise UnknownSystemException(distributor)
- else:
+ elif os.path.exists("/etc/os-release"):
+ version = open("/etc/os-release").readline()
+ if version.find("openSUSE") != -1:
+ self.distro = "openSUSE"
+ else:
+ raise UnknownSystemException(distributor)
Review comment:
```suggestion
```
since `distributor` is not set and those `raise` from two lines below will
be enough
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
