Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make kiwix-serve easily deployable on podman Openshift #581

Merged
merged 3 commits into from
Oct 26, 2022

Conversation

neyder
Copy link
Contributor

@neyder neyder commented Oct 23, 2022

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.

@kelson42 kelson42 requested a review from rgaudin October 24, 2022 05:25
@kelson42 kelson42 added this to the 3.3.1 milestone Oct 24, 2022
Copy link
Member

@rgaudin rgaudin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

docker/server/README.md Outdated Show resolved Hide resolved
@neyder
Copy link
Contributor Author

neyder commented Oct 24, 2022 via email

@rgaudin
Copy link
Member

rgaudin commented Oct 24, 2022

Oh yes, you're right ; my bad.

@rgaudin
Copy link
Member

rgaudin commented Oct 24, 2022

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
Copy link
Contributor

@neyder Please fix the README.md

@neyder
Copy link
Contributor Author

neyder commented Oct 24, 2022

@kelson42 fixed.

@kelson42
Copy link
Contributor

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

@rgaudin
Copy link
Member

rgaudin commented Oct 26, 2022

@kelson42 FYI issue is from previous code that was changed

@kelson42 kelson42 merged commit 4137d9f into kiwix:master Oct 26, 2022
mgautierfr added a commit that referenced this pull request Nov 30, 2022
 * Remove last reference to kiwix-read tool (@legoktm #569)

kiwix-serve
-----------

 * Fix broken indentation in usage (@kelson42 #560)
 * Exit if wrong arguments are passed (@kelson42 #567)
 * Do not allow multiple values for same option (@juuz0 #564)
 * Fix default location of "rootLocation" (@rgaudin #571)
 * [DOCKER] Change default port to 8080 (@neyder #581)
 * [DOCKER] Simplify dockerfile (@rgaudin #582)

kiwix-manage
------------

 * Fix man page (@kelson42 #576)
@mgautierfr mgautierfr mentioned this pull request Nov 30, 2022
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.

3 participants