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

Addressing compatibility and remove DOCKER_BUILDKIT in the Docker setup #815

Merged
merged 1 commit into from
Aug 23, 2024

Conversation

DonRichards
Copy link
Contributor

@DonRichards DonRichards commented Aug 16, 2024

Link to Github issue or other discussion

Issue #814 - Improve Dockerfile to Handle Permissions (run as non-root user) and Enhance Build Compatibility.
PR #18 - Add docs for using Dockerfile

What does this PR do?

This PR enhances the existing Dockerfile by addressing build warning issues that were confusing when running building the container. It also optimizes the build process and improves compatibility with different host environments by aligning user and group IDs inside the container with those on the host system.

What changes were made?

  • User and Group ID Handling: Introduced build arguments USER_ID and GROUP_ID to ensure the Docker container runs with the same user and group IDs as the host, preventing permission issues when mounting directories.
  • Permission Management: Added steps to create a group with the specified GROUP_ID and set proper ownership for the /workbench directory inside the container.
  • Environment Variable Update: Added the .local/bin directory to the PATH to ensure that the scripts installed by pip are accessible.
  • DOCKER_BUILDKIT=0: Removed the need for most situations.

How to test / verify this PR?

  1. Build the Docker Image: Run the following command:
    docker build --build-arg USER_ID=$(id -u) --build-arg GROUP_ID=$(id -g) -t workbench-docker .
  2. Run the Docker Container: Execute a migration task using the following command:
    docker run -it --rm --network="host" \
        -v .:/workbench \
        -v /path/to/your/tmp:/tmp \
        -v /path/to/your/files:/mnt/data/local \
        --name update_existing_objects workbench-docker \
        bash -lc "./workbench --config /workbench/prod/update_islandora_objects.yml --check"
  3. Verify: Ensure that:
    • No permission errors occur.
    • Logs are correctly written to the /workbench/workbench.log.
    • The container interacts correctly with the mounted directories.

Include any .yml configuration files and CSV data necessary for testing. Unit or integration tests should be run if applicable.

Interested Parties

@mjordan, @maintainers


Checklist

  • Before opening this PR, have you opened an issue explaining what you want to do?
  • Have you included some configuration and/or CSV files useful for testing this PR?
  • Have you written unit or integration tests if applicable?
  • Does the code added in this PR require a version of Python that is higher than the current minimum version?
  • If the changes in this PR require an additional Python library, have you included it in setup.py?
  • If the changes in this PR add a new configuration option, have you provided a default for when the option is not present in the .yml file?

@mjordan
Copy link
Owner

mjordan commented Aug 21, 2024

@DonRichards docker build --build-arg USER_ID=$(id -u) --build-arg GROUP_ID=$(id -g) -t workbench-docker . built without error, and I was able to run a Workbench task using docker run. Is it expected that docker run will only work within islandora_workbench? If so I think this is ready to merge. I tried running my docker run command outside of islandora_workbench and it didn't work - is that normal?

@DonRichards
Copy link
Contributor Author

DonRichards commented Aug 21, 2024

Yes and no. It's copying the files in (workbench and migration csv and other files upon running it).

You can replace the period in this -v .:/workbench with the workbench directory and that should be fine.

@DonRichards
Copy link
Contributor Author

I might need to add that to the docs since you've asked.

@mjordan
Copy link
Owner

mjordan commented Aug 21, 2024

Might be a good idea. I assumed that the image could be used from anywhere, in other words that it was portable and even sharable. I guess one overall question that might need to be answered in the docs is why someone want to use a Docker image for Workbench in the first place, if not to avoid local installs of Python and the required libraries.

@mjordan
Copy link
Owner

mjordan commented Aug 21, 2024

I'm a complete Docker newb so it might be useful to assume other potential users would be as well.

@mjordan
Copy link
Owner

mjordan commented Aug 22, 2024

@DonRichards Unless you have any additional changes to the Dockerfile I can merge this now and we can fix up any docs after that. Is that OK?

@DonRichards
Copy link
Contributor Author

I don't have any changes for the dockerfile, yeah. Just the docs need to be updated.

@mjordan mjordan merged commit 132062b into mjordan:main Aug 23, 2024
5 checks passed
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