Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR updates Docker configuration for the DSpace Angular application, focusing on setting up proper directory structure and modifying build workflows. The changes appear to prepare the distribution Docker image for production use while updating the development workflow configuration.
Key changes:
- Added directory creation and permissions setup in the distribution Dockerfile
- Updated GitHub workflow configuration to enable the distribution build and modify naming conventions
- Removed the "-dist" suffix from tags and enabled the previously disabled distribution job
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Dockerfile.dist | Added RUN command to create assets directory structure with proper permissions |
| .github/workflows/docker.yml | Updated build IDs, image names, enabled dist job, and modified tagging strategy |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
milanmajchrak
force-pushed
the
customer/palo-docker
branch
from
f5a9299 to
eee7b46
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
vidiecan
force-pushed
the
customer/palo-docker
branch
from
48e3e46 to
5912619
Compare
permissions fix
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 1 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Dockerfile.dist
Outdated
| RUN mkdir -p /app/dist/browser/assets \ | ||
| && touch /app/dist/browser/assets/config.json \ | ||
| && chown -R node:node /app/dist/browser/assets \ | ||
| && find /app/dist/browser/assets -type d -exec chmod 755 {} \; \ |
There was a problem hiding this comment.
why is this required if USER is node and you changed chown above
There was a problem hiding this comment.
If I move the run after switching user it is possibly redundant to set chmod but it cannot break anything and at least we have explicit permissions... I believe it cannot break anything but it will most likely work even without it so I can remove it if you want.
remove reduntant ownership modification
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 1 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Dockerfile.dist
Outdated
| RUN mkdir -p /app/dist/browser/assets \ | ||
| && touch /app/dist/browser/assets/config.json \ | ||
| && find /app/dist/browser/assets -type d -exec chmod 755 {} \; \ | ||
| && find /app/dist/browser/assets -type f -exec chmod 644 {} \; |
ensure that node has proper permissions
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 1 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
remove redundant chmod commands
…v/dspace-angular into palo-docker-permissions-fix
Palo docker permissions fix
creating config.json is unnecesary???
4 participants
Problem description
Reported issues
Not-reported issues
Analysis
(Write here, if there is needed describe some specific problem. Erase it, when it is not needed.)
Problems
(Write here, if some unexpected problems occur during solving issues. Erase it, when it is not needed.)