kiwix / kiwix-tools

Command line Kiwix tools: kiwix-serve, kiwix-manage, ...
https://download.kiwix.org/release/kiwix-tools/
GNU General Public License v3.0
408 stars 79 forks source link

Make kiwix-serve easily deployable on podman Openshift #581

Closed neyder closed 1 year ago

neyder commented 1 year ago

With default port set to 80, it is not posible to run in user-mode, so added PORT enviroment and defaulted to 8080.

This make kiwix-serve container to be easily deployed on podman or even on OpenShift environment.

neyder commented 1 year ago

Greetings Kiwix team,

I'm sorry if it was rude to make a PR without a ticket. My bad.

In regard of modification, you are right, but this needs some clarification:

I think this is only one thing: "Simplify running kiwix-server container on non-root environments, that means changing default port 80 to another like non-root's 8080"

Also, this change is just for kiwix-serve container inside /docker/server , this change makes container's default exposed port to 8080 and is possible to change PORT to another end-user defined value if needed.

I will read more about ticketing on kiwix developer page.

Love, and thanks for your magnificent work on kiwix!

On Mon, Oct 24, 2022 at 4:15 AM rgaudin @.***> wrote:

@.**** requested changes on this pull request.

I am not comfortable with this PR as it is linked to no ticket and tries to tackle two different things:

  • simplify running non-root kiwix-serve containers
  • change default kiwix-serve port

I am not against changing the default kiwix-serve port to a userland-mapped one but this is a cli-api change that needs to be discussed in a ticket and agreed-upon.

It is not required to achieve the second part. You can pass --port 8080 as Pod command and get the same result.

Also, you changed the docker/server files but not the Dockerfile and README of the docker/ folder for kiwix-tools image. The EXPOSE line is now incorrect.

I am not sure what the way forward should be. Maybe open a ticket about default port?

In docker/server/README.md https://github.com/kiwix/kiwix-tools/pull/581#discussion_r1003047445:

With remote ZIM file


-docker run -e "DOWNLOAD=https://download.kiwix.org/zim/wikipedia_bm_all.zim" -p 8080:80 kiwix/kiwix-serve
+docker run -e "DOWNLOAD=https://download.kiwix.org/zim/wikipedia_bm_all.zim" -p 8080:8080 kiwix/kiwix-serve
+```
+
+Change default port
+-------------------
+
+You can change port to expose with environment PORT, useful if running on Podman, K8s or OpenShift

s/environment/environment variable/

—
Reply to this email directly, view it on GitHub
<https://github.com/kiwix/kiwix-tools/pull/581#pullrequestreview-1152767959>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAN7MCMHE6CUCHRRZVEBGELWEZHTFANCNFSM6AAAAAARMKMIYA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>

-- Neyder Achahuanco Apaza "Educación, Cultura y Tecnología"

rgaudin commented 1 year ago

Oh yes, you're right ; my bad.

rgaudin commented 1 year ago

In this case, this is fine but since it has limited value (adding --port 8080 as command is as easy) and it changes our default (adding discrepancy with kiwix-tools), I'll ask @kelson42 to make the call.

Thank you anyway @neyder and sorry for the confusion earlier 😅

kelson42 commented 1 year ago

@neyder Please fix the README.md

neyder commented 1 year ago

@kelson42 fixed.

kelson42 commented 1 year ago

@neyder Last point: please fix the codefactor issue and we are good

rgaudin commented 1 year ago

@kelson42 FYI issue is from previous code that was changed