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

when updating config due to version change, block command on it #615

Merged

Conversation

joeshaw
Copy link
Member

@joeshaw joeshaw commented Jul 28, 2022

Normally the CLI updates the config in the background for subsequent
executions. However, if the version changes we need to get the updated
config in place before we execute the command.

Normally the CLI updates the config in the background for subsequent
executions.  However, if the version changes we need to get the updated
config in place before we execute the command.
@joeshaw
Copy link
Member Author

joeshaw commented Jul 28, 2022

An alternative approach I explored was having the fastly update command execute the new binary as a test, and rely on it writing out the config because the versions don't match.

diff --git pkg/commands/update/root.go pkg/commands/update/root.go
index d832861..a434f82 100644
--- pkg/commands/update/root.go
+++ pkg/commands/update/root.go
@@ -5,6 +5,7 @@ import (
        "fmt"
        "io"
        "os"
+       "os/exec"
        "path/filepath"

        "github.com/fastly/cli/pkg/cmd"
@@ -122,6 +123,15 @@ func (c *RootCommand) Exec(in io.Reader, out io.Writer) error {
                }
        }

+       progress.Step("Testing binary...")
+       if err := exec.Command(currentPath, "version").Run(); err != nil {
+               c.Globals.ErrLog.AddWithContext(err, map[string]interface{}{
+                       "Executable": currentPath,
+               })
+               progress.Fail()
+               return fmt.Errorf("error running new binary: %w", err)
+       }
+
        progress.Done()

        text.Success(out, "Updated %s to %s.", currentPath, latest)

The problem with this is that there's a race because the old code could write out the config file after the new code does because it's done in the background.

Copy link
Collaborator

@Integralist Integralist left a comment

Choose a reason for hiding this comment

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

LGTM. I think for these occasions (i.e. when updating to a new CLI version) it's OK to block for the config 👍🏻

@Integralist Integralist merged commit 5c7cb7a into fastly:main Jul 28, 2022
@joeshaw joeshaw deleted the joeshaw/update-config-on-version-change branch July 28, 2022 18:42
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