Skip to content

Select default emulator on first run#217

Open
anisaoshafi wants to merge 16 commits intomainfrom
drg-381-add-option-to-select-default-emulator
Open

Select default emulator on first run#217
anisaoshafi wants to merge 16 commits intomainfrom
drg-381-add-option-to-select-default-emulator

Conversation

@anisaoshafi
Copy link
Copy Markdown
Contributor

@anisaoshafi anisaoshafi commented Apr 30, 2026

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.

  • When run with --non-interactive / no TTY it silently uses the AWS by default.
Screenshot 2026-04-30 at 16 59 20 Note to user they can change to the other emulator: Screenshot 2026-04-30 at 16 59 43

@anisaoshafi anisaoshafi force-pushed the drg-381-add-option-to-select-default-emulator branch 17 times, most recently from e1db26a to 4204284 Compare May 4, 2026 15:34
@anisaoshafi anisaoshafi marked this pull request as ready for review May 4, 2026 15:39
@anisaoshafi anisaoshafi requested a review from gtsiolis May 4, 2026 15:39
@carole-lavillonniere
Copy link
Copy Markdown
Collaborator

@anisaoshafi @gtsiolis it feels a bit odd to me that the --emulator flag overwrites the config. The convention for flags as far as I know is to be transient/ephemeral.

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 lstk start --reset-config or lstk config emulator that would prompt again Which emulator would you like to use?

lstk start --emulator <type> could override the config, but only temporarily.

@anisaoshafi
Copy link
Copy Markdown
Contributor Author

@anisaoshafi @gtsiolis it feels a bit odd to me that the --emulator flag overwrites the config. The convention for flags as far as I know is to be transient/ephemeral.

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 lstk start --reset-config or lstk config emulator that would prompt again Which emulator would you like to use?

lstk start --emulator <type> could override the config, but only temporarily.

Good point, can be too much of a responsibility for a flag.

I like lstk config emulator option. Does the same but in a more controlled way since the user have to confirm the option, or cancel if that's not what they intended.
Will wait for @gtsiolis's opinion before making the changes 🙏🏼

@carole-lavillonniere
Copy link
Copy Markdown
Collaborator

@anisaoshafi @gtsiolis it feels a bit odd to me that the --emulator flag overwrites the config. The convention for flags as far as I know is to be transient/ephemeral.
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 lstk start --reset-config or lstk config emulator that would prompt again Which emulator would you like to use?
lstk start --emulator <type> could override the config, but only temporarily.

Good point, can be too much of a responsibility for a flag.

I like lstk config emulator option. Does the same but in a more controlled way since the user have to confirm the option, or cancel if that's not what they intended. Will wait for @gtsiolis's opinion before making the changes 🙏🏼

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.

Comment thread internal/config/switch.go Outdated
return loadConfig(path)
}

func switchEmulatorContent(content string, to EmulatorType) (updated string, changed bool, err error) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: we can remove the err in the returned values since we never return an error

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Tackled here:1684152

Comment thread cmd/root.go Outdated
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),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point, tackled here: 1684152

Comment thread internal/config/switch.go Outdated
return EmulatorType(strings.ToLower(m[1]))
}
}
return ""
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

thanks for the heads up, didn't properly test it. Handled it here: 9927276

Comment thread internal/config/switch.go Outdated
return blocks
}

var typeLineRe = regexp.MustCompile(`type\s*=\s*"(\w+)"`)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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"

Copy link
Copy Markdown
Contributor Author

@anisaoshafi anisaoshafi May 5, 2026

Choose a reason for hiding this comment

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

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")
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: can we still check returned error/add assertions for the expected failures, to make sure we pass for the right reason?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, addressed here: 3706dc8

Comment thread cmd/root.go Outdated
return "", fmt.Errorf("unsupported emulator %q: must be 'aws' or 'snowflake'", s)
}
}

Copy link
Copy Markdown
Collaborator

@carole-lavillonniere carole-lavillonniere May 5, 2026

Choose a reason for hiding this comment

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

nit: can we move this function next to EmulatorType in internal/config/containers.go to keep it in its domain?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Moved it here, good point: b85896c

Comment thread cmd/root.go Outdated
}

if requestedEmulator != "" {
emType, err := parseEmulatorType(requestedEmulator)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Attempted here: aa2efc9

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good! Are we now able to inline ParseEmulatorType in ParseOptionalEmulatorType (if not used anywhere else)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good idea, tried here: fe5fa04

Comment thread internal/config/switch.go Outdated
tag = "latest"
port = "4566"
# volume = "" # Host directory for persistent state (default: OS cache dir)
# env = [] # Named environment profiles to apply (see [env.*] sections below)`
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed here 🙏🏼 0aeca91

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: these tests seem to be a good fit for table-driven tests, WDYT?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

great observation, done: abc89ac

Comment thread cmd/root.go Outdated
EmulatorLabel: config.CachedPlanLabel(),
LabelCh: labelCh,
NeedsEmulatorSelection: needsEmulatorSelection,
OnEmulatorSelected: func(emType config.EmulatorType) ([]config.ContainerConfig, error) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Attempted to refactor here, let me know if I'm doing sth terrible 🙏🏼5e7723a

@anisaoshafi anisaoshafi marked this pull request as draft May 5, 2026 11:24
@gtsiolis
Copy link
Copy Markdown
Member

gtsiolis commented May 5, 2026

Looking at this now! 👀

@anisaoshafi anisaoshafi force-pushed the drg-381-add-option-to-select-default-emulator branch from ada0134 to 5e7723a Compare May 5, 2026 12:34
Comment thread internal/config/switch.go Outdated
Comment on lines +18 to +20
type = "snowflake"
tag = "latest"
port = "4566"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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)
[...]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm going to tackle this 💯

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed here 🙏🏼 0aeca91

Comment thread internal/ui/run.go Outdated
Comment thread internal/ui/run.go Outdated
Comment thread internal/ui/run.go Outdated
Prompt: "Which emulator would you like to use?",
Options: []output.InputOption{
{Key: "a", Label: "AWS [A]"},
{Key: "s", Label: "Snowflake [S]"},
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

issue(non-blocking): Here's what most users with no access to SNO emulation will see. I guess this comes from the emulator, right? Could we mask now or later this error?

Image

SNO users in trial see no error currently.

Have to check what SNO-only users in production see.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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 🙏🏼

Comment thread internal/ui/run.go Outdated

msg := selected.DisplayName() + " emulator selected."
if configPath != "" {
msg += fmt.Sprintf(" You can change this anytime in %s.", configPath)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

issue: The message here wraps early on line 2, can we make the message wrap normally in two lines, I think we do so for other messages, right?

Image

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@gtsiolis Does this look a bit better? Also changed the message as you proposed in the other comment image

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What do you think of using the same success message as the AWS profile creation? This will avoid ending up with multiple NOTE messages when the emulator is running (although edge case) and make the output more functional upon selection.

1/2 2/2
Screenshot 2026-05-05 at 20.56.01.png Screenshot 2026-05-05 at 20.56.03.png

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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?

Comment thread internal/ui/run.go
labelCh <- label
}

func selectEmulatorInTUI(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

praise: Thanks for reusing this UX!

@anisaoshafi anisaoshafi force-pushed the drg-381-add-option-to-select-default-emulator branch from b4e27ea to e1d9e9c Compare May 5, 2026 16:47
@anisaoshafi anisaoshafi marked this pull request as ready for review May 5, 2026 17:08
@anisaoshafi anisaoshafi marked this pull request as draft May 6, 2026 08:27
@anisaoshafi anisaoshafi force-pushed the drg-381-add-option-to-select-default-emulator branch 3 times, most recently from 66f5972 to 33e0742 Compare May 6, 2026 11:54
@anisaoshafi anisaoshafi force-pushed the drg-381-add-option-to-select-default-emulator branch from 33e0742 to 9b1d670 Compare May 6, 2026 11:55
@anisaoshafi
Copy link
Copy Markdown
Contributor Author

Apologies for the back and forth and the huge PR. I removed the --emulator flag from the scope of this PR (discussing it in linear), should have removed that before tackling the review comments which were mostly around the switching between emulators, but a bit too late now.
This simplifies the PR. Thanks for your patience @carole-lavillonniere @gtsiolis 🙏🏼

@anisaoshafi anisaoshafi marked this pull request as ready for review May 6, 2026 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants