-
Notifications
You must be signed in to change notification settings - Fork 446
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
cli: Introduce glooctl export report #9327
Conversation
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 generally makes sense to me and I really like the seperation of export and how it will be composed of sub functions that may run in parallel.
Left some more implementation based thoughts rather than large picture thoughts.
"path/filepath" | ||
) | ||
|
||
func RunCommandOutputToFile(cmd cmdutils.Cmd, path string) func() error { |
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.
nit: export comment?
pkg/utils/cmdutils/local.go
Outdated
@@ -31,7 +31,7 @@ func (c *LocalCmder) Command(ctx context.Context, name string, arg ...string) Cm | |||
} | |||
} | |||
|
|||
// LocalCmd wraps os/exec.Cmd, implementing the kind/pkg/exec.Cmd interface | |||
// LocalCmd wraps os/exec.Cmd, implementing the cmdutils.Cmd interface |
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.
do we no longer implement the kind exec interface? https://pkg.go.dev/sigs.k8s.io/kind/pkg/exec
"strings" | ||
) | ||
|
||
const ( |
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.
Can we minimally include a link to the source we are building against ? https://github.com/envoyproxy/envoy/blob/63bc9b564b1a76a22a0d029bcac35abeffff2a61/source/server/admin/admin.cc#L127
Perhaps a todo to autogenerate from the makeHandler calls?
pkg/utils/errutils/concurrent.go
Outdated
// AggregateConcurrent runs fns concurrently, returning a NewAggregate if there are > 1 errors | ||
func AggregateConcurrent(funcs []func() error) error { | ||
// run all fns concurrently | ||
ch := make(chan error, len(funcs)) |
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.
consider a chan of struct{fxnIdx int, err error} to make sure that error output is ordered consistently
// We include the time from when the report was captured, to easily distinguish between | ||
// separate exports | ||
outputFile := filepath.Join( | ||
exportOptions.OutputDir, |
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.
If this is to be in the same location but have potentially different types of subchecks run based on exportOptions I think that we MUST include the options used to run this in the report file.
I decided to tackle this work in smaller PRs. Notably #9365 is one of them. Closing this PR |
Description
Introduce
glooctl export report
Context
Background
As I work on e2e tests for Gloo Gateway, I have observed that many of our tools/utilities have different implementations spread across the codebase. The following actions are things that we have different implementations of:
I think we need to have standard ways of executing actions and then re-use that everywhere it is needed. I have merged previous PRs that attempt to improve this, and this PR is an extension of:
CLI
I'm happy to discuss other names for this tool. My hope was that the CLI code is as minimal as possible, and just passes user-defined options to some re-usable code.
envoyutils.AdminCtl
This is a reusable utility for executing curl requests agains the Envoy Admin API. It is intended to store all details about accessing the Envoy API, and allow developer to use the utility methods that it exposes.
This is used by the export utility (defined below) to capture details from a running envoy instance.
Export utility
This is a utility which will be invoked by our CLI (
glooctl export report
) or our tests, when they fail. I could see this utility living in other places in the codebase too, but for now I just placed it in a sharedcliutils
location.It is built with the intention of just collecting lots of small modules, which should be re-used in other areas of our codebase.
The current implementation just tries to define a structure, and is not feature-complete. I'd rather get consensus on the existing code, merge it, and then expand the usage of it in follow-up PRs.
Testing steps
gloo-system
namespaceNotes for reviewers
Checklist: