Skip to content

Conversation

@emrothenberg
Copy link

Added docker support to installation. Need to run setup or quickstart with --docker flag to enable.

Additional suggestions before publishing:

  1. Remove container script must be updated to account for docker
  2. Incorporate the feature in README in the desired format.

Additional fix:

  • Waiting for app to stop rather than arbitrarily killing xfreerdp after 30 seconds.
    Please also feel free to test and let me know if anything needs changing :)

@eylenburg
Copy link
Owner

Hi, thanks for the PR. I'm not opposed to adding Docker support, but I want to make sure that nothing breaks for Podman users. A few questions:

  1. Why this check removed from linoffice.sh?
if [[ -x "/usr/bin/podman-compose" ]]; then
    COMPOSE_COMMAND="/usr/bin/podman-compose"
    echo "Using system podman-compose from /usr/bin/"
elif command -v podman-compose &>/dev/null; then
    COMPOSE_COMMAND="podman-compose"
    echo "Using podman-compose from PATH""

and replaced with:

if ! command -v "$COMPOSE_COMMAND" &>/dev/null; then

The reason for the original code was that while I was experimenting with the quickstart script, it then wanted to use a version of podman-compose installed by pip that didn't actually work for whatever reason, while my system podman-compose was available and working. So this was meant to be defaulting to the system version if available as this should generally be preferable to use.

  1. Why did you remove these things from compose.yaml:
            sed -i 's/:Z//g; s/:z//g' "$COMPOSE_FILE"
            sed -i '/^[[:space:]]*annotations:/,/^[[:space:]]*[^[:space:]]/{
/^[[:space:]]*annotations:/d
/^[[:space:]]*run\.oci\.keep_original_groups:/d
}' "$COMPOSE_FILE"
            sed -i '/^[[:space:]]*group_add:/,/^[[:space:]]*[^[:space:]]/{
/^[[:space:]]*group_add:/d
/^[[:space:]]*-[[:space:]]*keep-groups/d

If these are not necessary for Docker, can this be removed only if the engine is set to Docker? Some user had problems with SELinux errors without the :z labels, and the keep-groups stuff is needed for rootless podman.

  1. What's the rationale behind: Waiting for app to stop rather than arbitrarily killing xfreerdp after 30 seconds? (in the freerdp process cleanup section in in linoffice.sh) What if a FreeRDP process hangs?

  2. Can you remove the engine.txt for the PR because by default it should still use podman?

  3. Would you be able to implement removing the docker container and its volume in the uninstall scripts (uninstall.sh and gui/installer/remove_container.sh) in the same way it's done for podman?

  4. Finally is there anything else you think could affect Podman users?

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.

2 participants