Skip to content

fix: remove install directory for version when install fails #2024

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.

Sign up for GitHub

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

Merged
Stratus3D merged 1 commit into from
Mar 19, 2025
Merged

fix: remove install directory for version when install fails #2024

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions internal/versions/versions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,9 @@ func InstallOneVersion(conf config.Config, plugin plugins.Plugin, versionStr str

err = plugin.RunCallback("install", []string{}, env, stdOut, stdErr)
if err != nil {
if rmErr := os.RemoveAll(installDir); rmErr != nil {
fmt.Fprintf(stdErr, "failed to clean up '%s' due to %s\n", installDir, rmErr)
}
return fmt.Errorf("failed to run install callback: %w", err)
}

Expand Down
20 changes: 20 additions & 0 deletions internal/versions/versions_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,26 @@ func TestInstallOneVersion(t *testing.T) {
assert.True(t, pathInfo.IsDir())
})

t.Run("deletes install directory when install fails", func(t *testing.T) {
conf, plugin := generateConfig(t)
stdout, stderr := buildOutputs()

installScript := filepath.Join(conf.DataDir, "plugins", plugin.Name, "bin", "install")
f, err := os.OpenFile(installScript, os.O_APPEND|os.O_WRONLY|os.O_CREATE, 0777)
assert.Nil(t, err)
_, err = f.WriteString("\nexit 1")
Copy link
Copy Markdown
Member

@Stratus3D Stratus3D Mar 19, 2025

Choose a reason for hiding this comment

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

I thought shebangs were required here. I guess they aren't since the file is run with Bash either way. I wonder if we should include shebangs in the test in case we choose to run them a different way. I imagine we might want to support other scripts besides Bash in the future.

Copy link
Copy Markdown
Contributor Author

@andrecloutier andrecloutier Mar 19, 2025

Choose a reason for hiding this comment

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

This is a bit of a hack in that it's appending this content to the end of the existing install script. I didn't think it was worth making a whole separate fixture for, so went with this approach to get it to "fail".

assert.Nil(t, err)
err = f.Close()
assert.Nil(t, err)

err = InstallOneVersion(conf, plugin, "1.0.0", false, &stdout, &stderr)
assert.Errorf(t, err, "failed to run install callback: exit status 1")

installPath := filepath.Join(conf.DataDir, "installs", plugin.Name, "1.0.0")
_, err = os.Stat(installPath)
assert.True(t, os.IsNotExist(err))
})

t.Run("runs pre-download, pre-install and post-install hooks when installation successful", func(t *testing.T) {
conf, plugin := generateConfig(t)
stdout, stderr := buildOutputs()
Expand Down