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

pretix-src should not be a shared volume #10

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

eMBee
Copy link

@eMBee eMBee commented May 18, 2024

this fixes the docker problems we had with the eventyay-tickets PR #76 ( fossasia/eventyay-tickets#76 )

because the src directory was a shared volume, it contained an old version of the code that was mounted on top of the updated code in the container.

we always want the code in the container.

if there is anything in src that needs to be shared, then it will have to be handled separately, ideally by moving it to data.

url must include the scheme
Copy link
Member

@norbusan norbusan left a comment

Choose a reason for hiding this comment

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

LGTM, but maybe for dev environments the /src dir should be a shared volume to enable easy development? WDYT?
Maybe a separate docker-compose-dev.yml?
Any suggestions?

@eMBee
Copy link
Author

eMBee commented May 18, 2024

how does the source get updated if it is a shared volume? it's not mounting out of a users work directory (at least it wasn't as configured). if it had been mounting a users work directory, then we would not have seen the error because the checked out code would have been used instead of an old version.

i think that the point here is to test the containers, so the dev containers should be as close as possible to production.

to test dev code locally the better change is to replace the image with build.

i apply this change locally, which allows me to rebuild the docker image after each code change:

#    image: eventyay/eventyay-tickets:development
build: ../eventyay-tickets

@@ -29,8 +29,7 @@ services:
volumes:
- ./pretix.cfg:/etc/pretix/pretix.cfg:ro
- pretix-data:/data
- pretix-src:/pretix/src


Copy link
Member

Choose a reason for hiding this comment

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

If we remove this, we shouldn't need the volume at the end either correct?

Copy link
Author

Choose a reason for hiding this comment

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

sorry, i missed this question. (for some reason i am not getting notifications on replies to my pull requests)
i don't understand what you are asking though. which volume do we not need?

in any case i am surprised that this is not merged yet. i believe this is a critical fix. without it we always risk testing the wrong code instead of the actual docker container.

@norbusan
Copy link
Member

I will update the whole repo according to what we have in eventyay-server-management repo, since this is and will be the source of truth (containing also the secrets etc).

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