fix(prune) respect dir permissions during prune #2066
Conversation
|
@chris-olszewski is attempting to deploy a commit to the Vercel Team on Vercel. A member of the Team first needs to authorize it. |
cli/internal/fs/copy_file.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| return os.MkdirAll(dest, mode) | ||
| } | ||
| // name is absolute, (originates from godirwalk) |
There was a problem hiding this comment.
I think this comment should move up to where src is allocated.
cli/internal/prune/prune.go
Outdated
| @@ -146,10 +146,16 @@ func (p *prune) prune(opts *opts) error { | |||
| continue | |||
| } | |||
| workspaces = append(workspaces, ctx.PackageInfos[internalDep].Dir) | |||
| originalDir := p.base.RepoRoot.UntypedJoin(ctx.PackageInfos[internalDep].Dir.ToStringDuringMigration()) | |||
There was a problem hiding this comment.
I think this is supposed to be ctx.PackageInfos[internalDep].Dir.RestoreAnchor(p.base.RepoRoot)?
@nathanhammond what do you think about also including a method on AbsoluteSystemPath that is essentially Join but accepts a single AnchoredSystemPath instance?
At any rate, this shouldn't be Untyped, we have the types here for both pieces of the path.
There was a problem hiding this comment.
Ah, thanks for pointing that out. I missed RestoreAnchor
There was a problem hiding this comment.
@gsoltis @chris-olszewski I'm game for that method; I just couldn't find a name for it that I was happy with so I didn't create it.
I agree that it isn't intuitive to start with the Anchored path and prepend the Absolute one.
cli/internal/prune/prune.go
Outdated
| targetDir := fullDir.UntypedJoin(ctx.PackageInfos[internalDep].Dir.ToStringDuringMigration()) | ||
| if err := targetDir.EnsureDir(); err != nil { | ||
| return errors.Wrapf(err, "failed to create folder %v for %v", targetDir, internalDep) | ||
| if err = targetDir.MkdirAllMode(info.Mode()); err != nil { |
There was a problem hiding this comment.
One advantage of this particular pattern is to scope a new err. By using if err := ..., you shadow any existing err, and don't leak this err beyond the if block. It's a common enough pattern that I would say you probably don't want to use it if you aren't using :=, or at least not without a comment explaining why.
In this case, you aren't using the value of err beyond this block so I would switch to :=
| return os.Chmod(p.ToString(), mode) | ||
| } else { | ||
| // Path exists as file, remove it | ||
| if err = p.Remove(); err != nil { |
| } | ||
| } | ||
| } | ||
| if err = os.MkdirAll(p.ToString(), mode); err != nil { |
|
Just notice this part of os.Chmod:
Which explains the failing windows tests |
Fixes #1872
This PR ended up having larger scope than I originally intended. A few things that changed:
RecursiveCopyFilenow attempts to copy directories with the correct permissionsMkdirAllModewhich is similar toMkdirAllbut it will ensure that the created directory has the permissions passedMkdirAllModeinstead ofEnsureDirInteraction with umask
Currently we don't consider
umaskwhen creating files. When copying files that we get from users we should make sure that the permissions are kept, but when creating files for turbo itself, we should probably respectumask.When copying user files and we want to make sure permissions remain the same, the standard solution is to
chmodafter creating a new dir/file. This would add an additional syscall to all of the file/directory copy methods which seems like a little much. Another option would be to blow awayumaskat the beginning to ensure that we are creating files with the permissions we expect.Testing
Wrote some unit tests, also did a quick spot check that dir permissions are copied over