-
Notifications
You must be signed in to change notification settings - Fork 44
Cleans up api.Run and backfills ephemeral admin server when not in config #467
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
Conversation
@@ -61,6 +60,27 @@ func Out(out io.Writer) RunOption { | |||
} | |||
} | |||
|
|||
// EnvoyOut sets the writer for Envoy stdout | |||
func EnvoyOut(w io.Writer) RunOption { |
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.
api changes here.. only 2 of them, and they are additional. In general, this package is a bomb as it ties api to impl, but we can shave that yak in a later PR.
c55e0a8
to
8551571
Compare
"github.com/tetratelabs/func-e/internal/test/morerequire" | ||
) | ||
|
||
var envoyStartedLine = "starting main dispatch loop" |
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.
here's the file with the new consolidated test cases which cover what was in e2e (now reusable with both a built binary and library calls)
…nfig This makes three changes that improve the quality of func-e from a user's perspective: - Change `api.Run` to call library code directly instead of routing through the CLI library (urfave) - This stops pollution of func-e arg and env parsing as well simplifies lifecycle in callers such as EG - Add `api.RunOptions` for `EnvoyOut` and `EnvoyErr` - This prevents callers from being unable to control subprocess logging, e.g. was going to `os.Stdout/Stderr` - Make admin server config optional from the user's POV - When a caller didn't pass an admin server, errors would go into the logs as func-e assumed it would be there --- Other changes were tech debt clearing to clear the path for this. There is still more to do, but this is the largest bit. - Made a new consolidated e2e suite that can be used regardless of if func-e is compiled. - Specifically, this allows use of a debugger to hunt down state issues. - Refactored a new, realistic fake Envoy binary for all test scenarios. - This actually crashes like envoy does on no config and starts listeners our tests use. - Removed legacy test helpers and fakebinary code, consolidating test logic and sources. - There were traps like main code copy/pasted into tests, esp in signal handling - Added config parsing code for admin and static listeners, and backfilled tests - Config parsing now hits edge cases from envoy's actual source, and tests use the listeners. - Updated Envoy to reflect latest version and updated test tools - Formatting and linters have drifted and this is all sorted now - Removed inner timeout in e2e tests - Having context timeouts inside e2e tests make a race between the go test timeout and the inner one Signed-off-by: Adrian Cole <[email protected]>
cc7e7de
to
a9bd01f
Compare
} | ||
|
||
funcECmd := cmd.NewApp(&o) |
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.
before, this routed library logic through urfave which was creepy and made it near impossible to understand
"github.com/tetratelabs/func-e/internal/test/e2e" | ||
) | ||
|
||
func TestRun(t *testing.T) { |
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.
example of "e2e" run tests applied to the library mode, which works in a debugger
Signed-off-by: Adrian Cole <[email protected]>
|
||
verifyRunArchive(t, cmd) | ||
func TestRun(t *testing.T) { | ||
e2e.TestRun(context.Background(), t, funcEFactory{}) |
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.
this file is shredded because there is now shared logic between main and e2e (compiled)
funcEBin = filepath.Join(projectRoot, funcEBin, "func-e"+moreos.Exe) | ||
} | ||
} else { | ||
fmt.Fprintf(os.Stderr, "%s was not set. Building %s...\n", funcEPathEnvKey, funcEBin) |
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.
func-e now builds on demand if the ENV val isn't set (as would be in make e2e
) this helps with running from the IDE.
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.
Given it has no public package change, approving LGTM
PS I ran with envoy gateway using go.mod replace and it is source compatible. internal/infrastructure/host/proxy_infra_test.go finished with no errors |
This makes three changes that improve the quality of func-e from a user's perspective:
api.Run
to call library code directly instead of routing through the CLI library (urfave)api.RunOptions
forEnvoyOut
andEnvoyErr
os.Stdout/Stderr
Other changes were tech debt clearing to clear the path for this. There is still more to do, but this is the largest bit.