Skip to content
This repository has been archived by the owner on Jan 8, 2024. It is now read-only.

fix: #495 and #488 #496

Merged
merged 3 commits into from
Mar 24, 2023
Merged

Conversation

tobybatch
Copy link
Owner

No description provided.

@tobybatch tobybatch marked this pull request as ready for review March 23, 2023 13:05
@Fernien
Copy link
Collaborator

Fernien commented Mar 23, 2023

It fixes the PHP memory problem.
But i'm having a strange effect, not to sure if it comes from the container or me. When i stop and start the container it changes permissions of my mapped folder for /opt/kimai/var and /opt/kimai/var/plugins to the user with id and gid 1000. The Container isnt starting properly. After changing it to www-data:www-data on the host it runs instantly.

@tobybatch
Copy link
Owner Author

Let me check that. There used to be a chown in the script, which I've abandoned, let the container owners manage file perms.

@tobybatch
Copy link
Owner Author

The repo does a chown (that I can see) at build time. Odd. I'll re-test too.

✔  ~/usr/kimai/kimai2 [ 495/_bug__memory_limit_is_not_set_at_runtime L | ✔  ] $ grep --exclude-dir=node_modules -r chown .
./Dockerfile:    chown -R www-data:www-data /composer
./Dockerfile:COPY --from=git-dev --chown=www-data:www-data /opt/kimai /opt/kimai
./Dockerfile:    chown -R www-data:www-data /opt/kimai /usr/local/etc/php/php.ini && \
./Dockerfile:COPY --from=git-prod --chown=www-data:www-data /opt/kimai /opt/kimai
./Dockerfile:    chown -R www-data:www-data /opt/kimai /usr/local/etc/php/php.ini && \
./docs/troubleshooting.md:docker exec --user root CONTAINER_NAME chown -R www-data:www-data /opt/kimai/var
./docs/troubleshooting.md:docker-compose --user root exec CONTAINER_NAME chown -R www-data:www-data /opt/kimai/var

@Fernien
Copy link
Collaborator

Fernien commented Mar 23, 2023

Alright! Let me know when i can help with anything

@tobybatch
Copy link
Owner Author

tobybatch commented Mar 23, 2023

@Fernien see 6907e45

OK, so I now default to www-data:www-data unless a UID/GID are passed. I think this will mean that existing set ups (like yours) should stay as www-data and new set ups can pass USER_ID and GROUP_ID and chown to those file.

I will get a subset of users for whom this will break things but it's the best fit.

@kevinpapst
Copy link
Collaborator

Why all this permission magic? This seems like an invitation for problems.
Can't this run with default permissions?
You could provide a documentation page with ideas how to achieve that permission setup, but leave it to the user if they have special needs.

Are there any best practices or docker examples out there, which include examples of what you are trying to solve?

@tobybatch
Copy link
Owner Author

Yes and no.

  1. It's the most asked for enhancement
  2. Many other dockers offer it, it solves the problem of sharing hosted folders into the container rather than using volumes
  3. I'm writing up the docker right now
  4. It will run it did by default

@kevinpapst
Copy link
Collaborator

All right, you are the maintainer 👍 I just receive all the email notifications and get the impression that it makes more problems than it solves, but then no wonder with all these auto-updates. When exactly did the IT stop testing? I must have missed that trend... 😁

@tstrohmeier
Copy link
Contributor

tstrohmeier commented Mar 23, 2023

@kevinpapst Good point with the testing!
But it has nothing to do with auto-update when you pin to a version and just redeploy the same version and everthing breaks. In a software supply chain you should be able to trust that "versions" are not changing over time.

@tobybatch It would be helpful to have some basic "test" for this repo. At least running the docker compose / run in the GitHub Workflow to see if the demo setups are breaking. This could have avoided some of the issues this week.

@tobybatch
Copy link
Owner Author

tobybatch commented Mar 23, 2023

@kevinpapst Good point with the testing! But it has nothing to do with auto-update when you pin to a version and just redeploy the same version and everthing breaks. In a software supply chain you should be able to trust that "versions" are not changing over time.

@tobybatch It would be helpful to have some basic "test" for this repo. At least running the docker compose / run in the GitHub Workflow to see if the demo setups break. This could have avoided some of the issues this week.

I'm adding tests at the moment. It dropped when we went to the GH CI.

I'm baffled by the older version not working as we don't re-build old version, only new ones. Previous images are not touched. The exception to that is that if you run a newer version then the DB migrations are run. That's why we advise you back up your DB before upgrading, https://github.com/tobybatch/kimai2/blob/main/docs/updating.md?plain=1#L5

The exception of this was when the pre-release 2.x versions were tagged then the latest version started picking that up. All my local tests passed as we tear up and drop the DB for each test. We disabled the auto builds when we realised but some mages made it out into the wild. I have a fix for that in the works too, so we can turn back on the auto builds.

@tobybatch tobybatch merged commit 0fae66b into main Mar 24, 2023
@tobybatch tobybatch deleted the 495/_bug__memory_limit_is_not_set_at_runtime branch March 24, 2023 15:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants