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

only remount if REPO_FOLDER does not contain any items #2242

Merged
merged 2 commits into from
Jul 15, 2024

Conversation

fekoch
Copy link
Collaborator

@fekoch fekoch commented Jul 8, 2024

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

Copy link
Collaborator

@Kakadus Kakadus left a comment

Choose a reason for hiding this comment

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

thanks!

@richardebeling
Copy link
Member

richardebeling commented Jul 9, 2024

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
Copy link
Collaborator Author

fekoch commented Jul 9, 2024

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
Copy link
Member

richardebeling commented Jul 9, 2024

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
Copy link
Collaborator Author

fekoch commented Jul 15, 2024

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.

@fekoch fekoch force-pushed the only-remount-when-not-mounted branch from 4197cff to 113febf Compare July 15, 2024 18:58
@Kakadus Kakadus merged commit 1870239 into e-valuation:main Jul 15, 2024
12 checks passed
@fekoch fekoch deleted the only-remount-when-not-mounted branch July 16, 2024 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants