Enable Snowflake emulator#186
Conversation
anisaoshafi
force-pushed
the
drg-742-support-snowflake
branch
from
9901607 to
0a0cd1a
Compare
anisaoshafi
force-pushed
the
drg-742-support-snowflake
branch
3 times, most recently
from
d073241 to
e599bc0
Compare
anisaoshafi
force-pushed
the
drg-742-support-snowflake
branch
2 times, most recently
from
556b5c4 to
3ba78eb
Compare
| tag := c.Tag | ||
| if tag == "" || tag == "latest" { | ||
| if c.Type == config.EmulatorSnowflake { | ||
| return "LocalStack Snowflake", false |
There was a problem hiding this comment.
The licensing for snowflake is complex and out of scope for now, it makes the TUI header inaccurate because we'd default to (No License) [1] as we've no easy way to determine it.
@gtsiolis what do you think about changing it to LocalStack Snowflake [2] for now?
| 1 | 2 |
|---|---|
 |
 |
There was a problem hiding this comment.
Totally valid, we need to clarify this on the product side. Can we ...
- Show LocalStack here until we clarify the licensing? This would avoid leaking back confusing terminology that does not exist, like LocalStack Snowflake.
- Hide the tip row when running SNO, until we figure out the licensing parts if it's trivial?
anisaoshafi
requested review from
carole-lavillonniere and
silv-io
as code owners
carole-lavillonniere
left a comment
carole-lavillonniere
left a comment
There was a problem hiding this comment.
It all looks good, I'm just thinking we could add a few integration tests for the cases where the behavior is slightly different than for aws:
- status command
- start skips license validation
WDYT?
I had some test for skipping license validation, but decided to remove them. Will add it back together with something for status, it's better to clarify what's currently supported in tests. |
anisaoshafi
force-pushed
the
drg-742-support-snowflake
branch
from
3ba78eb to
8835f03
Compare
|
|
||
| [[containers]] | ||
| type = "aws" # Emulator type. Currently supported: "aws" | ||
| type = "aws" # Emulator type. Currently supported: "aws", "snowflake" |
There was a problem hiding this comment.
praise: Thanks for updating the config! 😁
| # To use the Snowflake emulator instead, comment out the AWS block above and uncomment this one. | ||
| # [[containers]] | ||
| # type = "snowflake" | ||
| # tag = "latest" | ||
| # port = "4566" | ||
|
|
There was a problem hiding this comment.
suggestion: I'd leave default config do the explaining, to avoid also encouraging multiple containers. Thoughts?
| # To use the Snowflake emulator instead, comment out the AWS block above and uncomment this one. | |
| # [[containers]] | |
| # type = "snowflake" | |
| # tag = "latest" | |
| # port = "4566" |
There was a problem hiding this comment.
Good point, thanks George. Done ✅
| tag := c.Tag | ||
| if tag == "" || tag == "latest" { | ||
| if c.Type == config.EmulatorSnowflake { | ||
| return "LocalStack Snowflake", false |
There was a problem hiding this comment.
Totally valid, we need to clarify this on the product side. Can we ...
- Show LocalStack here until we clarify the licensing? This would avoid leaking back confusing terminology that does not exist, like LocalStack Snowflake.
- Hide the tip row when running SNO, until we figure out the licensing parts if it's trivial?
|
@gtsiolis thanks for your feedback. Feel free to take another look 🙏🏼 |
anisaoshafi
force-pushed
the
drg-742-support-snowflake
branch
from
425b583 to
9afa20d
Compare
anisaoshafi
force-pushed
the
drg-742-support-snowflake
branch
from
9afa20d to
fe1fad0
Compare
|
ℹ️ The default tests timeout is 10 mins for go. The tests in ubuntu are taking longer than 10 min now, so increased the limit to 15m for now to unblock my PR. |
3 participants
Enable support for Snowflake emulator.
ℹ️ Licensing checks are skipped for snowflake at the moment, there are efforts to improve this but not in lstk scope for now. Improvements will be incorporated by lstk when ready on the platform side of the things.
There is going to be follow up PRs to smoothen multi-emulator experience (PRO-198, DRG-381)