Skip to content

fix: update dockerfile and requirements.txt to bring in line with ind… #14

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

Open
wants to merge 10 commits into
base: pywb2
Choose a base branch
from

Conversation

jt55401
Copy link

@jt55401 jt55401 commented Jan 21, 2025

  • bring python in line with production (3.8.10)
  • modify docker command to match production (use uwsgi)
  • add formal volume statement for collections in dockerfile
  • update requirements.txt so everything works when checked out
  • remove install_collections from docker (it should be run on host collections directory, and bind-mounted to container)

@jt55401 jt55401 self-assigned this Jan 21, 2025
Copy link
Member

@wumpus wumpus left a comment

Choose a reason for hiding this comment

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

Seems like awscli is installed twice, once by apt and once by pip?

@jt55401
Copy link
Author

jt55401 commented Jan 24, 2025

Seems like awscli is installed twice, once by apt and once by pip?

Well, sort of - I assume the one in requirements.txt is to support folks not using docker/installing directly on local. (That was there before my change - I just had un-commented the requirements.txt one inadvertantly)

Let me try quick to confirm we can remove the docker one, and be covered only by the requirements.txt for both cases.

@jt55401
Copy link
Author

jt55401 commented Jan 24, 2025

A few things @wumpus

bottom line up front - I actually think we should remove that install-collections script from docker build all together.

details:

  1. installing awscli via python only does work
    1. There are some small issues with OS level depedencies (for example, aws ... help doesn't work)
  2. no matter which way we install awscli, that install script doesn't work anymore, since we've disabled anon access to s3, and, if we pass ENV vars or build args to docker at build time, the variable values are stored in the image, which, for credentials, is a no-no.
  3. we can keep the script local, and have collections provided by volume mapping, like the private index server project (I have yet to check in) - IMHO, this is better in almost every way. (it's usually a bad idea to put a lot of actual data in the docker image)

…i from docker and requirements.txt - data doesn't belong directly in docker image
@jt55401
Copy link
Author

jt55401 commented Jan 24, 2025

@wumpus I updated Readme, requirements, and docker to be in alignment with that POV explained in last comment.

@wumpus
Copy link
Member

wumpus commented Jan 24, 2025

@sebastian-nagel looking forward to your comments!

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