canonical / pebble

Take control of your internal daemons!
GNU General Public License v3.0
136 stars 51 forks source link

fix: add chmod to mkdir to ignore umask settings #405

Closed IronCore864 closed 1 month ago

IronCore864 commented 3 months ago

Add a flag so that pebble mkdir won't be affected by umask settings.

Closes https://github.com/canonical/pebble/issues/372.

benhoyt commented 2 months ago

I've looked at this more closely -- sorry for the delay. The function signatures, as well as the number of functions in the implementation, is getting unwieldy. I think we should go back to the drawing board and design a better function. How about this: make the two required arguments positional (path and perm) and put everything else in an options struct (which can be nil or the zero value for defaults):

package osutil

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

type MkdirOptions struct {
    // True to create any parent directories if they don't already exist.
    //
    // If false and path already exists, Mkdir returns an error that wraps os.ErrExist (like os.Mkdir).
    // If true and path already exists, Mkdir does nothing and returns nil (like os.MkdirAll).
    MakeParents bool

    // True to perform an explicit chmod on any directories created.
    Chmod bool

    // True to call chown on any directories created, using the user ID and group ID provided.
    Chown bool // NOTE: additional bool is needed because 0 is a valid value for UserID and GroupID (root)
    UserID sys.UserID
    GroupID sys.GroupID
}

I'm not sure about the "clever" MakeParents behaviour in the case where the path already exists. It imitates Go's Mkdir / MkdirAll quirks, but IMO that's a bit of a design flaw. Might be better to have an explicit ExistOK bool flag.

The above is somewhat modelled after https://docs.python.org/3/library/pathlib.html#pathlib.Path.mkdir

IronCore864 commented 1 month ago

Closing this PR for now, see the new PR which does the refactor and implements the feature here.