Add snapshot save command#210
Conversation
anisaoshafi
force-pushed
the
drg-764-migrate-state-commands-from-cli-v1-export
branch
4 times, most recently
from
f6bbfbf to
6a24cdf
Compare
| return fmt.Errorf("no emulator configured") | ||
| } | ||
|
|
||
| c := appConfig.Containers[0] |
There was a problem hiding this comment.
issue: this could potentially panic. Snapshots are only for aws AFAIK so we could do this instead:
awsContainer := config.ContainerConfig{Type: config.EmulatorAWS, Port: config.DefaultAWSPort}
| dest = "ls-state-export" | ||
| } else if strings.Contains(dest, "://") { | ||
| return nil, fmt.Errorf("cloud destinations are not yet supported — use a file path like ./my-snapshot") | ||
| } else if !strings.HasPrefix(dest, ".") && !strings.HasPrefix(dest, "/") && !strings.HasPrefix(dest, "~") && !strings.Contains(dest, "/") { |
There was a problem hiding this comment.
issue: this looks like it would not work on Windows? Can we make unit-tests more OS-agnostic to verify? Instead of defining the paths as strings we could use filepath.Join("subdir", "state")
| return nil, fmt.Errorf("LocalStack returned status %d", resp.StatusCode) | ||
| } | ||
| return resp.Body, nil | ||
| } |
There was a problem hiding this comment.
question: we already have internal/snapshot/client.go for the aws emulator http api, can we add the new method there instead? Keeping a dedicated interface StateExporter just for the endpoints related to snapshots make sense to me though
| _ = resp.Body.Close() | ||
| return nil, fmt.Errorf("LocalStack returned status %d", resp.StatusCode) | ||
| } | ||
| return resp.Body, nil |
There was a problem hiding this comment.
question: instead of returning io.ReadCloser can we instead pass the io.Writer destination as argument?
This way we're sure the caller won't forget to close the returned body.
| } | ||
|
|
||
| func (d LocalDestination) Writer() (io.WriteCloser, error) { | ||
| return os.Create(d.Path) |
There was a problem hiding this comment.
issue: this abstraction won't work once we introduce pods because in the case of pods we are not saving to a file so we don't have a io.WriteCloser. We could just drop the interface for now?
|
I've posted a comment[1] in PRO-212 to discuss some potential changes, comments welcome! |
anisaoshafi
force-pushed
the
drg-764-migrate-state-commands-from-cli-v1-export
branch
from
19cab09 to
cc8b494
Compare
3 participants
Added a new snapshot subcommand with an initial save action that saves the current LocalStack state to a snapshot file.
ℹ️ Subject to change/be adjusted once the design is ready.