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

Cloudfuse service linux cmd #338

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from
Draft

Conversation

Dabnsky
Copy link
Contributor

@Dabnsky Dabnsky commented Oct 10, 2024

What type of Pull Request is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

Describe your changes in brief

This adds the following commands available to the cloudfuse executable:
cloudfuse service install
cloudfuse service uninstall

Checklist

  • Tested locally
  • Added new dependencies
  • Updated documentation
  • Added tests

Related Issues

  • Related Issue #
  • Closes #

@foodprocessor foodprocessor removed the request for review from brayan-trejo October 31, 2024 17:52
@Dabnsky Dabnsky marked this pull request as ready for review November 1, 2024 17:32
Comment on lines 63 to 64
Short: "Installs the startup process for Cloudfuse. Requires elevated permissions.",
Long: "Installs the startup process for Cloudfuse which remounts any active previously active mounts on startup. elevated permissions.",
Copy link
Contributor

Choose a reason for hiding this comment

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

These descriptions should probably not refer to "the" startup process, since each call to install only affects a single mount, and we can use this command to make multiple "startup processes."

}

var installCmd = &cobra.Command{
Use: "install",
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you require two arguments, the usage string should include those.

Comment on lines 79 to 83
// assumes dir is in cloudfuse repo dir
serviceData, err := collectServiceData(fmt.Sprintf("%s/setup/cloudfuse.service", dir))
if err != nil {
return fmt.Errorf("error collecting data from cloudfuse.service file due to the following error: [%s]", err)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

For deployment, we need to add the cloudfuse.service file to the list of deployed files (in .goreleaser.yaml), and something like this should work. We may need to use filepath.Dir(os.Executable()) (documented here), instead of os.Getwd() to reliably get the path we need.

Comment on lines +94 to +98
isDirEmpty := common.IsDirectoryEmpty(mountPath)
if !isDirEmpty {
return fmt.Errorf("error, the mount path provided is not empty")
// TODO: add useage output upon failure with input
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to check this.

Copy link
Contributor Author

@Dabnsky Dabnsky Nov 13, 2024

Choose a reason for hiding this comment

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

cloudfuse outputs an error and will not mount if the provided mount path is not empty

// TODO: add useage output upon failure with input
}

configExists := common.DirectoryExists(configPath)
Copy link
Contributor

Choose a reason for hiding this comment

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

Although this works, I don't love that DirectoryExists is checking existence for a file.
I found another place where we check if the config file exists, and we use os.Stat directly:

if _, err := os.Stat(options.ConfigFile); errors.Is(err, fs.ErrNotExist) {
	return errors.New("config file does not exist")
}

},
}

var uninstallCmd = &cobra.Command{
Copy link
Contributor

Choose a reason for hiding this comment

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

The uninstall command will have to require the mount path to know which mount's startup service we need to remove.

Comment on lines 105 to 113
err = modifySericeFile(mountPath, dir)
if err != nil {
return fmt.Errorf("error when attempting to write defaults into service file: [%s]", err.Error())
}

err = modifySericeFile(configPath, dir)
if err != nil {
return fmt.Errorf("error when attempting to write defaults into service file: [%s]", err.Error())
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're creating a separate service file for each mount that the user wants to mount on startup, we'll want to

  • not change our template cloudfuse.service file (we should write the modified unit file data straight to the destination)
  • name each new service/unit file after the mount path, so they don't conflict, and so we can find them to remove them when uninstalling

Comment on lines 115 to 118
// 2. retrieve the newUser account from cloudfuse.service file and create it if it doesn't exist

serviceUser := serviceData["User"]
setUser(serviceUser)
Copy link
Contributor

Choose a reason for hiding this comment

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

On Linux I don't think we need a dedicated user.


//--------------- command section ends

func collectServiceData(serviceFilePath string) (map[string]string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know, but I feel like these helper functions could be a lot shorter, and more readable. I feel like these details should be abstracted away by a standard library function that does most or all of this for us...

@Dabnsky Dabnsky marked this pull request as draft November 8, 2024 21:12
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.

2 participants