Select default emulator on first run#217
Conversation
e1db26a to
4204284
Compare
|
@anisaoshafi @gtsiolis it feels a bit odd to me that the Users can still manually edit their config if they want to. If we want to provide a way to reset the emulator in the config from command line, what about something like
|
Good point, can be too much of a responsibility for a flag. I like |
If we decide for another command, I think we can leave it out of the scope of this PR/create another issue for it to keep this PR small. |
| return loadConfig(path) | ||
| } | ||
|
|
||
| func switchEmulatorContent(content string, to EmulatorType) (updated string, changed bool, err error) { |
There was a problem hiding this comment.
nit: we can remove the err in the returned values since we never return an error
| emName := appConfig.Containers[0].Type.DisplayName() | ||
| sink.Emit(output.MessageEvent{ | ||
| Severity: output.SeverityNote, | ||
| Text: fmt.Sprintf("No emulator configured; defaulting to %s. Use --emulator to change this.", emName), |
There was a problem hiding this comment.
question: isn't the message slightly misleading since the default emulator is written to the config?
suggestion: Configured with default emulator AWS. Pass --emulator to change.
| return EmulatorType(strings.ToLower(m[1])) | ||
| } | ||
| } | ||
| return "" |
There was a problem hiding this comment.
nit:
If the user has such config and requests "snowflake", it would match (it would ignore the fact that line is commented out)
[[containers]]
# type = "snowflake"
type = "aws"
There was a problem hiding this comment.
thanks for the heads up, didn't properly test it. Handled it here: 9927276
| return blocks | ||
| } | ||
|
|
||
| var typeLineRe = regexp.MustCompile(`type\s*=\s*"(\w+)"`) |
There was a problem hiding this comment.
nit: this regex only matches double quotes, can we update it so it also matches single quotes? e.g.
[[containers]]
type = 'aws'
and
[[containers]]
type = "aws"
There was a problem hiding this comment.
thoughtless of me 😬 thanks for raising. Addressed it here: 9927276
| ctx := testContext(t) | ||
| // The process will fail at container.Start (no Docker / no real auth), but the | ||
| // config switch happens earlier so the file should already be updated. | ||
| _, _, _ = runLstk(t, ctx, "", e.With(env.AuthToken, "test-token"), "--emulator", "snowflake", "--non-interactive") |
There was a problem hiding this comment.
nit: can we still check returned error/add assertions for the expected failures, to make sure we pass for the right reason?
| return "", fmt.Errorf("unsupported emulator %q: must be 'aws' or 'snowflake'", s) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
nit: can we move this function next to EmulatorType in internal/config/containers.go to keep it in its domain?
| } | ||
|
|
||
| if requestedEmulator != "" { | ||
| emType, err := parseEmulatorType(requestedEmulator) |
There was a problem hiding this comment.
nit: can we parse the emulator type earlier, right after reading the flag in cmd/root.go line 40, to fail early and strong-type it when passing it down. I would also make it a pointer *config.EmulatorType so that an unset value is clear (nil), rather than relying on empty string.
There was a problem hiding this comment.
Looks good! Are we now able to inline ParseEmulatorType in ParseOptionalEmulatorType (if not used anywhere else)?
| tag = "latest" | ||
| port = "4566" | ||
| # volume = "" # Host directory for persistent state (default: OS cache dir) | ||
| # env = [] # Named environment profiles to apply (see [env.*] sections below)` |
There was a problem hiding this comment.
question:
At the moment those blocks are duplicated and there is a chance of drift.
How hard it is to parse those blocks from default_config.toml?
Another solution could be to use go templates in default_config.toml so the container blocks could come from another file.
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
There was a problem hiding this comment.
nit: these tests seem to be a good fit for table-driven tests, WDYT?
| EmulatorLabel: config.CachedPlanLabel(), | ||
| LabelCh: labelCh, | ||
| NeedsEmulatorSelection: needsEmulatorSelection, | ||
| OnEmulatorSelected: func(emType config.EmulatorType) ([]config.ContainerConfig, error) { |
There was a problem hiding this comment.
issue: such callback is not very "go-like". It's not clear to me why we need it, don't we have all dependencies in ui/run.go to execute what we must execute after selection?
There was a problem hiding this comment.
Attempted to refactor here, let me know if I'm doing sth terrible 🙏🏼5e7723a
|
Looking at this now! 👀 |
ada0134 to
5e7723a
Compare
| type = "snowflake" | ||
| tag = "latest" | ||
| port = "4566" |
There was a problem hiding this comment.
issue: Similar to what @carole-lavillonniere mentioned above, this leads to content duplication.
# [[containers]]
# type = "aws" # Emulator type. Currently supported: "aws", "snowflake"
# tag = "latest" # Docker image tag, e.g. "latest", "2026.03"
# port = "4566" # Host port the emulator will be accessible on
# # volume = "" # Host directory for persistent state (default: OS cache dir)
# # env = [] # Named environment profiles to apply (see [env.*] sections below)
[...]
[[containers]]
type = "snowflake"
tag = "latest"
port = "4566"
# volume = "" # Host directory for persistent state (default: OS cache dir)
# env = [] # Named environment profiles to apply (see [env.*] sections below)I'd expect if I select SNO on start, to simply use snowflake in the first [[containers]] section.
[...]
[[containers]]
type = "snowflake" # Emulator type. Currently supported: "aws", "snowflake"
tag = "latest" # Docker image tag, e.g. "latest", "2026.03"
port = "4566" # Host port the emulator will be accessible on
volume = "" # Host directory for persistent state (default: OS cache dir)
env = [] # Named environment profiles to apply (see [env.*] sections below)
[...]There was a problem hiding this comment.
I'm going to tackle this 💯
| Prompt: "Which emulator would you like to use?", | ||
| Options: []output.InputOption{ | ||
| {Key: "a", Label: "AWS [A]"}, | ||
| {Key: "s", Label: "Snowflake [S]"}, |
There was a problem hiding this comment.
There was a problem hiding this comment.
yes, comes from emulator side. I created a follow up task: DRG-818 for handling this error in a nicer way since it's out of the scope for this PR 🙏🏼
|
|
||
| msg := selected.DisplayName() + " emulator selected." | ||
| if configPath != "" { | ||
| msg += fmt.Sprintf(" You can change this anytime in %s.", configPath) |
There was a problem hiding this comment.
@gtsiolis Does this look a bit better? Also changed the message as you proposed in the other comment 
There was a problem hiding this comment.
same success message as the AWS profile creation
What does this mean when the user selects snowflake on first try?
@gtsiolis sorry, I'm not following. Can you rephrase your suggestion?
Did you mean Change configuration in ~/.config/lstk/config.toml should have a green tick in front?
| labelCh <- label | ||
| } | ||
|
|
||
| func selectEmulatorInTUI( |
There was a problem hiding this comment.
praise: Thanks for reusing this UX!
b4e27ea to
e1d9e9c
Compare
66f5972 to
33e0742
Compare
…ation message is secondary
33e0742 to
9b1d670
Compare
|
Apologies for the back and forth and the huge PR. I removed the |


First-time users are prompted to choose their emulator (AWS or Snowflake) before lstk starts (to simulate this, rename or delete config.toml). The choice is saved to the config file immediately.
--non-interactive/ no TTY it silently uses the AWS by default.