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

feat: unified osutil.Mkdir API with options #418

Merged
merged 13 commits into from
Jun 5, 2024

Conversation

IronCore864
Copy link
Contributor

@IronCore864 IronCore864 commented May 27, 2024

Implement a unified osutil.Mkdir API, refactor existing osutil.MkdirChown and osutil.MkdirAllChown.

Closes #372.

Background

There is an issue (372) where umask settings would affect the permissions of the newly created directories.

There was originally a PR to implement the feature (see here), but the code became even more complicated. As per our discussion (see here), we decide to refactor Mkdir and implement the required feature.

The original PR was closed, using this one instead. This PR only refactors the existing code, the support for chmod will come in a separate PR.

Spec

See here.

API

func Mkdir(path string, perm os.FileMode, options *MkdirOptions) error { ... }

Options

type MkdirOptions struct {
    // If false (default), a missing parent raises an error.
    // If true, any missing parents of this path are created as needed.
    MakeParents bool

    // If false (default), an error is raised if the target directory already exists. In case MakeParents is true but ExistOK is false, an error won't be raised if the parent directory already exists but the target directory doesn't.
    // If true, an error won't be raised unless the given path already exists in the file system and isn't a directory (same behaviour as the POSIX mkdir -p command).
    ExistOK bool

    // If false (default), no explicit chmod is performed. In this case, the permission of the created directories will be affected by umask settings.
    // If true, perform an explicit chmod on any directories created.
    Chmod bool

    // If false (default), no explicit chmod is performed. In this case, the permission of the created directories will be affected by umask settings.
    // If true, perform an explicit chmod on any directories created.
    Chown bool // NOTE: additional bool is needed because 0 is a valid value for UserID and GroupID (root)

    UserID sys.UserID

    GroupID sys.GroupID
}

Manual Tests

Tests Before After
mkdir ~/normal 755, same user/group as pebble, exist not ok same behaviour
mkdir -p ~/nested/folder; mkdir -p ~/nested1 755, parent/child both the same user/group as pebble, exist ok same behaviour
mkdir ~/normal-chown --user test --group test 755, user/group changed exist not ok, error: rename /root/normal-chown.mkdir-new /root/normal-chown: file exists same behaviour, better error msg: error: mkdir /root/normal-chown: file exists
mkdir -p ~/nested1/folder-chown --user test --group test 755, parent/child user/group both changed, exist OK same behaviour

@IronCore864 IronCore864 requested a review from benhoyt May 27, 2024 07:46
@IronCore864 IronCore864 marked this pull request as ready for review May 27, 2024 07:51
Copy link
Contributor

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

This is a good start, thanks.

internals/cli/cmd_warnings.go Outdated Show resolved Hide resolved
internals/cli/cmd_warnings.go Outdated Show resolved Hide resolved
internals/daemon/api_files.go Outdated Show resolved Hide resolved
internals/daemon/api_files.go Outdated Show resolved Hide resolved
internals/daemon/api_files.go Show resolved Hide resolved
internals/osutil/mkdir_test.go Outdated Show resolved Hide resolved
internals/osutil/mkdir_test.go Outdated Show resolved Hide resolved
internals/osutil/mkdir_test.go Outdated Show resolved Hide resolved
internals/osutil/mkdir_test.go Outdated Show resolved Hide resolved
internals/osutil/mkdir_test.go Outdated Show resolved Hide resolved
@IronCore864 IronCore864 requested a review from benhoyt May 28, 2024 11:49
Copy link
Contributor

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

Just a few further comments.

internals/daemon/api_files.go Outdated Show resolved Hide resolved
internals/daemon/api_files_test.go Show resolved Hide resolved
internals/daemon/api_files_test.go Show resolved Hide resolved
internals/osutil/mkdir.go Outdated Show resolved Hide resolved
@IronCore864 IronCore864 requested a review from benhoyt June 3, 2024 23:58
@IronCore864
Copy link
Contributor Author

Manual test results:

Tests Before After
mkdir ~/normal 755, same user/group as pebble, exist not ok same behaviour
mkdir -p ~/nested/folder; mkdir -p ~/nested1 755, parent/child both the same user/group as pebble, exist ok same behaviour
mkdir ~/normal-chown --user test --group test 755, user/group changed exist not ok, error: rename /root/normal-chown.mkdir-new /root/normal-chown: file exists same behaviour, better error msg: error: mkdir /root/normal-chown: file exists
mkdir -p ~/nested1/folder-chown --user test --group test 755, parent/child user/group both changed, exist OK same behaviour

Copy link
Contributor

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

Looking good now, and thanks for the manual tests + table.

@benhoyt benhoyt merged commit b8b1de7 into canonical:master Jun 5, 2024
16 checks passed
@IronCore864 IronCore864 deleted the unified-mkdir-with-options branch November 14, 2024 10:17
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.

pebble make-dirs permission is masked by pebble daemon's umask
2 participants