search

e-valuation / EvaP

a university course evaluation system written in Python using Django
Other
95 stars 146 forks source link

only remount if REPO_FOLDER does not contain any items #2242

Closed fekoch closed 3 weeks ago

fekoch commented 4 weeks ago

This allows users to rerun vagrant provision without a failure, if the folder is already mounted

richardebeling commented 3 weeks ago

My mental model was that bindfs will print an error (something like "mountpoint not empty") and exit immediately if the destination directory contains any files. This PR then just suppresses the error message, but doesn't fix any actual failure (because there is none), right?

I'm wondering if it makes sense to keep the error message. vagrant provision is noisy anyway. If stuff breaks because the mountpoint is not empty with the wrong files (some leftover files in there, wrong source dir mounted, idk), the error message might actually help finding that problem.

fekoch commented 3 weeks ago

The issue is that the mounting is run as part of provisioning the VM. I believe it sometimes makes sense to run vagrant provision to ensure all software is up to date. In that case it is expected that the folder contains files because it is already mounted and should not be remounted.

Another idea is that we could, if already mounted, unmount the folder before mounting it.

richardebeling commented 3 weeks ago

The issue is that the mounting is run as part of provisioning the VM. I believe it sometimes makes sense to run vagrant provision to ensure all software is up to date. In that case it is expected that the folder contains files because it is already mounted and should not be remounted.

What I was trying to say is: On main, there is no problem blocking the remaining provision operations, right? The bindfs call will print an error and exit, but since the folder is already mounted, all following commands work fine.

I might be missing something here, but from my current understanding, the change here only suppesses an error message output to the console, but doesn't change any other part of the behavior. I'm thinking that we'd rather want to keep the error message, so that if something is unexpected with the mounting point, we will be hinted towards that.

Edit: Sorry, I was mistaken about this, I thought our shell script line had a || true suffix, but we were doing || exit 1, which obviously is different. We can proceed as discussed earlier and as written below.

fekoch commented 3 weeks ago

It would change the behaviour. Currently, if one runs vagrant provision and the folder is already mounted, the provision script will not move past the mounting-attempt. It exits there.

I was thinking that it would be useful to continue with the provision script (eg. to make sure all dependencies are up to date, all migrations are run, ...) even if the folder is already mounted.