-
Notifications
You must be signed in to change notification settings - Fork 75
Expose a new --codec-endpoint flag to start command #174
Expose a new --codec-endpoint flag to start command #174
Conversation
cc @feedmeapples - we may need to have this option elsewhere too |
This allows starting a temporalite instance with a remote data converter endpoint preconfigured.
485ac1d
to
3c1013d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will wait on @jlegrone's approval. @lminaudier - if this is ready for review, you can move the PR out of draft status.
Ha the linter is failing with a too many arguments for the |
added a tracking item |
This moves `uiconfig.Config` struct initialization up to the main function which is then forwarded across the stack.
cfg.TemporalGRPCAddress = frontendAddr | ||
cfg.EnableUI = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not really sure why those two options where set after having loaded the file based config. The behavior looked inconsistent to me, either the config file overrides the CLI flags values or the opposite, but doing both sounds confusing.
My manual test seem to show I can still load the UI and the UI can still reach out to the frontend
when I start temporalite
with
./temporalite start \
--namespace default \
--log-format pretty \
--ephemeral \
--ui-codec-endpoint 'https://<the-remote-endpoint>'
And the --headless
flag is still respected.
Maybe I missed something though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
either the config file overrides the CLI flags values or the opposite, but doing both sounds confusing
I would not be surprised if the config file required the frontend address and this just set it back to whatever dummy value was set by a user. Or maybe there is a common/default UI config used in one of our projects that this is explicitly overriding values of. Or maybe our docker compose or helm charts just use some config default template that puts some values here by default we choose to override for Temporalite. At this point you can't be sure that people aren't relying on these two fields to be overridden back to their Temporalite expectation.
Might as well leave these forced overrides.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, right indeed, it's probably safer. Fixing.
@lminaudier - looks like CI is failing, can you look? |
Ran tests locally successfully, I'll need a bit of time to debug what's going in CI. |
Codecov Report
@@ Coverage Diff @@
## main #174 +/- ##
==========================================
+ Coverage 63.57% 64.17% +0.60%
==========================================
Files 13 13
Lines 969 991 +22
==========================================
+ Hits 616 636 +20
- Misses 314 315 +1
- Partials 39 40 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
You need to fix the signature in |
Thanks for doing the investigation. It looks like the tests are ensuring we are not importing |
Ensures we don't import github.com/temporalio/ui-server/v2 dependency when we use the headless build tag.
All right tests are passing locally in both headless and non headless mode with last commit.
|
This allows starting a temporalite instance with a remote data converter endpoint preconfigured.
What changed?
Adds a new
--ui-codec-endpoint
flag totemporalite start
.Why?
Exposing the flag allows to send a single command to run to users that can rely on a remote data converter hosted somewhere instead of telling them to run 2 processes on their laptop.
How did you test it?
Manually
Potential risks
None
Is hotfix candidate?
No