-
Notifications
You must be signed in to change notification settings - Fork 42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bump server to 1.26.2, enable HTTP by default #734
base: next-server
Are you sure you want to change the base?
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,26 +25,29 @@ import ( | |
|
||
func TestServer_StartDev_Simple(t *testing.T) { | ||
port := strconv.Itoa(devserver.MustGetFreePort("127.0.0.1")) | ||
httpPort := strconv.Itoa(devserver.MustGetFreePort("127.0.0.1")) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are these test updates needed? Can't we just let it choose the random default? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They're not, I left it here in prep for when we make the HTTP port fixed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think that's a reasonable assumption/expectation yet. But so long as there's some code path somewhere that is covering the random part, all good, even if it's not needed. |
||
startDevServerAndRunSimpleTest( | ||
t, | ||
// TODO(cretz): Remove --headless when | ||
// https://github.com/temporalio/ui/issues/1773 fixed | ||
[]string{"server", "start-dev", "-p", port, "--headless"}, | ||
[]string{"server", "start-dev", "-p", port, "--http-port", httpPort, "--headless"}, | ||
"127.0.0.1:"+port, | ||
) | ||
} | ||
|
||
func TestServer_StartDev_IPv4Unspecified(t *testing.T) { | ||
port := strconv.Itoa(devserver.MustGetFreePort("0.0.0.0")) | ||
httpPort := strconv.Itoa(devserver.MustGetFreePort("127.0.0.1")) | ||
startDevServerAndRunSimpleTest( | ||
t, | ||
[]string{"server", "start-dev", "--ip", "0.0.0.0", "-p", port, "--headless"}, | ||
[]string{"server", "start-dev", "--ip", "0.0.0.0", "-p", port, "--http-port", httpPort, "--headless"}, | ||
"0.0.0.0:"+port, | ||
) | ||
} | ||
|
||
func TestServer_StartDev_SQLitePragma(t *testing.T) { | ||
port := strconv.Itoa(devserver.MustGetFreePort("0.0.0.0")) | ||
httpPort := strconv.Itoa(devserver.MustGetFreePort("127.0.0.1")) | ||
dbFilename := filepath.Join(os.TempDir(), "devserver-sqlite-pragma.sqlite") | ||
defer func() { | ||
_ = os.Remove(dbFilename) | ||
|
@@ -55,7 +58,9 @@ func TestServer_StartDev_SQLitePragma(t *testing.T) { | |
t, | ||
[]string{ | ||
"server", "start-dev", | ||
"-p", port, "--headless", | ||
"-p", port, | ||
"--http-port", httpPort, | ||
"--headless", | ||
"--db-filename", dbFilename, | ||
"--sqlite-pragma", "journal_mode=WAL", | ||
"--sqlite-pragma", "synchronous=NORMAL", | ||
|
@@ -76,12 +81,14 @@ func TestServer_StartDev_IPv6Unspecified(t *testing.T) { | |
} | ||
|
||
port := strconv.Itoa(devserver.MustGetFreePort("::")) | ||
httpPort := strconv.Itoa(devserver.MustGetFreePort("::")) | ||
startDevServerAndRunSimpleTest( | ||
t, | ||
[]string{ | ||
"server", "start-dev", | ||
"--ip", "::", "--ui-ip", "::1", | ||
"-p", port, | ||
"--http-port", httpPort, | ||
"--ui-port", strconv.Itoa(devserver.MustGetFreePort("::")), | ||
"--http-port", strconv.Itoa(devserver.MustGetFreePort("::")), | ||
"--metrics-port", strconv.Itoa(devserver.MustGetFreePort("::"))}, | ||
|
@@ -138,9 +145,10 @@ func TestServer_StartDev_ConcurrentStarts(t *testing.T) { | |
|
||
// Start in background, then wait for client to be able to connect | ||
port := strconv.Itoa(devserver.MustGetFreePort("127.0.0.1")) | ||
httpPort := strconv.Itoa(devserver.MustGetFreePort("127.0.0.1")) | ||
resCh := make(chan *CommandResult, 1) | ||
go func() { | ||
resCh <- h.Execute("server", "start-dev", "-p", port, "--headless", "--log-level", "never") | ||
resCh <- h.Execute("server", "start-dev", "-p", port, "--http-port", httpPort, "--headless", "--log-level", "never") | ||
}() | ||
|
||
// Try to connect for a bit while checking for error | ||
|
@@ -191,11 +199,13 @@ func TestServer_StartDev_WithSearchAttributes(t *testing.T) { | |
|
||
// Start in background, then wait for client to be able to connect | ||
port := strconv.Itoa(devserver.MustGetFreePort("127.0.0.1")) | ||
httpPort := strconv.Itoa(devserver.MustGetFreePort("127.0.0.1")) | ||
resCh := make(chan *CommandResult, 1) | ||
go func() { | ||
resCh <- h.Execute( | ||
"server", "start-dev", | ||
"-p", port, | ||
"--http-port", httpPort, | ||
"--headless", | ||
"--search-attribute", "search-attr-1=Int", | ||
"--search-attribute", "search-attr-2=kEyWoRdLiSt", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1647,9 +1647,8 @@ commands: | |
default: 7233 | ||
- name: http-port | ||
type: int | ||
description: | | ||
Port for the HTTP API service. | ||
Default is off. | ||
description: Port for the HTTP API service. Defaults to a random free port. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we have any way to disable the HTTP API service? I can't think of a good reason at this time, so can probably not worry about it for now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought about this but don't think it's a concern if it defaults to a random port. We can always add a |
||
default: 0 | ||
- name: metrics-port | ||
type: int | ||
description: | | ||
|
@@ -3502,4 +3501,4 @@ option-sets: | |
description: | | ||
An external Nexus Endpoint that receives forwarded Nexus requests. | ||
May be used as an alternative to `--target-namespace` and | ||
`--target-task-queue`. | ||
`--target-task-queue`. |
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 a little concerned about this part. The HTTP API is not considered stable and is not really documented. So listing this as though it is I think is a bit rough. Can we only display this when the port was given explicitly?
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.
Sure.
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.
Hmm... for metrics we print even if the port isn't specified. Let's keep this consistent.
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.
The concern is not about whether the port is specified, it's about promoting the HTTP API. Metrics is stable/GA and documented w/ a cloud equivalent, HTTP API is not. We should not by default encourage use of the HTTP API in dev server at this time until it is a documented product available to all cloud users by default. But if a user explicitly sets an option, that's different/understandable, though I would also be ok just not showing it at all.
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.
de52ee3