Improve rb-site's method of checking the site directory on install.

Review Request #11443 — Created Feb. 11, 2021 and submitted

Information

Review Board
release-3.0.x

Reviewers

Before walking a user through the installation process, rb-site checks
to make sure the directory can be written to and that it's free of
files. The way it would historically do this would be to try to remove
the directory, since installation would re-create it. However, this
approach isn't particularly great, for a few reasons:

1) It might have been pre-created with proper ownership, which the
removal would undo.

2) If that directory was a mounted filesystem, it couldn't just remove
it.

3) When things went wrong, our error message just had to guess at the
cause.

This change redoes the logic, explicitly checking first if the path
exists. If it does, it checks for existing files, and bails with a
helpful error if it finds any. If no files exist, it tries to write a
temporary file to it to check permissions.

If the path does not exist, it then tries to create and then remove it,
checking permissions on the parent directory.

This is far safer, and gives more flexibility when running in a Docker
image or installing to a top-level NFS mount.

Successfully installed in a brand-new directory.

Successfully installed in an empty directory.

Tried installing in a directory with files, and saw a suitable error.

Tried installing in an unwritable empty directory, and saw a suitable error.

Tried installing to a brand-new directory that couldn't be created, and saw
a suitable error.

Summary ID
Improve rb-site's method of checking the site directory on install.
Before walking a user through the installation process, `rb-site` checks to make sure the directory can be written to and that it's free of files. The way it would historically do this would be to try to remove the directory, since installation would re-create it. However, this approach isn't particularly great, for a few reasons: 1) It might have been pre-created with proper ownership, which the removal would undo. 2) If that directory was a mounted filesystem, it couldn't just remove it. 3) When things went wrong, our error message just had to guess at the cause. This change redoes the logic, explicitly checking first if the path exists. If it does, it checks for existing files, and bails with a helpful error if it finds any. If no files exist, it tries to write a temporary file to it to check permissions. If the path does not exist, it then tries to create and then remove it, checking permissions on the parent directory. This is far safer, and gives more flexibility when running in a Docker image or installing to a top-level NFS mount.
84cb8a1d2a59cdc112f685ce87bf7b2c67df665f
Description From Last Updated

F841 local variable 'can_create' is assigned to but never used

reviewbotreviewbot

F841 local variable 'fp' is assigned to but never used

reviewbotreviewbot
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

chipx86
david
  1. Ship It!
  2. 
      
chipx86
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (d7502be)
Loading...