Skip to content
This repository has been archived by the owner on Apr 3, 2024. It is now read-only.

Expose a new --codec-endpoint flag to start command #174

Merged

Conversation

lminaudier
Copy link
Contributor

@lminaudier lminaudier commented Nov 22, 2022

This allows starting a temporalite instance with a remote data converter endpoint preconfigured.

What changed?

Adds a new --ui-codec-endpoint flag to temporalite 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

cmd/temporalite/main.go Outdated Show resolved Hide resolved
@cretz
Copy link
Member

cretz commented Nov 28, 2022

cc @feedmeapples - we may need to have this option elsewhere too

This allows starting a temporalite instance with a remote data converter
endpoint preconfigured.
@lminaudier lminaudier force-pushed the expose-codec-endpoint-as-temporalite-flag branch from 485ac1d to 3c1013d Compare November 28, 2022 13:45
Copy link
Contributor

@robholland robholland left a comment

Choose a reason for hiding this comment

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

lgtm.

Copy link
Member

@cretz cretz left a 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.

@lminaudier lminaudier marked this pull request as ready for review November 28, 2022 14:19
@lminaudier
Copy link
Contributor Author

Ha the linter is failing with a too many arguments for the newUIOption function. It's true it might become time to have a UI config object here 🤔

@jlegrone jlegrone changed the title Expose a new --codec-enpoint flag to start command Expose a new --codec-endpoint flag to start command Nov 29, 2022
@feedmeapples
Copy link
Contributor

cc @feedmeapples - we may need to have this option elsewhere too

added a tracking item

This moves `uiconfig.Config` struct initialization up to the main
function which is then forwarded across the stack.
Comment on lines -51 to -52
cfg.TemporalGRPCAddress = frontendAddr
cfg.EnableUI = true
Copy link
Contributor Author

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

image

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.

Copy link
Member

@cretz cretz Nov 30, 2022

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.

Copy link
Contributor Author

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.

@cretz
Copy link
Member

cretz commented Dec 1, 2022

@lminaudier - looks like CI is failing, can you look?

@lminaudier
Copy link
Contributor Author

Ran tests locally successfully, I'll need a bit of time to debug what's going in CI.

@codecov-commenter
Copy link

codecov-commenter commented Dec 1, 2022

Codecov Report

Merging #174 (5432436) into main (fdc0165) will increase coverage by 0.60%.
The diff coverage is 91.89%.

@@            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     
Impacted Files Coverage Δ
cmd/temporalite/main.go 69.96% <86.95%> (+1.16%) ⬆️
cmd/temporalite/ui.go 79.31% <100.00%> (+4.31%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@cretz
Copy link
Member

cretz commented Dec 1, 2022

Error: cmd/temporalite/main.go:282:35: not enough arguments in call to newUIOption
	have (*"github.com/temporalio/ui-server/v2/server/config".Config, string)
	want (string, string, int, string)

You need to fix the signature in cmd/temporalite/ui_disabled.go (one that is run on headless build tag)

@lminaudier
Copy link
Contributor Author

Thanks for doing the investigation.

It looks like the tests are ensuring we are not importing github.com/temporalio/ui-server/v2 when build with the headless tag. I'll have to create an intermediary type instead of importing that directly. Let me do that and repush.

Ensures we don't import github.com/temporalio/ui-server/v2 dependency
when we use the headless build tag.
@lminaudier
Copy link
Contributor Author

All right tests are passing locally in both headless and non headless mode with last commit. go build also. 🤞🏻 CI is green on the next run.

𝝺 go test -tags headless ./...
?   	github.com/temporalio/temporalite	[no test files]
ok  	github.com/temporalio/temporalite/cmd/temporalite	(cached)
?   	github.com/temporalio/temporalite/internal/copyright	[no test files]
?   	github.com/temporalio/temporalite/internal/examples/helloworld	[no test files]
?   	github.com/temporalio/temporalite/internal/examples/mtls	[no test files]
?   	github.com/temporalio/temporalite/internal/liteconfig	[no test files]
ok  	github.com/temporalio/temporalite/temporaltest	(cached)
𝝺 go test ./...
?   	github.com/temporalio/temporalite	[no test files]
ok  	github.com/temporalio/temporalite/cmd/temporalite	(cached)
?   	github.com/temporalio/temporalite/internal/copyright	[no test files]
?   	github.com/temporalio/temporalite/internal/examples/helloworld	[no test files]
?   	github.com/temporalio/temporalite/internal/examples/mtls	[no test files]
?   	github.com/temporalio/temporalite/internal/liteconfig	[no test files]
ok  	github.com/temporalio/temporalite/temporaltest	(cached)
𝝺 go build ./cmd/temporalite/
𝝺 go build -tags headless ./cmd/temporalite/

@cretz cretz merged commit 902abe4 into temporalio:main Dec 1, 2022
@lminaudier lminaudier deleted the expose-codec-endpoint-as-temporalite-flag branch December 2, 2022 09:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants