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

Expose frontend host:port #11

Merged
merged 2 commits into from
Nov 15, 2021
Merged

Conversation

cretz
Copy link
Member

@cretz cretz commented Nov 9, 2021

What changed?

Exposed FrontendHostPort

Why?

Some uses of temporalite may want to use this value for their own client creation, or in my case, pass it somewhere else for their use.

Ideally this would be FrontendAddr() net.Addr and the system would support things like local unix paths and such, but I understand this is a Temporal server limitation too. I also considered just exposing FrontendPort since 127.0.0.1 is hardcoded, but I assumed it may not be hardcoded always and host:port can be directly passed to different forms of Dial.

How did you test it?

Untested getter.

Potential risks

None

Is hotfix candidate?

No

server.go Outdated
Comment on lines 129 to 133
// FrontendHostPort returns the host:port for this server.
func (s *Server) FrontendHostPort() string {
return s.frontendHostPort
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

For your use case, would a method to return client.Options work as well?

Right now Temporalite really only needs a hostport to be set on the client, but in the future more client options may need to have modified default values and I would like to avoid confusion for end users about how to instantiate clients. I think this would make it possible to introspect things like hostport, but also decrease the chances that dependent code would break if/when Temporalite needs to further customize default client options.

Copy link
Member Author

Choose a reason for hiding this comment

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

client.Options would work, though I might just be extracting the host:port out of it for some uses.

@codecov-commenter
Copy link

Codecov Report

Merging #11 (94401c5) into main (1cccf14) will decrease coverage by 0.14%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #11      +/-   ##
==========================================
- Coverage   46.72%   46.57%   -0.15%     
==========================================
  Files          11       11              
  Lines         625      627       +2     
==========================================
  Hits          292      292              
- Misses        315      317       +2     
  Partials       18       18              
Impacted Files Coverage Δ
server.go 63.01% <0.00%> (-1.78%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1cccf14...94401c5. Read the comment docs.

Copy link
Collaborator

@jlegrone jlegrone left a comment

Choose a reason for hiding this comment

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

I thought about it a bit more and the simplicity of a function just to return the host:port feels nicer than returning the whole client options object. We can always add that later on if we need it.

Lgtm after the suggested doc comment update. Thanks!

server.go Outdated Show resolved Hide resolved
Co-authored-by: Jacob LeGrone <jlegrone@users.noreply.github.com>
@jlegrone jlegrone merged commit 1ecb757 into temporalio:main Nov 15, 2021
@cretz cretz deleted the expose-frontend-addr branch November 15, 2021 18:17
leowmjw pushed a commit to leowmjw/temporalite that referenced this pull request Dec 3, 2021
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

3 participants