[GitHub] [guacamole-server] mike-jumper commented on a change in pull request #348: GUACAMOLE-1447: bump docker base image to Debian Bullseye

[GitBox <gi...@apache.org>]

mike-jumper commented on a change in pull request #348:
URL: https://github.com/apache/guacamole-server/pull/348#discussion_r752819258



##########
File path: src/guacd-docker/bin/link-freerdp-plugins.sh
##########
@@ -58,26 +57,25 @@ where_is_freerdp() {
     fi
 
     echo "$PATHS"
-
 }
 
 #
 # Create symbolic links as necessary to include all given plugins within the
 # search path of FreeRDP
 #
 
-while [ -n "$1" ]; do
+GUAC_PLUGIN="$1$2"
+FREERDP_DIR="$(where_is_freerdp "$1")"
+FREERDP_PLUGIN_DIR="${FREERDP_DIR}/freerdp2"
 
-    # Determine correct install location for FreeRDP plugins
-    FREERDP_DIR="$(where_is_freerdp "$1")"
-    FREERDP_PLUGIN_DIR="${FREERDP_DIR}/freerdp2"
+for value in $GUAC_PLUGIN; do

Review comment:
       This seems a bit wonky. With `GUAC_PLUGIN` being assigned `"$1$2"`, won't `for value in $GUAC_PLUGIN` always just loop through a single value except in the case where `GUAC_PLUGIN` happens to contain whitespace, in which case it would (presumably undesirably) split that value up?

##########
File path: Dockerfile
##########
@@ -148,7 +148,8 @@ RUN apt-get update
 
 # Link FreeRDP plugins into proper path
 RUN ${PREFIX_DIR}/bin/link-freerdp-plugins.sh \
-        ${PREFIX_DIR}/lib/freerdp2/libguac*.so
+        ${PREFIX_DIR}                         \
+        /lib/freerdp2/libguac*.so

Review comment:
       The guacamole-server build automatically locates the proper directory for FreeRDP plugins and installs its plugins there. Perhaps it would be better to do away with `link-freerdp-plugins.sh` entirely and avoid this problem going forward?

##########
File path: Dockerfile
##########
@@ -22,7 +22,7 @@
 #
 
 # The Debian image that should be used as the basis for the guacd image
-ARG DEBIAN_BASE_IMAGE=buster-slim
+ARG DEBIAN_BASE_IMAGE=bullseye-slim

Review comment:
       Rather than referencing bullseye directly for `DEBIAN_BASE_IMAGE` and `DEBIAN_RELEASE`, would it be better to reference Debian stable? I'd think that would allow the nightly rebuilds of the image to pick up on distro updates even if there has not been a corresponding Guacamole update.




-- 
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.

To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org